Re: [PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Lee Jones <lee@kernel.org>]

On Wed, 25 Jun 2025, Alex Elder wrote:

> On 6/25/25 3:21 AM, Lee Jones wrote:
> > On Fri, 20 Jun 2025, Alex Elder wrote:
> > 
> > > On 6/19/25 9:40 AM, Lee Jones wrote:
> > > > On Fri, 13 Jun 2025, Alex Elder wrote:
> > > > 
> > > > > Add support for SpacemiT PMICs. Initially only the P1 PMIC is supported
> > > > > but the driver is structured to allow support for others to be added.
> > > > > 
> > > > > The P1 PMIC is controlled by I2C, and is normally implemented with the
> > > > > SpacemiT K1 SoC.  This PMIC provides six buck converters and 12 LDO
> > > > 
> > > > six or 12.  Please pick a format and remain consistent.
> > > 
> > > "Numbers smaller than ten should be spelled out."
> > 
> > Never heard of that before Googling it.  Formal writing is odd. :)
> > 
> > > But I'll use 6 and 12.
> 
> . . .
> 
> > > > > +/* The name field defines the *driver* name that should bind to the device */
> > > > 
> > > > This comment is superfluous.
> > > 
> > > I'll delete it.
> > > 
> > > I was expecting the driver to recognize the device, not
> > > the device specifying what driver to use, but I guess
> > > I'm used to the DT model.
> > 
> > Even in DT, the *driver* compatible is specified.
> > 
> >    .driver.of_match_table->compatible
> 
> I guess I just interpret that differently than you do.  I think
> of the device compatible string as saying "this is what I am,"
> much like a VID/PID in USB or PCI.
> 
> Then the driver's of_match table says "if a device claims to
> be compatible with any of these it should be bound to me."
> 
> Meanwhile, the MFD device method has the device (cell) saying
> "I should be bound to the driver having this name."

In all cases that I'm aware of (platform code, DT, ACPI, etc), and as
far back as I can remember, the platform devices specify some predefined
data (IDs or strings) that is associated with (hard-coded directly into
the driver in fact) the driver it wishes to be bound to.  This
pre-defined identifier is stored in the driver's data structure:

struct device_driver {
        const char              	*name;
        const struct of_device_id       *of_match_table;
        const struct acpi_device_id     *acpi_match_table;
};

All of these are statically hard-coded items which a device can specify
in order to be bound to the driver.

> > > > > +	/* We currently have no need for a device-specific structure */
> > > > 
> > > > Then why are we adding one?
> > > 
> > > I don't understand, but it might be moot once I add support
> > > for another (sub)device.
> > 
> > There are 2 rules in play here:
> > 
> >    - Only add what you need, when you need it
> >    - MFDs must contain more than 1 device
> > 
> > ... and you're right.  The second rule moots the first here.
> 
> What the comment meant to say is "we have no need to kzalloc()
> any special structure here" as most drivers do. Simply adding
> the set of MFDs defined by the cells is enough.  The same is
> true in "simple-mfd-i2c.c".

I see.  Driver data is not compulsory.  There are plenty of drivers
which refrain from storing data for the child to make use of.

> But this entire source file is gone now, so it's moot for that
> reason.
> 
> . . .
> 
> > > > > +static const struct of_device_id spacemit_pmic_match[] = {
> > > > > +	{
> > > > > +		.compatible	= "spacemit,p1",
> > > > > +		.data		= &p1_pmic_data,
> > > > 
> > > > Ah, now I see.
> > > > 
> > > > We do not allow one data from registration mechanism (MFD) to be piped
> > > > through another (OF).  If you have to match platform data to device (you
> > > > don't), then pass through identifiers and match on those in a switch()
> > > > statement instead.
> > > 
> > > I haven't done an MFD driver before and it took some time
> > > to get this working.  I'll tell you what led me to it.
> > > 
> > > I used code posted by Troy Mitchell (plus downstream) as a
> > > starting point.
> > >    https://lore.kernel.org/lkml/20241230-k1-p1-v1-0-aa4e02b9f993@gmail.com/
> > > 
> > > Krzysztof Kozlowski made this comment on Troy's DT binding:
> > >    Drop compatible, regulators are not re-usable blocks.
> > > 
> > > So my goal was to have the PMIC regulators get bound to a
> > > driver without specifying a DT compatible string, and I
> > > found this worked.
> > > 
> > > You say I don't need to match platform data to device, but
> > > if I did I would pass through identifiers.  Can you refer
> > > me to an example of code that correctly does what I should
> > > be doing instead?
> > 
> > git grep -A5 compatible -- drivers/mfd | grep -E "\.data = .*[A-Z]+"
> > 
> > Those identifiers are usually matched in a swtich() statement.
> 
> OK now I see what you you're talking about.  But these
> compatible strings (and data) are for the PMIC.  I was
> trying to avoid using compatible strings for the *regulators*,
> based on Krzysztof's comment.  And in the process I learned
> that the MFD cell needs to specify the name of a driver,
> not a compatible string.

That's correct.  The compatible attribute is voluntary.

> > > One other comment/question:
> > >    This driver is structured as if it could support a different
> > >    PMIC (in addition to P1).  Should I *not* do that, and simply
> > >    make a source file hard-coded for this one PMIC?
> > 
> > This comes back to the "add only what you need, when you need it" rule.
> 
> Yes, and I agree with that rule.  Thanks for your clarifications.
> 
> Using simple-mfd-i2c.c is much better.  I was surprised (and I guess
> pleased) to see that it was almost *identical* to what I came up with.

*thumbs-up*

-- 
Lee Jones [李琼斯]