Re: [PATCH v4 2/5] mfd: axp20x: Add support for AXP318W PMIC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The system power-off handler `axp20x_power_off()` is missing a switch case for `AXP318_ID`, causing it to write the power-off command to a regulator control register.
- [Medium] The newly added `axp318_cells` array uses a hardcoded cell ID instead of `PLATFORM_DEVID_AUTO`, violating MFD subsystem guidelines.
--

--- Patch [2]: [PATCH v4 2/5] mfd: axp20x: Add support for AXP318W PMIC ---
commit ffbc50262db7d7fef4ebd802fb44cd11951bf0bc
Author: Andre Przywara <andre.przywara@arm.com>

mfd: axp20x: Add support for AXP318W PMIC

This commit adds MFD core support for the X-Powers AXP318W PMIC. It defines
the regmap configurations, IRQs, and child device cells for the regulator.

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 679364189ea58..c8aeebd01bbcd 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
[ ... ]
> @@ -1061,6 +1129,10 @@ static 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]
Does this hardcoded cell ID of 1 violate the MFD subsystem guidelines?

If multiple AXP devices with hardcoded IDs of 1 (such as the AXP313a and
AXP318W) are instantiated on the same system, could they both attempt to
register a platform device named axp20x-regulator.1, resulting in a fatal
-EEXIST failure?

Should this use PLATFORM_DEVID_AUTO instead?

> +};
> +
>  static struct mfd_cell axp717_cells[] = {
>  	MFD_CELL_NAME("axp20x-regulator"),
[ ... ]
> @@ -1313,6 +1385,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
>  		axp20x->regmap_cfg = &axp313a_regmap_config;
>  		axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
>  		break;
> +	case AXP318_ID:
> +		axp20x->nr_cells = ARRAY_SIZE(axp318_cells);
> +		axp20x->cells = axp318_cells;
> +		axp20x->regmap_cfg = &axp318_regmap_config;
> +		axp20x->regmap_irq_chip = &axp318_regmap_irq_chip;
> +		break;

[Severity: High]
Is AXP318_ID missing from the switch statement in axp20x_power_off()?

When the system attempts to power off, axp20x_power_off() is called. If
AXP318_ID is not handled in the switch statement there, will it fall through
to the default case and select AXP20X_OFF_CTRL (0x32) instead of the correct
AXP318_SHUTDOWN_CTRL (0x52)?

Could this failure to shut down the PMIC corrupt the state of the CLDO4
regulator by writing the power-off command to AXP318_CLDO4_CONTROL (which
is also 0x32)?

>  	case AXP323_ID:
>  		axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-axp318-regulator-v4-0-50aff97084dd@baylibre.com?part=2