Re: [alsa-devel] [PATCH v4] tas2770: Add tas2770 smart PA kernel driver

On Mon, Sep 16, 2019 at 09:38:01AM +0800, shifu0704@thundersoft.com wrote:

From: Frank Shi shifu0704@thundersoft.com

Add tas2770 smart PA kernel driver

This is a great improvement, thanks. There's still some issues though, including some confusion over the use of bias levels - it seems like there's fairly poor integration with standard power management, it's not clear what's going on there. We do also still have some code that resets the CODEC which is concerning.

+#define TAS2770_CHECK_PERIOD 5000 /* 5 second */

We don't need this any more.

• case SND_SOC_BIAS_STANDBY:

• snd_soc_component_update_bits(component, TAS2770_PWR_CTRL,
  

• 	TAS2770_PWR_CTRL_MASK,
  

• 	TAS2770_PWR_CTRL_ACTIVE);
  

• tas2770->power_state = SND_SOC_BIAS_STANDBY;
  

• break;
  

We already store the bias level in the component's dapm struct, no need to duplicate it.

• case SND_SOC_BIAS_PREPARE:

• snd_soc_component_update_bits(component, TAS2770_PWR_CTRL,
  

• 	TAS2770_PWR_CTRL_MASK,
  

• 	TAS2770_PWR_CTRL_MUTE);
  

We shouldn't be muting and unmuting here, leave this to the digital mute operation.

+#ifdef CONFIG_PM +static int tas2770_codec_suspend(struct snd_soc_component *component) +{

• int ret;
• ret = tas2770_set_bias_level(component, SND_SOC_BIAS_OFF);
• if (ret)

• return -EINVAL;
  

Pass back error codes you get from functions, however it looks like you can just set the device as idle_bias_off and have the core do this for you anyway (plus more at runtime) - there's no appreciable delays I can see in power on and off.

+static int tas2770_dac_event(struct snd_soc_dapm_widget *w,

• 	     struct snd_kcontrol *kcontrol, int event)
  

+{

• struct snd_soc_component *component =

• 	snd_soc_dapm_to_component(w->dapm);
  

• struct tas2770_priv *tas2770 =

• 	snd_soc_component_get_drvdata(component);
  

• int ret;
• switch (event) {
• case SND_SOC_DAPM_POST_PMU:

• ret = tas2770_set_bias_level(component,
  

• 	SND_SOC_BIAS_PREPARE);
  

• if (ret)
  

• 	goto end;
  

This is very, very bad - DAPM will control the bias level for the CODEC, you should not be trying to control it from within DAPM callbacks. This will only lead to breakage. What is this intended to do?

+static int tas2770_mute(struct snd_soc_dai *dai, int mute) +{

• struct snd_soc_component *component = dai->component;
• int ret;
• if (mute)

• ret = tas2770_set_bias_level(component, SND_SOC_BIAS_PREPARE);
  

• else

• ret = tas2770_set_bias_level(component, SND_SOC_BIAS_STANDBY);
  

• return ret;

+}

This is more bias level misuse, you are independently setting the bias level in multiple different ways from many different call sites. This can only need to problems, I am surprised this works well for you as things stand.

If the device doesn't have a mute operation it is fine to not have one.

• switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
• case (SND_SOC_DAIFMT_I2S):

• tdm_rx_start_slot = 1;
  

These brackets around the case statements are not the normal Linux coding style (this isn't an issue in the rest of the driver...).

• case 0:
• /* Do not change slot width */

• ret = 0;
  

• break;

Please align the breaks and comments with the rest of the code inside the case.

+static const struct snd_kcontrol_new tas2770_snd_controls[] = {

• SOC_SINGLE_TLV("Amp Output Gain", TAS2770_PLAY_CFG_REG0,

• 0, 0x14, 0,
  

• tas2770_digital_tlv),
  

Volume controls should end in Volume - see control-names.rst

+static irqreturn_t tas2770_irq_handler(int irq, void *dev_id) +{

• struct tas2770_priv *tas2770 = (struct tas2770_priv *)dev_id;
• struct snd_soc_component *component = tas2770->component;
• unsigned int device_status_1 = 0, device_status_2 = 0;
• int result;
• if (tas2770->runtime_suspend)

• goto end;
  

• if (tas2770->power_state == TAS2770_POWER_SHUTDOWN)

• goto end;
  

• result = snd_soc_component_write(component, TAS2770_INT_MASK_REG0,

• 		TAS2770_INT_MASK_REG0_DISABLE);
  

• if (result)

• goto reload;
  

• result = snd_soc_component_write(component, TAS2770_INT_MASK_REG1,

• 		TAS2770_INT_MASK_REG1_DISABLE);
  

• if (result)

• goto reload;
  

The interrupt handler appears to be masking interrupts? Why?

• result = snd_soc_component_read(tas2770->component,

• 	TAS2770_LAT_INT_REG0, &device_status_1);
  

• if (!result && (device_status_1 & 0x3)) {

If we fail to do I/O with the device we should report an error.

+reload:

• /* hardware reset and reload */
• tas2770_load_config(tas2770);

Why are we doing a hardware reset in the interrupt handler, and how is this coordinated with anything else that's going on.

+end:

• return IRQ_HANDLED;

+}

This unconditionally reports the interrupt as handled, this prevents the interrupt line being shared and stops the interrupt core from doing error handling if anything goes wrong.

• tas2770->dev = &client->dev;
• tas2770->irq_gpio = client->irq;

This isn't a GPIO, it's an interrupt - calling this irq_gpio is very confusing.