diff mbox series

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

Message ID 1557122501-5183-2-git-send-email-harish_kandiga@mentor.com
State New
Headers show
Series gpio: set active-state of GPIO lines using device tree | expand

Commit Message

Harish Jenny K N May 6, 2019, 6:01 a.m. UTC 
Device could decide to have different convention about what "active" means.
( i.e Active-High (output signal "1" means "active", the default)
and Active-Low (output signal "0" means "active")).
Therefore it is possible to define a GPIO as being either active-high or
active-low .

Make it possible to add the information of active state of gpio pin
as property into device tree configuration using a
"active-state" property u8 array.

This is useful for user space applications to identify
active state of pins.

This commit updates gpio_desc flag for active-state.

Note: The active-state attribute is completely optional.

example device tree line
"active-state = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0>;"

Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
---
 drivers/gpio/gpiolib-devprop.c | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib-of.c      |  3 +++
 drivers/gpio/gpiolib.h         |  3 +++
 3 files changed, 44 insertions(+)

--
2.7.4

Comments

Linus Walleij May 6, 2019, 6:57 a.m. UTC | #1 
Hi Harish,

thank you for your patch!

On Mon, May 6, 2019 at 8:02 AM Harish Jenny K N
<harish_kandiga@mentor.com> wrote:

> Device could decide to have different convention about what "active" means.
> ( i.e Active-High (output signal "1" means "active", the default)
> and Active-Low (output signal "0" means "active")).
> Therefore it is possible to define a GPIO as being either active-high or
> active-low .
>
> Make it possible to add the information of active state of gpio pin
> as property into device tree configuration using a
> "active-state" property u8 array.
>
> This is useful for user space applications to identify
> active state of pins.
>
> This commit updates gpio_desc flag for active-state.
>
> Note: The active-state attribute is completely optional.
>
> example device tree line
> "active-state = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0>;"
>
> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>

I see from the code that the intention of the code is to
encode on the producer side (the GPIO chip) whether to handle
a certain line with active high or active low semantics.
In effect setting FLAG_ACTIVE_LOW on some descriptors.

The usual convention is to refer to this as "polarity" so
you would probably want to name it "polarity" rather than
"active-state".

However:

This is problematic because the convention in the device
tree is to encode this on the consumer side and not on
the producer side, so e.g. a device node using a GPIO
line will use something like this:

                button@1 {
                        debounce-interval = <50>;
                        wakeup-source;
                        linux,code = <2>;
                        label = "userpb";
                        gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;
                };

If the polarity differs between the consumer and the
producer, who is going to win? Defining polarity on both
sides is going to lead to ambiguities.

Even when using GPIOs from userspace (which I do not
recommend) the character device suppors a polarity flag
GPIOLINE_FLAG_ACTIVE_LOW so also userspace
consumers define polarity.

What kind of consumers do you have in mind for this
feature?

Yours,
Linus Walleij
Harish Jenny K N May 6, 2019, 7:57 a.m. UTC | #2 
Hi,

On 06/05/19 12:27 PM, Linus Walleij wrote:
> Hi Harish,
>
> thank you for your patch!
>
> On Mon, May 6, 2019 at 8:02 AM Harish Jenny K N
> <harish_kandiga@mentor.com> wrote:
>
>> Device could decide to have different convention about what "active" means.
>> ( i.e Active-High (output signal "1" means "active", the default)
>> and Active-Low (output signal "0" means "active")).
>> Therefore it is possible to define a GPIO as being either active-high or
>> active-low .
>>
>> Make it possible to add the information of active state of gpio pin
>> as property into device tree configuration using a
>> "active-state" property u8 array.
>>
>> This is useful for user space applications to identify
>> active state of pins.
>>
>> This commit updates gpio_desc flag for active-state.
>>
>> Note: The active-state attribute is completely optional.
>>
>> example device tree line
>> "active-state = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0>;"
>>
>> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> I see from the code that the intention of the code is to
> encode on the producer side (the GPIO chip) whether to handle
> a certain line with active high or active low semantics.
> In effect setting FLAG_ACTIVE_LOW on some descriptors.
>
> The usual convention is to refer to this as "polarity" so
> you would probably want to name it "polarity" rather than
> "active-state".

