Re: [RFC 0/2] gpio: Support for shared GPIO lines on boards

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Tue, Nov 19, 2019 at 9:33 AM Peter Ujfalusi <peter.ujfalusi@xxxxxx> wrote:
> On 13/11/2019 19.06, Linus Walleij wrote:
> > On Fri, Nov 8, 2019 at 12:20 PM Peter Ujfalusi <peter.ujfalusi@xxxxxx> wrote:

> >> gpiod_deassert() would be equivalent to Philipp's
> >> gpiod_politely_suggest_value()
> >
> > I don't intuitively understand the semantics of these calls.
> > Consider Rusty Russells API design manifesto:
> > http://sweng.the-davies.net/Home/rustys-api-design-manifesto
>
> imho the gpiod is API level -7 in the Rusty scale.

I would agree that the double-inversion of DT or machine flags
for active low/high and then gpiod_set_value() to high makes it
low and vice versa mutatis mutandis is pretty -7

But my comment was not about that, but about the ambiguity
of some *_politely_suggest_* API.

I mean adding a new confusing API to the mess is hardly
going to make things better, two wrongs doesn't make one
right.

> > It seems like a totally different and entirely syntactic problem
> > separate from the reset business you're trying to solve?
> >
> > We had this discussion before this week and yeah, if we
> > historically named the logical levels on the line "asserted"
> > and "deasserted" everywhere it would be great.
>
> Yeah, it is extremely awkward currently:
> rst-gpois = <&gpio0 1 GPIO_ACTIVE_LOW>;
>
> devm_gpiod_get_optional(dev, "rst",  GPIOD_OUT_LOW |
>                                      GPIOD_FLAGS_BIT_NONEXCLUSIVE);
>
> Would set the initial output level to _high_

Yeah I understand what the problem is.... :/

> > These APIs really need names that can be understood right off
> > and they should be compile-time optional (a Kconfig option) so
> > that drivers that really need them can select to have them
> > explicitly.
>
> At the end the goal is to have only assert/deassert API for GPIO, or do
> you want to keep set_value()?

The problem can me split in two, non-conflated things:

1. Rename gpiod_set_value() to e.g. gpiod_set_state(bool asserted)
    this is a purely syntactic change with no semantic effect.
    This makes it clear what happens and fixes the issue with
    the hard to understand name of the function, but does not
    change the semantic allowing you to say assert a GPIO line
    several times after another, as that would wreak havoc in the
    kernel. And after all there is nothing about that name that
    suggest it would be reference counting anything.

2. Add a new reference-counted stateful API that does not allow you to
   handle your usecase.

I would recommend not conflating the two things.

> Imho the gpiod_direction_output_raw() should not be allowed to be used
> by drivers.

So how do you suggest that drivers/w1/masters/w1-gpio.c
handle the usecase of overriding a nominally open drain-flagged
line to pump some voltage in the line at some regular intervals,
notwithstanding the logical level of the line?

This use of GPIO isn't dealing with something boolean logical,
but something directly electromagnetic.

It would be nice if GPIO was only about logical levels,
but it is not, and that is why gpiod_set_raw() exists.

There are other users in the kernel that are just cheating
or thinking wrong, it'd be nice if those could be fixed.
It doesn't mean the API has no valid usecases.

> CONFIG_GPIOLIB_ASSER_API as selectable config option?

CONFIG_GPIOLIB_REFCOUNTING_API is more to the
point I think, because that is what you want to achieve.

Then the function calls can very well be named something
with *assert* in them.

> But it is one thing to change gpiod users as we have heavy use of the
> legacy gpio API:
> git grep gpio_request | wc -l
> 1868

That can be done with a sed script if someone takes on the
task.

> That does not really matter in this case.

Nope.

> > So begin with creating a way to pull the shared handling of
> > regulators into gpiolib with these clearly cut semantics
> > delete the NONEXCLUSIVE thing and then when you are
> > done with that exploit the same
> > infrastructure for GPIO reset.
>
> The logic is relatively simple, 229 lines in gpio-shared, but moving
> that into core will explode things a bit and going to add more
> complexity to all gpio lines.

I would just add a flag such as in drivers/gpio/gpiolib.h:

#define FLAG_REFERENCE_COUNTED  15      /* GPIO uses the reference
counting API */

If anyone grabs a GPIO with this flag it needs to be accessed
using the refcounting API and all other uses with the
regular API denied.

The same the other direction, if FLAG_REQUESTED is already
set, the refcounting API should bail out.

Do not try to support a use case such as allowing the gpiod
to be grabbed unrefcounted and later turned into a refcounted
gpiod. Only grab it through the refcount-specific API.

This way it's almost as cleanly separated as the code is
separated into regulator right now, just that it lives in
a place where it can be reused by others needing
reference counting.

Then surround code with:

if (IS_ENABLED(CONFIG_GPIOLIB_REFCOUNTING_API)
     /* test flag */

Then the code size impact should be zero if the refcounting API
is not selected.

Then just create an add-on that only affects the lines that explicitly
want refcounting. Wrap a gpiod in another struct or something
struct gpio_refcount_desc?

> For one, we must maintain a list of clients requesting the line to be
> able to do proper refcounting and this needs to be done for all pins as
> we don't know beforehand that the given line is going to be shared.

If you want to deny the same client to ask for the same line
twice then you need a list like that indeed. (It's a good strict semantic
check anyways.)

> Or add gpio-shared block similar to gpio-hog to prepare a given line for
> sharing? I think this might be a better thing to do and some of the code
> from gpio-shared.c can be reused.

I would just add a flag and try to keep this API entirely on the
side for now.

Yours,
Linus Walleij