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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The probe function unconditionally dereferences `dev->parent` and `dev->parent->parent`, which will cause a kernel panic if the parent device is NULL.
- [High] The driver incorrectly uses the logical device tree channel index instead of the physical ADC channel index when setting the hardware request bitmask.
- [Medium] The AUXADC voltage buffer (`VBUF`) is left permanently enabled on error paths, leading to a resource/power leak.
- [Medium] Incorrect IIO channel types are hardcoded as `IIO_VOLTAGE` for temperature and current channels, violating IIO ABI.
--

commit 5d793b85b8942cff4538a9fd7c909fd225402286
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.

Tested-by: Ben Grisdale <bengris32@protonmail.ch> # Amazon Echo Dot (2nd Generation)
Signed-off-by: Roman Vivchar <rva333@protonmail.com>

[ ... ]
> diff --git a/drivers/iio/adc/mt6323-auxadc.c b/drivers/iio/adc/mt6323-auxadc.c
> new file mode 100644
> index 0000000000000..f2cef989d3ce7
> --- /dev/null
> +++ b/drivers/iio/adc/mt6323-auxadc.c
[ ... ]
> +#define AUXADC_DEFAULT_OSR		3
> +
> +#define MTK_PMIC_IIO_CHAN(_name, _chan, _addr)                  \
> +{                                                               \
> +	.type = IIO_VOLTAGE,                                    \

[Severity: Medium]
Does hardcoding IIO_VOLTAGE here break the IIO ABI for temperature and
current channels?

It looks like bat_temp, chip_temp, and isense will be exported as
in_voltage_*_raw instead of IIO_TEMP and IIO_CURRENT.

> +	.indexed = 1,                                           \
> +	.channel = _chan,                                       \
[ ... ]
> +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: High]
Does BIT(channel) request the correct physical hardware channel here?

The channel parameter receives chan->channel, which is a logical sequential
index (e.g., MT6323_AUXADC_BATON2 = 0), but the hardware request register
appears to expect bits corresponding to the physical ADC channels. For
instance, setting BIT(0) for BATON2 would request ADC0 from the hardware,
but the driver will later poll the ADC6 data register.

[Severity: Medium]
If regmap_set_bits() fails to set MT6323_AUXADC_CON22, does this leave the
AUXADC voltage buffer (VBUF) permanently enabled? 

Since the caller aborts on error without calling mt6323_auxadc_release(),
the AUXADC_CON11_VBUF_EN bit is never cleared.

> +}
> +
> +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;

[Severity: Medium]
If clearing MT6323_AUXADC_CON22 fails here, does it skip disabling the
voltage buffer? 

Returning early would leave AUXADC_CON11_VBUF_EN enabled.

> +
> +	return regmap_clear_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);
> +}
[ ... ]
> +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 this cause a null pointer dereference if the driver is probed directly
from a device tree node without an MFD parent?

Because the driver defines an of_match_table, it could be instantiated where
dev->parent is null. Even if dev->parent is valid but dev->parent->parent is
null, passing null to dev_get_regmap() will cause a kernel panic when it
attempts to lock devres.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-mt6323-adc-v2-0-aa93a22309f9@protonmail.com?part=2