I agree. Thanks.

>
> However:
>
> This is problematic because the convention in the device
> tree is to encode this on the consumer side and not on
> the producer side, so e.g. a device node using a GPIO
> line will use something like this:
>
>                 button@1 {
>                         debounce-interval = <50>;
>                         wakeup-source;
>                         linux,code = <2>;
>                         label = "userpb";
>                         gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;
>                 };
>
> If the polarity differs between the consumer and the
> producer, who is going to win? Defining polarity on both
> sides is going to lead to ambiguities.
Can the userspace consumers define the polarity?

Intention was to define polarity for lines which are not having consumers from kernelspace. But yes, there is a possibility of ambiguity if both consumer and producer start defining polarity. But again both consumer and producer should define the same polarity in device tree.
>
> Even when using GPIOs from userspace (which I do not
> recommend) the character device suppors a polarity flag
> GPIOLINE_FLAG_ACTIVE_LOW so also userspace
> consumers define polarity.
yes. aware of the GPIOLINE_FLAG_ACTIVE_LOW flag to get the status.
> What kind of consumers do you have in mind for this
> feature?
Intention is to make it generic. Some pins in hardware be configured as active low, this can vary between hardware samples. 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. As of now there is no configuration available for user space applications for polarity.


Thanks,

Harish
Linus Walleij May 6, 2019, 8:32 a.m. UTC | #3 
On Mon, May 6, 2019 at 9:57 AM Harish Jenny K N
<harish_kandiga@mentor.com> wrote:

> Can the userspace consumers define the polarity?

Yes. From userspace after opening the GPIO character
device:

#include <linux/gpio.h>

struct gpiohandle_request req;

req.flags = GPIOHANDLE_REQUEST_ACTIVE_LOW | GPIOHANDLE_REQUEST_OUTPUT;
req.lines = 1;
req.lineoffsets[0] = 0;

