[PATCH v4 2/3] update tas27xx.c to support either TAS2764 or TAS2780

[Amadeusz Sławiński]

On 3/23/2022 5:26 AM, Raphael-Xu wrote:
> Signed-off-by: Raphael-Xu <13691752556 at 139.com>
> ---
>   sound/soc/codecs/tas27xx.c | 695 +++++++++++++++++++++++--------------
>   1 file changed, 434 insertions(+), 261 deletions(-)
> 
> diff --git a/sound/soc/codecs/tas27xx.c b/sound/soc/codecs/tas27xx.c
> index 9265af41c235..4a0ba32bdcdb 100644
> --- a/sound/soc/codecs/tas27xx.c
> +++ b/sound/soc/codecs/tas27xx.c
> @@ -1,7 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0
> -//
> -// Driver for the Texas Instruments TAS2764 CODEC
> -// Copyright (C) 2020 Texas Instruments Inc.
> +// Driver for the Texas Instruments TAS2764/TAS2780 Mono
> +//		Audio amplifier
> +// Copyright (C) 2022 Texas Instruments Inc.
>   
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
> @@ -12,118 +12,158 @@
>   #include <linux/i2c.h>
>   #include <linux/gpio.h>
>   #include <linux/gpio/consumer.h>
> -#include <linux/regulator/consumer.h>
>   #include <linux/regmap.h>
>   #include <linux/of.h>
>   #include <linux/of_gpio.h>
> -#include <linux/slab.h>
>   #include <sound/soc.h>
>   #include <sound/pcm.h>
>   #include <sound/pcm_params.h>
>   #include <sound/initval.h>
>   #include <sound/tlv.h>
> +#include <linux/version.h>
>   
> -#include "tas2764.h"
> +#include "tas27xx.h"
>   
> -struct tas2764_priv {
> +struct tas27xx_priv {

I've scrolled through this patch, but it feels like there is too many 
things happening at the same time in it.
For example above renaming from tas2764 to tas27xx, feels to me like it 
should be part of separate patch.

>   	struct snd_soc_component *component;
>   	struct gpio_desc *reset_gpio;
>   	struct gpio_desc *sdz_gpio;
>   	struct regmap *regmap;
>   	struct device *dev;
> -	
> +	struct mutex codec_lock;
>   	int v_sense_slot;
>   	int i_sense_slot;
> +	int device_id;
> +	bool mbPowerUp;

Any reason not to follow naming convention of other variables:
bool mb_power_up;
?

>   };
>   
> -static void tas2764_reset(struct tas2764_priv *tas2764)
> +typedef enum tas27xx {
> +	TAS2764 = 0,
> +	TAS2780 = 1,
> +} tas27xx_t;
> +
> +static void tas27xx_reset(struct tas27xx_priv *tas27xx)
>   {
> -	if (tas2764->reset_gpio) {
> -		gpiod_set_value_cansleep(tas2764->reset_gpio, 0);
> +	if (tas27xx->reset_gpio) {
> +		gpiod_set_value_cansleep(tas27xx->reset_gpio, 0);
>   		msleep(20);
> -		gpiod_set_value_cansleep(tas2764->reset_gpio, 1);
> +		gpiod_set_value_cansleep(tas27xx->reset_gpio, 1);
>   	}
>   
> -	snd_soc_component_write(tas2764->component, TAS2764_SW_RST,
> +	snd_soc_component_write(tas27xx->component, TAS2764_SW_RST,
>   				TAS2764_RST);
>   }
>   
> -static int tas2764_set_bias_level(struct snd_soc_component *component,
> -				 enum snd_soc_bias_level level)
> +static int tas27xx_set_bias_level(
> +	struct snd_soc_component *component,
> +	enum snd_soc_bias_level level)
>   {
> -	struct tas2764_priv *tas2764 = snd_soc_component_get_drvdata(component);
> +	struct tas27xx_priv *tas27xx =
> +		snd_soc_component_get_drvdata(component);

Line limit was raised to 100 characters in kernel, and here as far as I 
can tell you don't even exceed 80 characters... so why break the line?

> +	int ret = 0;
>   
> +	mutex_lock(&tas27xx->codec_lock);
>   	switch (level) {
>   	case SND_SOC_BIAS_ON:
> -		snd_soc_component_update_bits(component, TAS2764_PWR_CTRL,
> -					      TAS2764_PWR_CTRL_MASK,
> -					      TAS2764_PWR_CTRL_ACTIVE);
> +		ret = snd_soc_component_update_bits(component,
> +			TAS2764_PWR_CTRL,
> +			TAS2764_PWR_CTRL_MASK,
> +			TAS2764_PWR_CTRL_ACTIVE);
> +		if (ret >= 0) {
> +			tas27xx->mbPowerUp = true;
> +			ret = 0;
> +		}
>   		break;
>   	case SND_SOC_BIAS_STANDBY:
>   	case SND_SOC_BIAS_PREPARE:
> -		snd_soc_component_update_bits(component, TAS2764_PWR_CTRL,
> -					      TAS2764_PWR_CTRL_MASK,
> -					      TAS2764_PWR_CTRL_MUTE);
> +		ret = snd_soc_component_update_bits(component,
> +			TAS2764_PWR_CTRL,
> +			TAS2764_PWR_CTRL_MASK,
> +			TAS2764_PWR_CTRL_MUTE);
> +		if (ret >= 0) {
> +			tas27xx->mbPowerUp = true;
> +			ret = 0;
> +		}
>   		break;
>   	case SND_SOC_BIAS_OFF:
> -		snd_soc_component_update_bits(component, TAS2764_PWR_CTRL,
> -					      TAS2764_PWR_CTRL_MASK,
> -					      TAS2764_PWR_CTRL_SHUTDOWN);
> +		ret = snd_soc_component_update_bits(component,
> +			TAS2764_PWR_CTRL,
> +			TAS2764_PWR_CTRL_MASK,
> +			TAS2764_PWR_CTRL_SHUTDOWN);
> +		if (ret >= 0) {
> +			tas27xx->mbPowerUp = false;
> +			ret = 0;
> +		}
>   		break;
> -
>   	default:
> -		dev_err(tas2764->dev,
> -				"wrong power level setting %d\n", level);
> -		return -EINVAL;
> +		dev_err(tas27xx->dev,
> +			"wrong power level setting %d\n", level);
> +		ret = -EINVAL;
>   	}
> -
> -	return 0;
> +	if (ret < 0)
> +		pr_err("%s:%u:errCode:0x%0x:set BIAS error\n",
> +			__func__, __LINE__, ret);
> +	mutex_unlock(&tas27xx->codec_lock);
> +	return ret;
>   }
>   
>   #ifdef CONFIG_PM
> -static int tas2764_codec_suspend(struct snd_soc_component *component)
> +static int tas27xx_codec_suspend(
> +	struct snd_soc_component *component)
>   {
> -	struct tas2764_priv *tas2764 = snd_soc_component_get_drvdata(component);
> -	int ret;
> +	struct tas27xx_priv *tas27xx =
> +		snd_soc_component_get_drvdata(component);

Again, unnecessary line break.

> +	int ret = 0;
>   
> +	mutex_lock(&tas27xx->codec_lock);
>   	ret = snd_soc_component_update_bits(component, TAS2764_PWR_CTRL,
> -					    TAS2764_PWR_CTRL_MASK,
> -					    TAS2764_PWR_CTRL_SHUTDOWN);
> -
> -	if (ret < 0)
> -		return ret;
> -
> -	if (tas2764->sdz_gpio)
> -		gpiod_set_value_cansleep(tas2764->sdz_gpio, 0);
> -
> -	regcache_cache_only(tas2764->regmap, true);
> -	regcache_mark_dirty(tas2764->regmap);
> +		TAS2764_PWR_CTRL_MASK,
> +		TAS2764_PWR_CTRL_SHUTDOWN);
>   
> -	return 0;
> +	if (ret < 0) {
> +		pr_err("%s:%u:errCode:0x%0x:power down error\n",
> +			__func__, __LINE__, ret);
> +		goto EXIT;
> +	}
> +	ret = 0;
> +	tas27xx->mbPowerUp = false;
> +	if (tas27xx->sdz_gpio)
> +		gpiod_set_value_cansleep(tas27xx->sdz_gpio, 0);
> +
> +	regcache_cache_only(tas27xx->regmap, true);
> +	regcache_mark_dirty(tas27xx->regmap);
> +EXIT:
> +	mutex_unlock(&tas27xx->codec_lock);
> +	return ret;
>   }
>   
> -static int tas2764_codec_resume(struct snd_soc_component *component)
> +static int tas27xx_codec_resume(
> +	struct snd_soc_component *component)

Even more unnecessary line break...

I'm stopping my review here, but overall it feels like 3 things happen 
at the same time:
renaming variables, reshuffling of code and changes... which makes it a 
bit hard to review for real changes in code.
As mentioned renaming should probably be done in separate patch. 
Unnecessary reshuffling should be kept to minimum.