Re: [PATCH v3 03/16] mfd: mfd-core: match device tree node against reg property

[Michael Walle <michael@walle.cc>]

Am 2020-05-26 09:24, schrieb Lee Jones:
> On Mon, 25 May 2020, Michael Walle wrote:
> 
>> Am 2020-05-15 12:28, schrieb Lee Jones:
>> > On Thu, 30 Apr 2020, Michael Walle wrote:
>> >
>> > > Hi Lee,
>> > >
>> > > Am 2020-04-23 19:45, schrieb Michael Walle:
>> > > > There might be multiple children with the device tree compatible, for
>> > > > example if a MFD has multiple instances of the same function. In this
>> > > > case only the first is matched and the other children get a wrong
>> > > > of_node reference.
>> > > > Add a new option to match also against the unit address of the child
>> > > > node. Additonally, a new helper OF_MFD_CELL_REG is added.
>> > >
>> > >
>> > > Do you think this is feasible? I guess this is the biggest uncertainty
>> > > for me at the moment in this patch series.
>> >
>> > I think it sounds fine in principle.  So long as it doesn't change the
>> > existing behaviour when of_reg isn't set.
>> >
>> > > > Signed-off-by: Michael Walle <michael@walle.cc>
>> > > > ---
>> > > >  drivers/mfd/mfd-core.c   | 29 ++++++++++++++++++++---------
>> > > >  include/linux/mfd/core.h | 26 ++++++++++++++++++++------
>> > > >  2 files changed, 40 insertions(+), 15 deletions(-)
> 
> [...]
> 
>> > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
>> > > > index d01d1299e49d..c2c0ad6b14f3 100644
>> > > > --- a/include/linux/mfd/core.h
>> > > > +++ b/include/linux/mfd/core.h
>> > > > @@ -13,8 +13,11 @@
>> > > >  #include <linux/platform_device.h>
>> > > >
>> > > >  #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
>> > > > +#define MFD_OF_REG_VALID	BIT(31)
>> >
>> > What about 64bit platforms?
>> 
>> The idea was to have this as a logical number. I.e. for now you may 
>> only
>> have one subdevice per unique compatible string. In fact, if you have 
>> a
>> look at the ab8500.c, there are multiple "stericsson,ab8500-pwm"
>> subdevices. But there is only one DT node for all three of it. I guess
>> this works as long as you don't use phandles to reference the pwm node
>> in the device tree. Or you don't want to use device tree properties
>> per subdevice (for example the "timeout-sec" of a watchdog device).
>> 
>> So to circumvent this, I thought of having the unit-address (and thus
>> the "reg" property) to differentiate between multiple subdevices. Now
>> there is one special case for me: this board management controller
>> might be upgradable and it might change internally. Thus I came up
>> with that logical numbering of subdevices. Rob doesn't seem to be a
>> fan of that, though. Therefore, having bit 31 as a valid indicator
>> leaves you with 2^31 logical devices, which should be enough ;)
>> 
>> Rob proposed to have the internal offset as the unit-address. But
>> in that case I can also use devm_of_platform_populate() and don't
>> need the OF_MFD_CELL_REG; I'd just parse the reg offset in each
>> individual subdevice driver. But like I said, I wanted to keep the
>> internal offsets out of the device tree.
> 
> Oh, I see what you're doing.
> 
> So you're adding an arbitrary ID to the device's reg property in DT?

Yes.

> How is this not a hack?

Well IMHO this is not more or less a hack as the current of_node
handling of MFD devices, which happens to work only because there
is only one device per compatible string (or it doesn't really work,
like in the stericsson,ab8500-pwm case). The of_node is assigned
according to the compatible string, just like in my case, only that
I have two subdevices with the same compatible string.

> Why don't you use the full address for identification?

Like I said, in the long term I would like to have support for
different versions of the board management controller without having
to change the device tree and have device tree bindings for the
subdevices at the same time. But it seems, that this is not possible
and I guess I have to bite the bullet and may need to provide another
device tree if the controller might be updated.

-michael