Re: [PATCH 1/2] ASoC: es8328: describe the power-on register values in the regmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ASoC: es8328: cache/hardware mismatch from reg_defaults without reset
--

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

ASoC: es8328: describe the power-on register values in the regmap

This commit populates es8328_reg_defaults with datasheet power-on reset values
for registers ES8328_CONTROL1 through ES8328_DACCONTROL30 and hooks them up to
es8328_regmap_config.

> diff --git a/sound/soc/codecs/es8328.c b/sound/soc/codecs/es8328.c
> index aaa6646ad4c5e..d7f93de82f69a 100644
> --- a/sound/soc/codecs/es8328.c
> +++ b/sound/soc/codecs/es8328.c
> @@ -856,11 +856,69 @@ static void es8328_remove(struct snd_soc_component *component)
>  			       es8328->supplies);
>  }
>  
> +static const struct reg_default es8328_reg_defaults[] = {
> +	{ ES8328_CONTROL1,	0x06 },
> +	{ ES8328_CONTROL2,	0x1c },
> +	{ ES8328_CHIPPOWER,	0xc3 },
> +	{ ES8328_ADCPOWER,	0xfc },
> +	{ ES8328_DACPOWER,	0xc0 },
> +	{ ES8328_CHIPLOPOW1,	0x00 },
> +	{ ES8328_CHIPLOPOW2,	0x00 },
> +	{ ES8328_ANAVOLMANAG,	0x7c },
> +	{ ES8328_MASTERMODE,	0x80 },
> +	{ ES8328_ADCCONTROL1,	0x00 },
> +	{ ES8328_ADCCONTROL2,	0x00 },
> +	{ ES8328_ADCCONTROL3,	0x06 },
> +	{ ES8328_ADCCONTROL4,	0x00 },
> +	{ ES8328_ADCCONTROL5,	0x06 },
> +	{ ES8328_ADCCONTROL6,	0x30 },
> +	{ ES8328_ADCCONTROL7,	0x30 },
> +	{ ES8328_ADCCONTROL8,	0xc0 },
> +	{ ES8328_ADCCONTROL9,	0xc0 },
> +	{ ES8328_ADCCONTROL10,	0x38 },
> +	{ ES8328_ADCCONTROL11,	0xb0 },
> +	{ ES8328_ADCCONTROL12,	0x32 },
> +	{ ES8328_ADCCONTROL13,	0x06 },
> +	{ ES8328_ADCCONTROL14,	0x00 },
> +	{ ES8328_DACCONTROL1,	0x00 },
> +	{ ES8328_DACCONTROL2,	0x06 },
> +	{ ES8328_DACCONTROL3,	0x32 },
> +	{ ES8328_DACCONTROL4,	0xc0 },
> +	{ ES8328_DACCONTROL5,	0xc0 },
> +	{ ES8328_DACCONTROL6,	0x08 },
> +	{ ES8328_DACCONTROL7,	0x06 },
> +	{ ES8328_DACCONTROL8,	0x1f },
> +	{ ES8328_DACCONTROL9,	0xf7 },
> +	{ ES8328_DACCONTROL10,	0xfd },
> +	{ ES8328_DACCONTROL11,	0xff },
> +	{ ES8328_DACCONTROL12,	0x1f },
> +	{ ES8328_DACCONTROL13,	0xf7 },
> +	{ ES8328_DACCONTROL14,	0xfd },
> +	{ ES8328_DACCONTROL15,	0xff },
> +	{ ES8328_DACCONTROL16,	0x00 },
> +	{ ES8328_DACCONTROL17,	0x38 },
> +	{ ES8328_DACCONTROL18,	0x38 },
> +	{ ES8328_DACCONTROL19,	0x38 },
> +	{ ES8328_DACCONTROL20,	0x38 },
> +	{ ES8328_DACCONTROL21,	0x38 },
> +	{ ES8328_DACCONTROL22,	0x38 },
> +	{ ES8328_DACCONTROL23,	0x00 },
> +	{ ES8328_LOUT1VOL,	0x00 },
> +	{ ES8328_ROUT1VOL,	0x00 },
> +	{ ES8328_LOUT2VOL,	0x00 },
> +	{ ES8328_ROUT2VOL,	0x00 },
> +	{ ES8328_MONOOUTVOL,	0x00 },
> +	{ ES8328_DACCONTROL29,	0x00 },
> +	{ ES8328_DACCONTROL30,	0x00 },
> +};
> +
>  const struct regmap_config es8328_regmap_config = {
>  	.reg_bits	= 8,
>  	.val_bits	= 8,
>  	.max_register	= ES8328_REG_MAX,
>  	.cache_type	= REGCACHE_MAPLE,
> +	.reg_defaults		= es8328_reg_defaults,
> +	.num_reg_defaults	= ARRAY_SIZE(es8328_reg_defaults),

[Severity: High]
Won't adding es8328_reg_defaults to es8328_regmap_config in
sound/soc/codecs/es8328.c without resetting the codec on probe cause the
regmap cache and hardware state to drift apart when the chip is left in a
non-default state?

Without reg_defaults, an empty REGCACHE_MAPLE cache causes _regmap_read() to
miss in regcache_read(), read the live register value from the chip via
map->reg_read(), and cache that value. With es8328_reg_defaults provided,
regcache_init() pre-populates the maple tree cache with power-on reset values
for every register without reading from or writing to the hardware.

However, neither es8328_probe() nor es8328_component_probe() issues a software
reset (such as ES8328_CONTROL1_SCP_RESET) or syncs the cache to hardware, and
regulator_bulk_enable() will not power-cycle the codec if the supplies were
already enabled (for example, by a bootloader, across a warm reboot/kexec, or
across driver unbind/rebind with always-on regulators):

sound/soc/codecs/es8328.c:es8328_component_probe() {
	es8328 = snd_soc_component_get_drvdata(component);

	ret = regulator_bulk_enable(ARRAY_SIZE(es8328->supplies),
				    es8328->supplies);
	...
}

Because all registers now hit in the pre-populated cache, _regmap_read() will
return the datasheet reset values instead of the actual hardware state during
ALSA control reads and initial DAPM widget setup. In addition,
snd_soc_component_update_bits() compares new values against the cached reset
defaults and elides the hardware write whenever they match.

For example, ES8328_DACCONTROL3 is cached as 0x32 (with
ES8328_DACCONTROL3_DACMUTE cleared). If the codec was left muted (0x36) prior
to probe, unmuting in es8328_mute() with mute == 0:

sound/soc/codecs/es8328.c:es8328_mute() {
	return snd_soc_component_update_bits(dai->component, ES8328_DACCONTROL3,
			ES8328_DACCONTROL3_DACMUTE,
			mute ? ES8328_DACCONTROL3_DACMUTE : 0);
}

will see ES8328_DACCONTROL3_DACMUTE already cleared in the cache and skip the
register write, leaving the DAC muted in hardware.

Similarly, in es8328_set_dai_fmt() and es8328_hw_params(), updates to
ES8328_MASTERMODE (cached as 0x80, ES8328_MASTERMODE_MSC), ES8328_DACCONTROL1
(cached as 0x00), ES8328_ADCCONTROL4 (cached as 0x00), and ES8328_DACCONTROL6
(cached as 0x08) will be skipped as no-ops when the requested mode, I2S
format, 24-bit word length, or de-emphasis setting matches the cached default
rather than the state left in the hardware.

Should the driver reset the hardware in es8328_component_probe() so the chip
is guaranteed to match es8328_reg_defaults?

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