Input: gpio_keys - allow separating gpio and irq in device tree

PATCH In review Dmitry Torokhov 3 participants 8 messages Dec 8, 2014 – Jan 4, 2015

Subsystems (3)

Device tree bindings HID Input drivers

lore

Share & tools

lore

Dmitry Torokhov

author maintainer

2014-12-08 07:19 UTC #0 lore raw

PATCH 1/2 Input: gpio_keys - allow separating gpio and irq in device tree

This change allows specify interrupt for buttons separately form gpio, potentially allowing to form several "clusters" of buttons on different interrupts.

Button defined without both gpio and irq in device tree is a hared error instead of a warning now.

Signed-off-by Dmitry Torokhov <[email protected]>

---
 .../devicetree/bindings/input/gpio-keys.txt        | 10 +++--
 drivers/input/keyboard/gpio_keys.c                 | 50 ++++++++++++----------
 2 files changed, 34 insertions(+), 26 deletions(-)

Documentation/devicetree/bindings/input/gpio-keys.txt +7 -3

index a4a38fc..44b7057 100644
@@ -10,12 +10,13 @@ Optional properties:
 Each button (key) is represented as a sub-node of "gpio-keys":
 Subnode properties:
 
+	- gpios: OF device-tree gpio specification.
+	- interrupts: the interrupt line for that input.
 	- label: Descriptive name of the key.
 	- linux,code: Keycode to emit.
 
-Required mutual exclusive subnode-properties:
-	- gpios: OF device-tree gpio specification.
-	- interrupts: the interrupt line for that input
+Note that either "interrupts" or "gpios" properties can be omitted, but not
+both at the same time. Specifying both properties is allowed.
 
 Optional subnode-properties:
 	- linux,input-type: Specify event type this button/key generates.

@@ -23,6 +24,9 @@ Optional subnode-properties:
 	- debounce-interval: Debouncing interval time in milliseconds.
 	  If not specified defaults to 5.
 	- gpio-key,wakeup: Boolean, button can wake-up the system.
+	- linux,can-disable: Boolean, indicates that button is connected
+	  to dedicated (not shared) interrupt which can be disabled to
+	  suppress events from the button.
 
 Example nodes:

drivers/input/keyboard/gpio_keys.c +27 ~~-23~~

80 lines not on this page · raw message

Signature · 5 lines

2.2.0.rc0.207.ga3a616c


-- 
Dmitry

Dmitry Torokhov

author maintainer

2014-12-08 07:21 UTC #1 lore raw

PATCH 2/2 Input: gpio_keys - replace timer and workqueue with delayed workqueue

We do not need to roll our own implementation of delayed work now that we have proper implementation of mod_delayed_work.

For interrupt-only driven buttons we retain the timer, but we rename it to release_timer to better reflect its purpose.

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/input/keyboard/gpio_keys.c |   65 +++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

drivers/input/keyboard/gpio_keys.c +31 ~~-34~~

