Re: [PATCH v3 3/4] mfd: mt6397-core: add mt6323 AUXADC support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mt6323_auxadc_init incorrectly powers down the AUXADC clock instead of enabling it.
- [Medium] Failure during mt6323_auxadc_request() does not revert the hardware voltage buffer enablement, leading to a permanent hardware state leak.
--

commit 6b92fe47672fd5e88ade7a2d9b916034b0dc7bca
Author: Roman Vivchar <rva333@protonmail.com>

mfd: mt6397-core: add mt6323 AUXADC support

The mt6323 PMIC includes an AUXADC. Register the AUXADC in the mt6323
devices array to allow the corresponding driver to probe using compatible
string.

> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> index 3e58d0764c7e0..013b0857fb548 100644
> --- a/drivers/mfd/mt6397-core.c
> +++ b/drivers/mfd/mt6397-core.c
> @@ -125,6 +125,9 @@ static const struct resource mt6323_pwrc_resources[] = {
>  
>  static const struct mfd_cell mt6323_devs[] = {
>  	{
> +		.name = "mt6323-auxadc",
> +		.of_compatible = "mediatek,mt6323-auxadc",
> +	}, {

[Severity: High]
While reviewing the driver this enables, does mt6323_auxadc_init()
incorrectly power down the AUXADC clock?

drivers/iio/adc/mt6323-auxadc.c:mt6323_auxadc_init() {
    ...
	ret = regmap_set_bits(map, MT6323_TOP_CKPDN2, AUXADC_TOP_CKPDN2_CTL_CK);
    ...
}

The Mediatek PMIC TOP_CKPDN2 register is a "Clock Power Down" register,
meaning that setting a bit to 1 disables the corresponding clock. By using
regmap_set_bits() here, the clock appears to be explicitly powered down
instead of enabled. This would prevent the AUXADC hardware from functioning
and cause subsequent reads in mt6323_auxadc_read() to perpetually time out.

[Severity: Medium]
Also, does a failure during mt6323_auxadc_request() leak the hardware
voltage buffer state?

drivers/iio/adc/mt6323-auxadc.c:mt6323_auxadc_request() {
    ...
	ret = regmap_set_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);
	if (ret)
		return ret;

	return regmap_set_bits(map, MT6323_AUXADC_CON22, BIT(channel));
}

If the second regmap_set_bits() fails, the function returns without undoing
the enablement of AUXADC_CON11_VBUF_EN.

This error then propagates up to mt6323_auxadc_read_raw():

drivers/iio/adc/mt6323-auxadc.c:mt6323_auxadc_read_raw() {
        ...
		ret = mt6323_auxadc_request(auxadc, chan->channel);
		if (ret)
			return ret;
        ...
}

Because this returns immediately on error, it skips the
mt6323_auxadc_release() cleanup step. This leaves the hardware voltage
buffer permanently enabled until the device is reset or a subsequent read
succeeds.

>  		.name = "mt6323-rtc",
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-mt6323-adc-v3-0-1c27c588185d@protonmail.com?part=3