Re: [PATCH v2 2/2] ASoC: cs42xx8: Add SPI bus support for CS42448/CS42888 codec

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Modifying the shared cs42xx8_regmap_config structure disables bulk I/O operations for the existing I2C driver, causing performance degradation.

Pre-existing issues:
- [High] Modifying the globally shared static cs42xx8_dai structure during probe introduces shared state corruption across multiple codecs.
--

commit e668063709eddf991f98c990850087567f6a8d52
Author: Chancel Liu <chancel.liu@nxp.com>

ASoC: cs42xx8: Add SPI bus support for CS42448/CS42888 codec
    
The existing cs42xx8 driver only supported I2C control interface.
Add SPI bus support for the Cirrus Logic CS42448/CS42888 Audio CODEC.

[ ... ]
> diff --git a/sound/soc/codecs/cs42xx8-spi.c b/sound/soc/codecs/cs42xx8-spi.c
> new file mode 100644
> index 0000000000000..b86fe2fe771ee
> --- /dev/null
> +++ b/sound/soc/codecs/cs42xx8-spi.c
> @@ -0,0 +1,104 @@
[ ... ]
> +static int cs42xx8_spi_probe(struct spi_device *spi)
> +{
> +	struct cs42xx8_driver_data *drvdata;
> +	struct regmap_config config;
> +	int ret;
> +
> +	drvdata = (struct cs42xx8_driver_data *)spi_get_device_match_data(spi);
> +	if (!drvdata)
> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				     "failed to find driver data\n");
> +
> +	config = cs42xx8_regmap_config;
> +	/*
> +	 * reg_bits=16 makes regmap send a 2-byte address field (big-endian).
> +	 * write_flag_mask/read_flag_mask are OR'd into that address field:
> +	 */
> +	config.reg_bits           = 16;

[Severity: Medium]
Since cs42xx8_spi_probe() already makes a local copy of the config here, could
the single read/write flags be overridden locally instead of modifying the
shared cs42xx8_regmap_config structure?

> +	config.write_flag_mask    = 0x9E;
> +	config.read_flag_mask     = 0x9F;
[ ... ]
> diff --git a/sound/soc/codecs/cs42xx8.c b/sound/soc/codecs/cs42xx8.c
> index 12fe9b3e2525e..5b689549c74ed 100644
> --- a/sound/soc/codecs/cs42xx8.c
> +++ b/sound/soc/codecs/cs42xx8.c
> @@ -478,6 +478,9 @@ const struct regmap_config cs42xx8_regmap_config = {
>  	.volatile_reg = cs42xx8_volatile_register,
>  	.writeable_reg = cs42xx8_writeable_register,
>  	.cache_type = REGCACHE_MAPLE,
> +	.reg_format_endian  = REGMAP_ENDIAN_BIG,
> +	.use_single_read = true,
> +	.use_single_write = true,

[Severity: Medium]
Does adding these restrictions directly to the shared config disable bulk I/O
operations for the existing I2C driver?

It appears the I2C driver uses this struct directly, so it is now globally
restricted to single-register reads and writes. This might force
regcache_sync() to perform a full I2C transaction for every single register
during device resume.

[Severity: High]
This isn't a bug introduced by this patch, but does modifying the globally
shared static cs42xx8_dai structure during probe introduce shared state
corruption across multiple codecs?

Looking at cs42xx8_probe():

    cs42xx8_dai.name = cs42xx8->drvdata->name;

    /* Each adc supports stereo input */
    cs42xx8_dai.capture.channels_max = cs42xx8->drvdata->num_adcs * 2;

If multiple codecs of this family are present on the same system, the probe
function of the second codec will overwrite the name and capture.channels_max
fields of the global cs42xx8_dai structure.

Because all instances share this single static struct via the pointer passed
to devm_snd_soc_register_component(), the first codec's DAI state will be
corrupted.

>  };
>  EXPORT_SYMBOL_GPL(cs42xx8_regmap_config);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603095041.3906558-1-chancel.liu@oss.nxp.com?part=2