Re: [PATCH v8 3/3] update to support either TAS2764 or TAS2780

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5236 bytes --]

On Mon, Apr 11, 2022 at 03:56:52PM +0800, Raphael-Xu wrote:

>  static void tas27xx_reset(struct tas27xx_priv *tas27xx)
>  {
>  	if (tas27xx->reset_gpio) {
>  		gpiod_set_value_cansleep(tas27xx->reset_gpio, 0);
> -		msleep(20);
> +		usleep_range(2000, 2050);
>  		gpiod_set_value_cansleep(tas27xx->reset_gpio, 1);
> +		usleep_range(5000, 5050);
>  	}

This looks like an unrelated but good fix?  It should be a separate
patch.

> +			TAS27XX_PWR_CTRL,
> +			TAS27XX_PWR_CTRL_MASK,
> +			TAS27XX_PWR_CTRL_SHUTDOWN);
> +		if (ret >= 0) {
> +			tas27xx->mb_power_up = false;
> +			ret = 0;

mb_power_up seems to never be read - what purpose does it serve?

> -	return 0;
> +	if (ret < 0)
> +		pr_err("%s:%u:errCode:0x%0x:set BIAS error\n",
> +			__func__, __LINE__, ret);

Please use something like normal kernel logging styles - use dev_err()
like the rest of the function, no __func__ or __line__ and log the error
code as an integer.  In general please try to follow the kernel coding
style.

> +	mutex_unlock(&tas27xx->codec_lock);

It's not clear what this lock is protecting - it seems to be serialising
things that the core will already ensure don't run concurrently.  It at
least needs some documentation.  If it's not needed at all then a lot of
the diff could be dropped which would help a lot since as far as I can
see the bulk of the changes here are for adding this lock so it's hard
to see the device specific changes.  I'd also suggest pulling this out
into a separate patch.

> -	return 0;
> +EXIT:
> +	mutex_unlock(&tas27xx->codec_lock);

Normal coding style for labels is lower case.

>  {
> -	struct tas27xx_priv *tas27xx =
> +	struct tas27xx_priv *tas27xx =

This looks like an unneeded whitespace change?  There's a lot of these
where I can't spot what the actual change is...

>  }
> -#else
> -#define tas27xx_codec_suspend NULL
> -#define tas27xx_codec_resume NULL
>  #endif

This (and the related change below adding ifdefs for the use) are an
unrelated stylistic change and should be in a separate patch if they
make sense though I can't see any reason for them?  It's generally
considered better style not to need the ifdefs.

>  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) {
> +		ret = snd_soc_component_read(component,
> +			TAS27XX_CLK_CFG);
> +		if (ret < 0) {
> +			dev_err(tas27xx->dev,
> +				"%s:%u:errCode:0x%x read "
> +				"TAS27XX_CLK_CFG error\n",
> +				__func__, __LINE__, ret);
> +			goto EXIT;
> +		}
> +		if ((ret & TAS27XX_CLK_CFG_MASK) != TAS27XX_CLK_CFG_ENABLE) {
> +			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(3000, 3050);
> +		}

This clock configuration on mute is suprising - what's going on here?
It's an unusal thing to do.

>  		ret = snd_soc_component_update_bits(component,
> -					TAS27XX_TDM_CFG2,
> -					TAS27XX_TDM_CFG2_RXW_MASK,
> -					TAS27XX_TDM_CFG2_RXW_16BITS);
> +			TAS27XX_TDM_CFG2,
> +			TAS27XX_TDM_CFG2_RXW_MASK,
> +			TAS27XX_TDM_CFG2_RXW_16BITS);

Unrelated indentation change.

> @@ -522,26 +648,54 @@ static int tas27xx_codec_probe(struct snd_soc_component *component)
>  		gpiod_set_value_cansleep(tas27xx->sdz_gpio, 1);
>  
>  	tas27xx_reset(tas27xx);
> +	usleep_range(5000, 5050);

There's already a sleep in the reset function, why does this caller need
an extra one?

> -	ret = snd_soc_component_update_bits(tas27xx->component,
> -						TAS27XX_TDM_CFG5,
> +	ret = snd_soc_component_update_bits(component,

The changes to use a local component variable could probably usefully be
a separate patch, it obscures everything else that's going on.

> +static bool tas27xx_volatile(struct device *dev,
> +	unsigned int reg)

This should be a separate change probably, it looks like a bug fix.

> +{
> +	switch (reg) {
> +	case TAS27XX_SW_RST:
> +	case TAS27XX_PWR_CTRL:
> +	case TAS27XX_PAGE:

It's suprising that the power control and paging registers would be
volatile?  Same for some of the other registers...

> +	case TAS27XX_DVC:
> +	case TAS27XX_CHNL_0:
> +	case TAS27XX_TDM_CFG0:
> +	case TAS27XX_TDM_CFG1:
> +	case TAS27XX_TDM_CFG2:
> +	case TAS27XX_TDM_CFG3:
> +	case TAS27XX_TDM_CFG5:
> +	case TAS27XX_TDM_CFG6:

...like the TDM configuration.

>  static const struct i2c_device_id tas27xx_i2c_id[] = {
>  	{ "tas2764", TAS2764},
> +	{ "tas2780", TAS2780},
>  	{ }

I don't see any runtime differences between the two variants - nothing
is keyed off the ID?

>  static const struct of_device_id tas27xx_of_match[] = {
>  	{ .compatible = "ti,tas2764" },
> +	{ .compatible = "ti,tas2780" },
>  	{},
>  };

If it were we'd need to also have something here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]