index a5ece3f..eefd976 100644
@@ -35,9 +35,13 @@
 struct gpio_button_data {
 	const struct gpio_keys_button *button;
 	struct input_dev *input;
-	struct timer_list timer;
-	struct work_struct work;
-	unsigned int timer_debounce;	/* in msecs */
+
+	struct timer_list release_timer;
+	unsigned int release_delay;	/* in msecs, for IRQ-only buttons */
+
+	struct delayed_work work;
+	unsigned int software_debounce;	/* in msecs, for GPIO-driven buttons */
+
 	unsigned int irq;
 	spinlock_t lock;
 	bool disabled;

@@ -116,11 +120,14 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata)
 {
 	if (!bdata->disabled) {
 		/*
-		 * Disable IRQ and possible debouncing timer.
+		 * Disable IRQ and associated timer/work structure.
 		 */
 		disable_irq(bdata->irq);
-		if (bdata->timer_debounce)
-			del_timer_sync(&bdata->timer);
+
+		if (gpio_is_valid(bdata->button->gpio))
+			cancel_delayed_work_sync(&bdata->work);
+		else
+			del_timer_sync(&bdata->release_timer);
 
 		bdata->disabled = true;
 	}

@@ -343,7 +350,7 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 static void gpio_keys_gpio_work_func(struct work_struct *work)
 {
 	struct gpio_button_data *bdata =
-		container_of(work, struct gpio_button_data, work);
+		container_of(work, struct gpio_button_data, work.work);
 
 	gpio_keys_gpio_report_event(bdata);

@@ -351,13 +358,6 @@ static void gpio_keys_gpio_work_func(struct work_struct *work)
 		pm_relax(bdata->input->dev.parent);
 }
 
-static void gpio_keys_gpio_timer(unsigned long _data)
-{
-	struct gpio_button_data *bdata = (struct gpio_button_data *)_data;
-
-	schedule_work(&bdata->work);
-}
-
 static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 {
 	struct gpio_button_data *bdata = dev_id;

@@ -366,11 +366,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 
 	if (bdata->button->wakeup)
 		pm_stay_awake(bdata->input->dev.parent);
-	if (bdata->timer_debounce)
-		mod_timer(&bdata->timer,
-			jiffies + msecs_to_jiffies(bdata->timer_debounce));
-	else
-		schedule_work(&bdata->work);
+
+	mod_delayed_work(system_wq,
+			 &bdata->work,
+			 msecs_to_jiffies(bdata->software_debounce));
 
 	return IRQ_HANDLED;
 }

@@ -408,7 +407,7 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 		input_event(input, EV_KEY, button->code, 1);
 		input_sync(input);
 
-		if (!bdata->timer_debounce) {
+		if (!bdata->release_delay) {
 			input_event(input, EV_KEY, button->code, 0);
 			input_sync(input);
 			goto out;

@@ -417,9 +416,9 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 		bdata->key_pressed = true;
 	}
 
-	if (bdata->timer_debounce)
-		mod_timer(&bdata->timer,
-			jiffies + msecs_to_jiffies(bdata->timer_debounce));
+	if (bdata->release_delay)
+		mod_timer(&bdata->release_timer,
+			jiffies + msecs_to_jiffies(bdata->release_delay));
 out:
 	spin_unlock_irqrestore(&bdata->lock, flags);
 	return IRQ_HANDLED;

@@ -429,10 +428,10 @@ static void gpio_keys_quiesce_key(void *data)
 {
 	struct gpio_button_data *bdata = data;
 
-	if (bdata->timer_debounce)
-		del_timer_sync(&bdata->timer);
-
-	cancel_work_sync(&bdata->work);
+	if (gpio_is_valid(bdata->button->gpio))
+		cancel_delayed_work_sync(&bdata->work);
+	else
+		del_timer_sync(&bdata->release_timer);
 }
 
 static int gpio_keys_setup_key(struct platform_device *pdev,

@@ -466,7 +465,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 					button->debounce_interval * 1000);
 			/* use timer if gpiolib doesn't provide debounce */
 			if (error < 0)
-				bdata->timer_debounce =
+				bdata->software_debounce =
 						button->debounce_interval;
 		}

@@ -484,9 +483,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 			bdata->irq = irq;
 		}
 
-		INIT_WORK(&bdata->work, gpio_keys_gpio_work_func);
-		setup_timer(&bdata->timer,
-			    gpio_keys_gpio_timer, (unsigned long)bdata);
+		INIT_DELAYED_WORK(&bdata->work, gpio_keys_gpio_work_func);
 
 		isr = gpio_keys_gpio_isr;
 		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;

@@ -503,8 +500,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 			return -EINVAL;
 		}
 