ret = ioctl(fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
(...)

For more details on how to use the character device see tools/gpio/*
in the kernel tree.

> Intention was to define polarity for lines which are not having consumers from kernelspace.

OK the above should work fine :)

> > Even when using GPIOs from userspace (which I do not
> > recommend) the character device suppors a polarity flag
> > GPIOLINE_FLAG_ACTIVE_LOW so also userspace
> > consumers define polarity.
>
> yes. aware of the GPIOLINE_FLAG_ACTIVE_LOW flag to get the status.

Sorry, I was being unclear,
GPIOHANDLE_REQUEST_ACTIVE_LOW
is what you want to use.

Yours,
Linus Walleij
Harish Jenny K N May 6, 2019, 9:15 a.m. UTC | #4 
On 06/05/19 2:02 PM, Linus Walleij wrote:
> On Mon, May 6, 2019 at 9:57 AM Harish Jenny K N
> <harish_kandiga@mentor.com> wrote:
>
>> Can the userspace consumers define the polarity?
> Yes. From userspace after opening the GPIO character
> device:
>
> #include <linux/gpio.h>
>
> struct gpiohandle_request req;
>
> req.flags = GPIOHANDLE_REQUEST_ACTIVE_LOW | GPIOHANDLE_REQUEST_OUTPUT;
> req.lines = 1;
> req.lineoffsets[0] = 0;
>
> ret = ioctl(fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
> (...)
>
> For more details on how to use the character device see tools/gpio/*
> in the kernel tree.
>
>> Intention was to define polarity for lines which are not having consumers from kernelspace.
> OK the above should work fine :)
>
>>> Even when using GPIOs from userspace (which I do not
>>> recommend) the character device suppors a polarity flag
>>> GPIOLINE_FLAG_ACTIVE_LOW so also userspace
>>> consumers define polarity.
>> yes. aware of the GPIOLINE_FLAG_ACTIVE_LOW flag to get the status.
> Sorry, I was being unclear,
> GPIOHANDLE_REQUEST_ACTIVE_LOW
> is what you want to use.

Thanks. I will explore GPIOHANDLE_REQUEST_ACTIVE_LOW to see if we can use this.

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. 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. As of now there is no configuration available for user space applications for polarity.)

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


Thanks,

Harish
Phil Reid May 6, 2019, 9:57 a.m. UTC | #5 
On 6/05/2019 17:15, Harish Jenny K N wrote:
> 
> On 06/05/19 2:02 PM, Linus Walleij wrote:
>> On Mon, May 6, 2019 at 9:57 AM Harish Jenny K N
>> <harish_kandiga@mentor.com> wrote:
>>
>>> Can the userspace consumers define the polarity?
>> Yes. From userspace after opening the GPIO character
>> device:
>>
>> #include <linux/gpio.h>
>>
>> struct gpiohandle_request req;
>>
>> req.flags = GPIOHANDLE_REQUEST_ACTIVE_LOW | GPIOHANDLE_REQUEST_OUTPUT;
>> req.lines = 1;
>> req.lineoffsets[0] = 0;
>>
>> ret = ioctl(fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
>> (...)
>>
>> For more details on how to use the character device see tools/gpio/*
>> in the kernel tree.
>>
>>> Intention was to define polarity for lines which are not having consumers from kernelspace.
>> OK the above should work fine :)
>>
>>>> Even when using GPIOs from userspace (which I do not
>>>> recommend) the character device suppors a polarity flag
>>>> GPIOLINE_FLAG_ACTIVE_LOW so also userspace
>>>> consumers define polarity.
>>> yes. aware of the GPIOLINE_FLAG_ACTIVE_LOW flag to get the status.
>> Sorry, I was being unclear,
>> GPIOHANDLE_REQUEST_ACTIVE_LOW
>> is what you want to use.
> 
> Thanks. I will explore GPIOHANDLE_REQUEST_ACTIVE_LOW to see if we can use this.
> 
> 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. 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. As of now there is no configuration available for user space applications for polarity.)
> 

This is pretty useful to be able to abstract polarity (and other properties from userspace).

I was thinking of adding a virtual gpio provider for one pin that just uses the dt consumer bindings to hide
polarities etc from userspace.

Or use the gpio led driver (or similar)
Harish Jenny K N May 7, 2019, 7:04 a.m. UTC | #6 
On 06/05/19 3:27 PM, Phil Reid wrote:
> On 6/05/2019 17:15, Harish Jenny K N wrote:
>>
>> On 06/05/19 2:02 PM, Linus Walleij wrote:
>>> On Mon, May 6, 2019 at 9:57 AM Harish Jenny K N
>>> <harish_kandiga@mentor.com> wrote:
>>>
>>>> Can the userspace consumers define the polarity?
>>> Yes. From userspace after opening the GPIO character
>>> device:
>>>
>>> #include <linux/gpio.h>
>>>
>>> struct gpiohandle_request req;
>>>
>>> req.flags = GPIOHANDLE_REQUEST_ACTIVE_LOW | GPIOHANDLE_REQUEST_OUTPUT;
>>> req.lines = 1;
>>> req.lineoffsets[0] = 0;
>>>
>>> ret = ioctl(fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
>>> (...)
>>>
>>> For more details on how to use the character device see tools/gpio/*
>>> in the kernel tree.
>>>
>>>> Intention was to define polarity for lines which are not having consumers from kernelspace.
>>> OK the above should work fine :)
>>>
>>>>> Even when using GPIOs from userspace (which I do not
>>>>> recommend) the character device suppors a polarity flag
>>>>> GPIOLINE_FLAG_ACTIVE_LOW so also userspace
>>>>> consumers define polarity.
>>>> yes. aware of the GPIOLINE_FLAG_ACTIVE_LOW flag to get the status.
>>> Sorry, I was being unclear,
>>> GPIOHANDLE_REQUEST_ACTIVE_LOW
>>> is what you want to use.
>>
>> Thanks. I will explore GPIOHANDLE_REQUEST_ACTIVE_LOW to see if we can use this.
>>
>> 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. 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. As of now there is no configuration available for user space applications for polarity.)
>>
>
> This is pretty useful to be able to abstract polarity (and other properties from userspace).
>
> I was thinking of adding a virtual gpio provider for one pin that just uses the dt consumer bindings to hide
> polarities etc from userspace.


Thanks for the input.

Can the maintainers please confirm how does they want to have this implemented?

i.e How to define the polarity for generic GPIOs exported to user space in the device tree, to be able to make user space GPIO usage independent of hardware revisions?


Thanks,

Harish Jenny K N
Linus Walleij May 7, 2019, 8:28 a.m. UTC | #7 
On Mon, May 6, 2019 at 11:15 AM Harish Jenny K N
<harish_kandiga@mentor.com> 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
Harish Jenny K N May 7, 2019, 11:12 a.m. UTC | #8 
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@mentor.com> 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
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-devprop.c b/drivers/gpio/gpiolib-devprop.c
index dd51709..4b3fbd4 100644
--- a/drivers/gpio/gpiolib-devprop.c
+++ b/drivers/gpio/gpiolib-devprop.c
@@ -10,6 +10,7 @@ 
 #include <linux/slab.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>

 #include "gpiolib.h"

@@ -56,3 +57,40 @@  void devprop_gpiochip_set_names(struct gpio_chip *chip,

 	kfree(names);
 }
+
+/**
+ * devprop_gpiochip_set_active_state - Set GPIO line active state using
+ * device properties
+ * @chip: GPIO chip whose lines should be set
+ * @fwnode: Property Node containing the active-state property
+ *
+ * Looks for device property "active-state" and if it exists assigns
+ * GPIO line active states for the chip.
+ */
+void devprop_gpiochip_set_active_state(struct gpio_chip *chip,
+				       const struct fwnode_handle *fwnode)
+{
+	struct gpio_device *gdev = chip->gpiodev;
+	int ret, i;
+	u8 *states;
+
+	states = kcalloc(gdev->ngpio, sizeof(u8), GFP_KERNEL);
+	if (!states)
+		return;
+
+	ret = fwnode_property_read_u8_array(fwnode, "active-state",
+					    states, gdev->ngpio);
+
+	if (ret < 0) {
+		dev_warn(&gdev->dev, "failed to read GPIO active-state\n");
+		kfree(states);
+		return;
+	}
+
+	for (i = 0; i < gdev->ngpio; i++) {
+		if (states[i] & GPIO_ACTIVE_LOW)
+			set_bit(FLAG_ACTIVE_LOW, &gdev->descs[i].flags);
+	}
+
+	kfree(states);
+}
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 6a3ec57..2279b3d 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -716,6 +716,9 @@  int of_gpiochip_add(struct gpio_chip *chip)
 		devprop_gpiochip_set_names(chip,
 					   of_fwnode_handle(chip->of_node));

+	devprop_gpiochip_set_active_state(chip,
+					  of_fwnode_handle(chip->of_node));
+
 	of_node_get(chip->of_node);

 	status = of_gpiochip_scan_gpios(chip);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 078ab17..0b5f6f3 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -246,6 +246,9 @@  static inline int gpio_chip_hwgpio(const struct gpio_desc *desc)
 void devprop_gpiochip_set_names(struct gpio_chip *chip,
 				const struct fwnode_handle *fwnode);

+void devprop_gpiochip_set_active_state(struct gpio_chip *chip,
+				       const struct fwnode_handle *fwnode);
+
 /* With descriptor prefix */

 #define gpiod_emerg(desc, fmt, ...)					       \