Re: [PATCH v2 1/4] ASoc: PCM6240: Create PCM6240 Family driver code

[Mark Brown]

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.

Attachment: signature.asc
Description: PGP signature