Re: [RFC 1/2] dt-bindings: gpio: Document shared GPIO line usage

[Rob Herring]

On Thu, Nov 28, 2019 at 11:06:35AM +0100, Linus Walleij wrote:
> On Fri, Nov 22, 2019 at 2:36 PM Peter Ujfalusi <peter.ujfalusi@xxxxxx> wrote:
> > On 22/11/2019 14.10, Linus Walleij wrote:
> > > On Wed, Nov 20, 2019 at 2:34 PM Peter Ujfalusi <peter.ujfalusi@xxxxxx> wrote:
> > >
> > >> Boards might use the same GPIO line to control several external devices.
> > >> Add section to document on how a shared GPIO pin can be described.
> > >>
> > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
> > >
> > > As I've stated earlier I think this information is surplus.
> > > If two devices have a phandle to the same GPIO line
> > > then it is by definition shared.
> >
> > Well, phandle + line number to be precise.
> 
> This is what I mean when I say "phandle to the same GPIO line".
> Like this:
> 
> foo-gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
> 
> If the phandle <&gpio0 5 *>; appear in some other
> (non-disabled) node it has > 1 users.
> 
> > >> +               line_a {
> > >> +                       gpio-shared;
> > >
> > > So this is unnecessary: if the same line is referenced
> > > by phandle from two places it is shared, simple as that.
> >
> > phandle is pointing to the gpio controller, not to the line.
> 
> Cleared up above.
> 
> > >> +                       gpios = <5 0>;
> > >> +                       output-low;
> > >
> > > This is overlapping with the use case to define initial
> > > state values for GPIOs, something that has been
> > > brought up repeatedly and I've collected links for
> > > previous discussions several times.
> >
> > I don't mind this to go away and the first set would configure the level.
> > Kept it here so I can reuse the gpio-hog code from gpiolib-of ;)
> 
> People have tried to reuse the hog code to set up
> initial line levels as well, it failed because they could
> not get the DT bindings through the door.
> 
> > > I guess if need be I have to look them up again.
> > >
> > > The DT maintainers don't like the hog syntax so
> > > something else is desired for this.
> >
> > I see, so the gpio-hog might change?
> 
> They will not change since they are ABI, but their
> use case will not be extended AFAICT.
> Not my pick, I liked the hog syntax but we need
> consensus.
> 
> > > (snip)
> > >> +The shared GPIO line management strategy can be selected with either of the
> > >> +following properties:
> > >> +- refcounted-low: The line must be kept low as long as there is at least one
> > >> +               request asking it to be low.
> > >> +- refcounted-high: The line must be kept high as long as there is at least one
> > >> +               request asking it to be high.
> > >
> > > Is this really needed? Isn't it more appropriate to just define the
> > > semantics such that as soon as some consumer requests the line
> > > high it will be refcounted high, and as soon as it is requested
> > > low by any consumer it will be refcounted low.
> >
> > Well. How do we decide which level is the one that should be preserved?
> 
> First come first serve.
> 
> If there is any conflict amongst the consumers we are
> screwed anyway so why try to establish where they should
> agree if they don't agree?
> 
> > How would the core decide what to in a simplest case:
> > two device, they are the same part.
> > ENABLE pin which needs to be high to enable the device.
> > When the driver probes it asks for initial deasserted GPIO as the device
> > is not in active use.
> 
> This makes me think it should be a unique driver
> with a unique compatible string, as it embodies
> use cases.
> 
> It is too broad to just define
> refcounted-high or refcounted-low, that is hiding the
> real use case, so I would go for something like a
> resource in the device tree that all other devices that
> need it can take.

I have similar thoughts.

> Like a reset controller, precisely:
> 
> reset: reset-controller {
>     compatible = "reset-gpio";
>     gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
>     #reset-cells = <0>;
> };
> 
> dev0 {
>     resets = <&reset>;
> };
> 
> dev1 {
>     resets = <&reset>;
> };

> 
> The ambition to use refcounted GPIOs to solve this
> usecase is probably wrong, I would say try to go for a
> GPIO-based reset controller instead.

Yes, but I think we can have that AND use the existing binding.

> The fact that some Linux drivers are already using explicit
> GPIO's for their reset handling is maybe unfortunate,
> they will simply have to grow code to deal with a reset
> alternatively to GPIO, like first try to grab a reset
> handle and if that doesn't fall back to use a GPIO.

I think this could just be all handled within the reset subsystem given 
that we've been consistent in using 'reset-gpios' (GPIO regulators are 
similar, but we never had such consistency with GPIO names for 
regulators). We can implement a reset driver for the 'reset-gpios' 
property that deals with the sharing. Drivers to DT nodes doesn't have 
to be 1:1. It's convenient when they are, but that's encoding the OS's 
(current) driver structure into DT.

Rob