Hi Alexandre, 2014-02-06 Alexandre Courbot <[email protected]>: > On Mon, Feb 3, 2014 at 7:26 PM, Jean-Jacques Hiblot > <[email protected]> wrote: >> Hi Alexandre, >> >> >> 2014-02-03 Alexandre Courbot <[email protected]>: >> >>> 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 >>> <[email protected]> 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. > >> >>> >>> 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). It could be kept simple: the gpio_hw_desc (the name is probably not the best but I lacked inspiration) is used only for the request, for the rest the gpio_desc is used.
>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. open-drain & open-source are for outputs only, so it shouldn't be a problem as an output can't shared. The active-low flag should probably be moved in the gpio_desc descriptor (not the gpio_hw_desc) as it's a reflection of how a consumer interprets an electrical value. I believe the only structural problem would be with the debounce setting as it's a hardware configuration used for inputs. But debounce is seldom used. > > 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. The RFC was just an invitation to such thinking. As the gpio_desc-based API is not yet widely used by drivers, it may be the right time to think about this. > >> 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). :o) you're thinking to far ahead, kdbus is not mainlined yet. And what about a cgroup for sharing GPIOs ? > > 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. The goal wasn't to be able to share a gpio with a foreign sub-system. The goal was for single sub-system to have an easy way to manage its gpios without having to have its own gpio list and ref counts to manages. I wanted to be able to do something like this: desc1 = gpiod_request_shared(pio0_hw_desc, "probe1", OUTPUT, "gpio_event_trigger'); desc2 =gpiod_request_shared(pio0_hw_desc, "probe2", OUTPUT, "gpio_event_trigger'); //ok desc3 =gpiod_request_shared(pio0_hw_desc, "backlight_control", OUTPUT, something_else); //ko the string "gpio_event_trigger" is the owner tag, it's supposed to identify the consumer. If the tags don't match the request fails I admit that it doesn't make sense for driving real hardware. It just would be convenient for the gpio tracing feature. Jean-Jacques > > Alex. > -- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
