Re: [PATCH v5 4/4] update tas27xx.c to support either TAS2764 or TAS2780

On 3/30/2022 4:45 PM, Raphael-Xu wrote:

Missing commit message in this patch and previous one. Even one sentence explaining what you are doing and why is better then nothing.

Overall the series looks a lot better now, I still wonder if changing coding style makes sense, but ultimately it's your code, so as long as it patches checkpatch I will leave decision to Mark.

And there are few nitpicks, below in this patch.

Signed-off-by: Raphael-Xu 13691752556@139.com

sound/soc/codecs/tas27xx.c | 378 ++++++++++++++++++++++++++----------- 1 file changed, 263 insertions(+), 115 deletions(-)

diff --git a/sound/soc/codecs/tas27xx.c b/sound/soc/codecs/tas27xx.c index 8118429bb2f5..bb845d4797ce 100644 --- a/sound/soc/codecs/tas27xx.c +++ b/sound/soc/codecs/tas27xx.c

...

@@ -146,8 +182,9 @@ static int tas27xx_dac_event(struct snd_soc_dapm_widget *w, snd_soc_dapm_to_component(w->dapm); struct tas27xx_priv *tas27xx = snd_soc_component_get_drvdata(component);

• int ret;

• int ret = 0;

• mutex_lock(&tas27xx->codec_lock); switch (event) { case SND_SOC_DAPM_POST_PMU: ret = snd_soc_component_update_bits(component,

@@ -163,13 +200,16 @@ static int tas27xx_dac_event(struct snd_soc_dapm_widget *w, break; default: dev_err(tas27xx->dev, "Unsupported event\n");

• return -EINVAL;
  

• 	ret = -EINVAL;
  

There seems to be one tab to many here.

}

• if (ret < 0)

• return ret;
  

• return 0;

• if (ret < 0) {

• pr_err("%s:%u:errCode:0x%0x:PWR_CTRL error\n",
  

• 	__func__, __LINE__, ret);
  

• } else {

• ret = 0;
  

• }

• mutex_unlock(&tas27xx->codec_lock);

• return ret; }

  static const struct snd_kcontrol_new isense_switch =

@@ -207,55 +247,96 @@ static const struct snd_soc_dapm_route tas27xx_audio_map[] = { static int tas27xx_mute(struct snd_soc_dai *dai, int mute, int direction) { struct snd_soc_component *component = dai->component;

• int ret;

• struct tas27xx_priv *tas27xx =

• snd_soc_component_get_drvdata(component);
  

• int ret = 0;

• mutex_lock(&tas27xx->codec_lock);

• if (mute == 0) {

alternatively if (!mute), but you can leave as is

• ret = snd_soc_component_update_bits(component,
  

• 	TAS27XX_CLK_CFG,
  

• 	TAS27XX_CLK_CFG_MASK,
  

• 	TAS27XX_CLK_CFG_ENABLE);
  

• if (ret < 0) {
  

• 	dev_err(tas27xx->dev,
  

• 		"%s:%u: Failed to CLK_CFG_ENABLE\n",
  

• 		__func__, __LINE__);
  

• 	goto EXIT;
  

• }
  

• usleep_range(2000, 2000);
  

• } ret = snd_soc_component_update_bits(component, TAS27XX_PWR_CTRL,

• 			    TAS27XX_PWR_CTRL_MASK,
  

• 			    mute ? TAS27XX_PWR_CTRL_MUTE : 0);
  

• if (ret < 0)

• return ret;
  

• TAS27XX_PWR_CTRL_MASK,
  

• mute ? TAS27XX_PWR_CTRL_MUTE : 0);
  

• if (ret >= 0) {

• tas27xx->mb_power_up = mute?false:true;
  

• ret = 0;
  

• }

• return 0;

• if (ret < 0) {

You could probably just use } else { here with above if.

• pr_err("%s:%u: Failed to set powercontrol\n",
  

• 	__func__, __LINE__);
  

• }

+EXIT:

• mutex_unlock(&tas27xx->codec_lock);
• return ret; }

...

static const struct snd_soc_dai_ops tas27xx_dai_ops = { @@ -495,13 +615,13 @@ static struct snd_soc_dai_driver tas27xx_dai_driver[] = { }, .capture = { .stream_name = "ASI1 Capture",

• 	.channels_min   = 0,
  

• 	.channels_min   = 1,
  .channels_max   = 2,
  .rates = TAS27XX_RATES,
  .formats = TAS27XX_FORMATS,
  

  }, .ops = &tas27xx_dai_ops,

• .symmetric_rate = 1,
  

• .symmetric_rates = 1,
  

I'm pretty sure struct snd_soc_dai_driver uses .symmetric_rate, so this change would result in build failure?

}, };

@@ -509,7 +629,7 @@ static int tas27xx_codec_probe(struct snd_soc_component *component) { struct tas27xx_priv *tas27xx = snd_soc_component_get_drvdata(component);

• int ret;

• int ret = 0;

  tas27xx->component = component;

...

static DECLARE_TLV_DB_SCALE(tas27xx_digital_tlv, 1100, 50, 0); @@ -551,8 +685,10 @@ static const struct snd_kcontrol_new tas27xx_snd_controls[] = {

static const struct snd_soc_component_driver soc_component_driver_tas27xx = { .probe = tas27xx_codec_probe, +#ifdef CONFIG_PM .suspend = tas27xx_codec_suspend, .resume = tas27xx_codec_resume, +#endif .set_bias_level = tas27xx_set_bias_level, .controls = tas27xx_snd_controls, .num_controls = ARRAY_SIZE(tas27xx_snd_controls), @@ -590,6 +726,12 @@ static const struct regmap_range_cfg tas27xx_regmap_ranges[] = { }, };

+static bool tas27xx_volatile(struct device *dev,

• unsigned int reg)

+{

• return true;
  

+}

This seems bit weird, are all registers considered volatile?

static const struct regmap_config tas27xx_i2c_regmap = { .reg_bits = 8, .val_bits = 8, @@ -599,6 +741,7 @@ static const struct regmap_config tas27xx_i2c_regmap = { .ranges = tas27xx_regmap_ranges, .num_ranges = ARRAY_SIZE(tas27xx_regmap_ranges), .max_register = 1 * 128,

• .volatile_reg = tas27xx_volatile, };

  static int tas27xx_parse_dt(struct device *dev, struct tas27xx_priv *tas27xx)

...