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

On sam. 12 janv. 2008 at 23:40, [email protected] wrote:

> 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.

Funny sashiko did not pick up on this when v6 was first sent. This is an
ordering problem. The regulator part should be applied before the mfd
one.

I could reorder here but there is not much point since the 2 changes will
land in different subsystems but it does bring a valid question:

Lee, Mark, are you OK with picking the patches indended for your
subsystem or would you prefer that I split the series, regulator on one
side, mfd on the other (with a dep on the regulator one, of course) ?

> - [Medium] The newly added `axp318_cells` array uses a hard-coded cell ID of 1 instead of using `PLATFORM_DEVID_AUTO`.
> --

... Again, already explained this in previous version and the cover letter.

>
> --- 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"),

-- 
Jerome