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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190919142939.GL3642@sirena.co.uk>
Date:   Thu, 19 Sep 2019 15:29:39 +0100
From:   Mark Brown <broonie@...nel.org>
To:     shifu0704@...ndersoft.com
Cc:     lgirdwood@...il.com, perex@...ex.cz, tiwai@...e.com,
        linux-kernel@...r.kernel.org, alsa-devel@...a-project.org,
        robh+dt@...nel.org, dmurphy@...com, navada@...com
Subject: Re: [PATCH v6] tas2770: add tas2770 smart PA kernel driver

On Wed, Sep 18, 2019 at 04:28:13PM +0800, shifu0704@...ndersoft.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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)