Received-SPF: pass (google.com: domain of linux-kernel+bounces-34111-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 22 Jan 2024 20:28:06 +0000
From: Mark Brown <broonie@kernel.org>
To: Seven Lee <wtli@nuvoton.com>
Cc: lgirdwood@gmail.com, alsa-devel@alsa-project.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	robh+dt@kernel.org, conor+dt@kernel.org, YHCHuang@nuvoton.com,
	KCHSU0@nuvoton.com, CTLIN0@nuvoton.com, SJLIN0@nuvoton.com,
	scott6986@gmail.com, supercraig0719@gmail.com, dardar923@gmail.com
Subject: Re: [PATCH 2/2] ASoC: nau8325: new driver
Message-ID: <820c5ff7-4329-46b0-9981-607b1593deb5@sirena.org.uk>
References: <20240122095650.60523-1-wtli@nuvoton.com>
 <20240122095650.60523-2-wtli@nuvoton.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="QAZjW4H1wKCsCURa"
Content-Disposition: inline
In-Reply-To: <20240122095650.60523-2-wtli@nuvoton.com>


--QAZjW4H1wKCsCURa
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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?

--QAZjW4H1wKCsCURa
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmWuz9UACgkQJNaLcl1U
h9BQjgf/QLIx/6Vn2gRBlBNbQfnoOasSe3itaUJwtDJ6sVFgb6jjZT0ZlOww4+rf
TX08tOTbrDHhWqh2RCuxxAFCHG9Uu0TYl46QikJIRfZSP/1dWtdY4dptsv+n5Kgh
UQDmnDiewqFaR4mlDBzgzmeDL+Gfr8L+ucedfTpRpwl7YNv1/Mwubh5U/oDGLGVB
+2O7QQ8OKWpdUW4XRvH3Rdh7GYrtx0YUPn+Yicp7wOnyKg7qs1OEvpTBpAQVPPvT
LpE2I+3swtCjetn2maB+cPUgDcOKL6hu9rIj/alple3YU5ux+il+PiJDlfTD5H1C
XNimTSNbAm/gNjZbrUQbShQj05Upcg==
=+I7g
-----END PGP SIGNATURE-----

--QAZjW4H1wKCsCURa--