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

[Jerome Brunet]

On jeu. 06 août 2026 at 13:09, Lee Jones <lee@xxxxxxxxxx> wrote:

> /* Sashiko Automation: Issues Found (2 Findings) */
>
> Please explain or fix these Sashiko reviews.

I already did (several times actually) but for some reason, sashiko was
only replying to the DT folks and me (something I did not see until now)
... so when I replied, it did not get to you.

>
> On Fri, 17 Jul 2026, Jerome Brunet wrote:
>

[...]

>>  
>> +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?

Already replied to this. As far as I can tell PLATFORM_DEVID_AUTO would
not go there. The driver is indeed registered PLATFORM_DEVID_NONE here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/axp20x.c?h=v7.2-rc6#n1452

But there is nothing new about this. If this is an issue, it is
pre-existing one. 

>
>
>> +};
>> +
>>  static struct mfd_cell axp717_cells[] = {
>>  	MFD_CELL_NAME("axp20x-regulator"),
>>  	MFD_CELL_RES("axp20x-pek", axp717_pek_resources),
>> @@ -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)?

That was fixed in v5

FYI the last version is v6 here:
https://lore.kernel.org/all/20260731-axp318-regulator-v6-0-b4e745552a35@xxxxxxxxxxxx/

Sashiko continued to complain about the platform id. I did not fix
because because it is unrelated to the series. I'm happy to replace
PLATFORM_DEVID_NONE by PLATFORM_DEVID_AUTO but I do not really why it
was set like that in the first place and if the change would be harmless
to all existing users.

It also commented on the irq controller but no, it is not required. I've checked.