Re: [PATCH] gpio: add gpio_of_helper

[Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx>]

Hi Alexandre,

> On Oct 23, 2014, at 11:53 AM, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
> 
> On Thu, Oct 23, 2014 at 3:23 PM, Jiří Prchal <jiri.prchal@xxxxxxxxxxx> wrote:
>> 
>> 
>> Dne 23.10.2014 v 07:08 Alexandre Courbot napsal(a):
>> 
>>> On Wed, Oct 22, 2014 at 6:58 PM, Pantelis Antoniou
>>> <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>>>> 
>>>> Hi Alexandre,
>>>> 
>>>>> On Oct 22, 2014, at 12:41 PM, Alexandre Courbot <gnurou@xxxxxxxxx>
>>>>> wrote:
>>>>> 
>>>>> On Wed, Oct 22, 2014 at 6:24 PM, Pantelis Antoniou
>>>>> <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>>>>>> 
>>>>>> Hi Alexandre,
>>>>>> 
>>>>>>> On Oct 22, 2014, at 12:18 PM, Alexandre Courbot <gnurou@xxxxxxxxx>
>>>>>>> wrote:
>>>>>>> 
>>>>>>> On Wed, Oct 22, 2014 at 5:26 PM, Jiri Prchal <jiri.prchal@xxxxxxxxxxx>
>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> This patch adds new driver "gpio-of-helper", witch has possibility to
>>>>>>>> export
>>>>>>>> gpios defined in dt. It exports them in defined name under
>>>>>>>> /sysfs/class/gpio/name.
>>>>>>>> It's rebased from Pantelis Antoniou patch to new kernel.
>>>>>>>> Usage example:
>>>>>>>>       gpio {
>>>>>>>>               compatible = "gpio-of-helper";
>>>>>>>>               status = "okay";
>>>>>>>>               pinctrl-names = "default";
>>>>>>>>               pinctrl-0 = <&pinctrl_gpio>;
>>>>>>>> 
>>>>>>>>               gsm_on {
>>>>>>>>                       gpio-name = "gsm_on";
>>>>>>>>                       gpio = <&pioB 13 GPIO_ACTIVE_HIGH>;
>>>>>>>>                       output;
>>>>>>>>                       init-low;
>>>>>>>>               };
>>>>>>>>       };
>>>>>>>> 
>>>>>>>> This patch needs Alexey Ignatov [PATCH] gpiolib: allow exporting
>>>>>>>> gpios with
>>>>>>>> custom names.
>>>>>>> 
>>>>>>> 
>>>>>>> We will need to see whether the pre-requisite patch can get merged
>>>>>>> first, but there are a couple of things that are wrong with your patch
>>>>>>> anyway:
>>>>>>> 
>>>>>>> - it is missing a bindings documentation
>>>>>>> - it is using the legacy integer GPIOs instead of the descriptor
>>>>>>> interface (see include/linux/gpio/consumer.h). Since this is DT-based,
>>>>>>> there is absolutely no reason to not use the descriptors interface.
>>>>>>> - it seems quite long for what it needs to do
>>>>>>> - the MODULE_AUTHOR has not signed-off this patch (?)
>>>>>>> 
>>>>>>> But what makes me nervous is that this encourages more usage of the
>>>>>>> sysfs interface, an in a way that is potentially harmful.
>>>>>>> 
>>>>>>> Also, I don't know if the DT people will be happy with this idea.
>>>>>>> Since this concerns DT, please also add the devicetree list and get a
>>>>>>> Acked-by for the bindings you want to push by a DT maintainer.
>>>>>> 
>>>>>> 
>>>>>> Since I’m the original perpetrator, let me put a few words here for
>>>>>> posterity.
>>>>>> 
>>>>>> This patch was meant as a quick and dirty method for doing the
>>>>>> automatic export
>>>>>> and pinmux configuration from DT on a 3.8 kernel. The kernel APIs have
>>>>>> moved on
>>>>>> since. It wasn’t meant to be submitted for mainline right as it is.
>>>>>> 
>>>>>> Unfortunately (or fortunately for many people) it does what a lot of
>>>>>> people need
>>>>>> i.e. configures a board with heavily pinmuxed GPIO right at boot time.
>>>>>> 
>>>>>> I’m open at having a small discussion about how to do what the users of
>>>>>> this patch
>>>>>> do ‘right’.
>>>>> 
>>>>> 
>>>>> Sure, although the discussion might turn out to not be that "small". :)
>>>>> 
>>>> 
>>>> Heh ;)
>>>> 
>>>>> Pinmux configuration sounds like a job for pinmux more than GPIO, and
>>>>> exporting potentially many GPIOs to user-space sounds like a work for
>>>>> a proper driver.
>>>>> 
>>>> 
>>>> I’m afraid that’s not the case. A great many users do not require
>>>> anything
>>>> more than setting a pinmux and the GPIO configuration (input/output).
>>>> They can then do low speed I/O using the sysfs interface, without having
>>>> to
>>>> use any complex APIs (shell works just fine).
>>>> 
>>>> Think of stuff like controlling a sprinkler valve, or something like a
>>>> mechanical
>>>> door detection open state.
>>> 
>>> 
>>> That sounds like any kind of arbitrary device that can be temporarily
>>> connected to a set of GPIOs that will be bit-banged. Is my
>>> interpretation correct?
>> 
>> 
>> Not only temporarily. For example on/off switch of some hw on board like gsm
>> modem.
> 
> ... which should be handled in-kernel.
> 

