Re: [PATCH v2 1/2] ASoC: es8328: reset the codec at probe time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ASoC: es8328: NULL pointer dereference in es8328_spi_probe() match lookup
- [Medium] ASoC: es8328: missing regcache reinit on reset in es8328_component_probe()
- [Medium] ASoC: es8328: missing everest,es8388 match entry in es8328_spi_probe()
--

commit 011e1ea21d759f33d35a0271cc926730dcdcfe1b
Author: Bob Hui <bg9oxa@163.com>

ASoC: es8328: reset the codec at probe time

This commit issues a control port reset in es8328_component_probe() so the
codec starts from its hardware defaults even after a warm reboot or driver
rebind. It also adds per-variant reg_default tables for ES8328 and ES8388 and
selects the regmap configuration using I2C and SPI match data.

> diff --git a/sound/soc/codecs/es8328-spi.c b/sound/soc/codecs/es8328-spi.c
> index 88e353ae52a10..606de21e98e1e 100644
> --- a/sound/soc/codecs/es8328-spi.c
> +++ b/sound/soc/codecs/es8328-spi.c
> @@ -14,15 +14,20 @@
>  #include "es8328.h"
>  
>  static const struct of_device_id es8328_of_match[] = {
> -	{ .compatible = "everest,es8328", },
> +	{ .compatible = "everest,es8328", .data = (void *)ES8328_TYPE_ES8328 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, es8328_of_match);
>  
>  static int es8328_spi_probe(struct spi_device *spi)
>  {
> -	return es8328_probe(&spi->dev,
> -			devm_regmap_init_spi(spi, &es8328_regmap_config));
> +	enum es8328_type type = (unsigned long)spi_get_device_match_data(spi);

[Severity: High]
Does calling spi_get_device_match_data(spi) here cause a NULL pointer
dereference when probing an SPI-connected ES8328?

In sound/soc/codecs/es8328.h, ES8328_TYPE_ES8328 is the first enumerator in
enum es8328_type and evaluates to 0, so es8328_of_match in
sound/soc/codecs/es8328-spi.c stores NULL in .data.

When es8328_spi_probe() calls spi_get_device_match_data(spi),
device_get_match_data(&sdev->dev) returns NULL and spi_get_device_match_data()
falls back to spi_get_device_id(sdev):

drivers/spi/spi.c:spi_get_device_match_data() {
	match = device_get_match_data(&sdev->dev);
	if (match)
		return match;

	id = spi_get_device_id(sdev);
}

Because es8328_spi_driver does not define an .id_table, spi_get_device_id()
passes sdrv->id_table (NULL) to spi_match_id(), which immediately dereferences
id->name[0].

> +	const struct regmap_config *config;
> +
> +	config = type == ES8328_TYPE_ES8388 ? &es8388_regmap_config
> +					    : &es8328_regmap_config;

[Severity: Medium]
Can type ever equal ES8328_TYPE_ES8388 in es8328_spi_probe()?

In sound/soc/codecs/es8328-spi.c, es8328_of_match only lists the
everest,es8328 entry with ES8328_TYPE_ES8328 and omits everest,es8388, while
es8328_spi_driver defines no spi_device_id table.

Because Documentation/devicetree/bindings/sound/everest,es8328.yaml specifies
the ES8388 compatible list as everest,es8388 followed by everest,es8328, an
SPI-connected ES8388 will match the everest,es8328 fallback entry and use
es8328_regmap_config instead of es8388_regmap_config (and also skip the
ES8388-specific ES8328_DACCONTROL21_SLRCK setup added in follow-up commit
db663b096dbe9f68fbdc51ad2211743c7793550e).

Should es8328_of_match (and an spi_device_id table) in
sound/soc/codecs/es8328-spi.c include the es8388 entry as well?

[ ... ]

> diff --git a/sound/soc/codecs/es8328.c b/sound/soc/codecs/es8328.c
> index aaa6646ad4c5e..f04e310262e79 100644
> --- a/sound/soc/codecs/es8328.c
> +++ b/sound/soc/codecs/es8328.c

[ ... ]

> @@ -836,6 +837,30 @@ static int es8328_component_probe(struct snd_soc_component *component)
>  		goto clk_fail;
>  	}
>  
> +	/*
> +	 * Put the codec into a known state: the supplies may already be on,
> +	 * a warm reboot or a kexec keeps the registers alive, and a driver
> +	 * rebind leaves whatever the previous instance programmed.  Without
> +	 * this every read-modify-write in the driver works from a value the
> +	 * hardware never had.  SCP_RESET is not self-clearing.
> +	 */
> +	ret = snd_soc_component_write(component, ES8328_CONTROL1,
> +				      ES8328_CONTROL1_SCP_RESET);
> +	if (ret) {
> +		dev_err(component->dev, "unable to reset the codec\n");
> +		clk_disable_unprepare(es8328->clk);
> +		goto clk_fail;
> +	}
> +	usleep_range(1000, 5000);
> +	ret = snd_soc_component_write(component, ES8328_CONTROL1,
> +				      ES8328_CONTROL1_ENREF |
> +				      ES8328_CONTROL1_VMIDSEL_500k);

[Severity: Medium]
Does resetting the hardware in es8328_component_probe() leave the regmap cache
out of sync if the ASoC component is rebound?

es8328->regmap and its REGCACHE_MAPLE cache are initialized once during I2C or
SPI bus probe in es8328_i2c_probe() and es8328_spi_probe(), whereas the
control port reset is performed in es8328_component_probe() when the ASoC card
binds the component.

While the card is active, registers such as ES8328_MASTERMODE (in
es8328_set_dai_fmt()), ES8328_CONTROL2, ES8328_CHIPPOWER, and mixer controls
are updated in both hardware and es8328->regmap. If the machine driver or
sound card is unbound and rebound while the I2C/SPI codec device remains
bound, or if snd_soc_bind_card() fails or defers after soc_init_pcm_runtime()
and retries later, es8328_component_probe() runs again and resets all hardware
registers to their power-on defaults without reinitializing the regmap cache.

Subsequent snd_soc_component_update_bits() calls then compare against the
stale cached values from the previous bind and skip hardware writes when the
new value matches the stale cache (for example leaving ES8328_MASTERMODE at its
0x80 master-mode reset default when consumer mode 0x00 is requested). Note
that while the unused es8328->type field added in this patch is wired up in
follow-up commit db663b096dbe9f68fbdc51ad2211743c7793550e, the regcache is
still not reinitialized after ES8328_CONTROL1_SCP_RESET.

Should es8328_component_probe() reinitialize the regmap cache (or bypass cache
for the reset sequence and call regcache_mark_dirty() / reinit) when resetting
the codec?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-es8328-vol-init-v2-0-d15b04c22f4f@163.com?part=1