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

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Mark Kettenis <mark.kettenis@xs4all.nl>
Cc: Hector Martin <marcan@marcan.st>,
	linus.walleij@linaro.org, robh@kernel.org,
	krzysztof.kozlowski@linaro.org, arnd@arndb.de, lee@kernel.org,
	alyssa@rosenzweig.io, asahi@lists.linux.dev, brgl@bgdev.pl,
	linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
	sven@svenpeter.dev, krzysztof.kozlowski+dt@linaro.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller
Date: Tue, 6 Sep 2022 15:54:50 +0100	[thread overview]
Message-ID: <YxdfOr6WCZiR3W1c@shell.armlinux.org.uk> (raw)
In-Reply-To: <d3ced0ffaec45e3c@bloch.sibelius.xs4all.nl>

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@marcan.st>
> > 
> > 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.

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

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.

That sounds like it won't break the existing OpenBSD.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel