Re: [PATCH v6] tas2770: add tas2770 smart PA kernel driver

[Mark Brown]

On Wed, Sep 18, 2019 at 04:28:13PM +0800, shifu0704@xxxxxxxxxxxxxxx wrote:

> +static int tas2770_codec_suspend(struct snd_soc_component *component)
> +{
> +	int ret;
> +
> +	ret = snd_soc_component_update_bits(component,
> +		TAS2770_PWR_CTRL,
> +		TAS2770_PWR_CTRL_MASK,
> +		TAS2770_PWR_CTRL_SHUTDOWN);
> +	if (ret) {
> +		snd_soc_component_update_bits(component,
> +			TAS2770_PWR_CTRL,
> +			TAS2770_PWR_CTRL_MASK,
> +			TAS2770_PWR_CTRL_ACTIVE);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

This error handling is a bit weird, if the write failed usually it's
best to leave things as they are rather than retrying the write.  You
should also pass back the error code you got from the I/O rather than
overwriting it with -EINVAL since that helps people diagnose problems.

> +static int tas2770_set_samplerate(struct tas2770_priv *tas2770,
> +								int samplerate)

The indentation on the second line here is really weird, it's not
aligned with anything.

> +	switch (slot_width) {
> +	case 16:
> +		ret = snd_soc_component_update_bits(component,
> +			TAS2770_TDM_CFG_REG2,
> +			TAS2770_TDM_CFG_REG2_RXS_MASK,
> +			TAS2770_TDM_CFG_REG2_RXS_16BITS);
> +	break;

The indentation of the break statements here is still off.

> +static const struct snd_kcontrol_new tas2770_snd_controls[] = {
> +	SOC_SINGLE_TLV("Playback Volume", TAS2770_PLAY_CFG_REG2,
> +		0, TAS2770_PLAY_CFG_REG2_VMAX, 1,
> +		tas2770_playback_volume),
> +	SOC_SINGLE_TLV("Amp Output Gain", TAS2770_PLAY_CFG_REG0,
> +		0, 0x14, 0,
> +		tas2770_digital_tlv),

Volume controls should still have names endinf in Volume as covered in
control-names.rst.  

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.

Attachment: signature.asc
Description: PGP signature