linux-kernel - Re: [PATCH 2/2] ASoC: nau8325: new 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] [thread-next>] [day] [month] [year] [list]

Date: Mon, 22 Jan 2024 20:28:06 +0000
From: Mark Brown <broonie@...nel.org>
To: Seven Lee <wtli@...oton.com>
Cc: lgirdwood@...il.com, alsa-devel@...a-project.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	robh+dt@...nel.org, conor+dt@...nel.org, YHCHuang@...oton.com,
	KCHSU0@...oton.com, CTLIN0@...oton.com, SJLIN0@...oton.com,
	scott6986@...il.com, supercraig0719@...il.com, dardar923@...il.com
Subject: Re: [PATCH 2/2] ASoC: nau8325: new driver

On Mon, Jan 22, 2024 at 05:56:50PM +0800, Seven Lee wrote:

> +++ b/sound/soc/codecs/nau8325.c
> @@ -0,0 +1,896 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * nau8325.c -- Nuvoton NAU8325 audio codec driver
> + *

Please use a C++ comment for the whole block to make things look more
consistent.

> +static int nau8325_clkdet_put(struct snd_kcontrol *kcontrol,
> +			      struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct soc_mixer_control *mc =
> +		(struct soc_mixer_control *)kcontrol->private_value;
> +	struct snd_soc_component *component =
> +		snd_soc_kcontrol_component(kcontrol);
> +	struct nau8325 *nau8325 = snd_soc_component_get_drvdata(component);
> +	unsigned int max = mc->max, min = mc->min, val;
> +	unsigned int mask = (1 << fls(max)) - 1;

AFAICT this will only work well if max is 1, just hard code that.

> +
> +	val = (ucontrol->value.integer.value[0] + min) & mask;
> +	nau8325->clock_detection = val;
> +
> +	if (nau8325->clock_detection)
> +		regmap_update_bits(nau8325->regmap, NAU8325_R40_CLK_DET_CTRL,
> +				   NAU8325_CLKPWRUP_DIS, 0);
> +	else
> +		regmap_update_bits(nau8325->regmap, NAU8325_R40_CLK_DET_CTRL,
> +				   NAU8325_CLKPWRUP_DIS, NAU8325_CLKPWRUP_DIS);
> +
> +	return nau8325->clock_detection;
> +}

Please use mixer-test to verify that your controls conform to the
expected API, the return value here is not what's expected - it should
be a negative value for an error, 0 for no change and 1 for change.

> +	SOC_SINGLE_EXT("Clock Detection", SND_SOC_NOPM, 0, 1, 0,
> +		       nau8325_clkdet_get, nau8325_clkdet_put),

Shouldn't this be a Switch?

> +	SOC_SINGLE("ALC Enable", NAU8325_R2E_ALC_CTRL3,
> +		   NAU8325_ALC_EN_SFT, 1, 0),

ALC Switch.

> +static int nau8325_powerup_event(struct snd_soc_dapm_widget *w,
> +				 struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_dapm_to_component(w->dapm);
> +	struct nau8325 *nau8325 = snd_soc_component_get_drvdata(component);
> +
> +	if (nau8325->clock_detection)
> +		return 0;
> +

What happens if someone enables clock detection while things are powered
up?

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