Re: [PATCH v5 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 is unreachable dead code because it is not instantiated by the MFD core.
- [High] Hardware clock is incorrectly powered down during initialization.
- [Medium] Hardware resource leak (power) on PMIC error paths.
--

--- Patch [2]: [PATCH v5 2/4] iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver ---
commit bd09ecf1750ddd84020e392e718056274b3e15a4
Author: Roman Vivchar <rva333@protonmail.com>

iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver

This commit adds a new driver for the AUXADC found in the MediaTek mt6323
PMIC. It provides support for reading various channels including battery
and charger voltages, temperature, and current sensing.

> diff --git a/drivers/iio/adc/mt6323-auxadc.c b/drivers/iio/adc/mt6323-auxadc.c
> new file mode 100644
> index 0000000000000..c450fb6f09cb5
> --- /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, could this leave
AUXADC_CON11_VBUF_EN permanently enabled?

Since mt6323_auxadc_read_raw() immediately returns without calling
mt6323_auxadc_release() when this function fails, it seems this might cause
a hardware power leak on the error path.

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

[Severity: Medium]
Similarly, if clearing MT6323_AUXADC_CON22 fails, will this skip clearing
AUXADC_CON11_VBUF_EN?

> +}
[ ... ]
> +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 the CKPDN (Clock Power Down) bit disable the clock instead of
enabling it? 

In MediaTek PMIC hardware, CKPDN registers usually represent Clock Power
Down, meaning writing a 1 powers down the clock. Should this use
regmap_clear_bits() instead?

> +	if (ret)
> +		return ret;
[ ... ]
> +static int mt6323_auxadc_probe(struct platform_device *pdev)
> +{

[Severity: High]
Will this probe function ever be executed?

The MT6323 is an MFD device handled by drivers/mfd/mt6397-core.c, which
instantiates its children strictly from the hardcoded mt6323_devs array
using mfd_add_devices(). 

Since the mt6323-auxadc driver was not added to that array, and the
mediatek,mt6323 PMIC device tree binding does not use simple-mfd, it appears
this driver is structurally unreachable unless the MFD core is also modified.

> +	struct device *dev = &pdev->dev;
> +	struct mt6323_auxadc *auxadc;
> +	struct regmap *regmap;
> +	struct iio_dev *iio;
> +	int ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-mt6323-adc-v5-0-d11b8332a735@protonmail.com?part=2