Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 15 May 2023 19:49:47 +0900
From:   Mark Brown <broonie@kernel.org>
To:     Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
Cc:     lgirdwood@gmail.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        perex@perex.cz, tiwai@suse.com, alsa-devel@alsa-project.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] ASoC: Add ESS ES9218P codec driver
Message-ID: <ZGIOSwDduR+Ihe4p@finisterre.sirena.org.uk>
References: <20230515074021.31257-1-aidanmacdonald.0x0@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="TTuW6cRhBiN1JvNK"
Content-Disposition: inline
In-Reply-To: <20230515074021.31257-1-aidanmacdonald.0x0@gmail.com>
Precedence: bulk


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

On Mon, May 15, 2023 at 08:40:19AM +0100, Aidan MacDonald wrote:

> +++ b/sound/soc/codecs/ess-es9218p.c
> @@ -0,0 +1,805 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022-2023 Aidan MacDonald
> + */

Please make the entire comment a C++ one so things look more
intentional.

> +enum es9218p_supply_id {
> +	ES9218P_SUPPLY_AVDD,
> +	ES9218P_SUPPLY_VCCA,
> +	ES9218P_SUPPLY_AVCC3V3,
> +	ES9218P_SUPPLY_AVCC1V8,
> +	ES9218P_NUM_SUPPLIES
> +};
> +
> +static const char * const es9218p_supply_names[ES9218P_NUM_SUPPLIES] = {
> +	"avdd",
> +	"vcca",
> +	"avcc3v3",
> +	"avcc1v8",
> +};

These could easily get out of sync, it would be better to explicitly
assign the names to the slots identified by the constants

	[ES9218P_SUPPLY_VCCA] = "vcca",

for example.

> +static int es9218p_wide_write(struct regmap *regmap, unsigned int reg,
> +			      int count, unsigned int value)
> +{
> +	u8 data[4];
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		data[i] = value & 0xff;
> +		value >>= 8;
> +	}

This needs a bounds check to make sure we don't overflow data.

> +static int outlevel_put(struct snd_kcontrol *kcontrol,
> +			struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> +	struct es9218p_priv *priv = snd_soc_component_get_drvdata(component);
> +
> +	priv->output_2v = ucontrol->value.enumerated.item[0];
> +	es9218p_update_amp_mode(component);
> +	return 1;
> +}

Running the mixer-test selftest on a card with this driver will report
that the driver generates spurious events when there is no change in
value and doesn't validate input.  Similar issues apply to the other
enums.

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

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmRiDkoACgkQJNaLcl1U
h9AP6gf/dNn2DGt53OeralAJP2Y5G89tKyYlSRgYuW/GN/mBiCQQfdbiAAn2FlUc
Jzbzp/ekvr0JfPfwizAOxbGmx70kOO7WO0y/jndTH5mLcCADpKawSCzV+dck4Vs/
wn6tX/um/Z0ufqvxSaNC5ARP54obPa0oJJwd2jaNTtML+pnjWNqL2UT9unkGpRK/
ehNS0pXvC4OKEvX1H5XDJpm4gV0gaAYTyesprYzrE5aM81YBhbYVAZylDIdXGspx
h+wjEE/WzH/bkBd7pCdfKE/RNyZA48ef/a2HWpALkZeLwBWECrKh6rs4FDSFHOC/
heNoKK3dB16ZW71fgEXtXE1kbZVHag==
=7dKY
-----END PGP SIGNATURE-----

--TTuW6cRhBiN1JvNK--