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

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.