linux-kernel - Re: [PATCH 2/2] ASoC: tas2764: Add the driver for the TAS2764

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201001162505.GO6715@sirena.org.uk>
Date:   Thu, 1 Oct 2020 17:25:05 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Dan Murphy <dmurphy@...com>
Cc:     lgirdwood@...il.com, tiwai@...e.com, robh+dt@...nel.org,
        devicetree@...r.kernel.org, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] ASoC: tas2764: Add the driver for the TAS2764

On Wed, Sep 30, 2020 at 11:38:09AM -0500, Dan Murphy wrote:

This all looks good - a few very minor things below but nothing
substantial:

> +	default:
> +		dev_err(tas2764->dev, "Not supported evevt\n");
> +		return -EINVAL;

evevt -> event

> +static int tas2764_mute(struct snd_soc_dai *dai, int mute, int direction)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	int ret = snd_soc_component_update_bits(component, TAS2764_PWR_CTRL,
> +						TAS2764_PWR_CTRL_MASK,
> +						mute ? TAS2764_PWR_CTRL_MUTE : 0);
> +
> +	if (ret < 0)
> +		return ret;

This looks weird with the ternery operator and extreme indentation -
could you please at least split the declaration of ret from the call to
make the line length a bit extreme?

> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S:
> +	case SND_SOC_DAIFMT_DSP_A:
> +		tdm_rx_start_slot = 1;
> +		break;
> +	case SND_SOC_DAIFMT_DSP_B:
> +	case SND_SOC_DAIFMT_LEFT_J:
> +		tdm_rx_start_slot = 0;
> +		break;

I'm not seeing any other handling that distinguishes between the I2S and
DSP modes anywhere - I'm guessing this is because the device is really
only implementing the DSP modes but because it's mono this is compatible
with the I2S modes?  It'd be worth having a comment saying this since
while that would be OK not distinguishing between modes properly is a
common error in drivers so it'd help avoid cut'n'paste issues if someone
uses this code as a reference.

> +static int tas2764_register_codec(struct tas2764_priv *tas2764)
> +{
> +	return devm_snd_soc_register_component(tas2764->dev,
> +					       &soc_component_driver_tas2764,
> +					       tas2764_dai_driver,
> +					       ARRAY_SIZE(tas2764_dai_driver));
> +}

This is a bit odd - can we not just inline the component registration
rather than having this function?

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