Re: [PATCH 1/2] ASoC: codecs: ak4619: Add optional PDN pin support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] gpio: API semantic violation via incorrect PDN logical polarity
- [High] sound: missing 10ms hardware wake delay in resume path
- [Medium] sound: resource leak due to missing physical power-down
--

commit 8cb28b5bd3bb703c3fa3ae908ee58f9cf02df5c7
Author: Alexander Koch <mail@alexanderkoch.net>

ASoC: codecs: ak4619: Add optional PDN pin support

This commit adds optional device tree binding to support the PDN pin on the
ak4619 codec. If defined, it asserts the PDN pin during driver probe and
waits 10ms before the first register access.

> diff --git a/sound/soc/codecs/ak4619.c b/sound/soc/codecs/ak4619.c
> index 5252b30669d48..776199489db79 100644
> --- a/sound/soc/codecs/ak4619.c
> +++ b/sound/soc/codecs/ak4619.c
> @@ -894,6 +895,12 @@ static int ak4619_i2c_probe(struct i2c_client *i2c)
>  
>  	i2c_set_clientdata(i2c, ak4619);
>  
> +	ak4619->pdn_gpio = devm_gpiod_get_optional(dev, "pdn", GPIOD_OUT_HIGH);

[Severity: High]
Does this code use the incorrect logical polarity for the power-down pin?

If a device tree correctly defines the PDN (power down, active-low) pin with
GPIO_ACTIVE_LOW, requesting GPIOD_OUT_HIGH (logical assert) will translate
to a physical LOW state. This permanently powers down the codec instead of
waking it up, causing probe to fail.

To work around this, the commit message example forces DT authors to
incorrectly define the active-low pin as GPIO_ACTIVE_HIGH.

Should this use GPIOD_OUT_LOW to logically deassert the power-down state,
which correctly yields a physical HIGH when paired with an ACTIVE_LOW DT
definition?

> +	if (IS_ERR(ak4619->pdn_gpio))
> +		return PTR_ERR(ak4619->pdn_gpio);
> +	if (ak4619->pdn_gpio)
> +		msleep(10);
> +

[Severity: High]
Does the addition of this PDN pin and 10ms hardware wake delay also require
an update to the resume path?

If power is cut or the GPIO controller loses context during system suspend,
the codec resets and loses state. Upon resume, the GPIO controller restores
the physical PDN pin to HIGH.

Shortly after, the ASoC core invokes ak4619_resume():

sound/soc/codecs/ak4619.c:ak4619_resume() {
    ...
	regcache_cache_only(regmap, false);
	return regcache_sync(regmap);
}

Because ak4619_resume() immediately executes regcache_sync() to burst-write
restored registers over I2C, it violates the mandatory 10ms hardware wake
delay specified by the datasheet.

Could this cause the codec to fail initialization on resume, leading to
NACKed I2C transfers and permanently silent audio?

>  	ak4619->regmap = devm_regmap_init_i2c(i2c, &ak4619_regmap_cfg);
>  	if (IS_ERR(ak4619->regmap)) {
>  		ret = PTR_ERR(ak4619->regmap);
>  		dev_err(dev, "regmap_init() failed: %d\n", ret);
>  		return ret;
>  	}

[Severity: Medium]
Does this error path leak power by failing to physically power down the codec?

If devm_regmap_init_i2c() or devm_snd_soc_register_component() fails during
probe, the devm framework calls gpiod_put() to release the GPIO. By design,
gpiolib frees the descriptor but intentionally leaves the physical pin in its
currently driven HIGH state, keeping the codec powered up.

Should devm_add_action_or_reset() be used to register a custom cleanup action
that calls gpiod_set_value_cansleep() to properly power down the device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928114019.1441752-1-mail@alexanderkoch.net?part=1