[PATCH 1/2] Input: gpio_keys - Do not report wake button presses as evdev events

[PATCH 1/2] Input: gpio_keys - Do not report wake button presses as evdev events

[Hans de Goede]

If a button is a wake button, it may still be bouncing from the press
to wakeup the device by the time the gpio interrupts get enabled again
and / or the gpio_keys_report_state call from gpio_keys_resume may
find the button still pressed and report this as a new press.

This is undesirable, esp. since the powerbutton on tablets is typically
a wakeup source and uses the gpio_keys driver on some tablets, leading
to userspace immediately re-suspending the tablet after the powerbutton
is pressed, due to it seeing a powerbutton press.

This commit ignores wakeup button presses for the first 1 second after
resume (and while resumed, as the workqueue may run before the resume
function runs), avoiding this problem.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note: maybe we should make WAKE_DEBOUNCE part of gpio_keys_button and
only do this when drivers / platform-data set this to a non-zero value ?
---
 drivers/input/keyboard/gpio_keys.c | 50 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 2909365..ffede98 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -32,6 +32,8 @@
 #include <linux/of_irq.h>
 #include <linux/spinlock.h>
 
+#define WAKE_DEBOUNCE msecs_to_jiffies(1000)
+
 struct gpio_button_data {
 	const struct gpio_keys_button *button;
 	struct input_dev *input;
@@ -42,10 +44,14 @@ struct gpio_button_data {
 	struct delayed_work work;
 	unsigned int software_debounce;	/* in msecs, for GPIO-driven buttons */
 
+	unsigned long resume_time;	/* in jiffies, for wakeup buttons */
+
 	unsigned int irq;
 	spinlock_t lock;
 	bool disabled;
 	bool key_pressed;
+	bool suspended;
+	bool resume_time_valid;
 };
 
 struct gpio_keys_drvdata {
@@ -353,6 +359,27 @@ static struct attribute_group gpio_keys_attr_group = {
 	.attrs = gpio_keys_attrs,
 };
 
+static bool gpio_keys_ignore_wakeup_button_press(struct gpio_button_data *bdata)
+{
+	unsigned long flags;
+	bool ret = false;
+
+	if (!bdata->button->wakeup)
+		return ret;
+
+	spin_lock_irqsave(&bdata->lock, flags);
+
+	if (bdata->suspended)
+		ret = true; /* Our resume method did not run yet */
+	else if (bdata->resume_time_valid &&
+		 time_before(jiffies, bdata->resume_time + WAKE_DEBOUNCE))
+		ret = true; /* Assume this is a wakeup press and ignore */
+
+	spin_unlock_irqrestore(&bdata->lock, flags);
+
+	return ret;
+}
+
 static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 {
 	const struct gpio_keys_button *button = bdata->button;
@@ -366,6 +393,10 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 	}
 
 	state = (state ? 1 : 0) ^ button->active_low;
+
+	if (state && gpio_keys_ignore_wakeup_button_press(bdata))
+		return;
+
 	if (type == EV_ABS) {
 		if (state)
 			input_event(input, type, button->code, button->value);
@@ -426,6 +457,9 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 
 	BUG_ON(irq != bdata->irq);
 
+	if (gpio_keys_ignore_wakeup_button_press(bdata))
+		return IRQ_HANDLED;
+
 	spin_lock_irqsave(&bdata->lock, flags);
 
 	if (!bdata->key_pressed) {
@@ -819,13 +853,18 @@ static int gpio_keys_suspend(struct device *dev)
 {
 	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
 	struct input_dev *input = ddata->input;
+	unsigned long flags;
 	int i;
 
 	if (device_may_wakeup(dev)) {
 		for (i = 0; i < ddata->pdata->nbuttons; i++) {
 			struct gpio_button_data *bdata = &ddata->data[i];
-			if (bdata->button->wakeup)
+			if (bdata->button->wakeup) {
+				spin_lock_irqsave(&bdata->lock, flags);
+				bdata->suspended = true;
+				spin_unlock_irqrestore(&bdata->lock, flags);
 				enable_irq_wake(bdata->irq);
+			}
 		}
 	} else {
 		mutex_lock(&input->mutex);
@@ -841,14 +880,21 @@ static int gpio_keys_resume(struct device *dev)
 {
 	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
 	struct input_dev *input = ddata->input;
+	unsigned long flags;
 	int error = 0;
 	int i;
 
 	if (device_may_wakeup(dev)) {
 		for (i = 0; i < ddata->pdata->nbuttons; i++) {
 			struct gpio_button_data *bdata = &ddata->data[i];
-			if (bdata->button->wakeup)
+			if (bdata->button->wakeup) {
 				disable_irq_wake(bdata->irq);
+				spin_lock_irqsave(&bdata->lock, flags);
+				bdata->resume_time = jiffies;
+				bdata->resume_time_valid = true;
+				bdata->suspended = false;
+				spin_unlock_irqrestore(&bdata->lock, flags);
+			}
 		}
 	} else {
 		mutex_lock(&input->mutex);
-- 
2.9.3

[PATCH 2/2] Input: gpio-keys - Add support for setkeycode

[Hans de Goede]

gpio-keys input devices created by the soc_button_array driver are
configured with key-codes based on ACPI provided information.

Unfortunately on some tablets this info is wrong, and we need to have
a quirk to fix things up.

Add support for input_setkeycode to the gpio-keys driver, so that
the existing udev hwdb mechanism can be used to fix things up on these
tablets.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/keyboard/gpio_keys.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index ffede98..e6ee316 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -37,6 +37,7 @@
 struct gpio_button_data {
 	const struct gpio_keys_button *button;
 	struct input_dev *input;
+	unsigned short *code;
 
 	struct timer_list release_timer;
 	unsigned int release_delay;	/* in msecs, for IRQ-only buttons */
@@ -58,6 +59,7 @@ struct gpio_keys_drvdata {
 	const struct gpio_keys_platform_data *pdata;
 	struct input_dev *input;
 	struct mutex disable_lock;
+	unsigned short *keymap;
 	struct gpio_button_data data[0];
 };
 
@@ -209,7 +211,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
 		if (only_disabled && !bdata->disabled)
 			continue;
 
-		__set_bit(bdata->button->code, bits);
+		__set_bit(*bdata->code, bits);
 	}
 
 	ret = scnprintf(buf, PAGE_SIZE - 1, "%*pbl", n_events, bits);
@@ -260,7 +262,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
 		if (bdata->button->type != type)
 			continue;
 
-		if (test_bit(bdata->button->code, bits) &&
+		if (test_bit(*bdata->code, bits) &&
 		    !bdata->button->can_disable) {
 			error = -EINVAL;
 			goto out;
@@ -275,7 +277,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
 		if (bdata->button->type != type)
 			continue;
 
-		if (test_bit(bdata->button->code, bits))
+		if (test_bit(*bdata->code, bits))
 			gpio_keys_disable_button(bdata);
 		else
 			gpio_keys_enable_button(bdata);
@@ -401,7 +403,7 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 		if (state)
 			input_event(input, type, button->code, button->value);
 	} else {
-		input_event(input, type, button->code, !!state);
+		input_event(input, type, *bdata->code, !!state);
 	}
 	input_sync(input);
 }
@@ -441,7 +443,7 @@ static void gpio_keys_irq_timer(unsigned long _data)
 
 	spin_lock_irqsave(&bdata->lock, flags);
 	if (bdata->key_pressed) {
-		input_event(input, EV_KEY, bdata->button->code, 0);
+		input_event(input, EV_KEY, *bdata->code, 0);
 		input_sync(input);
 		bdata->key_pressed = false;
 	}
@@ -451,7 +453,6 @@ static void gpio_keys_irq_timer(unsigned long _data)
 static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 {
 	struct gpio_button_data *bdata = dev_id;
-	const struct gpio_keys_button *button = bdata->button;
 	struct input_dev *input = bdata->input;
 	unsigned long flags;
 
@@ -466,11 +467,11 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 		if (bdata->button->wakeup)
 			pm_wakeup_event(bdata->input->dev.parent, 0);
 
-		input_event(input, EV_KEY, button->code, 1);
+		input_event(input, EV_KEY, *bdata->code, 1);
 		input_sync(input);
 
 		if (!bdata->release_delay) {
-			input_event(input, EV_KEY, button->code, 0);
+			input_event(input, EV_KEY, *bdata->code, 0);
 			input_sync(input);
 			goto out;
 		}
@@ -570,7 +571,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 		irqflags = 0;
 	}
 
-	input_set_capability(input, button->type ?: EV_KEY, button->code);
+	*bdata->code = button->code;
+	input_set_capability(input, button->type ?: EV_KEY, *bdata->code);
 
 	/*
 	 * Install custom action to cancel release timer and
@@ -776,6 +778,12 @@ static int gpio_keys_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	ddata->keymap = devm_kzalloc(dev,
+				     pdata->nbuttons * sizeof(unsigned short),
+				     GFP_KERNEL);
+	if (!ddata->keymap)
+		return -ENOMEM;
+
 	input = devm_input_allocate_device(dev);
 	if (!input) {
 		dev_err(dev, "failed to allocate input device\n");
@@ -800,6 +808,10 @@ static int gpio_keys_probe(struct platform_device *pdev)
 	input->id.product = 0x0001;
 	input->id.version = 0x0100;
 
+	input->keycode = ddata->keymap;
+	input->keycodesize = sizeof(unsigned short);
+	input->keycodemax = pdata->nbuttons;
+
 	/* Enable auto repeat feature of Linux input subsystem */
 	if (pdata->rep)
 		__set_bit(EV_REP, input->evbit);
@@ -808,6 +820,7 @@ static int gpio_keys_probe(struct platform_device *pdev)
 		const struct gpio_keys_button *button = &pdata->buttons[i];
 		struct gpio_button_data *bdata = &ddata->data[i];
 
+		bdata->code = &ddata->keymap[i];
 		error = gpio_keys_setup_key(pdev, input, bdata, button);
 		if (error)
 			return error;
-- 
2.9.3

Re: [PATCH 2/2] Input: gpio-keys - Add support for setkeycode

[Dmitry Torokhov]

On Mon, Jan 09, 2017 at 06:57:00PM +0100, Hans de Goede wrote:
> gpio-keys input devices created by the soc_button_array driver are
> configured with key-codes based on ACPI provided information.
> 
> Unfortunately on some tablets this info is wrong, and we need to have
> a quirk to fix things up.
> 
> Add support for input_setkeycode to the gpio-keys driver, so that
> the existing udev hwdb mechanism can be used to fix things up on these
> tablets.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied with minor edits:

>  
> +	ddata->keymap = devm_kzalloc(dev,
> +				     pdata->nbuttons * sizeof(unsigned short),
> +				     GFP_KERNEL);

Changed to devm_kcalloc().

> +		bdata->code = &ddata->keymap[i];

Pushed into gpio_keys_setup_key().

Thanks.

-- 
Dmitry