Re: [PATCH] gpio: add gpio_of_helper

[Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx>]

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.

> We need to understand why this is needed, and then see if the DT
> bindings are acceptable before going any further.

I can take care of the bindings if we get into some kind of agreement on the basic
interface. I’m CCing device tree and the DT maintainer.

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