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

[Amadeusz Sławiński]

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 at 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)

...