gpio: export 'debounce' attribute if supported by the gpio chip

PATCH In review Guenter Roeck 6 participants 14 messages Dec 6, 2012 – Dec 13, 2012 GPIO

lore

Share & tools

lore

Guenter Roeck

author

2012-12-06 06:32 UTC #0 lore raw

PATCH 1/1 gpio: export 'debounce' attribute if supported by the gpio chip

Create a 'debounce' attribute if debounce is supported by the gpio chip and a gpio pin is exported.

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/gpio/gpiolib.c |   59 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)

drivers/gpio/gpiolib.c +59 -0

index a971e3d..13ee9a7 100644
@@ -67,6 +67,7 @@ struct gpio_desc {
 #ifdef CONFIG_DEBUG_FS
 	const char		*label;
 #endif
+	unsigned		debounce;
 };
 static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];

@@ -215,6 +216,10 @@ static DEFINE_MUTEX(sysfs_lock);
  *      * is read/write as zero/nonzero
  *      * also affects existing and subsequent "falling" and "rising"
  *        /edge configuration
+ *   /debounce
+ *      * configures debounce time in uS
+ *      * available only if debounce is supported by the chip
+ *      * is read/write; 0 to disable or debounce time
  */
 
 static ssize_t gpio_direction_show(struct device *dev,

@@ -320,6 +325,55 @@ static ssize_t gpio_value_store(struct device *dev,
 static const DEVICE_ATTR(value, 0644,
 		gpio_value_show, gpio_value_store);
 
+static ssize_t gpio_debounce_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	ssize_t status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else
+		status = sprintf(buf, "%u\n", desc->debounce);
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static ssize_t gpio_debounce_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t size)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	unsigned		gpio = desc - gpio_desc;
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else if (test_bit(FLAG_IS_OUT, &desc->flags))
+		status = -EPERM;
+	else {
+		long		value;
+
+		status = kstrtoul(buf, 0, &value);
+		if (status == 0) {
+			status = gpio_set_debounce(gpio, value);
+			if (status == 0)
+				status = size;
+		}
+	}
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static const DEVICE_ATTR(debounce, 0644,
+			 gpio_debounce_show, gpio_debounce_store);
+
 static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
 	struct sysfs_dirent	*value_sd = priv;

@@ -741,6 +795,10 @@ int gpio_export(unsigned gpio, bool direction_may_change)
 				status = device_create_file(dev,
 						&dev_attr_direction);
 
+			if (!status && desc->chip->set_debounce)
+				status = device_create_file(dev,
+						&dev_attr_debounce);
+
 			if (!status && gpio_to_irq(gpio) >= 0
 					&& (direction_may_change
 						|| !test_bit(FLAG_IS_OUT,

@@ -1507,6 +1565,7 @@ int gpio_set_debounce(unsigned gpio, unsigned debounce)
 
 	might_sleep_if(chip->can_sleep);
 
+	desc->debounce = debounce;
 	return chip->set_debounce(chip, gpio, debounce);
 
 fail:

Signature · 1 line

1.7.5.4

Linus Walleij

2012-12-07 08:07 UTC #1 lore raw

Quoted 4 lines · Guenter Roeck

Create a 'debounce' attribute if debounce is supported by the gpio chip and a gpio pin is exported.
Signed-off-by Guenter Roeck <[email protected]>

Can you describe the usecase for this?

I have this problem when working as a back-up GPIO maintainer that I don't really understand the userspace apps doing this.

I would guess something like a userspace app reading a GPIO switch and needing to set this to avoid key bounces, but it'd be nice to know if this is really the case.

If this is the usecase I am slightly concerned why these are not used:
drivers/input/keyboard/gpio_keys_polled.c drivers/input/keyboard/gpio_keys.c

The latter even uses the in-kernel debounce interface.

I'd agree if this is not user input at all but something like a switch in a factory production line.

So please help me understand this.

Yours,
Linus Walleij

Guenter Roeck

2012-12-07 14:59 UTC #2 lore raw

Quoted 7 lines · Linus Walleij

Quoted 4 lines · Guenter Roeck
Create a 'debounce' attribute if debounce is supported by the gpio chip and a gpio pin is exported.
Signed-off-by Guenter Roeck <[email protected]>

Quoted 9 lines

Can you describe the usecase for this?
I have this problem when working as a back-up GPIO maintainer that I don't really understand the userspace apps doing this.
I would guess something like a userspace app reading a GPIO switch and needing to set this to avoid key bounces, but it'd be nice to know if this is really the case.

Yes, that is one if the use cases. Button pressed on the chassis/board requesting user space action. Another is board presence detect pins which require rebounce support and are handled in user space. Yes, the later should be handled in the kernel, and most of them are, but there are some which don't need immediate kernel activity and are handled completely by applications.

There may be other use cases - there are hundreds of gpio pins in the system I am working on, and I have not looked into all of them.

Quoted 9 lines

If this is the usecase I am slightly concerned why these are not used:
drivers/input/keyboard/gpio_keys_polled.c drivers/input/keyboard/gpio_keys.c
The latter even uses the in-kernel debounce interface.
I'd agree if this is not user input at all but something like a switch in a factory production line.

I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.

Thanks,
Guenter

Guenter Roeck

2012-12-07 16:20 UTC #3 lore raw

Quoted 17 lines · Guenter Roeck

Quoted 7 lines · Linus Walleij
Quoted 4 lines · Guenter Roeck
Create a 'debounce' attribute if debounce is supported by the gpio chip and a gpio pin is exported.
Signed-off-by Guenter Roeck <[email protected]>
Quoted 9 lines
Can you describe the usecase for this?
I have this problem when working as a back-up GPIO maintainer that I don't really understand the userspace apps doing this.
I would guess something like a userspace app reading a GPIO switch and needing to set this to avoid key bounces, but it'd be nice to know if this is really the case.

Quoted 9 lines

Yes, that is one if the use cases. Button pressed on the chassis/board requesting user space action. Another is board presence detect pins which require rebounce support and are handled in user space. Yes, the later should be handled in the kernel, and most of them are, but there are some which don't need immediate kernel activity and are handled completely by applications.
There may be other use cases - there are hundreds of gpio pins in the system I am working on, and I have not looked into all of them.

There are three use cases, all related to each other.

- board present (connector pin)
- board removal request (button)
- board voltage good (connector pin)

Guenter

Quoted 15 lines

Quoted 9 lines
If this is the usecase I am slightly concerned why these are not used:
drivers/input/keyboard/gpio_keys_polled.c drivers/input/keyboard/gpio_keys.c
The latter even uses the in-kernel debounce interface.
I'd agree if this is not user input at all but something like a switch in a factory production line.
I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.
Thanks,
Guenter

Alan Cox

2012-12-07 16:43 UTC #4 lore raw

In reply to Guenter Roeck

Quoted 2 lines

I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.

Android tries to address this with its switch class driver, but I'm not sure its actually got anything over making them input devices.

Alan

anish kumar

2012-12-09 09:58 UTC #5 lore raw

Quoted 5 lines · Alan Cox

Quoted 2 lines
I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.
Android tries to address this with its switch class driver, but I'm not sure its actually got anything over making them input devices.

Sorry for not understanding the context here.How the debounce sysfs added by Guenter has anything to do with switch driver in android?

AFAIK android just uses switch driver api to talk to driver to get the status of the headset and the status of the keys if headset has any.This API in turn updates the sysfs to let userspace know the status change using kobject uevent.

Switch driver is replaced by extcon in the mainline.

Quoted 7 lines

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Alan Cox

2012-12-09 10:58 UTC #6 lore raw

Quoted 9 lines · On Sun, 09 Dec 2012 01:58:19 -0800 anish kumar <[email protected]>…

Quoted 5 lines · Alan Cox
Quoted 2 lines
I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.
Android tries to address this with its switch class driver, but I'm not sure its actually got anything over making them input devices.
Sorry for not understanding the context here.How the debounce sysfs added by Guenter has anything to do with switch driver in android?

The other more general option is to make the input layer do the debounce and make them all inputs rather than just relying on any gpio layer support.

Guenter Roeck

2012-12-09 17:07 UTC #7 lore raw

Quoted 10 lines · Alan Cox

Quoted 7 lines · On Sun, 09 Dec 2012 01:58:19 -0800 anish kumar <[email protected]>…
Quoted 5 lines · Alan Cox
Quoted 2 lines
I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.
Android tries to address this with its switch class driver, but I'm not sure its actually got anything over making them input devices.

Quoted 7 lines

Quoted 2 lines
Sorry for not understanding the context here.How the debounce sysfs added by Guenter has anything to do with switch driver in android?
The other more general option is to make the input layer do the debounce and make them all inputs rather than just relying on any gpio layer support.

The gpio pins I am dealing with are provided by an FPGA which is used on various boards. While the gpio access registers are always the same, the actual usage is board specific. This means I either need to write ugly code, or use the gpio subsystem to provide access to the gpio pins. Ugly code is out of the question, which means I'll need gpio support.

Anyway, I want to keep things simple, not add unnecessary complexity. Having to go through the input subsystem just to be able to support debounce on a couple of input pins doesn't really sound simple. Guess I'll have to find another solution if the patch is not accepted. Maybe I'll add a "debounce" property to the gpio driver's of properties.

Thanks,
Guenter

Grant Likely

2012-12-09 22:36 UTC #8 lore raw

Quoted 24 lines · Guenter Roeck

Quoted 10 lines · Alan Cox
Quoted 7 lines · On Sun, 09 Dec 2012 01:58:19 -0800 anish kumar <[email protected]>…
Quoted 5 lines · Alan Cox
Quoted 2 lines
I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.
Android tries to address this with its switch class driver, but I'm not sure its actually got anything over making them input devices.
Quoted 7 lines
Quoted 2 lines
Sorry for not understanding the context here.How the debounce sysfs added by Guenter has anything to do with switch driver in android?
The other more general option is to make the input layer do the debounce and make them all inputs rather than just relying on any gpio layer support.
The gpio pins I am dealing with are provided by an FPGA which is used on various boards. While the gpio access registers are always the same, the actual usage is board specific. This means I either need to write ugly code, or use the gpio subsystem to provide access to the gpio pins. Ugly code is out of the question, which means I'll need gpio support.

Quoted 5 lines

Anyway, I want to keep things simple, not add unnecessary complexity. Having to go through the input subsystem just to be able to support debounce on a couple of input pins doesn't really sound simple. Guess I'll have to find another solution if the patch is not accepted. Maybe I'll add a "debounce" property to the gpio driver's of properties.

I haven't looked deeply at the patch to give you an answer yet, but I'd recommend you go with the DT property approach anyway. The gpio sysfs interface is horribly designed and I'm not keen on adding new features to it.

Linus Walleij

2012-12-10 10:11 UTC #9 lore raw

In reply to Guenter Roeck

Quoted 11 lines · Guenter Roeck

The gpio pins I am dealing with are provided by an FPGA which is used on various boards. While the gpio access registers are always the same, the actual usage is board specific. This means I either need to write ugly code, or use the gpio subsystem to provide access to the gpio pins. Ugly code is out of the question, which means I'll need gpio support.
Anyway, I want to keep things simple, not add unnecessary complexity. Having to go through the input subsystem just to be able to support debounce on a couple of input pins doesn't really sound simple. Guess I'll have to find another solution if the patch is not accepted. Maybe I'll add a "debounce" property to the gpio driver's of properties.

I would like you to seriously consider both gpio-input and the new drivers/extcon/extcon-gpio.c driver (maybe adding debounce into that driver). The latter also has a sysfs interface.

Your usecases seem to be EXTCON_MECHANICAL. If not, new classes can surely be added.

There is some risk that sysfs makes everything that is a GPIO line appear as GPIO instead of what it really is, which is not helpful for userspace which will have to keep track of all the routing of the electronics.

The latter might be comfortable if you're designing and maintaining the whole system with firmware and so on, but from a newcomer examining the system in sysfs it's not helpful, and we're designing that ABI for a purpose.

(And I share Grant's hesitant stance on extending the GPIO sysfs.)

Yours,
Linus Walleij

Linus Walleij

2012-12-10 10:04 UTC #10 lore raw

In reply to Alan Cox

Quoted 5 lines · Alan Cox

Quoted 2 lines
I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.
Android tries to address this with its switch class driver, but I'm not sure its actually got anything over making them input devices.

This has actually been merged into the kernel proper as drivers/extcon.

So another poke on Günther if this fulfills the needs?

Yours,
Linus Walleij

Guenter Roeck

2012-12-10 18:48 UTC #11 lore raw

Quoted 5 lines · Linus Walleij

Quoted 3 lines · Alan Cox
Quoted 2 lines
I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.

Quoted 7 lines

Quoted 2 lines
Android tries to address this with its switch class driver, but I'm not sure its actually got anything over making them input devices.
This has actually been merged into the kernel proper as drivers/extcon.
So another poke on Günther if this fulfills the needs?

I'll look into it. Currently I am hampered by a cold which seems to mug my brain, and technically by the need to backport extcon to 3.0 (if that is even possible) since our chip vendor does not yet support a more recent kernel.

Thanks,
Guenter

anish singh

2012-12-10 19:37 UTC #12 lore raw

Quoted 9 lines · Guenter Roeck

Quoted 8 lines · Linus Walleij
Quoted 5 lines · Alan Cox
Quoted 2 lines
I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.
Android tries to address this with its switch class driver, but I'm not sure its actually got anything over making them input devices.

Quoted 7 lines

Quoted 4 lines
This has actually been merged into the kernel proper as drivers/extcon.
So another poke on Günther if this fulfills the needs?
I'll look into it. Currently I am hampered by a cold which seems to mug my brain, and technically by the need to backport extcon to 3.0 (if that is even possible) since our chip vendor does not yet support a more recent kernel.

It is very much possible.I have already tried that and it works.

Quoted 8 lines

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Guenter Roeck

author

2012-12-13 17:08 UTC #13 lore raw

Quoted 11 lines · anish singh

Quoted 9 lines · Guenter Roeck
Quoted 8 lines · Linus Walleij
Quoted 5 lines · Alan Cox
Quoted 2 lines
I could imagine declaring the activity request buttons to be "input", but for presence detects it is a bit far fetched and would add too much complexity.
Android tries to address this with its switch class driver, but I'm not sure its actually got anything over making them input devices.

Quoted 8 lines

Quoted 7 lines
Quoted 4 lines
This has actually been merged into the kernel proper as drivers/extcon.
So another poke on Günther if this fulfills the needs?
I'll look into it. Currently I am hampered by a cold which seems to mug my brain, and technically by the need to backport extcon to 3.0 (if that is even possible) since our chip vendor does not yet support a more recent kernel.
It is very much possible.I have already tried that and it works.

Confirmed.

Looks like this is going to work for me, so I won't need the gpio patch. Please ignore it.

Thanks,
Guenter