Re: [PATCH RESEND v6 4/5] mfd: axp20x: Add support for AXP318W PMIC

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The corresponding regulator driver lacks support for `AXP318_ID`, causing the `axp20x-regulator` child device probe to fail.
- [Medium] The newly added `axp318_cells` array uses a hard-coded cell ID of 1 instead of using `PLATFORM_DEVID_AUTO`.
--

--- Patch [4]: [PATCH RESEND v6 4/5] mfd: axp20x: Add support for AXP318W PMIC ---
commit 5a33005ba66225f7df0e6747b0fcb9263e977f13
Author: Andre Przywara <[email protected]>

mfd: axp20x: Add support for AXP318W PMIC

This commit introduces MFD core and I2C support for the X-Powers AXP318W
PMIC. It defines regmap properties, interrupt maps, and adds the regulator
child device.

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 84839d4aa1ca5..ea00c0cdce58c 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c

[ ... ]

> @@ -1061,6 +1129,10 @@ static const struct mfd_cell axp313a_cells[] = {
>  	MFD_CELL_RES("axp313a-pek", axp313a_pek_resources),
>  };
>  
> +static const struct mfd_cell axp318_cells[] = {
> +	MFD_CELL_BASIC("axp20x-regulator", NULL, NULL, 0, 1),

[Severity: Medium]
Should this cell use PLATFORM_DEVID_AUTO instead of a hard-coded ID of 1?

Hard-coding cell IDs or mapping them to fixed instance numbers can lead to
device naming collisions in sysfs and driver load failures if multiple PMICs
defining the same cell name exist on the same board.

[Severity: High]
Will this child device successfully probe, or are the regulator driver
changes missing from this patch series?

When the MFD core registers the "axp20x-regulator" cell, the regulator driver's
axp20x_regulator_probe() is invoked. It checks the axp20x->variant, but since
AXP318_ID is not handled in the regulator driver:

drivers/regulator/axp20x-regulator.c:axp20x_regulator_probe() {
    ...
    switch (axp20x->variant) {
    ...
    default:
        dev_err(&pdev->dev, "Unsupported AXP variant: %d\n",
            axp20x->variant);
        return -EINVAL;
    }
}

It appears this will fall through to the default case, print an error,
and return -EINVAL unconditionally.

> +};
> +
>  static const struct mfd_cell axp717_cells[] = {
>  	MFD_CELL_NAME("axp20x-regulator"),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4