Re: [PATCH 4/6] mfd: sprd-sc27xx: Switch to devm_mfd_add_devices()

[Lee Jones <lee@kernel.org>]

> Could you clarify what should be changed?

Sure.

> On Mon, Mar 09, 2026 at 06:58:56PM +0000, Lee Jones wrote:
> > On Sun, 22 Feb 2026, Otto Pflüger wrote:
> > 
> > > To allow instantiating subdevices such as the regulator and poweroff
> > > devices that do not have corresponding device tree nodes with a
> > > "compatible" property, use devm_mfd_add_devices() with MFD cells instead
> > > of devm_of_platform_populate(). Since different PMICs in the SC27xx
> > > series contain different components, use separate MFD cell tables for
> > > each PMIC model. Define cells for all components that have upstream
> > > drivers at this point.
> > 
> > We're not passing one device registration API's data (MFD)
> > through another (Device Tree).
> > 
> > Pass an identifier through and match on that instead.
> > 
> > Look at how all of the other drivers in MFD do it.
> >
> > [...]
> > > +static const struct mfd_cell sc2730_devices[] = {
> > > +	MFD_CELL_OF("sc2730-adc", NULL, NULL, 0, 0, "sprd,sc2730-adc"),
> > > +	MFD_CELL_OF("sc2730-bltc", NULL, NULL, 0, 0, "sprd,sc2730-bltc"),
> > > +	MFD_CELL_OF("sc2730-efuse", NULL, NULL, 0, 0, "sprd,sc2730-efuse"),
> > > +	MFD_CELL_OF("sc2730-eic", NULL, NULL, 0, 0, "sprd,sc2730-eic"),
> > > +	MFD_CELL_OF("sc2730-fgu", NULL, NULL, 0, 0, "sprd,sc2730-fgu"),
> > > +	MFD_CELL_OF("sc2730-rtc", NULL, NULL, 0, 0, "sprd,sc2730-rtc"),
> > > +	MFD_CELL_OF("sc2730-vibrator", NULL, NULL, 0, 0, "sprd,sc2730-vibrator"),
> > > +};
> > > +
> > > +static const struct mfd_cell sc2731_devices[] = {
> > > +	MFD_CELL_OF("sc2731-adc", NULL, NULL, 0, 0, "sprd,sc2731-adc"),
> > > +	MFD_CELL_OF("sc2731-bltc", NULL, NULL, 0, 0, "sprd,sc2731-bltc"),
> > > +	MFD_CELL_OF("sc2731-charger", NULL, NULL, 0, 0, "sprd,sc2731-charger"),
> > > +	MFD_CELL_OF("sc2731-efuse", NULL, NULL, 0, 0, "sprd,sc2731-efuse"),
> > > +	MFD_CELL_OF("sc2731-eic", NULL, NULL, 0, 0, "sprd,sc2731-eic"),
> > > +	MFD_CELL_OF("sc2731-fgu", NULL, NULL, 0, 0, "sprd,sc2731-fgu"),
> > > +	MFD_CELL_NAME("sc2731-poweroff"),
> > > +	MFD_CELL_NAME("sc2731-regulator"),
> > > +	MFD_CELL_OF("sc2731-rtc", NULL, NULL, 0, 0, "sprd,sc2731-rtc"),
> > > +	MFD_CELL_OF("sc2731-vibrator", NULL, NULL, 0, 0, "sprd,sc2731-vibrator"),
> > >  };
> 
> Assuming that these tables are the "registration API's data", I don't
> see where it is being passed through the device tree. The device tree
> contains nodes for some of these MFD components, and I've listed their
> compatibles here so that the MFD core finds these nodes and registers
> them with the corresponding devices (which was previously done
> automatically by devm_of_platform_populate).
> 
> > >  
> > >  /*
> > > @@ -59,12 +84,16 @@ static const struct sprd_pmic_data sc2730_data = {
> > >  	.irq_base = SPRD_SC2730_IRQ_BASE,
> > >  	.num_irqs = SPRD_SC2730_IRQ_NUMS,
> > >  	.charger_det = SPRD_SC2730_CHG_DET,
> > > +	.cells = sc2730_devices,
> > > +	.num_cells = ARRAY_SIZE(sc2730_devices),

Remove these from here.

Either replace them with an ID that you can match on or stop passing
'sc2730_data' through .data and pass an ID through there instead.  Then
choose 'sc2730_data' and 'sc2730_devices' in an switch() statement
instead, just like the vast majority of existing MFD drivers do.

> > >  };
> > >  
> > >  static const struct sprd_pmic_data sc2731_data = {
> > >  	.irq_base = SPRD_SC2731_IRQ_BASE,
> > >  	.num_irqs = SPRD_SC2731_IRQ_NUMS,
> > >  	.charger_det = SPRD_SC2731_CHG_DET,
> > > +	.cells = sc2731_devices,
> > > +	.num_cells = ARRAY_SIZE(sc2731_devices),
> > >  };
> 
> Here I am simply referencing the tables above in the device-specific
> MFD data. These structs containing device-specific data already exist,
> they are private to the MFD driver, and I wouldn't consider them part
> of the device tree.
> 
> I've looked at mt6397-core.c and it seems to be doing the exact same
> thing with its "struct chip_data".

That was a momentary oversight.  It's also passing a driver-level
call-back which I despise.  However, past mistakes are not good
justifications for new ones.

> Some other drivers use a numeric ID
> for this purpose, but how would that be different from a pointer as long
> as it identifies the same data within the MFD driver?

The point is that sc2731_data->cells would be passed through the Device
Tree's .data attribute, which is not allowed.

-- 
Lee Jones [李琼斯]