gpio: add set_active_low() function to gpio_chip

RFC In review David Cohen 4 participants 4 messages Mar 18, 2015 – Mar 27, 2015 GPIO

Share & tools

David Cohen

author

2015-03-18 20:14 UTC #0 lore raw

RFC 1/1 gpio: add set_active_low() function to gpio_chip

Some gpio controllers are capable of programming its pins' active-low state. Let's add this new gpio_chip function for such cases and use it in gpiolib.

When set_active_low() is implemented, we no longer need to do soft flips on values from non-raw get functions.

Signed-off-by David Cohen <[email protected]>

---

Hi,

This is a RFC, not meant for integration (yet).

We have a GPIO controller that is capable of inverting the GPIO logical value on hardware using a register (and GPIO voltage level if configured for output):
- If GPIO pin is configured for input, only the logical value is affected: the GPIO level stays the same but the read values are inverted (it affects even interrupt event triggers).
- If GPIO pin is configured for output, the GPIO level is also inverted.

Is it acceptable to expose this functionality via new gpio chip operation set_active_low()?

Comments are welcome.

Br, David
---

 drivers/gpio/gpiolib-sysfs.c | 4 ++++
 drivers/gpio/gpiolib.c       | 6 ++++--
 include/linux/gpio/driver.h  | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

drivers/gpio/gpiolib-sysfs.c +4 -0

index 7722ed53bd65..316a1d008cad 100644
@@ -289,11 +289,15 @@ static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store);
 static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
 				int value)
 {
+	struct gpio_chip	*chip = desc->chip;
 	int			status = 0;
 
 	if (!!test_bit(FLAG_ACTIVE_LOW, &desc->flags) == !!value)
 		return 0;
 
+	if (chip->set_active_low)
+		chip->set_active_low(chip, gpio_chip_hwgpio(desc), value);
+
 	if (value)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
 	else

drivers/gpio/gpiolib.c +4 -2

index 1ca9295b2c10..028a0d34ddab 100644
@@ -1196,7 +1196,8 @@ int gpiod_get_value(const struct gpio_desc *desc)
 	WARN_ON(desc->chip->can_sleep);
 
 	value = _gpiod_get_raw_value(desc);
-	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
+	if (!desc->chip->set_active_low &&
+	    test_bit(FLAG_ACTIVE_LOW, &desc->flags))
 		value = !value;
 
 	return value;

@@ -1550,7 +1551,8 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc)
 		return 0;
 
 	value = _gpiod_get_raw_value(desc);
-	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
+	if (!desc->chip->set_active_low &&
+	    test_bit(FLAG_ACTIVE_LOW, &desc->flags))
 		value = !value;
 
 	return value;

include/linux/gpio/driver.h +2 -0

index c497c62889d1..2d665352fd69 100644
@@ -90,6 +90,8 @@ struct gpio_chip {
 						unsigned offset);
 	void			(*set)(struct gpio_chip *chip,
 						unsigned offset, int value);
+	void			(*set_active_low)(struct gpio_chip *chip,
+						unsigned offset, int value);
 	void			(*set_multiple)(struct gpio_chip *chip,
 						unsigned long *mask,
 						unsigned long *bits);

Signature · 1 line

2.1.4

Alexandre Courbot

2015-03-19 02:07 UTC #1 lore raw

Quoted 21 lines · David Cohen

Some gpio controllers are capable of programming its pins' active-low state. Let's add this new gpio_chip function for such cases and use it in gpiolib.
When set_active_low() is implemented, we no longer need to do soft flips on values from non-raw get functions.
Signed-off-by David Cohen <[email protected]>
---
Hi,
This is a RFC, not meant for integration (yet).
We have a GPIO controller that is capable of inverting the GPIO logical value on hardware using a register (and GPIO voltage level if configured for output):
- If GPIO pin is configured for input, only the logical value is affected: the GPIO level stays the same but the read values are inverted (it affects even interrupt event triggers).
- If GPIO pin is configured for output, the GPIO level is also inverted.

Quoted 2 lines

Is it acceptable to expose this functionality via new gpio chip operation set_active_low()?

At first I thought "why not", but then I couldn't help but ask "why?"

Handling the active low state in software is basically free and requires no extra support. What is the benefit of handling it in hardware that would counterbalance the complexity this adds to the GPIO framework?

Also, wouldn't that require us to reverse this active-low logic in the *_raw functions for them to work as expected?

Stephen Warren

2015-03-19 15:33 UTC #2 lore raw

Quoted 28 lines · Alexandre Courbot

Quoted 21 lines · David Cohen
Some gpio controllers are capable of programming its pins' active-low state. Let's add this new gpio_chip function for such cases and use it in gpiolib.
When set_active_low() is implemented, we no longer need to do soft flips on values from non-raw get functions.
Signed-off-by David Cohen <[email protected]>
---
Hi,
This is a RFC, not meant for integration (yet).
We have a GPIO controller that is capable of inverting the GPIO logical value on hardware using a register (and GPIO voltage level if configured for output):
- If GPIO pin is configured for input, only the logical value is affected: the GPIO level stays the same but the read values are inverted (it affects even interrupt event triggers).
- If GPIO pin is configured for output, the GPIO level is also inverted.
Quoted 2 lines
Is it acceptable to expose this functionality via new gpio chip operation set_active_low()?
At first I thought "why not", but then I couldn't help but ask "why?"

Quoted 7 lines

Handling the active low state in software is basically free and requires no extra support. What is the benefit of handling it in hardware that would counterbalance the complexity this adds to the GPIO framework?
Also, wouldn't that require us to reverse this active-low logic in the *_raw functions for them to work as expected?

What happens when some generic driver starts to make use of this new API because it's convenient? Then that API has to work everywhere, including on chips that don't support this feature in HW. Then, we're back to emulating it in SW on those platforms. Given how trivial it is to emulate, shouldn't we emulate it everywhere, so that the code paths are identical everywhere?

Linus Walleij

2015-03-27 08:45 UTC #3 lore raw

In reply to David Cohen

Quoted 6 lines · David Cohen

+++ b/drivers/gpio/gpiolib-sysfs.c

drivers/gpio/gpiolib-sysfs.c +1 -0

@@ -289,11 +289,15 @@ static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store);
 static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
                                int value)
 {
+       struct gpio_chip        *chip = desc->chip;

drivers/gpio/gpiolib-sysfs.c static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store); · line 302

Quoted 10 lines

drivers/gpio/gpiolib-sysfs.c +3 -0

        int                     status = 0;
 
        if (!!test_bit(FLAG_ACTIVE_LOW, &desc->flags) == !!value)
                return 0;
 
+       if (chip->set_active_low)
+               chip->set_active_low(chip, gpio_chip_hwgpio(desc), value);
+
        if (value)
                set_bit(FLAG_ACTIVE_LOW, &desc->flags);

Since the only user seems to be this abominable sysfs, and since that is definitely not on the hotpath due to context switching, this cannot possibly gain any performance benefits.

Unless we can come up with a use case where this actually improves performance we should just let software handle this instead.

I like to have all hardware features implemented usually, so I'm a bit torn. But this one lacks a plausible use case. It looks like some hardware engineers attempt to be helpful to software developers.

Any takers?

Yours,
Linus Walleij