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

[Peter Ujfalusi <peter.ujfalusi@xxxxxx>]

Hi Linus,

On 01/11/2019 17.56, Linus Walleij wrote:
> Hi folks,
> 
> cutting all the discussions in this thread

the mail got disconnected from the thread and almost missed it ;)

> we need to see the bigger
> pattern:
> 
> On GPIO rails
> 
> People want "something like rails" for GPIO. In power supplies
> and thus the regulator subsystem, rails are connected to many
> logical endpoints.
> 
> - The suggested inverter bindings would be effectively an
>   inverter on a GPIO rail.
> 
> - This suggestion would be equal to many power consumers
>   on a rail, such as the usecase of shared gpio-enable lines in
>   the regulator subsystem already provides.
> 
> The former seems to have been identified as solveable for the
> userspace that needed it and absorbed into the drafts for a
> virtualized GPIO controller. (Aggregating and creating a new
> virtual GPIO chip for some select physical GPIO lines.)
> 
> I haven't seen an exact rationale from the DT community as
> to why these things should not be modeled, but as can be
> clearly seen in
> Documentation/devicetree/bindings/regulator/regulator.yaml
> the "rail abstraction" from the regulator subsystem which
> is in effect struct regulation_constraints and it sibling
> struct regulator_init_data is not in the DT bindings, instead
> this is encoded as properties in the regulator itself, so this
> is pretty consistent: the phandle from regulator to consumer
> *is* the rail.
> 
> This goes back to Rajendras initial DT regulator support code
> see:
> git log -p 69511a452e6d
> 
> So it would be logical then to just have:
> 
> - More than one phandle taking the same GPIO line
> - Figure this situation out in the gpiolib OF core
> - Resolve the manageability of the situation (same
>   consumer flags etc)
> - Instantiate a kernel component as suggested,
>   mediating requests.
> - Handle it from there.
> 
> So:
> 
> gpio: gpio-controller@0 {
>         compatible = "foo,gpio";
>         gpio-controller;
>         #gpio-cells = <2>;
> };
> 
> consumer-a {
>        compatible = "foo,consumer-a";
>        rst-gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
> };
> 
> consumer-b {
>        compatible = "foo,consumer-b";
>        rst-gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
> };

Should be fine I think.
Again, I would trust the board design to take into consideration of the
consumer's needs when sharing the same GPIO line for any purpose.

> Hi kernel: figure it out.
> 
> From this point the kernel driver(s) have to figure it out.
> 
> I don't think this requires any changes to the DT bindings
> other than perhaps spelling out that if you link more than one
> phandle to a GPIO line, magic will happen. (We should probably
> make very verbose dmesg prints about this magic.)

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

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.

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.

> This is enough to start with. After that we can discuss adding
> flags and constraint properties to a certain GPIO line if
> need be. (That will be a big discussion as well, as we haven't
> even figured out how to assign default values to individual
> GPIO lines yet.)

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);

Basically assert would set the level to the active state defined by the
DT. deassert would, well, de-assert the active state.

gpiod_deassert() would be equivalent to Philipp's
gpiod_politely_suggest_value()

Gradually drivers can be moved to this API pair from gpiod_set_value()
when it makes sense.
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?

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.

gpiod_get should also use the polite version when setting the initial
level most likely.

Another thing is that currently gpio core does not have refcounting and
most of the client drivers treat it like that. 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);

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.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki