Re: [PATCH v3 4/4] ARM: dts: mediatek: mt6323: add AUXADC support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unconditional dereference of `dev->parent->parent` can lead to a kernel panic.
- [High] The driver actively powers down the AUXADC clock and disables the 1.8V voltage reference during initialization.
--

commit 0ad66b352744a9242754a09a7f9af3042d3da009
Author: Roman Vivchar <rva333@protonmail.com>

ARM: dts: mediatek: mt6323: add AUXADC support

Add the devicetree node for the mt6323 AUXADC.

> diff --git a/arch/arm/boot/dts/mediatek/mt6323.dtsi b/arch/arm/boot/dts/mediatek/mt6323.dtsi
> index c230c865116d0..c070f4b0936c2 100644
> --- a/arch/arm/boot/dts/mediatek/mt6323.dtsi
> +++ b/arch/arm/boot/dts/mediatek/mt6323.dtsi
> @@ -14,6 +14,11 @@ pmic: mt6323 {
>  		interrupt-controller;
>  		#interrupt-cells = <2>;
>  
> +		mt6323_adc: adc {
> +			compatible = "mediatek,mt6323-auxadc";
> +			#io-channel-cells = <1>;
> +		};

[Severity: High]
This isn't a bug introduced by this patch, but adding this devicetree node
exposes a pre-existing issue in the mt6323-auxadc driver. Can the driver
panic if it evaluates dev->parent->parent as NULL?

In drivers/iio/adc/mt6323-auxadc.c:mt6323_auxadc_probe():

    regmap = dev_get_regmap(dev->parent->parent, NULL);

If the device is instantiated directly without a parent (such as via a
standalone devicetree node or manual sysfs binding), platform_device_add()
sets dev->parent to &platform_bus. Because platform_bus.parent is NULL,
passing NULL to dev_get_regmap() will cause an immediate kernel panic when
devres_find() attempts to acquire the devres_lock spinlock.

[Severity: High]
This is also a pre-existing issue, but does the driver actively power down
the AUXADC clock and disable the 1.8V voltage reference during initialization?

In drivers/iio/adc/mt6323-auxadc.c:mt6323_auxadc_init():

    ret = regmap_set_bits(map, MT6323_TOP_CKPDN2, AUXADC_TOP_CKPDN2_CTL_CK);

And later in the same function:

    ret = regmap_set_bits(map, MT6323_AUXADC_CON27,
                  AUXADC_CON27_VREF18_ENB_MD |
                  AUXADC_CON27_MD_STATUS);

In MediaTek PMIC terminology, CKPDN stands for Clock Power Down (setting the
bit to 1 actively powers down the clock), and ENB stands for Enable Bar
(an active-low enable signal, where setting the bit to 1 disables the
reference voltage). By setting these bits to 1 instead of clearing them to 0,
the driver appears to disable the hardware it is attempting to initialize,
which could cause subsequent ADC read operations to time out and fail.

>  		mt6323_leds: leds {
>  			compatible = "mediatek,mt6323-led";
>  			#address-cells = <1>;

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