Re: [PATCH v5 3/4] mfd: mt6397-core: add mt6323 AUXADC support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The AUXADC clock is explicitly powered down instead of being enabled during initialization, resulting in a dead IP block.
- [Medium] The child driver directly accesses the grandparent's regmap instead of retrieving it via standard MFD parent data accessors, violating API contracts and risking a NULL pointer dereference.
- [Medium] The AUXADC request and release functions fail to clean up partially acquired hardware state if an intermediate register write fails.
- [Low] The commit message description is not capitalized, violating MFD subsystem guidelines.
--

--- Patch [3]: [PATCH v5 3/4] mfd: mt6397-core: add mt6323 AUXADC support ---
commit 7593019cba73d4a6a4b6233e95fc0594442ed409
Author: Roman Vivchar <rva333@protonmail.com>

mfd: mt6397-core: add mt6323 AUXADC support
    
The mt6323 PMIC includes an AUXADC. Register the AUXADC in the mt6323
devices array to allow the corresponding driver to probe using compatible
string.

> mfd: mt6397-core: add mt6323 AUXADC support

[Severity: Low]
This isn't a bug, but the commit message description is not capitalized,
violating MFD subsystem guidelines.

> 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]
Could this leave the voltage buffer enabled indefinitely if setting the
channel bit fails? There appears to be no rollback for AUXADC_CON11_VBUF_EN
when the second register write returns an error.

> +}
> +
> +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]
If clearing the channel bit fails here, will this prevent AUXADC_CON11_VBUF_EN
from being cleared and leave the hardware in an inconsistent state?

[ ... ]

> +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]
Is this actually powering down the AUXADC clock instead of enabling it?

In MediaTek PMICs, CKPDN registers control clock power down. Setting this
bit to 1 explicitly powers down the clock, which would render the AUXADC
IP block non-functional. Should this be using regmap_clear_bits() instead?

> +	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: Medium]
Is it safe to directly access dev->parent->parent here?

If the device tree topology changes or the parent device does not have a
parent, dev->parent->parent will evaluate to NULL. Calling dev_get_regmap()
with a NULL device would then trigger a NULL pointer dereference.

The MFD subsystem guidelines state that child drivers must retrieve parent data
using standard APIs like dev_get_drvdata(pdev->dev.parent) instead of bypassing
the parent to access the grandparent directly.

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

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