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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unchecked pointer dereference of `dev->parent` and `dev->parent->parent` when fetching the PMIC wrapper regmap.
- [High] Channel interference and power leak due to missing teardown of active channels and voltage buffer.
--

commit 3ea46406f2075e002ef31a93fd209e78c86e9790
Author: Roman Vivchar <rva333@protonmail.com>

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.

> diff --git a/drivers/iio/adc/mt6323-auxadc.c b/drivers/iio/adc/mt6323-auxadc.c
> new file mode 100644
> index 0000000000000..da6c11a5079c0
> --- /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;
> +
> +	ret = regmap_clear_bits(map, MT6323_AUXADC_CON22, BIT(channel));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_set_bits(map, MT6323_AUXADC_CON22, BIT(channel));
> +}

[Severity: High]
Does this leave previously requested channels active in MT6323_AUXADC_CON22?

Since it only clears and sets the bit for the currently requested channel,
multiple channel bits could accumulate in the register over time. Could this
cause hardware contention or mixed readings when channels are used
sequentially?

[ ... ]
> +static int mt6323_auxadc_read_raw(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan,
> +				  int *val, int *val2, long mask)
> +{
> +	struct mt6323_auxadc *auxadc = iio_priv(indio_dev);
> +	int ret, mult;
> +
> +	switch (mask) {
[ ... ]
> +	case IIO_CHAN_INFO_RAW:
> +		scoped_guard(mutex, &auxadc->lock) {
> +			ret = mt6323_auxadc_prepare_channel(auxadc);
> +			if (ret)
> +				return ret;
> +
> +			ret = mt6323_auxadc_request(auxadc, chan->channel);
> +			if (ret)
> +				return ret;
> +
> +			/* Hardware limitation: the AUXADC needs a delay to become ready. */
> +			fsleep(300);
> +
> +			ret = mt6323_auxadc_read(auxadc, chan, val);
> +			if (ret)
> +				return ret;
> +		}
> +		return IIO_VAL_INT;

[Severity: High]
Does this leak power by leaving AUXADC_CON11_VBUF_EN enabled after the
reading finishes?

It appears the voltage buffer is enabled in mt6323_auxadc_request() but never
disabled once the conversion completes and exits here. Also, the active
channel is never cleared.

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

[Severity: High]
Is it possible for dev->parent->parent to be NULL here?

If this driver is instantiated directly via a Device Tree node (because of
its of_match_table) rather than as an MFD child, dev->parent would evaluate
to the platform_bus, making dev->parent->parent NULL.

Passing NULL to dev_get_regmap() causes an immediate kernel panic in
devres_find() when it attempts to acquire a spinlock. Should we verify the
parent hierarchy before dereferencing it?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-mt6323-adc-v1-0-68ec737508ee@protonmail.com?part=2