Received-SPF: pass (google.com: domain of linux-kernel+bounces-76811-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 22 Feb 2024 15:47:34 +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 v2 2/2] ASoC: nau8325: new driver
Message-ID: <6ffb4165-95e0-4ccd-868f-8ecda56b4079@sirena.org.uk>
References: <20240222083825.190854-1-wtli@nuvoton.com>
 <20240222083825.190854-3-wtli@nuvoton.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="e7yScaGYIov+nAEz"
Content-Disposition: inline
In-Reply-To: <20240222083825.190854-3-wtli@nuvoton.com>


--e7yScaGYIov+nAEz
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Feb 22, 2024 at 04:38:25PM +0800, Seven Lee wrote:

A few very minor things below but basically this looks good.

> +static int nau8325_clksrc_choose(struct nau8325 *nau8325,
> +				 const struct nau8325_srate_attr **srate_table,
> +				 int *n1_sel, int *mult_sel, int *n2_sel)
> +{

> +proc_done:
> +	dev_err(nau8325->dev, "nau8325->fs=%d,range=0x%x, %s, (n1,mu,n2,dmu):(%d,%d,%d), MCLK_SRC=%uHz (%d)",
> +		nau8325->fs, (*srate_table)->range,
> +		(*srate_table)->max ? "MAX" : "MIN",
> +		*n1_sel == CLK_PROC_BYPASS ?
> +		CLK_PROC_BYPASS : mclk_n1_div[*n1_sel].param,
> +		*mult_sel == CLK_PROC_BYPASS ?
> +		CLK_PROC_BYPASS : 1 << mclk_n3_mult[*mult_sel].param,
> +		1 << mclk_n2_div[*n2_sel].param,
> +		(*srate_table)->mclk_src[ratio],
> +		(*srate_table)->mclk_src[ratio] / nau8325->fs);

This should be a dev_dbg(), dev_err() will be very noisy in normal
operation.

> +static int nau8325_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct nau8325 *nau8325 = snd_soc_component_get_drvdata(component);
> +	unsigned int ctrl1_val = 0;
> +
> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBS_CFS:
> +		break;

Please use the modern _CBC_CFC defines.

> +	ret = regmap_read(nau8325->regmap, NAU8325_R02_DEVICE_ID, &value);
> +	if (ret) {
> +		dev_err(dev, "Failed to read device id (%d)", ret);
> +		goto err;
> +	}

Probably a good idea to check that the device ID is what we expected
too.

--e7yScaGYIov+nAEz
Content-Type: application/pgp-signature; name="signature.asc"

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmXXbJYACgkQJNaLcl1U
h9B1UQf/bbkyPq72hB16DeF5r7ZEpD8pxmWbH7Z+fKteU+6U295SCSHqnxBE+KX2
taIj/NfQ5964S9buK72KAeaiwI4o7d6duiqXCiy2vlLIBe4X+RBG4Ch+5E7SUdAu
ovjwmHLvfyJPCqcflNpxDQ35sVOrph6kFTi7UywOiY0X3oWAnIMuQK/wnGq2rfgB
tSKpUJU9AhGUog03Fg8D4kK+5SuQ5Wb6kfUfrYbDLrrxSxQeAWlgLuTJ84RDZ5gq
QAoLemPwWspvf8wLK71i22ETnllb87XHFzCuRv9ZXlqckJOjHSFHBHNcBGIAjXqb
2cMhEzQZj137Wwu6n+Dc8NE5g9hO4Q==
=9vMY
-----END PGP SIGNATURE-----

--e7yScaGYIov+nAEz--