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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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?

> I also wanted to know if there are any security concerns with
> the patch sent.

None that I can think of. Security concerns on a one-off
test rig should not be a big issue anyways.

Linus Walleij