Re: [PATCH v3 2/4] iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver incorrectly powers down the AUXADC clock during initialization, preventing the ADC from functioning.
- [High] Unchecked dereference of `dev->parent` when fetching the regmap could lead to a kernel panic.
- [Medium] The voltage buffer (`VBUF_EN`) is permanently left enabled if an error occurs during channel request or release.
--

commit ec67a1445925e2d4ca9ef40c52f5417793b977c8
Author: Roman Vivchar <rva333@protonmail.com>
Subject: iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver

The mt6323 AUXADC is a 15-bit ADC used for system monitoring. This driver
provides support for reading various channels including battery and
charger voltages, battery and chip temperature, current sensing and
accessory detection. Add a driver for the AUXADC found in the MediaTek 
mt6323 PMIC.

Tested-by: Ben Grisdale <bengris32@protonmail.ch>
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

[ ... ]
> diff --git a/drivers/iio/adc/mt6323-auxadc.c b/drivers/iio/adc/mt6323-auxadc.c
> new file mode 100644
> index 0000000000000..572466c3f375f
> --- /dev/null
> +++ b/drivers/iio/adc/mt6323-auxadc.c
[ ... ]
> +static int mt6323_auxadc_request(struct mt6323_auxadc *auxadc,
> +				 unsigned long channel)
> +{
> +	struct regmap *map = auxadc->regmap;
> +	int ret;
> +
> +	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));

[Severity: Medium]
If regmap_set_bits() for MT6323_AUXADC_CON22 fails here, does this
leave the AUXADC_CON11_VBUF_EN bit permanently enabled?

It looks like mt6323_auxadc_read_raw() aborts on a request error without
calling mt6323_auxadc_release().

> +}
> +
> +static int mt6323_auxadc_release(struct mt6323_auxadc *auxadc,
> +				 unsigned long channel)
> +{
> +	struct regmap *map = auxadc->regmap;
> +	int ret;
> +
> +	ret = regmap_clear_bits(map, MT6323_AUXADC_CON22, BIT(channel));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_clear_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);

Similarly, if mt6323_auxadc_release() fails to clear the channel bit, it
returns early and skips disabling VBUF_EN. Should these error paths ensure
VBUF_EN is disabled before returning?

> +}
[ ... ]
> +static int mt6323_auxadc_init(struct mt6323_auxadc *auxadc)
> +{
> +	struct regmap *map = auxadc->regmap;
> +	int ret;
> +
> +	ret = regmap_set_bits(map, MT6323_STRUP_CON10,
> +			      AUXADC_STRUP_CON10_RSTB_SW |
> +			      AUXADC_STRUP_CON10_RSTB_SEL);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(map, MT6323_TOP_CKPDN2, AUXADC_TOP_CKPDN2_CTL_CK);

[Severity: High]
Does setting this bit actually disable the AUXADC clock?

In MediaTek PMICs, CKPDN typically stands for Clock Power Down. If setting
this bit to 1 disables the clock, and it is never cleared, wouldn't the
clock remain powered down indefinitely and cause all subsequent ADC reads
to time out or hang the hardware?

Should this use regmap_clear_bits() instead to enable the clock?

> +	if (ret)
> +		return ret;
[ ... ]
> +static int mt6323_auxadc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mt6323_auxadc *auxadc;
> +	struct regmap *regmap;
> +	struct iio_dev *iio;
> +	int ret;
> +
> +	regmap = dev_get_regmap(dev->parent->parent, NULL);

[Severity: High]
Can dev->parent or dev->parent->parent be NULL here if the device is
instantiated outside of its expected MFD hierarchy?

For example, if instantiated via a misconfigured Device Tree overlay or
direct sysfs binding, dev->parent could be &platform_bus, which has a NULL
parent.

Since dev_get_regmap() invokes devres_find() which unconditionally
dereferences the device pointer, would passing a NULL parent cause a
kernel panic during probe? Should the parent chain be validated before
dereferencing?

> +	if (!regmap)
> +		return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
[ ... ]

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