Re: [PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines

[Harish Jenny K N <harish_kandiga@xxxxxxxxxx>]

On 07/05/19 1:58 PM, Linus Walleij wrote:
> On Mon, May 6, 2019 at 11:15 AM Harish Jenny K N
> <harish_kandiga@xxxxxxxxxx> wrote:
>
>> Again I wanted to highlight that the intention of the patch was to make it generic
>> and avoid changes in userspace for different hardware samples. (i.e  Some pins in
>> hardware be configured as active low, this can vary between hardware samples
> First, can you explain what you mean by that? Is it that you mean to use the
> kernel gpiolib for prototyping, so we are not talking about production
> systems, such as any kind of product coming off a factory line?
>
> In that case I think it is in the maker-prototyping charter, which means
> it is actually appropriate for having that configuration in userspace,
> since it is a one-off. It will not have any generic use. The kernel is
> generally for reusable stuff.
>
> Second, I question the use of a property on the gpio chip for this. I
> highly doubt
> that the silicon chip will be manufactured with some random inverters
> on some lines depending on which silicon sample we are using.
> (Correct me if I'm wrong.)
>
> What I think is happening is that you are using different PCBs or
> wiremeshes and you have inverters outside the gpio chip.
> That should not be a property of the gpio chip.
>
> In this case what you need is either encode it on the consumer side
> as we already do, or start to model inverters in the device tree
> to properly describe the hardware, so we have a hierarchy of
> gpio lines:
>
> gpio0: gpio {
>    compatible = "foo,chip";
>    gpio-controller;
>    (...)
> };
>
> inv0: inverter {
>     compatible = "inverter";
>     gpio-controller;
>     gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
> };
>
> consumer {
>    compatible = "bar";
>    gpios = <&inv0 0 GPIO_ACTIVE_HIGH>;
> };
>
> This is a rough sketch, it would need some elaborate thinking
> and DT-bindings and changes in gpiolib to deal with those
> inverters as gpiochips.
>
> It is a better model of what is really happening: altering the
> polarity on the gpiochip is wrong since the signal out from
> the chip is actually the same, and altering the consumer flag
> as we do today is also wrong because the component does
> have a very specific polarity.
>
> We have several boards like this already, but they all just
> summarize the inversions happening between the gpio chip
> and the consumer and put the resulting flag in the consumer
> polarity flag, so no explicit inverters in the device tree so far.
> This is a simplification of the actual electronics, but the goal
> with those device trees is running systems, not perfect
> abstraction of hardware in the device tree.
>
> However your usecase might warrant an introduction of
> this inverter concept, if it is like you say that you get new stuff
> every week that need testing and you like to use the DT to
> help with this. Again, this is under the assumption that you
> are actually not changing the GPIO chip, just the PCB.
>
> But I think real inverter nodes is what you should use if this
> is your usecase.
>
>> . User application uses gpio-line-name property to map pins
>> and port, this helps the application to handle pin change
>> from hardware sample to sample.
> I'm happy you can use this :)
> I worked a lot to make that available.
>
>> As of now there is no
>> configuration available for user space applications for polarity.)
> I think GPIOHANDLE_REQUEST_ACTIVE_LOW does
> that? Is there some misunderstanding?


Sorry I was not aware of this earlier. But yes, I confirmed that we could GPIOHANDLE_REQUEST_ACTIVE_LOW.

Thanks for the detailed comments.

Yes, We are talking about different PCBs. An example would be a product PCB A and a product PCB B, mainly identical, but due to slightly different hardware different GPIO polarity on some lines. Driven by the same Linux kernel and user space, just different device trees.

The proposal of "inverters" defining the polarity configuration makes sense.


Thanks,

Harish Jenny K N