Re: [PATCH] gpio: add gpio_of_helper

[Alexandre Courbot <gnurou@xxxxxxxxx>]

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". :)

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.

We need to understand why this is needed, and then see if the DT
bindings are acceptable before going any further.
--
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