Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring <robh@xxxxxxxxxx>]

On Tue, Sep 06, 2022 at 03:54:50PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 06, 2022 at 04:25:49PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 6 Sep 2022 22:53:47 +0900
> > > From: Hector Martin <marcan@xxxxxxxxx>
> > > 
> > > I agree that this is something to think about (I was about to reply on
> > > the subject).
> > > 
> > > I can think of two ways: using `reg` for the key name, but that feels
> > > icky since it's ASCII and not *really* a register number/address, or
> > > something like this:
> > > 
> > > gpio@0 {
> > > 	apple,smc-key-base = "gP00";
> > > 	...
> > > }
> > > 
> > > gpio@1 {
> > > 	apple,smc-key-base = "gp00";
> > > 	...
> > > }
> > 
> > This would still require us to add a (one-cell) "reg" property and
> > would require adding the appropriate "#address-cells" and
> > "#size-cells" properties to the SMC node.
> 
> Yes, and at that point, as I suggested, it probably would be better
> to use:
> 
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	gpio@67503030 {
> 		reg = <0x67503030>;
> 	};
> 
> 	gpio@67703030 {
> 		reg = <0x67703030>;
> 	};
> 
> Then the "reg" has a meaning that is directly related to the SMC.

That's certainly odd, but if that's how it is addressed, then okay I 
suppose.

> 
> > > But this ties back to the device enumeration too, since right now the DT
> > > does not drive that (we'd have to add the subdevice to the mfd subdevice
> > > list somehow anyway, if we don't switch to compatibles).
> > > 
> > > I'd love to hear Rob's opinion on this one, and also whether the
> > > existing Linux and OpenBSD code would currently find gpio@0 {} instead
> > > of gpio {} for backwards compat.

Node names are generally not considered ABI except when they are. :)

Generally, core code doesn't care. Specific bindings with defined child 
nodes often do, but finding nodes by compatible is strongly preferred. 
Linux drivers can bind by node name (w/o unit-address), but that's 
really only ever done on ancient h/w.

> > 
> > The OpenBSD driver does a lookup by name and the "@0" is part of that
> > name.  So that would break backwards compat.
> 
> Oh, that's annoying - and is a different behaviour to Linux.
> 
> On Linux, we only look at the node name up to the @ when matching (see
> of_node_name_eq() in drivers/of/base.c, so it doesn't matter to Linux
> what follows the @ when you try to look up a node named "gpio" - you'll
> find gpio@anythingyoulike.
>
> > Maybe just name the slave GPIO controller "gpio-slave"?  If we add
> > compatibles, the compatibles for the nodes should propbably be
> > different such that we can switch to do a lookup by compatible?
> 
> I don't think the DT folk would be happy with "gpio-slave" because
> node names are supposed to be generic. Also, "slave" probably isn't
> a good choice of name in this modern era given past history.

Yeah, not a great choice for both reasons.
 
> Rather than the above, we could use "reg" to indicate which GPIO
> controller we're talking about, and lookup the reg value in a table
> to give the key. So gpio@0, reg=<0> => gP00, gpio@1, reg=<1> => gp00.
> gpio@2, reg=<2> => whatever next.

Keep in mind that for any level, there is only 1 address space. So if 
there's anything else with multiple instances, they share the same 
address space. IOW, you couldn't have say 'rtc@1'.

(Another example of why I want to see a full picture.)
> 
> That sounds like it won't break the existing OpenBSD.

No? Isn't OpenBSD looking for 'gpio' which wouldn't find 'gpio@0'?

Rob