Re: [PATCH v3 2/4] mfd: axp20x: Add support for AXP318W PMIC

[Jerome Brunet <jbrunet@baylibre.com>]

On ven. 17 juil. 2026 at 09:02, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] The newly added I2C device ID `axp318w` fails to use named initializers and omits the `.driver_data` field, resulting in it defaulting to `0` (`AXP152_ID`).
> - [Medium] The MFD cell and resources for the power key (`axp20x-pek`) are completely omitted from `axp318_cells`, despite the commit message claiming to support it and IRQs being defined.
> - [Low] The `axp318_cells` array uses a hard-coded ID of `1` for the regulator cell, which violates MFD subsystem guidelines and risks sysfs naming collisions.
> --
>
> --- Patch [2]: [PATCH v3 2/4] mfd: axp20x: Add support for AXP318W PMIC ---
> commit c0a61d967fff87b52d26e4cb329e69e10684fb89
> Author: Andre Przywara <andre.przywara@arm.com>
>
> mfd: axp20x: Add support for AXP318W PMIC
>
> This patch adds support for the AXP318W PMIC connected via I2C. It sets up
> the regmap configurations, volatile ranges, interrupts, and registers the
> MFD cells for the regulator devices.
>
>> Describe the regmap and the MFD bits, along with the registers exposed
>> via I2C only. This covers the regulator, interrupts and power key
>> devices for now.
>
> [Severity: Medium]
> Is the power key device missing from the MFD cells array in this patch? The
> commit message mentions it is covered, but there are no power key resources
> or cells defined for the AXP318W.

Had fixed that but rebasing multiple time a lost it somehow ... will fix.

>
>> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
>> index 5c93136f977e7..4e4ebfc78525c 100644
>> --- a/drivers/mfd/axp20x-i2c.c
>> +++ b/drivers/mfd/axp20x-i2c.c
>> @@ -83,6 +84,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = {
>>  	{ "axp221" },
>>  	{ "axp223" },
>>  	{ "axp313a" },
>> +	{ "axp318w" },
>
> [Severity: Medium]
> Does this need named initializers to provide the .driver_data payload?
>
> Without setting .driver_data like the of_device_id table does, won't this
> default to 0, which is AXP152_ID? If this device is instantiated via legacy
> I2C methods, device_get_match_data() might return AXP152_ID instead,
> causing the driver to mistakenly configure it as an AXP152 variant.

I don't think this driver as ever been intanciated through the legacy
i2c interface or was meant to be. While this may need to looked at for
correctness, this is not something new introduced by this change

>
>> 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: Low]
> Should this use PLATFORM_DEVID_AUTO instead of a hard-coded ID of 1?
>
> Assigning a hardcoded ID can lead to device naming collisions in sysfs if
> multiple instances of the device are present.

PLATFORM_DEVID_AUTO does not go in here as far as can tell.

The driver uses PLATFORM_DEVID_NONE while registering the device.
I honestly do not know the reason, if there is one. If it is problem, it
is not related to this specific change. I don't mind submitting a change
for this but
1) it should not be mixed with this series
2) I can't test the other devices

>
>> +};
>> +

-- 
Jerome