-		bdata->timer_debounce = button->debounce_interval;
-		setup_timer(&bdata->timer,
+		bdata->release_delay = button->debounce_interval;
+		setup_timer(&bdata->release_timer,
 			    gpio_keys_irq_timer, (unsigned long)bdata);
 
 		isr = gpio_keys_irq_isr;

@@ -514,7 +511,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	input_set_capability(input, button->type ?: EV_KEY, button->code);
 
 	/*
-	 * Install custom action to cancel debounce timer and
+	 * Install custom action to cancel release timer and
 	 * workqueue item.
 	 */
 	error = devm_add_action(&pdev->dev, gpio_keys_quiesce_key, bdata);

Signature · 1 line

Dmitry

Andy Shevchenko

Tested-by on v1 2/2

2014-12-10 18:32 UTC #2 lore raw

Re: [PATCH 2/2] Input: gpio_keys - replace timer and workqueue with delayed workqueue

Quoted 5 lines · Dmitry Torokhov

We do not need to roll our own implementation of delayed work now that we have proper implementation of mod_delayed_work.
For interrupt-only driven buttons we retain the timer, but we rename it to release_timer to better reflect its purpose.

At least it doesn't break the driver on Intel Medfield device.

Tested-by Andy Shevchenko <[email protected]>

Quoted 162 lines

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/input/keyboard/gpio_keys.c |   65 +++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

drivers/input/keyboard/gpio_keys.c +31 ~~-34~~

index a5ece3f..eefd976 100644
@@ -35,9 +35,13 @@
 struct gpio_button_data {
 	const struct gpio_keys_button *button;
 	struct input_dev *input;
-	struct timer_list timer;
-	struct work_struct work;
-	unsigned int timer_debounce;	/* in msecs */
+
+	struct timer_list release_timer;
+	unsigned int release_delay;	/* in msecs, for IRQ-only buttons */
+
+	struct delayed_work work;
+	unsigned int software_debounce;	/* in msecs, for GPIO-driven buttons */
+
 	unsigned int irq;
 	spinlock_t lock;
 	bool disabled;

@@ -116,11 +120,14 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata)
 {
 	if (!bdata->disabled) {
 		/*
-		 * Disable IRQ and possible debouncing timer.
+		 * Disable IRQ and associated timer/work structure.
 		 */
 		disable_irq(bdata->irq);
-		if (bdata->timer_debounce)
-			del_timer_sync(&bdata->timer);
+
+		if (gpio_is_valid(bdata->button->gpio))
+			cancel_delayed_work_sync(&bdata->work);
+		else
+			del_timer_sync(&bdata->release_timer);
 
 		bdata->disabled = true;
 	}

@@ -343,7 +350,7 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 static void gpio_keys_gpio_work_func(struct work_struct *work)
 {
 	struct gpio_button_data *bdata =
-		container_of(work, struct gpio_button_data, work);
+		container_of(work, struct gpio_button_data, work.work);
 
 	gpio_keys_gpio_report_event(bdata);

@@ -351,13 +358,6 @@ static void gpio_keys_gpio_work_func(struct work_struct *work)
 		pm_relax(bdata->input->dev.parent);
 }
 
-static void gpio_keys_gpio_timer(unsigned long _data)
-{
-	struct gpio_button_data *bdata = (struct gpio_button_data *)_data;
-
-	schedule_work(&bdata->work);
-}
-
 static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 {
 	struct gpio_button_data *bdata = dev_id;

@@ -366,11 +366,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 
 	if (bdata->button->wakeup)
 		pm_stay_awake(bdata->input->dev.parent);
-	if (bdata->timer_debounce)
-		mod_timer(&bdata->timer,
-			jiffies + msecs_to_jiffies(bdata->timer_debounce));
-	else
-		schedule_work(&bdata->work);
+
+	mod_delayed_work(system_wq,
+			 &bdata->work,
+			 msecs_to_jiffies(bdata->software_debounce));
 
 	return IRQ_HANDLED;
 }

@@ -408,7 +407,7 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 		input_event(input, EV_KEY, button->code, 1);
 		input_sync(input);
 
-		if (!bdata->timer_debounce) {
+		if (!bdata->release_delay) {
 			input_event(input, EV_KEY, button->code, 0);
 			input_sync(input);
 			goto out;

@@ -417,9 +416,9 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 		bdata->key_pressed = true;
 	}
 
-	if (bdata->timer_debounce)
-		mod_timer(&bdata->timer,
-			jiffies + msecs_to_jiffies(bdata->timer_debounce));
+	if (bdata->release_delay)
+		mod_timer(&bdata->release_timer,
+			jiffies + msecs_to_jiffies(bdata->release_delay));
 out:
 	spin_unlock_irqrestore(&bdata->lock, flags);
 	return IRQ_HANDLED;

@@ -429,10 +428,10 @@ static void gpio_keys_quiesce_key(void *data)
 {
 	struct gpio_button_data *bdata = data;
 
-	if (bdata->timer_debounce)
-		del_timer_sync(&bdata->timer);
-
-	cancel_work_sync(&bdata->work);
+	if (gpio_is_valid(bdata->button->gpio))
+		cancel_delayed_work_sync(&bdata->work);
+	else
+		del_timer_sync(&bdata->release_timer);
 }
 
 static int gpio_keys_setup_key(struct platform_device *pdev,

@@ -466,7 +465,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 					button->debounce_interval * 1000);
 			/* use timer if gpiolib doesn't provide debounce */
 			if (error < 0)
-				bdata->timer_debounce =
+				bdata->software_debounce =
 						button->debounce_interval;
 		}

@@ -484,9 +483,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 			bdata->irq = irq;
 		}
 
-		INIT_WORK(&bdata->work, gpio_keys_gpio_work_func);
-		setup_timer(&bdata->timer,
-			    gpio_keys_gpio_timer, (unsigned long)bdata);
+		INIT_DELAYED_WORK(&bdata->work, gpio_keys_gpio_work_func);
 
 		isr = gpio_keys_gpio_isr;
 		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;

@@ -503,8 +500,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 			return -EINVAL;
 		}
 
