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

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

[Linus Walleij]

Hi folks,

cutting all the discussions in this thread 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>;
};

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

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

Yours,
Linus Walleij

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

[Peter Ujfalusi]

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

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

[Peter Ujfalusi]

Hi,

On 08/11/2019 13.21, Peter Ujfalusi wrote:
> 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.

Well, this is not going to work either. The core must know what level it
needs to 'protect'.

If we have two devices, both is enabled when their RST/ENABLE pin is
high, but D1 documents it's RST line as low active reset (low=reset,
high=enabled), D2 documents it's ENABLE line as high active (low=reset,
high=enabled).

dpiod_assert() would want to set the line low for D1 and high for D2 as
per how we supposed to interpret the GPIO bindings.

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

Another thing which came to my mind is that the gpiod_get's are also
need to be refcounted to make sure that when used by different drivers
and one of them is removed the GPIO is still usable by the remaining
users (and to remove the constraints placed by the removed module).

So convert things to look more like the regulator core than what we have
atm?
But the difference is that in regulators we protect the enabled state
(if anyone needs the given regulator on then it is on), but in GPIO the
level we might want to protect is either high or low depending on the
users of the shared line.
If the devices enabled when the line is high -> protect high
If devices enabled when the line is low -> protect low

All of this can be handled with the RFC gpio-shared driver and
representation of the hardware in DT...

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

- Péter

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

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

[Linus Walleij]

On Fri, Nov 8, 2019 at 12:20 PM Peter Ujfalusi <peter.ujfalusi@ti.com> 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

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

[Peter Ujfalusi]

On 13/11/2019 19.06, Linus Walleij wrote:
> On Fri, Nov 8, 2019 at 12:20 PM Peter Ujfalusi <peter.ujfalusi@ti.com> 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.

Agree.

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

imho the gpiod is API level -7 in the Rusty scale.

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

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_

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

Sure.

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

I know, I'm trying to catch up on my promise on DMAengine API ;)

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

At the end the goal is to have only assert/deassert API for GPIO, or do
you want to keep set_value()?
Imho the gpiod_direction_output_raw() should not be allowed to be used
by drivers.

CONFIG_GPIOLIB_ASSER_API as selectable config option?

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 does not really matter in this case.

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

Yes.

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

The refcounting needs to be seeded with a level that needs to be
preserved. It can be low or high.

And we have cases probably when the board does not want to use
refcounting on the shared line, juts pass-through (as it is with the
GPIOD_FLAGS_BIT_NONEXCLUSIVE flag currently).

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

Sure, I agree.

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

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.

- Péter

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

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

[Peter Ujfalusi]

Hi Linus,

On 19/11/2019 10.34, Peter Ujfalusi wrote:
>> 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.
> 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.
> 
> 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 had some time today and now have a working support for shared GPIOs in
the gpio core.

Works fine with regulators and with my board which have two ocm3168a
codec with shared reset GPIO.

There are couple of things initially not going to work, like supporting
clients with different GPIO_ACTIVE_HIGH/LOW as we have only one
gpio_desc which got it's GPIO_ACTIVE_* form the gpio-shared child.

The means that all clients has to have the same GPIO_ACTIVE_* as how the
gpio-shared child is configured:

gpio1 {
	p00 {
		gpio-shared;
		/* raw level high is refcounted */
		gpios = <0 GPIO_ACTIVE_HIGH>;
		/* Initially set the gpio line to raw level high */
		output-high;
		line-name = "CODEC RESET";
	};
};

now clients can:
...
enable-gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>;
...
enable-gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>;
...

I'm reusing some of the gpio-hog code for this from gpiolib-of.c

For supporting different GPIO_ACTIVE_* on the client side might be
tricky as we might need to create dummy gpio_desc for each client and
put them on a list for the shared gpio_desc.

In any case I try to clean up the patch and add some documentation and
send it for review as RFC in couple of days.



- Péter

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

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

[Linus Walleij]

On Tue, Nov 19, 2019 at 9:33 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 13/11/2019 19.06, Linus Walleij wrote:
> > On Fri, Nov 8, 2019 at 12:20 PM Peter Ujfalusi <peter.ujfalusi@ti.com> 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

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

[Peter Ujfalusi]

On 21/11/2019 16.47, Linus Walleij wrote:
> On Tue, Nov 19, 2019 at 9:33 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> On 13/11/2019 19.06, Linus Walleij wrote:
>>> On Fri, Nov 8, 2019 at 12:20 PM Peter Ujfalusi <peter.ujfalusi@ti.com> 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.

Yep, this is really a bad idea..

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

An intuitive name in place of gpiod_set_value()/gpiod_get_value() family
would be really nice...

Another thing which bugged me for a while is the _cansleep postfixing of
the get/set.
In most cases drivers do not care if the gpio access sleeps or not, but
there are (few) cases when the access should not sleep.

I think if a cleaner API is proposed it might worth to make the normal
API to not warn when it might sleep and _nosleep postfixed versions must
be used in place where it is not desired to sleep (and WARN if it would
sleep).

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

Not sure if we really need refcounted access just for fun. Imho even the
shared GPIO access should not be refcounted as such, but have a logic to
use the user's requested level to set the line's level.

As the gpio-shared driver is doing in this RFC.

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

Sure, if there is a real use for it then it should be preserved.

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

I believe my problem starts with that I also see this from
electromagnetic point and not logical level point ;)

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

I'm still not convinced that refcounting API would help or give anything
useful at the table.

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

Which went nicely with the regulators ;)

Adding GPIOD_FLAGS_BIT_NONEXCLUSIVE:
b0ce7b29bfcd regulator/gpio: Allow nonexclusive GPIO access

to fix:
efdfeb079cc3 regulator: fixed: Convert to use GPIO descriptor only

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

You mean that everyone must request it with this flag, right?
At the first request this flag would be set to the gpio_desc. The
upcoming requests would be denied if the requester is not using the
proper API/flag to request it, right?

For single user this does not matter apart from the fact that the gpio
accesses will be refcounted for a single client as well (and API check
as well).

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

Hrm, so you want FLAG_REQUESTED / FLAG_REFERENCE_COUNTED to be exclusive?
I don't see why FLAG_REQUESTED would be in the way of
FLAG_REFERENCE_COUNTED.

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

Sure, that would not work anyways.

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

Ah, I see where you are heading now..

struct gpio_refcount_desc *gpiod_refcounted_get() and it's family with
own set of API operating on top of 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.)

It is not just that.

Global refcounting is just not going to work.

For simplicity: you have three codecs connected to same GPIO for ENABLE
(active high).

When the driver probes it will ask for the gpio to deasserted (no need
to power on things when they are not in use).

We have registered three low requests.

Then one needs to be powered up, which gives us two low requests and one
high request, but we must set the line high.

How does the core know this?

We should care for the asserted state?

What if they have RESET pin which is active low?
they will ask it to be asserted initially and when any of them needs to
be powered it will ask for deassert.

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

- Péter

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