Received-SPF: pass (google.com: domain of linux-kernel+bounces-40186-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 26 Jan 2024 14:33:14 +0000
From: Mark Brown <broonie@kernel.org>
To: Shenghao Ding <shenghao-ding@ti.com>
Cc: conor+dt@kernel.org, krzysztof.kozlowski@linaro.org, robh+dt@kernel.org,
	andriy.shevchenko@linux.intel.com, kevin-lu@ti.com,
	baojun.xu@ti.com, devicetree@vger.kernel.org, v-po@ti.com,
	lgirdwood@gmail.com, perex@perex.cz,
	pierre-louis.bossart@linux.intel.com, 13916275206@139.com,
	mohit.chawla@ti.com, linux-sound@vger.kernel.org,
	linux-kernel@vger.kernel.org, liam.r.girdwood@intel.com,
	soyer@irl.hu, jkhuang3@ti.com, tiwai@suse.de, pdjuandi@ti.com,
	j-mcpherson@ti.com, navada@ti.com
Subject: Re: [PATCH v2 1/4] ASoc: PCM6240: Create PCM6240 Family driver code
Message-ID: <6c1d04be-c558-4aa4-96a3-ac21ae36bfae@sirena.org.uk>
References: <20240126035855.1785-1-shenghao-ding@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="zICUD1iSPynTqKb5"
Content-Disposition: inline
In-Reply-To: <20240126035855.1785-1-shenghao-ding@ti.com>


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

On Fri, Jan 26, 2024 at 11:58:51AM +0800, Shenghao Ding wrote:

This looks mostly good - I've got a few comments that are mainly
stylistic or otherwise very minor, there's one issue with validation of
profile IDs that does look like it's important to fix though.

> +static int pcmdev_dev_read(struct pcmdevice_priv *pcm_dev,
> +	unsigned int dev_no, unsigned int reg, unsigned int *val)
> +{
> +	int ret = -EINVAL;
> +
> +	if (dev_no < pcm_dev->ndev) {

You could write all these functions a bit more simply if you rewrote
these error checks to return immediately on error, that way there's less
indentation and fewer paths later on.

	if (dev_no >= pcm_dev->ndev)
		return -EINVAL;

and so on.  For the ones dealing with locking it can help to have a
single exit path but functions like this don't deal directly with the
locks.

> +
> +		ret = regmap_read(map, reg, val);
> +		if (ret < 0)
> +			dev_err(pcm_dev->dev, "%s, E=%d\n", __func__, ret);
> +	} else
> +		dev_err(pcm_dev->dev, "%s, no such channel(%d)\n", __func__,
> +			dev_no);
> +

The kernel coding style is that if one side of an if/else has { } both
should.

> +static int pcmdevice_set_profile_id(
> +	struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *codec
> +		= snd_soc_kcontrol_component(kcontrol);
> +	struct pcmdevice_priv *pcm_dev =
> +		snd_soc_component_get_drvdata(codec);
> +	int ret = 0;
> +
> +	if (pcm_dev->cur_conf != ucontrol->value.integer.value[0]) {
> +		pcm_dev->cur_conf = ucontrol->value.integer.value[0];
> +		ret = 1;
> +	}
> +
> +	return ret;
> +}

This will accept any configuration number, shouldn't there be some
validation here?  The put functions doing regmap_update_bits() have
some limiting of values in the regmap_update_bits() but this just stores
the value directly.

> +static int pcmdevice_get_volsw(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{

> +	mutex_lock(&pcm_dev->codec_lock);
> +	rc = pcmdev_dev_read(pcm_dev, dev_no, reg, &val);
> +	if (rc) {
> +		dev_err(pcm_dev->dev, "%s:read, ERROR, E=%d\n",
> +			__func__, rc);
> +		goto out;
> +	}

It would be kind of nice if the device switching could be hidden inside
a custom regmap and we didn't have all this code duplication but I'm not
thinking of a way of doing that which doesn't just create complications
so probably this is fine.

> +	val = (val >> shift) & mask;
> +	val = (val > max) ? max : val;
> +	val = mc->invert ? max - val : val;
> +	ucontrol->value.integer.value[0] = val;

There's the FIELD_GET() macro (and FIELD_SET() for writing values) - the
core predates them and hence doesn't use them, we might want to update
some time.

> +static int pcmdevice_codec_probe(struct snd_soc_component *codec)
> +{

> +	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
> +		pcm_dev->regbin_name, pcm_dev->dev, GFP_KERNEL, pcm_dev,
> +		pcmdev_regbin_ready);
> +	if (ret) {
> +		dev_err(pcm_dev->dev, "load %s error = %d\n",
> +			pcm_dev->regbin_name, ret);
> +		goto out;
> +	}

It might be better to request the firmware in the I2C probe rather than
in the ASoC level probe, that way there's more time for the firmware to
be loaded before we actually need it.  That does mean you can't register
the controls immediately though so it may be more trouble than it's
worth.

Similarly for the reset, if we reset as early as possible that seems
better.

> +static int pcmdevice_startup(struct snd_pcm_substream *substream,
> +	struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *codec = dai->component;
> +	struct pcmdevice_priv *pcm_priv = snd_soc_component_get_drvdata(codec);
> +	int ret = 0;
> +
> +	if (pcm_priv->fw_state != PCMDEVICE_FW_LOAD_OK) {
> +		dev_err(pcm_priv->dev, "DSP bin file not loaded\n");
> +		ret = -EINVAL;
> +	}

Perhaps -EBUSY instead?  What the user is doing is valid.

> +static const struct regmap_config pcmdevice_i2c_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_RBTREE,

Use _MAPLE for new devices, it's a more modern design with tradeoffs
that work better for most current systems.

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

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmWzwqoACgkQJNaLcl1U
h9BCzwf+L41HbR0BgZxeKdyUNHfjw//dNQQiokt5X33hzy2MfX0cEw4MOf8k9xyv
x7kjEUSTwhyF6hxi1Ov9iApcnot6U5L1aoxrAS19xBs3zCqO5dEDtDQz8B6jHXF3
hR6dT0dwXyKQxOx57BhEPbF16MY0ZpJuE3Od6i4s54zt/rqJw2hUH9xoZNA7BgRH
8w2OMYMbbY+FL9QyLrDBxMJCgxDDnblMbeBp4yIS35A20M7d2MnxgedAmCmDWZth
Zkhut1zLOwqZydXjkXrJQAB3izplMzHYS1lxmBOqdAjC+cwTrVIxr0O8GwmYc8bI
P4rWI9yDw6TygfQlYVCQd3jyHNiuSA==
=OLs3
-----END PGP SIGNATURE-----

--zICUD1iSPynTqKb5--