Re: [PATCH] gpio: add gpio_of_helper

["Jiří Prchal" <jiri.prchal@aksignal.cz>]

Dne 23.10.2014 v 10:53 Alexandre Courbot napsal(a):
> On Thu, Oct 23, 2014 at 3:23 PM, Jiří Prchal <jiri.prchal@aksignal.cz> wrote:
>>
>>
>> Dne 23.10.2014 v 07:08 Alexandre Courbot napsal(a):
>>
>>> On Wed, Oct 22, 2014 at 6:58 PM, Pantelis Antoniou
>>> <panto@antoniou-consulting.com> wrote:
>>>>
>>>> Hi Alexandre,
>>>>
>>>>> On Oct 22, 2014, at 12:41 PM, Alexandre Courbot <gnurou@gmail.com>
>>>>> wrote:
>>>>>
>>>>> On Wed, Oct 22, 2014 at 6:24 PM, Pantelis Antoniou
>>>>> <panto@antoniou-consulting.com> wrote:
>>>>>>
>>>>>> Hi Alexandre,
>>>>>>
>>>>>>> On Oct 22, 2014, at 12:18 PM, Alexandre Courbot <gnurou@gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>> On Wed, Oct 22, 2014 at 5:26 PM, Jiri Prchal <jiri.prchal@aksignal.cz>
>>>>>>> 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.
How to? It's just connected to ttyS and after apply power it has to be switched
on by pulling some pin.
>
>> Or some optocoupler isoleted in/outs for general use from userspace.
>
> ... which should thus be configured from userspace.
Why not from dt?
>
>>>
>>>
>>> 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.
But we don't need anything else just raw named gpio.
>
>> 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.
Maybe I didn't give good example.
There is a board. It has 8 outputs. They are relay outputs. Let's give them
rel0 - 7 name. And user can same of them use for valve, some for light...
Ant board has 8 inputs. They are optocoupler isolated. And user can some of
them use to get water level, light...
>
>>>
>>>
>>> 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.
So making some kind of device "inout" wold be good idea?

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