Not really. This is part of some kind user-space ABI but which has to do
with the mapping of an named GPIO in the board schematics to user-space.

>> Or some optocoupler isoleted in/outs for general use from userspace.
> 
> ... which should thus be configured from userspace.
> 

No. It is used from userspace, but the the configuration of such is performed
by board specific means in the kernel.
 
>>> 
>>> 
>>> In that case, I seriously doubt that this should be part of the DT.
>>> Right now the DT is part of the firmware, and an immutable description
>>> of the hardware layout of a board - definitely such devices do not fit
>> 
>> This is our case of gpio. It's like LEDs, they are hw layout of board,
>> but they are in/out instead of LEDs are only output.
>>> 
>>> into that definition. This might change once the Device Tree Overlays
>>> are merged, but we are not there yet. If the DT maintainers say this
>>> is a valid use-case for overlays, then I will reconsider my position,
>>> but in any case it really looks like this could/should be done from
>>> user-space.
>> 
>> I don't think so as I explained - HW layout of boerd.
> 
> If it is a static hardware layout that makes the case even clearer -
> these devices should have a proper kernel driver that configures these
> GPIOs, and exports a proper interface to userspace, not just raw
> GPIOs.

Raw GPIOs is what the user-space application wants to use. There is no
higher abstraction possible.

What these applications need are:

1. A way to map a GPIO to a name that is board invariant. Many boards
move GPIOs around but the function typically stays the same. So for
instance rev-0 might have GARAGE_DOOR to GPIO_A10, and on subsequent
revision rev-1 it might be GARAGE_DOOR to GPIO_B08. The user-space
application that controls everything does not want to deal with revisions
ideally, just to know that the named GARAGE_DOOR gpio exists.

2. A way to have the pinmux configuration of said GPIO configured without
having to do anything explicit. I.e. on am335x the pinmux configuration is
decoupled from the GPIO driver; having a GPIO configured as a GPIO does not
alter the pinmux settings.

3. The user-space facing API must be simple, preferably file based like the
sysfs method right now. It is very simple to use even from within scripts.
 
> 
>> In other way I'm looking forward for DT overlays.
>>> 
>>> 
>>> You have almost all the necessary pieces: you can export, configure
>>> and manipulate any GPIO from the sysfs interface. The only thing you
>>> would be missing is a way to give a name to a GPIO, something that can
>>> easily be fixed.
>> 
>> But isn't it nice to have already exported gpios which are layouted out
>> from board. All other unexported gpios aren't connected to anywhere on
>> board. So user don't have to export or unexport anything.
>>> 
>>> 
>>> Since the devices you want to configure that way are typically
>>> removable or usage-specific, why would you want to store that
>>> information all the way up into the firmware? A commonly accepted
>>> wisdom is that what can be done in user-space should be done in
>>> user-space, and it really looks like this applies here.
>> 
>> They are NOT removable.
>>> 
>>> 
>>> Say you buy a Jetson TK1 to control that sprinkler valve (a good use
>>> for all these GPU cores!). The mainline DT has no knowledge about the
>>> valve, so using your proposed way you will have to re-build and flash
>>> a custom device tree just to be able to use your sprinkler. If you
>>> decide to assign your board to something else, you will need another
>>> DT.
>> 
>> But if you buy more "real world ready" board with for example 8 power
>> outputs for sprinkler valves, why has user think about what gpio to
>> export, if there could be valve0 - 7 ready. If he will use only valve0
>> does not mater, all unused gpio can't be used for anything else since
>> they has layout for valves.
> 
> The problem is indeed different.
> 
> If your sprinkler valves are wired on your board and the lines cannot
> be used for anything else, then what you need is a driver for the
> valve that will acquire the GPIOs and export its own userspace
> interface under /sys/.
> 
> You device tree must reflect the layout of the board ; in this case
> "here is a valve that uses this GPIO active-low". Saying "this GPIO is
> output active-low and named valve0" is not a good description of your
> hardware.
> 

There is no higher layer driver API. We don’t want to litter the kernel
with ‘valve’ drivers, there’s no such thing. There’s only digital I/Os
that’s connected to low speed devices.

If we are going for a higher layer API that would be something very thin
like a user-space GPIO driver. Do you see the conundrum?

>>> 
>>> 
>>> Instead, have a shell script or a systemd unit tmpfile that will
>>> perform the correct setup at boot time. Then you can easily switch
>>> usages from user-space (systemctl stop sprinkler && systemctl start
>>> doordetect). This is much, much more flexible. At least until the
>>> Device Tree Overlays are merged, but even then this seems overkill.
>> 
>> I seriously think about it, but I think if LEDs are in DT, GPIOs should
>> be there too. In other way, I don't like exporting without given name.
> 
> LEDs are actually a perfect example of what you should do. They are a
> simple device often plugged to a GPIO that controls them. We could
> very well do what you propose: export GPIOs named "ledXX" and call it
> a day. Instead we have a driver that exports a proper device node in
> /sys with the operations that are relevant to a LED.
> 
> Once your GPIOs are assigned to a given function, they are not
> "general purpose" anymore. Therefore you should expose the right
> abstraction to your users, even if that means writing some more code.

Please, try to understand how our users use our linux kernel drivers.

Jiri’s concerns are valid and I have seen his use patterns in countless
other cases.

Regards

— Pantelis
  --
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html