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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Fri, Nov 8, 2019 at 12:20 PM Peter Ujfalusi <peter.ujfalusi@xxxxxx> wrote:

> To start with I would let GPIO core to allow requesting the same GPIO
> line by multiple consumers all the time.

It already allows that with GPIOD_FLAGS_BIT_NONEXCLUSIVE.

> If the flags for the gpio_request does not contain
> GPIOD_FLAGS_BIT_NONEXCLUSIVE (probably we can have another define for
> BIT(4) as GPIOD_FLAGS_BIT_MIGHT_BE_SHARED?) then print with dev_warn()
> to get the attention of the developer that all users of the shared GPIO
> line must be checked and change the current dev_info() to dev_dbg() when
> the flag is provided.

We already have that. Unless all users request it with the same
GPIOD_FLAGS_BIT_NONEXCLUSIVE they will simply fail,
it is an excellent warning since it creates a strict semantic
requirement for all participating consumer to explicitly specify
if they want to share a line.

Adding a GPIOD_FLAGS_BIT_MIGHT_BE_SHARED with some
soft semantics sort of allowing deviant nonstrict behavior seems
like a real bad idea to me, it will likely create all kind of ambiguities
we cannot foresee.

> When the consumer drivers are checked (and modified if needed) that they
> behave OK in this situation we can snap the
> GPIOD_FLAGS_BIT_MIGHT_BE_SHARED to silence the warning.

I have burnt myself a bit on "let's poke in this transitional
thing that I promise to remove later" and that later never
happens so I'd say don't do that.

> 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

> Not sure how the core would refcount things, but to align with what Rob
> was saying about the misleading API naming:
> gpiod_set_value(priv->en_gpio, 1/0) against the DT's
> GPIO_ACTIVE_HIGH/LOW of the line's active state we might want to have:
> gpiod_assert(priv->en_gpio);
> gpiod_deassert(priv->en_gpio);

This is what gpiod_set_value() already does today in a way
just name
gpiod_set_value() -> gpio_set_asserted() and change
the second argument to a bool named "asserted".

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.

It is up to someone driving the change and changing it
everywhere in that case. Preferably with a semantic
coccinelle patch or sed script since it is purely
syntactic, then plan where and when to
run that. Then do that first, wait a kernel cycle and scoop up
any fallout and leftovers and then start the next thing.

> Basically assert would set the level to the active state defined by the
> DT.

Or ACPI. Or machine descriptor tables. I suppose.
Doing APIs becomes generic, the suggestion I had
above was more like doing something like detecting
a shared line *specifically* for device tree and nothing
else and handle it in gpiolib-of.c but maybe that is not
possible.

> Gradually drivers can be moved to this API pair from gpiod_set_value()
> when it makes sense.

The problem I have right now as subsystem maintainer is people
starting things like that and never finishing them.

If you wanna do this I suggest a fix it everywhere in one swift stroke
approach with broad buy-in from everyone-approach or fail totally.
We have too many in-transit API changes.

> The current gpiod_set_* would operate like as it is right now by not
> asking politely a level, whatever it is.
>
> Hrm, probably both gpiod_assert() and gpiod_deassert() should be polite
> when asking for level change?

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.

> If all consumers of the shared line is using gpiod_assert/deassert, then
> the core should 'protect' the raw level of the gpiod_assert() calls.
>
> At the end we will see drivers converted to assert/deassert API when a
> developer faces issues that they use shared GPIO line on a board.

> Another thing is that currently gpio core does not have refcounting and
> most of the client drivers treat it like that.

Notably all the drivers specifying GPIOD_FLAGS_BIT_NONEXCLUSIVE
does not treat it like that and that is why they specify that
flag. All regulators, I think.

> It is perfectly fine to
> gpiod_get(priv->en_gpio,1);
> gpiod_get(priv->en_gpio,1);
> gpiod_get(priv->en_gpio,1);
> gpiod_get(priv->en_gpio,0);

I guess you mean gpiod_set()

> at the last call the GPIO value is going to be set to 0 no matter if it
> was set to 1 three times prior, but I guess this can be worked out when
> the driver(s) are converted to assert/deassert.

I don't understand why that would not be allowed?

Again I guess not really related to the original problem,
so if you want to work on that it can be done in isolation.

To the overall question of a refcounting GPIO API:

OK to add a new API like that I would say first convert the
regulators to use them so we have a strong buy-in from a
subsystem that already does this. That way we can get rid of
the existing GPIOD_FLAGS_BIT_NONEXCLUSIVE and pull
the handling of shared GPIO lines into gpiolib using these
new APIs.

I'm all for this.

But the general usability needs to be proven.
It is not a very huge task:
 git grep BIT_NONEXCLUSIVE |wc -l
24

24 occurrences in the whole kernel.

If the suggested API doesn't fit regulators as well it is dead
in the water. Then the usecase is likely specific to resets,
and what you would need to do is rather improve the
available semantics in the reset subsystem.

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.

Yours,
Linus Walleij