[PATCH 1/6] Input: gpio_keys_polled - keep button data constant

[PATCH 1/6] Input: gpio_keys_polled - keep button data constant

[Dmitry Torokhov]

Commit 633a21d80b4a ("input: gpio_keys_polled: Add support for GPIO
descriptors") placed gpio descriptor into gpio_keys_button structure, which
is supposed to be part of platform data and not modifiable by the driver.
To keep the data constant, let's move the descriptor to
gpio_keys_button_data structure instead.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/gpio_keys.c        |  10 +--
 drivers/input/keyboard/gpio_keys_polled.c | 105 +++++++++++++++++-------------
 include/linux/gpio_keys.h                 |   4 +-
 3 files changed, 64 insertions(+), 55 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 2909365..890eb39 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -624,7 +624,6 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 	struct gpio_keys_button *button;
 	int error;
 	int nbuttons;
-	int i;
 
 	node = dev->of_node;
 	if (!node)
@@ -640,19 +639,18 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 	if (!pdata)
 		return ERR_PTR(-ENOMEM);
 
-	pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
+	button = (struct gpio_keys_button *)(pdata + 1);
+
+	pdata->buttons = button;
 	pdata->nbuttons = nbuttons;
 
 	pdata->rep = !!of_get_property(node, "autorepeat", NULL);
 
 	of_property_read_string(node, "label", &pdata->name);
 
-	i = 0;
 	for_each_available_child_of_node(node, pp) {
 		enum of_gpio_flags flags;
 
-		button = &pdata->buttons[i++];
-
 		button->gpio = of_get_gpio_flags(pp, 0, &flags);
 		if (button->gpio < 0) {
 			error = button->gpio;
@@ -694,6 +692,8 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 		if (of_property_read_u32(pp, "debounce-interval",
 					 &button->debounce_interval))
 			button->debounce_interval = 5;
+
+		button++;
 	}
 
 	if (pdata->nbuttons == 0)
diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index 62bdb1d..2cf4078 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -30,6 +30,7 @@
 #define DRV_NAME	"gpio-keys-polled"
 
 struct gpio_keys_button_data {
+	struct gpio_desc *gpiod;
 	int last_state;
 	int count;
 	int threshold;
@@ -46,7 +47,7 @@ struct gpio_keys_polled_dev {
 };
 
 static void gpio_keys_button_event(struct input_polled_dev *dev,
-				   struct gpio_keys_button *button,
+				   const struct gpio_keys_button *button,
 				   int state)
 {
 	struct gpio_keys_polled_dev *bdev = dev->private;
@@ -70,15 +71,15 @@ static void gpio_keys_button_event(struct input_polled_dev *dev,
 }
 
 static void gpio_keys_polled_check_state(struct input_polled_dev *dev,
-					 struct gpio_keys_button *button,
+					 const struct gpio_keys_button *button,
 					 struct gpio_keys_button_data *bdata)
 {
 	int state;
 
 	if (bdata->can_sleep)
-		state = !!gpiod_get_value_cansleep(button->gpiod);
+		state = !!gpiod_get_value_cansleep(bdata->gpiod);
 	else
-		state = !!gpiod_get_value(button->gpiod);
+		state = !!gpiod_get_value(bdata->gpiod);
 
 	gpio_keys_button_event(dev, button, state);
 
@@ -142,48 +143,35 @@ static void gpio_keys_polled_close(struct input_polled_dev *dev)
 		pdata->disable(bdev->dev);
 }
 
-static struct gpio_keys_platform_data *gpio_keys_polled_get_devtree_pdata(struct device *dev)
+static struct gpio_keys_platform_data *
+gpio_keys_polled_get_devtree_pdata(struct device *dev)
 {
 	struct gpio_keys_platform_data *pdata;
 	struct gpio_keys_button *button;
 	struct fwnode_handle *child;
-	int error;
 	int nbuttons;
 
 	nbuttons = device_get_child_node_count(dev);
 	if (nbuttons == 0)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 
 	pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons * sizeof(*button),
 			     GFP_KERNEL);
 	if (!pdata)
 		return ERR_PTR(-ENOMEM);
 
-	pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
+	button = (struct gpio_keys_button *)(pdata + 1);
+
+	pdata->buttons = button;
+	pdata->nbuttons = nbuttons;
 
 	pdata->rep = device_property_present(dev, "autorepeat");
 	device_property_read_u32(dev, "poll-interval", &pdata->poll_interval);
 
 	device_for_each_child_node(dev, child) {
-		struct gpio_desc *desc;
-
-		desc = devm_get_gpiod_from_child(dev, NULL, child);
-		if (IS_ERR(desc)) {
-			error = PTR_ERR(desc);
-			if (error != -EPROBE_DEFER)
-				dev_err(dev,
-					"Failed to get gpio flags, error: %d\n",
-					error);
-			fwnode_handle_put(child);
-			return ERR_PTR(error);
-		}
-
-		button = &pdata->buttons[pdata->nbuttons++];
-		button->gpiod = desc;
-
-		if (fwnode_property_read_u32(child, "linux,code", &button->code)) {
-			dev_err(dev, "Button without keycode: %d\n",
-				pdata->nbuttons - 1);
+		if (fwnode_property_read_u32(child, "linux,code",
+					     &button->code)) {
+			dev_err(dev, "button without keycode\n");
 			fwnode_handle_put(child);
 			return ERR_PTR(-EINVAL);
 		}
@@ -206,10 +194,9 @@ static struct gpio_keys_platform_data *gpio_keys_polled_get_devtree_pdata(struct
 		if (fwnode_property_read_u32(child, "debounce-interval",
 					     &button->debounce_interval))
 			button->debounce_interval = 5;
-	}
 
-	if (pdata->nbuttons == 0)
-		return ERR_PTR(-EINVAL);
+		button++;
+	}
 
 	return pdata;
 }
@@ -220,7 +207,7 @@ static void gpio_keys_polled_set_abs_params(struct input_dev *input,
 	int i, min = 0, max = 0;
 
 	for (i = 0; i < pdata->nbuttons; i++) {
-		struct gpio_keys_button *button = &pdata->buttons[i];
+		const struct gpio_keys_button *button = &pdata->buttons[i];
 
 		if (button->type != EV_ABS || button->code != code)
 			continue;
@@ -230,6 +217,7 @@ static void gpio_keys_polled_set_abs_params(struct input_dev *input,
 		if (button->value > max)
 			max = button->value;
 	}
+
 	input_set_abs_params(input, code, min, max, 0, 0);
 }
 
@@ -242,6 +230,7 @@ MODULE_DEVICE_TABLE(of, gpio_keys_polled_of_match);
 static int gpio_keys_polled_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct fwnode_handle *child = NULL;
 	const struct gpio_keys_platform_data *pdata = dev_get_platdata(dev);
 	struct gpio_keys_polled_dev *bdev;
 	struct input_polled_dev *poll_dev;
@@ -254,10 +243,6 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
 		pdata = gpio_keys_polled_get_devtree_pdata(dev);
 		if (IS_ERR(pdata))
 			return PTR_ERR(pdata);
-		if (!pdata) {
-			dev_err(dev, "missing platform data\n");
-			return -EINVAL;
-		}
 	}
 
 	if (!pdata->poll_interval) {
@@ -300,20 +285,40 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
 		__set_bit(EV_REP, input->evbit);
 
 	for (i = 0; i < pdata->nbuttons; i++) {
-		struct gpio_keys_button *button = &pdata->buttons[i];
+		const struct gpio_keys_button *button = &pdata->buttons[i];
 		struct gpio_keys_button_data *bdata = &bdev->data[i];
 		unsigned int type = button->type ?: EV_KEY;
 
 		if (button->wakeup) {
 			dev_err(dev, DRV_NAME " does not support wakeup\n");
+			fwnode_handle_put(child);
 			return -EINVAL;
 		}
 
-		/*
-		 * Legacy GPIO number so request the GPIO here and
-		 * convert it to descriptor.
-		 */
-		if (!button->gpiod && gpio_is_valid(button->gpio)) {
+		if (!dev_get_platdata(dev)) {
+			/* No legacy static platform data */
+			child = device_get_next_child_node(dev, child);
+			if (!child) {
+				dev_err(dev, "missing child device node\n");
+				return -EINVAL;
+			}
+
+			bdata->gpiod = devm_get_gpiod_from_child(dev, NULL,
+								 child);
+			if (IS_ERR(bdata->gpiod)) {
+				error = PTR_ERR(bdata->gpiod);
+				if (error != -EPROBE_DEFER)
+					dev_err(dev,
+						"failed to get gpio: %d\n",
+						error);
+				fwnode_handle_put(child);
+				return error;
+			}
+		} else if (gpio_is_valid(button->gpio)) {
+			/*
+			 * Legacy GPIO number so request the GPIO here and
+			 * convert it to descriptor.
+			 */
 			unsigned flags = GPIOF_IN;
 
 			if (button->active_low)
@@ -322,18 +327,22 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
 			error = devm_gpio_request_one(&pdev->dev, button->gpio,
 					flags, button->desc ? : DRV_NAME);
 			if (error) {
-				dev_err(dev, "unable to claim gpio %u, err=%d\n",
+				dev_err(dev,
+					"unable to claim gpio %u, err=%d\n",
 					button->gpio, error);
 				return error;
 			}
 
-			button->gpiod = gpio_to_desc(button->gpio);
+			bdata->gpiod = gpio_to_desc(button->gpio);
+			if (!bdata->gpiod) {
+				dev_err(dev,
+					"unable to convert gpio %u to descriptor\n",
+					button->gpio);
+				return -EINVAL;
+			}
 		}
 
-		if (IS_ERR(button->gpiod))
-			return PTR_ERR(button->gpiod);
-
-		bdata->can_sleep = gpiod_cansleep(button->gpiod);
+		bdata->can_sleep = gpiod_cansleep(bdata->gpiod);
 		bdata->last_state = -1;
 		bdata->threshold = DIV_ROUND_UP(button->debounce_interval,
 						pdata->poll_interval);
@@ -344,6 +353,8 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
 							button->code);
 	}
 
+	fwnode_handle_put(child);
+
 	bdev->poll_dev = poll_dev;
 	bdev->dev = dev;
 	bdev->pdata = pdata;
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index ee2d8c6..d1250ad 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -2,7 +2,6 @@
 #define _GPIO_KEYS_H
 
 struct device;
-struct gpio_desc;
 
 /**
  * struct gpio_keys_button - configuration parameters
@@ -31,7 +30,6 @@ struct gpio_keys_button {
 	bool can_disable;
 	int value;
 	unsigned int irq;
-	struct gpio_desc *gpiod;
 };
 
 /**
@@ -46,7 +44,7 @@ struct gpio_keys_button {
  * @name:		input device name
  */
 struct gpio_keys_platform_data {
-	struct gpio_keys_button *buttons;
+	const struct gpio_keys_button *buttons;
 	int nbuttons;
 	unsigned int poll_interval;
 	unsigned int rep:1;
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/6] Input: gpio_keys_polled - always use gpiod_get_value_cansleep

[Dmitry Torokhov]

It does not matter if given GPIO may sleep or not when reading state,
polling is always done in a non-atomic context, so we should always
be able to simply use gpiod_get_value_cansleep().

Also let's note in the logs when we fail to read gpio state.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index 2cf4078..72b3503 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -34,7 +34,6 @@ struct gpio_keys_button_data {
 	int last_state;
 	int count;
 	int threshold;
-	int can_sleep;
 };
 
 struct gpio_keys_polled_dev {
@@ -76,16 +75,17 @@ static void gpio_keys_polled_check_state(struct input_polled_dev *dev,
 {
 	int state;
 
-	if (bdata->can_sleep)
-		state = !!gpiod_get_value_cansleep(bdata->gpiod);
-	else
-		state = !!gpiod_get_value(bdata->gpiod);
-
-	gpio_keys_button_event(dev, button, state);
+	state = gpiod_get_value_cansleep(bdata->gpiod);
+	if (state < 0) {
+		dev_err(dev->input->dev.parent,
+			"failed to get gpio state: %d\n", state);
+	} else {
+		gpio_keys_button_event(dev, button, state);
 
-	if (state != bdata->last_state) {
-		bdata->count = 0;
-		bdata->last_state = state;
+		if (state != bdata->last_state) {
+			bdata->count = 0;
+			bdata->last_state = state;
+		}
 	}
 }
 
@@ -342,7 +342,6 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
 			}
 		}
 
-		bdata->can_sleep = gpiod_cansleep(bdata->gpiod);
 		bdata->last_state = -1;
 		bdata->threshold = DIV_ROUND_UP(button->debounce_interval,
 						pdata->poll_interval);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 3/6] Input: gpio_keys - annotate PM methods as __maybe_unused

[Dmitry Torokhov]

Instead of using #ifdef, let's mark suspend and resume methods as
__maybe_unused to provide better compile coverage.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/gpio_keys.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 890eb39..8f7c20b 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -814,8 +814,7 @@ static int gpio_keys_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int gpio_keys_suspend(struct device *dev)
+static int __maybe_unused gpio_keys_suspend(struct device *dev)
 {
 	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
 	struct input_dev *input = ddata->input;
@@ -837,7 +836,7 @@ static int gpio_keys_suspend(struct device *dev)
 	return 0;
 }
 
-static int gpio_keys_resume(struct device *dev)
+static int __maybe_unused gpio_keys_resume(struct device *dev)
 {
 	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
 	struct input_dev *input = ddata->input;
@@ -863,7 +862,6 @@ static int gpio_keys_resume(struct device *dev)
 	gpio_keys_report_state(ddata);
 	return 0;
 }
-#endif
 
 static SIMPLE_DEV_PM_OPS(gpio_keys_pm_ops, gpio_keys_suspend, gpio_keys_resume);
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH 4/6] Input: gpio_keys - fix leaking DT node references

[Dmitry Torokhov]

for_each_available_child_of_node(node, pp) takes reference to 'pp' and
drops it when attempting next iteration. However if we exit the loop early
we need to drop the reference ourselves.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/gpio_keys.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 8f7c20b..d75a25c 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -659,6 +659,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 					dev_err(dev,
 						"Failed to get gpio flags, error: %d\n",
 						error);
+				of_node_put(pp);
 				return ERR_PTR(error);
 			}
 		} else {
@@ -669,12 +670,14 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 
 		if (!gpio_is_valid(button->gpio) && !button->irq) {
 			dev_err(dev, "Found button without gpios or irqs\n");
+			of_node_put(pp);
 			return ERR_PTR(-EINVAL);
 		}
 
 		if (of_property_read_u32(pp, "linux,code", &button->code)) {
 			dev_err(dev, "Button without keycode: 0x%x\n",
 				button->gpio);
+			of_node_put(pp);
 			return ERR_PTR(-EINVAL);
 		}
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH 5/6] Input: gpio_keys - add support for GPIO descriptors

[Dmitry Torokhov]

From: Geert Uytterhoeven <geert+renesas@glider.be>

GPIO descriptors are the preferred way over legacy GPIO numbers
nowadays. Convert the driver to use GPIO descriptors internally but
still allow passing legacy GPIO numbers from platform data to support
existing platforms.

Based on commits 633a21d80b4a2cd6 ("input: gpio_keys_polled: Add support
for GPIO descriptors") and 1ae5ddb6f8837558 ("Input: gpio_keys_polled -
request GPIO pin as input.").

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/gpio_keys.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index d75a25c..0f04cb1 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -26,6 +26,7 @@
 #include <linux/gpio_keys.h>
 #include <linux/workqueue.h>
 #include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/of_gpio.h>
@@ -35,6 +36,7 @@
 struct gpio_button_data {
 	const struct gpio_keys_button *button;
 	struct input_dev *input;
+	struct gpio_desc *gpiod;
 
 	struct timer_list release_timer;
 	unsigned int release_delay;	/* in msecs, for IRQ-only buttons */
@@ -140,7 +142,7 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata)
 		 */
 		disable_irq(bdata->irq);
 
-		if (gpio_is_valid(bdata->button->gpio))
+		if (bdata->gpiod)
 			cancel_delayed_work_sync(&bdata->work);
 		else
 			del_timer_sync(&bdata->release_timer);
@@ -358,19 +360,20 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 	const struct gpio_keys_button *button = bdata->button;
 	struct input_dev *input = bdata->input;
 	unsigned int type = button->type ?: EV_KEY;
-	int state = gpio_get_value_cansleep(button->gpio);
+	int state;
 
+	state = gpiod_get_value_cansleep(bdata->gpiod);
 	if (state < 0) {
-		dev_err(input->dev.parent, "failed to get gpio state\n");
+		dev_err(input->dev.parent,
+			"failed to get gpio state: %d\n", state);
 		return;
 	}
 
-	state = (state ? 1 : 0) ^ button->active_low;
 	if (type == EV_ABS) {
 		if (state)
 			input_event(input, type, button->code, button->value);
 	} else {
-		input_event(input, type, button->code, !!state);
+		input_event(input, type, button->code, state);
 	}
 	input_sync(input);
 }
@@ -456,7 +459,7 @@ static void gpio_keys_quiesce_key(void *data)
 {
 	struct gpio_button_data *bdata = data;
 
-	if (gpio_is_valid(bdata->button->gpio))
+	if (bdata->gpiod)
 		cancel_delayed_work_sync(&bdata->work);
 	else
 		del_timer_sync(&bdata->release_timer);
@@ -478,18 +481,30 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	bdata->button = button;
 	spin_lock_init(&bdata->lock);
 
+	/*
+	 * Legacy GPIO number, so request the GPIO here and
+	 * convert it to descriptor.
+	 */
 	if (gpio_is_valid(button->gpio)) {
+		unsigned flags = GPIOF_IN;
+
+		if (button->active_low)
+			flags |= GPIOF_ACTIVE_LOW;
 
-		error = devm_gpio_request_one(&pdev->dev, button->gpio,
-					      GPIOF_IN, desc);
+		error = devm_gpio_request_one(&pdev->dev, button->gpio, flags,
+					      desc);
 		if (error < 0) {
 			dev_err(dev, "Failed to request GPIO %d, error %d\n",
 				button->gpio, error);
 			return error;
 		}
 
+		bdata->gpiod = gpio_to_desc(button->gpio);
+		if (!bdata->gpiod)
+			return -EINVAL;
+
 		if (button->debounce_interval) {
-			error = gpio_set_debounce(button->gpio,
+			error = gpiod_set_debounce(bdata->gpiod,
 					button->debounce_interval * 1000);
 			/* use timer if gpiolib doesn't provide debounce */
 			if (error < 0)
@@ -500,7 +515,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 		if (button->irq) {
 			bdata->irq = button->irq;
 		} else {
-			irq = gpio_to_irq(button->gpio);
+			irq = gpiod_to_irq(bdata->gpiod);
 			if (irq < 0) {
 				error = irq;
 				dev_err(dev,
@@ -575,7 +590,7 @@ static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
 
 	for (i = 0; i < ddata->pdata->nbuttons; i++) {
 		struct gpio_button_data *bdata = &ddata->data[i];
-		if (gpio_is_valid(bdata->button->gpio))
+		if (bdata->gpiod)
 			gpio_keys_gpio_report_event(bdata);
 	}
 	input_sync(input);
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 5/6] Input: gpio_keys - add support for GPIO descriptors

[Linus Walleij]

On Sat, Oct 29, 2016 at 1:14 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> From: Geert Uytterhoeven <geert+renesas@glider.be>
>
> GPIO descriptors are the preferred way over legacy GPIO numbers
> nowadays. Convert the driver to use GPIO descriptors internally but
> still allow passing legacy GPIO numbers from platform data to support
> existing platforms.
>
> Based on commits 633a21d80b4a2cd6 ("input: gpio_keys_polled: Add support
> for GPIO descriptors") and 1ae5ddb6f8837558 ("Input: gpio_keys_polled -
> request GPIO pin as input.").
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 6/6] Input: gpio_keys - switch to using generic device properties

[Dmitry Torokhov]

Make use of the device property API in this driver so that both OF based
systems and ACPI based systems can use this driver.

Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/gpio_keys.c | 141 ++++++++++++++++++-------------------
 1 file changed, 69 insertions(+), 72 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 0f04cb1..5576f2a 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -28,8 +28,6 @@
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
-#include <linux/of_platform.h>
-#include <linux/of_gpio.h>
 #include <linux/of_irq.h>
 #include <linux/spinlock.h>
 
@@ -468,7 +466,8 @@ static void gpio_keys_quiesce_key(void *data)
 static int gpio_keys_setup_key(struct platform_device *pdev,
 				struct input_dev *input,
 				struct gpio_button_data *bdata,
-				const struct gpio_keys_button *button)
+				const struct gpio_keys_button *button,
+				struct fwnode_handle *child)
 {
 	const char *desc = button->desc ? button->desc : "gpio_keys";
 	struct device *dev = &pdev->dev;
@@ -481,11 +480,28 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	bdata->button = button;
 	spin_lock_init(&bdata->lock);
 
-	/*
-	 * Legacy GPIO number, so request the GPIO here and
-	 * convert it to descriptor.
-	 */
-	if (gpio_is_valid(button->gpio)) {
+	if (child) {
+		bdata->gpiod = devm_get_gpiod_from_child(dev, NULL, child);
+		if (IS_ERR(bdata->gpiod)) {
+			error = PTR_ERR(bdata->gpiod);
+			if (error == -ENOENT) {
+				/*
+				 * GPIO is optional, we may be dealing with
+				 * purely interrupt-driven setup.
+				 */
+				bdata->gpiod = NULL;
+			} else {
+				if (error != -EPROBE_DEFER)
+					dev_err(dev, "failed to get gpio: %d\n",
+						error);
+				return error;
+			}
+		}
+	} else if (gpio_is_valid(button->gpio)) {
+		/*
+		 * Legacy GPIO number, so request the GPIO here and
+		 * convert it to descriptor.
+		 */
 		unsigned flags = GPIOF_IN;
 
 		if (button->active_low)
@@ -502,7 +518,9 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 		bdata->gpiod = gpio_to_desc(button->gpio);
 		if (!bdata->gpiod)
 			return -EINVAL;
+	}
 
+	if (bdata->gpiod) {
 		if (button->debounce_interval) {
 			error = gpiod_set_debounce(bdata->gpiod,
 					button->debounce_interval * 1000);
@@ -533,9 +551,10 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 
 	} else {
 		if (!button->irq) {
-			dev_err(dev, "No IRQ specified\n");
+			dev_err(dev, "Found button without gpio or irq\n");
 			return -EINVAL;
 		}
+
 		bdata->irq = button->irq;
 
 		if (button->type && button->type != EV_KEY) {
@@ -627,24 +646,18 @@ static void gpio_keys_close(struct input_dev *input)
  * Handlers for alternative sources of platform_data
  */
 
-#ifdef CONFIG_OF
 /*
- * Translate OpenFirmware node properties into platform_data
+ * Translate properties into platform_data
  */
 static struct gpio_keys_platform_data *
 gpio_keys_get_devtree_pdata(struct device *dev)
 {
-	struct device_node *node, *pp;
 	struct gpio_keys_platform_data *pdata;
 	struct gpio_keys_button *button;
-	int error;
+	struct fwnode_handle *child;
 	int nbuttons;
 
-	node = dev->of_node;
-	if (!node)
-		return ERR_PTR(-ENODEV);
-
-	nbuttons = of_get_available_child_count(node);
+	nbuttons = device_get_child_node_count(dev);
 	if (nbuttons == 0)
 		return ERR_PTR(-ENODEV);
 
@@ -659,64 +672,43 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 	pdata->buttons = button;
 	pdata->nbuttons = nbuttons;
 
-	pdata->rep = !!of_get_property(node, "autorepeat", NULL);
+	pdata->rep = device_property_read_bool(dev, "autorepeat");
 
-	of_property_read_string(node, "label", &pdata->name);
+	device_property_read_string(dev, "label", &pdata->name);
 
-	for_each_available_child_of_node(node, pp) {
-		enum of_gpio_flags flags;
+	device_for_each_child_node(dev, child) {
+		if (is_of_node(child))
+			button->irq =
+				irq_of_parse_and_map(to_of_node(child), 0);
 
-		button->gpio = of_get_gpio_flags(pp, 0, &flags);
-		if (button->gpio < 0) {
-			error = button->gpio;
-			if (error != -ENOENT) {
-				if (error != -EPROBE_DEFER)
-					dev_err(dev,
-						"Failed to get gpio flags, error: %d\n",
-						error);
-				of_node_put(pp);
-				return ERR_PTR(error);
-			}
-		} else {
-			button->active_low = flags & OF_GPIO_ACTIVE_LOW;
-		}
-
-		button->irq = irq_of_parse_and_map(pp, 0);
-
-		if (!gpio_is_valid(button->gpio) && !button->irq) {
-			dev_err(dev, "Found button without gpios or irqs\n");
-			of_node_put(pp);
-			return ERR_PTR(-EINVAL);
-		}
-
-		if (of_property_read_u32(pp, "linux,code", &button->code)) {
-			dev_err(dev, "Button without keycode: 0x%x\n",
-				button->gpio);
-			of_node_put(pp);
+		if (fwnode_property_read_u32(child, "linux,code",
+					     &button->code)) {
+			dev_err(dev, "Button without keycode\n");
+			fwnode_handle_put(child);
 			return ERR_PTR(-EINVAL);
 		}
 
-		button->desc = of_get_property(pp, "label", NULL);
+		fwnode_property_read_string(child, "label", &button->desc);
 
-		if (of_property_read_u32(pp, "linux,input-type", &button->type))
+		if (fwnode_property_read_u32(child, "linux,input-type",
+					     &button->type))
 			button->type = EV_KEY;
 
-		button->wakeup = of_property_read_bool(pp, "wakeup-source") ||
-				 /* legacy name */
-				 of_property_read_bool(pp, "gpio-key,wakeup");
+		button->wakeup =
+			fwnode_property_read_bool(child, "wakeup-source") ||
+			/* legacy name */
+			fwnode_property_read_bool(child, "gpio-key,wakeup");
 
-		button->can_disable = !!of_get_property(pp, "linux,can-disable", NULL);
+		button->can_disable =
+			fwnode_property_read_bool(child, "linux,can-disable");
 
-		if (of_property_read_u32(pp, "debounce-interval",
+		if (fwnode_property_read_u32(child, "debounce-interval",
 					 &button->debounce_interval))
 			button->debounce_interval = 5;
 
 		button++;
 	}
 
-	if (pdata->nbuttons == 0)
-		return ERR_PTR(-EINVAL);
-
 	return pdata;
 }
 
@@ -726,20 +718,11 @@ static const struct of_device_id gpio_keys_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, gpio_keys_of_match);
 
-#else
-
-static inline struct gpio_keys_platform_data *
-gpio_keys_get_devtree_pdata(struct device *dev)
-{
-	return ERR_PTR(-ENODEV);
-}
-
-#endif
-
 static int gpio_keys_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	const struct gpio_keys_platform_data *pdata = dev_get_platdata(dev);
+	struct fwnode_handle *child = NULL;
 	struct gpio_keys_drvdata *ddata;
 	struct input_dev *input;
 	size_t size;
@@ -792,14 +775,28 @@ 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];
 
-		error = gpio_keys_setup_key(pdev, input, bdata, button);
-		if (error)
+		if (!dev_get_platdata(dev)) {
+			child = device_get_next_child_node(&pdev->dev, child);
+			if (!child) {
+				dev_err(&pdev->dev,
+					"missing child device node for entry %d\n",
+					i);
+				return -EINVAL;
+			}
+		}
+
+		error = gpio_keys_setup_key(pdev, input, bdata, button, child);
+		if (error) {
+			fwnode_handle_put(child);
 			return error;
+		}
 
 		if (button->wakeup)
 			wakeup = 1;
 	}
 
+	fwnode_handle_put(child);
+
 	error = sysfs_create_group(&pdev->dev.kobj, &gpio_keys_attr_group);
 	if (error) {
 		dev_err(dev, "Unable to export keys/switches, error: %d\n",
@@ -889,7 +886,7 @@ static struct platform_driver gpio_keys_device_driver = {
 	.driver		= {
 		.name	= "gpio-keys",
 		.pm	= &gpio_keys_pm_ops,
-		.of_match_table = of_match_ptr(gpio_keys_of_match),
+		.of_match_table = gpio_keys_of_match,
 	}
 };
 
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 6/6] Input: gpio_keys - switch to using generic device properties

[Mika Westerberg]

On Fri, Oct 28, 2016 at 04:14:44PM -0700, Dmitry Torokhov wrote:
> Make use of the device property API in this driver so that both OF based
> systems and ACPI based systems can use this driver.
> 
> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I tested 1-6 on Intel Joule with both gpio_keys and gpio_keys_polled and
it works fine. Thanks for doing this!

For the whole series,

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 1/6] Input: gpio_keys_polled - keep button data constant

[Mika Westerberg]

On Fri, Oct 28, 2016 at 04:14:39PM -0700, Dmitry Torokhov wrote:
> diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
> index ee2d8c6..d1250ad 100644
> --- a/include/linux/gpio_keys.h
> +++ b/include/linux/gpio_keys.h
> @@ -2,7 +2,6 @@
>  #define _GPIO_KEYS_H
>  
>  struct device;
> -struct gpio_desc;
>  
>  /**
>   * struct gpio_keys_button - configuration parameters
> @@ -31,7 +30,6 @@ struct gpio_keys_button {
>  	bool can_disable;
>  	int value;
>  	unsigned int irq;
> -	struct gpio_desc *gpiod;

Please remove the kernel-doc comment of this member as well.

>  };