Re: [PATCH v3 2/2] ASoC: add support for TAS6424 digital amplifier

[Michael Stecklein]

On 07/19/2017 06:08 AM, Mark Brown wrote:
> On Tue, Jul 18, 2017 at 02:20:04PM -0500, Michael Stecklein wrote:
>
>> +	if (!tx_mask) {
>> +		dev_err(codec->dev, "tdm mask must not be 0\n");
>> +		return -EINVAL;
>> +	}
> Setting the mask to 0 is used when turning off TDM.
When turning off TDM, will the set_tdm_slot function ever get
called? (Since the TDM was turned off the slots don't need to
be set.) Also, for this device TDM mode is automatically
selected based on the presence of a TDM clock, so there is
nothing to do to switch into or out of TDM mode. Similar
parts handle this the same way, such as: tlv320aic3x.c,
tas2552.c, tas5720.c, ssm4567.c
>
>> +	if ((reg & TAS6424_WARN_VDD_UV) && !(tas6424->last_warn & TAS6424_WARN_VDD_UV))
>> +		dev_warn(dev, "experienced a VDD under voltage condition\n");
>> +
>> +	if ((reg & TAS6424_WARN_VDD_POR) && !(tas6424->last_warn & TAS6424_WARN_VDD_POR))
>> +		dev_warn(dev, "experienced a VDD POR condition\n");
> These look like they are errors rather than warnings and should be
> critical prints like the other errors.
These conditions happened on the previous shutdown/reset
of the device. We are letting the user know about them, but
they are not critical to the current device state, so I have left
them as warnings.
>
>> +	/* Store current warn value so we can detect any changes next time */
>> +	tas6424->last_warn = reg;
> It would be better to clear these at some point, perhaps after a delay.
> Especially with thermal issues they could come and go over device
> operation.
Since this function is called every 200ms, if an issue comes
and goes, last_warn will be cleared during the next function
call after the issue ends; the function regmap_read will read
in a 0 for that flag's bit, and that new reg value will be saved.
Therefore, the user will be warned only once if the issue is
persistent, but rewarned if the issues leaves and comes
back, so I do not think clearing after a delay is necessary.
>
>> +static int tas6424_codec_probe(struct snd_soc_codec *codec)
>> +{
>> +	struct tas6424_data *tas6424 = snd_soc_codec_get_drvdata(codec);
>> +	int ret;
>> +
>> +	tas6424->codec = codec;
>> +
>> +	ret = regulator_bulk_enable(ARRAY_SIZE(tas6424->supplies),
>> +				    tas6424->supplies);
>> +	if (ret != 0) {
>> +		dev_err(codec->dev, "failed to enable supplies: %d\n", ret);
>> +		return ret;
>> +	}
> This init stuff should be in either the main I2C probe, DAPM or bias
> level management.  The CODEC probe should be very minimal.
This is a single codec device, does it make sense to leave the
device running without its codec? Other single codec devices
initialize regulators in codec probe similarly.
>
>> +	if (event & SND_SOC_DAPM_POST_PMU) {
>> +	} else if (event & SND_SOC_DAPM_PRE_PMD) {
> You're trying to write a switch statement here.
Acked.
>
>> +	switch (level) {
>> +	case SND_SOC_BIAS_ON:
>> +	case SND_SOC_BIAS_PREPARE:
>> +		msleep(500);
>> +		break;
> You need a 500ms sleep in all these cases?  That's a lot of sleeping.
Unfortunately, the device runs diagnostics at this point,
which take upwards of 480ms.
>
>> +	case SND_SOC_BIAS_STANDBY:
>> +		ret = snd_soc_update_bits(codec, TAS6424_CH_STATE_CTRL_REG,
>> +					  0xff, TAS6424_CH1_STATE_MUTE | TAS6424_CH2_STATE_MUTE |
>> +					  TAS6424_CH3_STATE_MUTE | TAS6424_CH4_STATE_MUTE);
>> +
>> +		if (ret < 0) {
>> +			dev_err(codec->dev, "error resuming device: %d\n", ret);
>> +			return ret;
>> +		}
>> +		break;
>> +	case SND_SOC_BIAS_OFF:
>> +		ret = snd_soc_update_bits(codec, TAS6424_CH_STATE_CTRL_REG,
>> +					  0xff, TAS6424_CH1_STATE_HIZ | TAS6424_CH2_STATE_HIZ |
>> +					  TAS6424_CH3_STATE_HIZ | TAS6424_CH4_STATE_HIZ);
>> +
>> +		if (ret < 0) {
>> +			dev_err(codec->dev, "error suspending device: %d\n", ret);
>> +			return ret;
>> +		}
> These mutes seem very random disassociated from anything else?
What do you mean by this comment? The control register is
set to STATE_MUTE and STATE_HIZ depending on what the
device needs for the new BIAS level.