Re: [PATCH 2/2] ASoC: codecs: nau8822: add support for supply regulators

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Regulators are enabled permanently during probe and never disabled during PM or DAPM transitions.
- [Medium] Inefficient and semantically incorrect use of optional regulators in a loop instead of the bulk API.
- [Medium] Missing hardware stabilization delay after enabling power supplies before the first I2C access.
- [Low] Missing <linux/regulator/consumer.h> header inclusion.
--

commit d49855c46afc5aae191093735a6b46a1097ad452
Author: Alexey Charkov <alchark@flipper.net>

ASoC: codecs: nau8822: add support for supply regulators

This commit adds support for requesting and enabling four power supply
regulators (VDDA, VDDB, VDDC, VDDSPK) during the I2C probe of the NAU8822
codec to ensure they are online before communication.

> diff --git a/sound/soc/codecs/nau8822.c b/sound/soc/codecs/nau8822.c
> index a11759f85eaca..c21a037e0f083 100644
> --- a/sound/soc/codecs/nau8822.c
> +++ b/sound/soc/codecs/nau8822.c
> @@ -1153,7 +1153,8 @@ static int nau8822_i2c_probe(struct i2c_client *i2c)
>  {
>  	struct device *dev = &i2c->dev;
>  	struct nau8822 *nau8822 = dev_get_platdata(dev);
> -	int ret;
> +	static const char * const regulators[] = { "vdda", "vddb", "vddc", "vddspk" };
> +	int ret, i;

Should <linux/regulator/consumer.h> be included in this file since it now
uses regulator APIs?

Relying on implicit inclusion might cause build failures if header
dependencies change in the future.

[ ... ]

> @@ -1167,6 +1168,14 @@ static int nau8822_i2c_probe(struct i2c_client *i2c)
>  		return dev_err_probe(&i2c->dev, PTR_ERR(nau8822->mclk),
>  			"Error getting mclk\n");
>  
> +	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> +		ret = devm_regulator_get_enable_optional(dev, regulators[i]);
> +		if (ret && ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get regulator %s\n",
> +					     regulators[i]);
> +	}
> +

Does using devm_regulator_get_enable_optional() here prevent the driver
from managing power during suspend/resume or DAPM transitions?

Since this API discards the regulator handles, the driver cannot disable
the regulators when the system suspends or when the DAPM bias level drops
to SND_SOC_BIAS_OFF.

Also, since these supplies are required for communication, could
devm_regulator_bulk_get_enable() be used instead of a loop?

The bulk API automatically substitutes dummy regulators if properties are
missing in older Device Trees, avoiding the need for manual -ENODEV
filtering. It also processes ramp-up delays in parallel, whereas this loop
forces serial waits for each regulator.

Finally, is a hardware stabilization delay needed before the first I2C
access?

While the regulator framework handles the PMIC voltage ramp-up delay, it
doesn't account for the codec's internal Power-On-Reset stabilization time.

Without a post-power-on delay (like usleep_range()), can the subsequent I2C
reset race with the codec's internal reset state machine and intermittently
fail with -EREMOTEIO?

nau8822_i2c_probe() {
    ...
    nau8822->regmap = devm_regmap_init_i2c(i2c, &nau8822_regmap_config);
    ...
    /* Reset the codec */
    ret = regmap_write(nau8822->regmap, NAU8822_REG_RESET, 0x00);
    ...
}

>  	nau8822->regmap = devm_regmap_init_i2c(i2c, &nau8822_regmap_config);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-nau8822-reg-v1-0-c532e18e92ad@flipper.net?part=2