Message-ID: <18d7bc91-2fd8-fe33-bda6-b3a200d280d0@ti.com>
Date:   Fri, 22 Nov 2019 17:14:17 +0200
From:   Peter Ujfalusi <peter.ujfalusi@...com>
To:     Linus Walleij <linus.walleij@...aro.org>
CC:     Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Mark Brown <broonie@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Re: [RFC 2/2] gpiolib: Support for (output only) shared GPIO line



On 22/11/2019 14.22, Linus Walleij wrote:
> On Wed, Nov 20, 2019 at 2:34 PM Peter Ujfalusi <peter.ujfalusi@...com> wrote:
> 
>> This patch adds basic support for handling shared GPIO lines in the core.
>> The line must be configured with a child node in DT.
>> Based on the configuration the core will use different strategy to manage
>> the shared line:
>> refcounted low: Keep the line low as long as there is at least one low
>>                 request is registered
>> refcounted high: Keep the line high as long as there is at least one high
>>                 request is registered
>> pass through: all requests are allowed to go through without refcounting.
>>
>> The pass through mode is equivalent to how currently the
>> GPIOD_FLAGS_BIT_NONEXCLUSIVE is handled.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@...com>
> 
> This is a good start! Some ideas on how I'd like this to develop.

Thanks!

> 
>>  drivers/gpio/gpiolib-of.c |  28 ++++++--
>>  drivers/gpio/gpiolib.c    | 132 +++++++++++++++++++++++++++++++++++---
> 
> Please put this API under its own Kconfig option
> and in its own file in
> drivers/gpio/gpiolib-refcounted.c
> local header in
> drivers/gpio/gpiolib-refcounted.h
> only built in if the appropriate Kconfig is selected.

This patch is not really an API, but extension to the current one so
that clients does not need to be aware of the shared use.

> Consumer header in
> include/linux/gpio/reference-counted.h
> And add external driver API to this last file.

would it be better to have
include/linux/gpio/consumer-refcounted.h

> 
>> --- a/drivers/gpio/gpiolib-of.c
> 
> No commenting on this because as pointed out in the binding
> patch I want this done by simply detecting the same GPIO
> being referenced by several <&gpio N> phandles.

Would you scan all pins for each gpio-chip during boot time or scan only
when a gpio is requested and it is not already requested (so it is shared)?

>> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
>> index ca9bc1e4803c..0eec0857e3a8 100644
>> --- a/drivers/gpio/gpiolib.h
>> +++ b/drivers/gpio/gpiolib.h
>> @@ -111,11 +111,18 @@ struct gpio_desc {
>>  #define FLAG_PULL_UP    13     /* GPIO has pull up enabled */
>>  #define FLAG_PULL_DOWN  14     /* GPIO has pull down enabled */
>>  #define FLAG_BIAS_DISABLE    15        /* GPIO has pull disabled */
>> +#define FLAG_IS_SHARED 16      /* GPIO is shared */
> 
> This is a good way of flagging that this is a refcounted GPIO
> I would call it FLAG_IS_REFERENCE_COUNTED as it is
> more precise to what it means.

As I said before, I think this refcounting is not going to work nicely
when we have actually shared gpio.

>> +#define FLAG_REFCOUNT_LOW 17   /* Shared GPIO is refcounted for raw low */
>> +#define FLAG_REFCOUNT_HIGH 18  /* Shared GPIO is refcounted for raw high */
> 
> Do not put this here, keep it in the local refcounted GPIO
> struct gpio_refcount_desc.

OK.

>>         /* Connection label */
>>         const char              *label;
>>         /* Name of the GPIO */
>>         const char              *name;
>> +       /* Number of users of a shared GPIO */
>> +       int                     shared_users;
>> +       /* Reference counter for shared GPIO (low or high level) */
>> +       int                     level_refcount;
> 
> We can't expand this struct for everyone on the planet like this.
> 
> In the local header
> 
> drivers/gpio/gpiolib-refcount.h create something like:
> 
> struct gpio_refcount_desc {
>     struct gpio_desc *gd;
>     int shared_users;
>     int level_refcount;
> };

So. If we give this to multiple users then how different GPIO_ACTIVE_*
would be handled? What flag would be used for gd?
How do we know which level that needs to be preserved?

struct gpio_refcount_desc *gpiod_refcounted_get(struct device *dev,
					const char *con_id,
					enum gpiod_flags flags,
					bool i_care_for_asserted_state);

Would take care of that, but we will still have the issue coming from
the global refcounting.

Hrm, actually we might not. If we use the level_refcount to count for
high for example, then we would never decrement below 0 (as I do in this
patch) then things might be balanced.
No, they are not:

We want high refcount for the gpio.
Driver1 probes, asks for gpio and it asks it to be low.
Driver1 sets the gpio to high as it is enabled.
Driver2 probes, asks for the gpio and it asks it to be low.
Device1 also got reset.
Driver2 is not enabling the gpio as it is not needing it
Driver1 bugs out on access to chip.

> This should be the opaque cookie returned to consumers of this new
> refcounted API.
> 
> It defines the reference counted API as separate and optional from
> the non-reference counted API, also using its own API.
> 
> The only effect on the core
> gpiolib will be the single flag in struct gpio_desc; and some
> calls into the shared code with stubs if the support for systems
> that do not enable the reference counting API.

One thing which might work, but it might be a longer shot:
If there is a chance that the driver is used in shared gpio case, the
driver needs to use the new API. I would not call it refcounted, but
perhaps 'managed'?

When the first driver requests the gpio, then we would allocate a
'master' managed descriptor, similar to "struct gpio_refcount_desc" but
with list_head and probably something else as well (lock/mutex, whatever
we need). Get the gpio for it, mark it as shared.
Allocate a 'client' managed struct which we add to the list and give it
back to the driver requesting the gpio line.

Likely the 'master' struct needs to be in a global list as well to be
able to find the correct one when someone else is requesting the same
gpio, which is already managed.
SO we might need to list_heads in there, one for the global 'masters'
list and one for it's 'clients' list.

>From here it is kind of simple as when a client driver asks for a level,
we will change the 'client' node's state the the level and then ask the
'master' to check all of it's 'clients' current state and decide if the
real GPIO needs to be changed and to what level.

> 
> Yours,
> Linus Walleij
> 

- Péter

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

Powered by blists - more mailing lists