Re: [PATCH v3 2/2] ASoC: codecs: add support for Everest ES7202 PDM ADC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ASoC: es7202: regcache overwrite and ordering in es7202_suspend() and es7202_resume()
- [Low] ASoC: es7202: positional initializer in es7202_i2c_id table
--

commit 8b7285d85de8871310d7776f1de182623a778972
Author: Jun Yan <jerrysteve1101@gmail.com>

ASoC: codecs: add support for Everest ES7202 PDM ADC

This commit adds an ASoC codec driver for the Everest Semiconductor ES7202
2-channel PDM analog-to-digital converter controlled over I2C.

Link: https://archive.org/details/es-7202-ds

> diff --git a/sound/soc/codecs/es7202.c b/sound/soc/codecs/es7202.c
> new file mode 100644
> index 0000000000000..b652916b44587
> --- /dev/null
> +++ b/sound/soc/codecs/es7202.c
> @@ -0,0 +1,373 @@

[ ... ]

> +static const struct reg_default es7202_reg_defaults[] = {
> +	{ ES7202_RESET,		0x10 },
> +	{ ES7202_SOFT_MODE,	0x00 },
> +	{ ES7202_CLK_DIV,	0x04 },
> +	{ ES7202_CLK_EN,	0x00 },
> +	{ ES7202_T1_VMID,	0x01 },
> +	{ ES7202_T2_VMID,	0x18 },
> +	{ ES7202_PDM_INF_CTL,	0x30 },

[ ... ]

> +static int es7202_suspend(struct snd_soc_component *component)
> +{
> +	struct es7202_priv *es7202 = snd_soc_component_get_drvdata(component);
> +
> +	return es7202_mute_pdm(es7202, true);
> +}
> +
> +static int es7202_resume(struct snd_soc_component *component)
> +{
> +	struct es7202_priv *es7202 = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	/* re-run the power-up sequence to bring the analog front-end back up */
> +	ret = es7202_hw_init(es7202);
> +	if (ret)
> +		return ret;
> +
> +	regcache_mark_dirty(es7202->regmap);
> +	return regcache_sync(es7202->regmap);
> +}

[Severity: Medium]
Does the suspend and resume sequence corrupt the cached mute state in
ES7202_PDM_INF_CTL and re-write the initialization sequence twice?

When a capture stream is prepared, __soc_pcm_prepare() ->
snd_soc_dai_digital_mute() -> es7202_mute_stream() -> es7202_mute_pdm()
clears ES7202_PDM_INF_CTL_MUTE, setting ES7202_PDM_INF_CTL to 0x30 in both
hardware and the regmap cache.

During system suspend, snd_soc_suspend() calls es7202_suspend(), which calls
es7202_mute_pdm(es7202, true) without bypassing the regmap cache. That
overwrites the cached value of ES7202_PDM_INF_CTL with 0x33 (muted).

On resume, es7202_resume() then runs es7202_hw_init(es7202), which also
writes init_seq through regmap_multi_reg_write() into both hardware and the
cache (again setting ES7202_PDM_INF_CTL to 0x33), before calling
regcache_mark_dirty() and regcache_sync().

In addition, even if ES7202_PDM_INF_CTL had remained 0x30 in the cache, 0x30
matches the entry in es7202_reg_defaults, so regcache_mark_dirty() tells
regcache_sync() to skip syncing registers whose cached value matches the
default, leaving the hardware at 0x33 as programmed by es7202_hw_init().
Meanwhile, calling regcache_mark_dirty() after es7202_hw_init() causes
regcache_sync() to redundantly write all 9 non-volatile registers from
init_seq that differ from es7202_reg_defaults a second time over I2C.

Since soc_resume_deferred() only unmutes playback streams via
soc_playback_digital_mute(), won't a capture stream in
SNDRV_PCM_STATE_PREPARED before suspend (or resumed via
SNDRV_PCM_IOCTL_RESUME without re-running .prepare()) remain muted after
resume?

[ ... ]

> +static const struct i2c_device_id es7202_i2c_id[] = {
> +	{ "es7202" },
> +	{ }
> +};

[Severity: Low]
This isn't a bug, but per I2C subsystem conventions for new struct
i2c_device_id tables, could the entry in es7202_i2c_id use a named
initializer:

	{ .name = "es7202" },

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101080926.267041-1-jerrysteve1101@gmail.com?part=2