Re: [RFC 1/2] gpio: make the GPIOs shareable

[Jean-Jacques Hiblot <jjhiblot@traphandler.com>]

2014-02-06 Alexandre Courbot <gnurou@gmail.com>:
> On Mon, Feb 3, 2014 at 7:26 PM, Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> Hi Alexandre,
>>
>>
>> 2014-02-03 Alexandre Courbot <gnurou@gmail.com>:
>>
>>> Hi Jean-Jacques,
>>>
>>> Sorry for taking so much time to reply, I had to go through the AT91
>>> thread several times to (hopefully) get a clear idea of what you need.
>>>
>>>
>>> On Thu, Jan 30, 2014 at 1:11 AM, Jean-Jacques Hiblot
>>> <jjhiblot@traphandler.com> wrote:
>>> > The patch implements a new requesting scheme for GPIOs that allow a gpio
>>> > to be
>>> > requested more than once.
>>> >
>>> > This new request scheme is:
>>> > * only 1 user can request a GPIO with a full control over the direction
>>> > of the
>>> >   GPIO. Full control means being able to configure the gpio as an input
>>> > or as
>>> >   an ouput.
>>> > * several users can request a GPIO with a limited control over the
>>> > direction.
>>> >   Limited control means: the gpio can be configured as an input if
>>> > someone
>>> >   doesn't have a full control of the direction. It can't be never be
>>> > configured
>>> >   as an output.
>>> > * a GPIO used as an interrupt source can't be configured as an output.
>>>
>>> So if I understand correctly (correct me if I don't), the problem is
>>> that you need to be able to read the value of a GPIO that is currently
>>> being used as an interrupt source. One example of this happening is
>>> the touchscreen node of arch/arm/boot/dts/imx28-tx28.dts:
>>>
>>>         touchscreen: tsc2007@48 {
>>>                 ...
>>>                 interrupt-parent = <&gpio3>;
>>>                 interrupts = <20 0>;
>>>                 pendown-gpio = <&gpio3 20 1>;
>>>         };
>>>
>>> While you are at it, you also want to allow a GPIO to be requested
>>> several times as long as these requests are not conflicting (which is
>>> a generalization of your initial need).
>>
>> exactly. Whle we're at it, we could try to make it work for other use cases.
>>>
>>> This should probably be
>>> considered dangerous for the integer-based interface, but with gpiod
>>> GPIOs are now assigned by platform files or firmware, so this sounds
>>> much more legitimate in this context.
>>
>> agreed. The integer-based interface must not be impacted by this.
>>>
>>>
>>> > To achieve this, a unique gpio_desc is returned by gpiod_request. The
>>> > old
>>> > gpio_desc is replaced by a gpio_hw_desc. Integer name space is still
>>> > supported
>>> > and a gpio requested by its number is requested with full direction
>>> > control.
>>> >
>>> > This patch is for RFC only. I feel that the API change need to be
>>> > discussed
>>> > before going further. Also there are probably some race conditions that
>>> > are
>>> > more important to fix now than when a gpio was an exclusive property.
>>>
>>> If I understand your goals correctly, I believe they can be reached by
>>> a simpler solution. For your initial problem the
>>> at91_gpio_irq_domain_xlate() should obtain a GPIO descriptor and call
>>> gpiod_lock_as_irq() on it. This will allow the GPIO from being
>>> requested as input later. Currently it is not possible to obtain a
>>> GPIO descriptor outside of gpiod_get() (which will request the GPIO at
>>> the same time), but it should be acceptable to consider that the
>>> holder of a gpio_chip * (either the GPIO driver itself, or in your
>>> case the AT91 pinctrl driver) is priviledged and can get access to any
>>> of the chip's descriptor through a new driver function (we already
>>> discussed doing so in https://lkml.org/lkml/2013/10/8/823 ).
>>>
>> For the touchscreen case, this is indeed a simple solution that would work.
>
> Great - in this case I would suggest to go for it, as you can
> implement it immediately (you will need to implement that driver
> function that allows a driver to access any of its descriptor, this
> should be easy if you follow the mail linked above) and it really is
> the best-fit solution to this particular problem.
I'll give it
>
>>
>>>
>>> As for the multiple-consumer case, couldn't we avoid the complexity
>>> introduced by the different kinds of descriptors by simply using read
>>> and write reference counters in each GPIO descriptor? Basically a call
>>> to gpiod_get() would always return the corresponding descriptor as
>>> this means the GPIO is mapped. But when attempting to set the
>>> direction, the reference counters are checked to confirm that this
>>> would not put the GPIO into one of the forbidden cases (e.g. no write
>>> if FLAG_USED_AS_IRQ is set, only one writer, but as many readers as we
>>> want). This sounds like it could be implemented much more succintly,
>>> and should (IIUC) do what you wanted.
>>>
>> Actually it was the first approach I tried.  It takes care of most of the
>> problem. But there are some drawbacks:
>> * no control of permissions for gpiod_set_value. A consumer requesting for
>> read would be able to set the gpio's value.
>> * need to modify the gpiod_free API to pass the same permissions flags as to
>> gpiod_request(). The consequence is that the flags need to be stored along
>> the gpio_desc* in the consumers' private data.
>> * same problem with the gpio's label.
>
> All valid points indeed. I am still a little bit turned off by the
> added complexity this brings to a subsystem that is supposed to remain
> simple to use (obtain a GPIO descriptor, drive the GPIO). We also need
> to consider all special cases (active-low, open-drain, etc) and make
> sure we handle all conflicts (what if a consumer requires open-source
> and the other open-drain?). I'm afraid this could quickly turn into a
> nightmare.
>
> Not that I am rejecting your idea. It's just that we are entering a
> new unknown zone with this and we really need to think it through.
>
>> There's another feature that I didn't post because its use case is probably
>> not very common. I wanted to be able to share output gpios. My use case is
>> the gpio tracing mechanism I posted a few weeks ago.
>
> Yes, I remember that patch.
>
>> To reduce the complexity of tracking the gpio used by the probes, I thought
>> that maybe this task could be delegated to the gpiolib. Implementation could
>> be very straightforward there:
>> * in gpiod_request (or equivalent) pass an ownership tag (NULL would be a
>> special default value)
>> * in the case were the ouput is already owned, check if the ownership tag
>> are the same and not NULL. If so the request succeeds otherwise it fails.
>
> So the two drivers would need to communicate that ownership tag so the
> second can "hijack" the GPIO with permission from the first? You could
> also pass a handle directly, like PRIME does for buffers (then we
> could plug kdbus in and have user processes exchange GPIO handles
> securely. Now I'm scared).
>
> I would be even more cautious about sharing output GPIOs. If possible
> at all, I'd really prefer to see a scheme where the two consuming
> drivers yield the GPIO when they don't need it. After all, if you
> enter a situation where both drivers want to drive the GPIO output,
> you are obviously going to have a problem.
>
> Alex.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html