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

[Mark Brown]

On Mon, Sep 16, 2019 at 09:38:01AM +0800, shifu0704@xxxxxxxxxxxxxxx wrote:
> From: Frank Shi <shifu0704@xxxxxxxxxxxxxxx>
> 
> 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.

Attachment: signature.asc
Description: PGP signature