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

From: Mark Brown <broonie@kernel.org>
To: shifu0704@thundersoft.com
Cc: lgirdwood@gmail.com, perex@perex.cz, tiwai@suse.com,
	linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org,
	robh+dt@kernel.org, dmurphy@ti.com, navada@ti.com
Subject: Re: [PATCH v6] tas2770: add tas2770 smart PA kernel driver
Date: Thu, 19 Sep 2019 15:29:39 +0100	[thread overview]
Message-ID: <20190919142939.GL3642@sirena.co.uk> (raw)
In-Reply-To: <1568795293-19697-2-git-send-email-shifu0704@thundersoft.com>

[-- Attachment #1: Type: text/plain, Size: 2116 bytes --]

On Wed, Sep 18, 2019 at 04:28:13PM +0800, shifu0704@thundersoft.com 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]