-		bdata->timer_debounce = button->debounce_interval;
-		setup_timer(&bdata->timer,
+		bdata->release_delay = button->debounce_interval;
+		setup_timer(&bdata->release_timer,
 			    gpio_keys_irq_timer, (unsigned long)bdata);
 
 		isr = gpio_keys_irq_isr;

@@ -514,7 +511,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	input_set_capability(input, button->type ?: EV_KEY, button->code);
 
 	/*
-	 * Install custom action to cancel debounce timer and
+	 * Install custom action to cancel release timer and
 	 * workqueue item.
 	 */
 	error = devm_add_action(&pdev->dev, gpio_keys_quiesce_key, bdata);

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Dmitry Torokhov

author maintainer

2014-12-13 19:09 UTC #3 lore raw

Re: [PATCH 2/2] Input: gpio_keys - replace timer and workqueue with delayed workqueue

Hi Andy,

Quoted 10 lines · Andy Shevchenko

Quoted 5 lines · Dmitry Torokhov
We do not need to roll our own implementation of delayed work now that we have proper implementation of mod_delayed_work.
For interrupt-only driven buttons we retain the timer, but we rename it to release_timer to better reflect its purpose.
At least it doesn't break the driver on Intel Medfield device.
Tested-by Andy Shevchenko <[email protected]>

Thank you for testing it. Assume it is for both patches, right?

Signature · 1 line

Dmitry

Andy Shevchenko

2014-12-15 10:20 UTC #4 lore raw

Re: [PATCH 2/2] Input: gpio_keys - replace timer and workqueue with delayed workqueue

Quoted 7 lines · Dmitry Torokhov

Hi Andy,
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Dmitry Torokhov
We do not need to roll our own implementation of delayed work now that we have proper implementation of mod_delayed_work.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
For interrupt-only driven buttons we retain the timer, but we rename it to release_timer to better reflect its purpose.
At least it doesn't break the driver on Intel Medfield device.
Tested-by Andy Shevchenko <[email protected]>
Thank you for testing it. Assume it is for both patches, right?

Right.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Linus Walleij

Reviewed-by on v1 2/2

2014-12-14 09:05 UTC #5 lore raw

Re: [PATCH 2/2] Input: gpio_keys - replace timer and workqueue with delayed workqueue

In reply to Dmitry Torokhov

Quoted 7 lines · Dmitry Torokhov

We do not need to roll our own implementation of delayed work now that we have proper implementation of mod_delayed_work.
For interrupt-only driven buttons we retain the timer, but we rename it to release_timer to better reflect its purpose.
Signed-off-by Dmitry Torokhov <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

What I want to do with this, on top of this, is to move debouncing to the GPIO subsystem. I want gpio(d)_set_debounce() to *always* work and the timer moved to gpiolib.c.

We can't have debouncing in every subsystem using GPIOs, it belongs in the GPIO core.

Yours,
Linus Walleij

Linus Walleij

Acked-by on v1 1/2

2014-12-31 08:22 UTC #6 lore raw

In reply to Dmitry Torokhov

Quoted 5 lines · Dmitry Torokhov

This change allows specify interrupt for buttons separately form gpio, potentially allowing to form several "clusters" of buttons on different interrupts.
Button defined without both gpio and irq in device tree is a hared error

hared?

Quoted 3 lines

instead of a warning now.
Signed-off-by Dmitry Torokhov <[email protected]>

Acked-by Linus Walleij <[email protected]>

...but it would be nice if it could also be converted to use GPIO descriptors internally at some point.

Yours,
Linus Walleij

Dmitry Torokhov

author maintainer

2015-01-04 22:27 UTC #7 lore raw

Quoted 10 lines · Linus Walleij

Quoted 5 lines · Dmitry Torokhov
This change allows specify interrupt for buttons separately form gpio, potentially allowing to form several "clusters" of buttons on different interrupts.
Button defined without both gpio and irq in device tree is a hared error
hared?

Should have been "hard", sorry...

Quoted 9 lines

Quoted 4 lines
instead of a warning now.
Signed-off-by Dmitry Torokhov <[email protected]>
Acked-by Linus Walleij <[email protected]>
...but it would be nice if it could also be converted to use GPIO descriptors internally at some point.

Thanks.

Signature · 1 line

Dmitry