[PATCH 0/3] input: gpio_keys: Convert to GPIO descriptors

[PATCH 0/3] input: gpio_keys: Convert to GPIO descriptors

[Geert Uytterhoeven]

	Hi Dmitry,

GPIO descriptors are the preferred way over legacy GPIO numbers
nowadays.  This patch series converts the gpio_keys driver to use GPIO
descriptors, like was done before for gpio_keys_polled.

As a bonus, support for ACPI based systems was added by making use of
the device property API. While this is not required for my use case, it
was easy to do, and keeps gpio_keys and gpio_keys_polled in sync.

This has been tested on a DT-based system only (r8a7791/koelsch).

Thanks for your comments!

Geert Uytterhoeven (3):
  input: gpio_keys: Add support for GPIO descriptors
  input: gpio_keys: Switch from irq_of_parse_and_map() to
    platform_get_irq()
  input: gpio_keys: Make use of the device property API

 drivers/input/keyboard/gpio_keys.c | 133 ++++++++++++++++++-------------------
 include/linux/gpio_keys.h          |   2 +-
 2 files changed, 66 insertions(+), 69 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/3] input: gpio_keys: Add support for GPIO descriptors

[Geert Uytterhoeven]

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>
---
 drivers/input/keyboard/gpio_keys.c | 42 +++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 29093657f2ef8233..d2b6c3acd9c32f1d 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>
@@ -140,7 +141,7 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata)
 		 */
 		disable_irq(bdata->irq);
 
-		if (gpio_is_valid(bdata->button->gpio))
+		if (!IS_ERR_OR_NULL(bdata->button->gpiod))
 			cancel_delayed_work_sync(&bdata->work);
 		else
 			del_timer_sync(&bdata->release_timer);
@@ -358,19 +359,18 @@ 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 = gpiod_get_value_cansleep(button->gpiod);
 
 	if (state < 0) {
 		dev_err(input->dev.parent, "failed to get gpio state\n");
 		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 +456,7 @@ static void gpio_keys_quiesce_key(void *data)
 {
 	struct gpio_button_data *bdata = data;
 
-	if (gpio_is_valid(bdata->button->gpio))
+	if (!IS_ERR_OR_NULL(bdata->button->gpiod))
 		cancel_delayed_work_sync(&bdata->work);
 	else
 		del_timer_sync(&bdata->release_timer);
@@ -465,7 +465,7 @@ 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)
+				struct gpio_keys_button *button)
 {
 	const char *desc = button->desc ? button->desc : "gpio_keys";
 	struct device *dev = &pdev->dev;
@@ -478,18 +478,32 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	bdata->button = button;
 	spin_lock_init(&bdata->lock);
 
-	if (gpio_is_valid(button->gpio)) {
+	/*
+	 * Legacy GPIO number so request the GPIO here and
+	 * convert it to descriptor.
+	 */
+	if (!button->gpiod && 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;
 		}
 
+		button->gpiod = gpio_to_desc(button->gpio);
+		if (!button->gpiod)
+			return -EINVAL;
+	}
+
+	if (!IS_ERR_OR_NULL(button->gpiod)) {
 		if (button->debounce_interval) {
-			error = gpio_set_debounce(button->gpio,
+			error = gpiod_set_debounce(button->gpiod,
 					button->debounce_interval * 1000);
 			/* use timer if gpiolib doesn't provide debounce */
 			if (error < 0)
@@ -500,12 +514,12 @@ 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(button->gpiod);
 			if (irq < 0) {
 				error = irq;
 				dev_err(dev,
 					"Unable to get irq number for GPIO %d, error %d\n",
-					button->gpio, error);
+					desc_to_gpio(button->gpiod), error);
 				return error;
 			}
 			bdata->irq = irq;
@@ -575,7 +589,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 (!IS_ERR_OR_NULL(bdata->button->gpiod))
 			gpio_keys_gpio_report_event(bdata);
 	}
 	input_sync(input);
@@ -771,7 +785,7 @@ static int gpio_keys_probe(struct platform_device *pdev)
 		__set_bit(EV_REP, input->evbit);
 
 	for (i = 0; i < pdata->nbuttons; i++) {
-		const struct gpio_keys_button *button = &pdata->buttons[i];
+		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);
-- 
1.9.1

[PATCH 2/3] input: gpio_keys: Switch from irq_of_parse_and_map() to platform_get_irq()

[Geert Uytterhoeven]

Note that irq_of_parse_and_map() returns 0 on failure, while
platform_get_irq() returns -ENXIO on failure, so we have to make irq
signed, and update all error checks.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/input/keyboard/gpio_keys.c | 16 ++++++++--------
 include/linux/gpio_keys.h          |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index d2b6c3acd9c32f1d..b6262d94aff19f70 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -30,7 +30,6 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/of_gpio.h>
-#include <linux/of_irq.h>
 #include <linux/spinlock.h>
 
 struct gpio_button_data {
@@ -511,7 +510,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 						button->debounce_interval;
 		}
 
-		if (button->irq) {
+		if (button->irq >= 0) {
 			bdata->irq = button->irq;
 		} else {
 			irq = gpiod_to_irq(button->gpiod);
@@ -531,7 +530,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
 
 	} else {
-		if (!button->irq) {
+		if (button->irq < 0) {
 			dev_err(dev, "No IRQ specified\n");
 			return -EINVAL;
 		}
@@ -631,8 +630,9 @@ static void gpio_keys_close(struct input_dev *input)
  * Translate OpenFirmware node properties into platform_data
  */
 static struct gpio_keys_platform_data *
-gpio_keys_get_devtree_pdata(struct device *dev)
+gpio_keys_get_devtree_pdata(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct device_node *node, *pp;
 	struct gpio_keys_platform_data *pdata;
 	struct gpio_keys_button *button;
@@ -681,9 +681,9 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 			button->active_low = flags & OF_GPIO_ACTIVE_LOW;
 		}
 
-		button->irq = irq_of_parse_and_map(pp, 0);
+		button->irq = platform_get_irq(pdev, 0);
 
-		if (!gpio_is_valid(button->gpio) && !button->irq) {
+		if (!gpio_is_valid(button->gpio) && button->irq < 0) {
 			dev_err(dev, "Found button without gpios or irqs\n");
 			return ERR_PTR(-EINVAL);
 		}
@@ -725,7 +725,7 @@ MODULE_DEVICE_TABLE(of, gpio_keys_of_match);
 #else
 
 static inline struct gpio_keys_platform_data *
-gpio_keys_get_devtree_pdata(struct device *dev)
+gpio_keys_get_devtree_pdata(struct platform_device *pdev)
 {
 	return ERR_PTR(-ENODEV);
 }
@@ -743,7 +743,7 @@ static int gpio_keys_probe(struct platform_device *pdev)
 	int wakeup = 0;
 
 	if (!pdata) {
-		pdata = gpio_keys_get_devtree_pdata(dev);
+		pdata = gpio_keys_get_devtree_pdata(pdev);
 		if (IS_ERR(pdata))
 			return PTR_ERR(pdata);
 	}
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index ee2d8c6f91300db7..0c04d38069535b86 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -30,7 +30,7 @@ struct gpio_keys_button {
 	int debounce_interval;
 	bool can_disable;
 	int value;
-	unsigned int irq;
+	int irq;
 	struct gpio_desc *gpiod;
 };
 
-- 
1.9.1

Re: [PATCH 2/3] input: gpio_keys: Switch from irq_of_parse_and_map() to platform_get_irq()

[Dmitry Torokhov]

On Fri, Feb 19, 2016 at 11:16:21AM +0100, Geert Uytterhoeven wrote:
> Note that irq_of_parse_and_map() returns 0 on failure, while
> platform_get_irq() returns -ENXIO on failure, so we have to make irq
> signed, and update all error checks.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/input/keyboard/gpio_keys.c | 16 ++++++++--------
>  include/linux/gpio_keys.h          |  2 +-
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index d2b6c3acd9c32f1d..b6262d94aff19f70 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -30,7 +30,6 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/of_gpio.h>
> -#include <linux/of_irq.h>
>  #include <linux/spinlock.h>
>  
>  struct gpio_button_data {
> @@ -511,7 +510,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>  						button->debounce_interval;
>  		}
>  
> -		if (button->irq) {
> +		if (button->irq >= 0) {
>  			bdata->irq = button->irq;
>  		} else {
>  			irq = gpiod_to_irq(button->gpiod);
> @@ -531,7 +530,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>  		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
>  
>  	} else {
> -		if (!button->irq) {
> +		if (button->irq < 0) {
>  			dev_err(dev, "No IRQ specified\n");
>  			return -EINVAL;
>  		}
> @@ -631,8 +630,9 @@ static void gpio_keys_close(struct input_dev *input)
>   * Translate OpenFirmware node properties into platform_data
>   */
>  static struct gpio_keys_platform_data *
> -gpio_keys_get_devtree_pdata(struct device *dev)
> +gpio_keys_get_devtree_pdata(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct device_node *node, *pp;
>  	struct gpio_keys_platform_data *pdata;
>  	struct gpio_keys_button *button;
> @@ -681,9 +681,9 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>  			button->active_low = flags & OF_GPIO_ACTIVE_LOW;
>  		}
>  
> -		button->irq = irq_of_parse_and_map(pp, 0);
> +		button->irq = platform_get_irq(pdev, 0);

There are a lot of current users of platform data that do not specify
button IRQ (and leave it as 0). If we start treating 0 as valid IRQ
number then we may break them.

I do not think that any real setup will actually use IRQ 0 as anything
but timer, so can we do:

		int irq;

		...

		irq = platform_get_irq(pdev, 0);
		if (irq < 0) {
			if (irq == -EPROBE_DEFER)
				return ERR_PTR(-EPROBE_DEFER);

			/* The driver treats IRQ 0 as invalid */
			irq = 0;
		}

		button->irq = irq;

and leave the rest of the code and users as is?


Or maybe inner condition should be:

			if (irq != -ENXIO)
				return ERR_PTR(irq);

Thanks.

-- 
Dmitry

[PATCH 3/3] input: gpio_keys: Make use of the device property API

[Geert Uytterhoeven]

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

Based on commits b26d4e2283b6d9b6 ("input: gpio_keys_polled: Make use of
device property API"), 99b4ffbd84ea4191 ("Input: gpio_keys[_polled] -
change name of wakeup property"), and 1feb57a245a4910b ("gpio: add
parameter to allow the use named gpios").

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Tested with DT only.
---
 drivers/input/keyboard/gpio_keys.c | 77 +++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 47 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index b6262d94aff19f70..5764308e3b26314a 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -27,9 +27,6 @@
 #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>
 #include <linux/spinlock.h>
 
 struct gpio_button_data {
@@ -625,26 +622,20 @@ 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 platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *node, *pp;
 	struct gpio_keys_platform_data *pdata;
 	struct gpio_keys_button *button;
+	struct fwnode_handle *child;
 	int error;
 	int nbuttons;
-	int i;
-
-	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);
 
@@ -655,32 +646,29 @@ gpio_keys_get_devtree_pdata(struct platform_device *pdev)
 		return ERR_PTR(-ENOMEM);
 
 	pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
-	pdata->nbuttons = nbuttons;
-
-	pdata->rep = !!of_get_property(node, "autorepeat", NULL);
 
-	of_property_read_string(node, "label", &pdata->name);
+	pdata->rep = device_property_present(dev, "autorepeat");
+	device_property_read_string(dev, "label", &pdata->name);
 
-	i = 0;
-	for_each_available_child_of_node(node, pp) {
-		enum of_gpio_flags flags;
+	device_for_each_child_node(dev, child) {
+		struct gpio_desc *desc;
 
-		button = &pdata->buttons[i++];
-
-		button->gpio = of_get_gpio_flags(pp, 0, &flags);
-		if (button->gpio < 0) {
-			error = button->gpio;
+		desc = devm_get_gpiod_from_child(dev, NULL, child);
+		if (IS_ERR(desc)) {
+			error = PTR_ERR(desc);
 			if (error != -ENOENT) {
 				if (error != -EPROBE_DEFER)
 					dev_err(dev,
 						"Failed to get gpio flags, error: %d\n",
 						error);
+				fwnode_handle_put(child);
 				return ERR_PTR(error);
 			}
-		} else {
-			button->active_low = flags & OF_GPIO_ACTIVE_LOW;
 		}
 
+		button = &pdata->buttons[pdata->nbuttons++];
+		button->gpiod = desc;
+
 		button->irq = platform_get_irq(pdev, 0);
 
 		if (!gpio_is_valid(button->gpio) && button->irq < 0) {
@@ -688,24 +676,29 @@ gpio_keys_get_devtree_pdata(struct platform_device *pdev)
 			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);
+		if (fwnode_property_read_u32(child, "linux,code",
+					     &button->code)) {
+			dev_err(dev, "Button without keycode: %d\n",
+				pdata->nbuttons - 1);
+			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_present(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;
 	}
@@ -722,16 +715,6 @@ 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 platform_device *pdev)
-{
-	return ERR_PTR(-ENODEV);
-}
-
-#endif
-
 static int gpio_keys_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -887,7 +870,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,
 	}
 };
 
-- 
1.9.1

Re: [PATCH 3/3] input: gpio_keys: Make use of the device property API

[Geert Uytterhoeven]

On Fri, Feb 19, 2016 at 11:16 AM, Geert Uytterhoeven
<geert+renesas@glider.be> 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.
>
> Based on commits b26d4e2283b6d9b6 ("input: gpio_keys_polled: Make use of
> device property API"), 99b4ffbd84ea4191 ("Input: gpio_keys[_polled] -
> change name of wakeup property"), and 1feb57a245a4910b ("gpio: add
> parameter to allow the use named gpios").
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Tested with DT only.
> ---
>  drivers/input/keyboard/gpio_keys.c | 77 +++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index b6262d94aff19f70..5764308e3b26314a 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c

> @@ -655,32 +646,29 @@ gpio_keys_get_devtree_pdata(struct platform_device *pdev)
>                 return ERR_PTR(-ENOMEM);
>
>         pdata->buttons = (struct gpio_keys_button *)(pdata + 1);
> -       pdata->nbuttons = nbuttons;
> -
> -       pdata->rep = !!of_get_property(node, "autorepeat", NULL);
>
> -       of_property_read_string(node, "label", &pdata->name);
> +       pdata->rep = device_property_present(dev, "autorepeat");
> +       device_property_read_string(dev, "label", &pdata->name);
>
> -       i = 0;
> -       for_each_available_child_of_node(node, pp) {
> -               enum of_gpio_flags flags;
> +       device_for_each_child_node(dev, child) {
> +               struct gpio_desc *desc;
>
> -               button = &pdata->buttons[i++];
> -
> -               button->gpio = of_get_gpio_flags(pp, 0, &flags);
> -               if (button->gpio < 0) {
> -                       error = button->gpio;
> +               desc = devm_get_gpiod_from_child(dev, NULL, child);
> +               if (IS_ERR(desc)) {
> +                       error = PTR_ERR(desc);
>                         if (error != -ENOENT) {
>                                 if (error != -EPROBE_DEFER)
>                                         dev_err(dev,
>                                                 "Failed to get gpio flags, error: %d\n",
>                                                 error);
> +                               fwnode_handle_put(child);
>                                 return ERR_PTR(error);
>                         }
> -               } else {
> -                       button->active_low = flags & OF_GPIO_ACTIVE_LOW;
>                 }
>
> +               button = &pdata->buttons[pdata->nbuttons++];
> +               button->gpiod = desc;
> +
>                 button->irq = platform_get_irq(pdev, 0);
>
>                 if (!gpio_is_valid(button->gpio) && button->irq < 0) {

Woops, I missed to convert one check. The above line should become:

        if (IS_ERR(button->gpiod) && button->irq < 0) {

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] input: gpio_keys: Make use of the device property API

[Dmitry Torokhov]

On Fri, Feb 19, 2016 at 11:16:22AM +0100, Geert Uytterhoeven wrote:
> @@ -887,7 +870,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,

Why are we changing this? I think match table should still be guarded
by #ifdef CONFIG_OF.

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] input: gpio_keys: Make use of the device property API

[Mika Westerberg]

On Mon, Feb 22, 2016 at 11:58:15AM -0800, Dmitry Torokhov wrote:
> On Fri, Feb 19, 2016 at 11:16:22AM +0100, Geert Uytterhoeven wrote:
> > @@ -887,7 +870,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,
> 
> Why are we changing this? I think match table should still be guarded
> by #ifdef CONFIG_OF.

This allows ACPI "PRP0001" ID to match DT .compatible strings in the
gpio_keys_of_match[] table.

Re: [PATCH 3/3] input: gpio_keys: Make use of the device property API

[Dmitry Torokhov]

On Tue, Feb 23, 2016 at 09:29:50AM +0200, Mika Westerberg wrote:
> On Mon, Feb 22, 2016 at 11:58:15AM -0800, Dmitry Torokhov wrote:
> > On Fri, Feb 19, 2016 at 11:16:22AM +0100, Geert Uytterhoeven wrote:
> > > @@ -887,7 +870,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,
> > 
> > Why are we changing this? I think match table should still be guarded
> > by #ifdef CONFIG_OF.
> 
> This allows ACPI "PRP0001" ID to match DT .compatible strings in the
> gpio_keys_of_match[] table.

Ah, I see.

-- 
Dmitry

Re: [PATCH 3/3] input: gpio_keys: Make use of the device property API

[sergk sergk2mail]

Sorry to interrupt with question, but I guess this thread has right
people for related to this topic question.
The question is reversed to the topic - how to, having working touch
driver in Android and Windows determine which exactly gpio pin is used
as INT\WAKE gpio by the driver?
Or for example even when I have some variant of such gpio pin how to
ensure that it is exactly this gpio?
For example practical task: I am trying to determine which exactly
gpio pin is responsible for INT/WAKE touchscreen on Chuwi Vi10
(baytrail x86_64 Arch 4.4.2 vanilla kernel with my custom config).
Exploring /sys/class/gpio/* have guessed that in Android it is
probably gpio134 but in Linux 4.4.2 probably gpio393. How to compare
such pins and ensure that in Linux 393 gpio is the same as 134 in
Android?
Is this possible to do in any predictable (not guessing or enumerating
all range) way?

Regards and thanks for replies,
                                                 Serge Kolotylo.

On Mon, Feb 22, 2016 at 7:58 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Feb 19, 2016 at 11:16:22AM +0100, Geert Uytterhoeven wrote:
>> @@ -887,7 +870,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,
>
> Why are we changing this? I think match table should still be guarded
> by #ifdef CONFIG_OF.
>
> Thanks.
>
> --
> Dmitry
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] input: gpio_keys: Make use of the device property API

[Mika Westerberg]

On Sun, Feb 28, 2016 at 02:03:34AM +0000, sergk sergk2mail wrote:
> Sorry to interrupt with question, but I guess this thread has right
> people for related to this topic question.
> The question is reversed to the topic - how to, having working touch
> driver in Android and Windows determine which exactly gpio pin is used
> as INT\WAKE gpio by the driver?

On ACPI systems the device description has either Interrupt() or
GpioInt() resource in its _CRS list (see
Documentation/acpi/gpio-properties.txt).

> Or for example even when I have some variant of such gpio pin how to
> ensure that it is exactly this gpio?
> For example practical task: I am trying to determine which exactly
> gpio pin is responsible for INT/WAKE touchscreen on Chuwi Vi10
> (baytrail x86_64 Arch 4.4.2 vanilla kernel with my custom config).
> Exploring /sys/class/gpio/* have guessed that in Android it is
> probably gpio134 but in Linux 4.4.2 probably gpio393. How to compare
> such pins and ensure that in Linux 393 gpio is the same as 134 in
> Android?
> Is this possible to do in any predictable (not guessing or enumerating
> all range) way?

Both DT and ACPI provide means to assign GPIOs to devices. We can then
use Linux APIs to query those. For example getting GPIO with name
"reset" is done like:

	struct gpio_desc *reset_desc;

	reset_desc = gpiod_get(dev, "reset", GPIOD_OUT_HIGH);

This will retrieve the GPIO using firmware description (ACPI, DT) if
available. No need to deal with the GPIO numbers.

Re: [PATCH 3/3] input: gpio_keys: Make use of the device property API

[sergk sergk2mail]

>
> Both DT and ACPI provide means to assign GPIOs to devices. We can then
> use Linux APIs to query those. For example getting GPIO with name
> "reset" is done like:
>
>         struct gpio_desc *reset_desc;
>
>         reset_desc = gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>
> This will retrieve the GPIO using firmware description (ACPI, DT) if
> available. No need to deal with the GPIO numbers.

Hi Mika,
Thanks for reply,

But how then to obtain gpio name or if it is possible the list of all
available names?
For example decoded ACPI DSDT shows the following:
how to get gpio name for mentioned in your reply function?
Does it according below DSDT should be "GPO1" or "INT33FC" or something other?
Kind regards,
                       Serge Kolotylo.

 Device (TCS5)
            {
                Name (_ADR, Zero)  // _ADR: Address
                Name (_HID, "CHPN0001")  // _HID: Hardware ID
                Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus)
*/)  // _CID: Compatible ID
                Name (_S0W, Zero)  // _S0W: S0 Device Wake State
                Name (_DEP, Package (0x02)  // _DEP: Dependencies
                {
                    GPO1,
                    I2C5
                })
                Method (_PS3, 0, Serialized)  // _PS3: Power State 3
                {
                }

                Method (_PS0, 0, Serialized)  // _PS0: Power State 0
                {
                    If ((^^^GPO1.AVBL == One))
                    {
                        ^^^GPO1.TCD3 = Zero
                    }

                    Sleep (0x05)
                    If ((^^^I2C5.PMI1.AVBG == One))
                    {
                        ^^^I2C5.PMI1.TCON = One
                    }

                    Sleep (0x1E)
                    If ((^^^GPO1.AVBL == One))
                    {
                        ^^^GPO1.TCD3 = One
                    }

                    Sleep (0x78)
                }

and GPO1 descr

  Device (GPO1)
        {
            Name (_ADR, Zero)  // _ADR: Address
            Name (_HID, "INT33FC" /* Intel Baytrail GPIO Controller
*/)  // _HID: Hardware ID
            Name (_CID, "INT33FC" /* Intel Baytrail GPIO Controller
*/)  // _CID: Compatible ID
            Name (_DDN, "ValleyView GPNCORE controller")  // _DDN: DOS
Device Name
            Name (_UID, 0x02)  // _UID: Unique ID
            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Name (RBUF, ResourceTemplate ()
                {
                    Memory32Fixed (ReadWrite,
                        0xFED0D000,         // Address Base
                        0x00001000,         // Address Length
                        )
                    Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
                    {
                        0x00000030,
                    }
                })
                Return (RBUF) /* \_SB_.GPO1._CRS.RBUF */
            }

            Name (AVBL, Zero)
            Method (_REG, 2, NotSerialized)  // _REG: Region Availability
            {
                If ((Arg0 == 0x08))
                {
                    AVBL = Arg1
                }
            }

            OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0C)
            Field (GPOP, ByteAcc, NoLock, Preserve)
            {
                Connection (
                    GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
IoRestrictionOutputOnly,
                        "\\_SB.GPO1", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x000F
                        }
                ),
                BST5,   1,
                Connection (
                    GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
IoRestrictionOutputOnly,
                        "\\_SB.GPO1", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x001A
                        }
                ),
                TCD3,   1
            }

Re: [PATCH 3/3] input: gpio_keys: Make use of the device property API

[Mika Westerberg]

On Mon, Feb 29, 2016 at 04:24:16PM +0000, sergk sergk2mail wrote:
> But how then to obtain gpio name or if it is possible the list of all
> available names?
> For example decoded ACPI DSDT shows the following:
> how to get gpio name for mentioned in your reply function?

For existing systems that do not provide _DSD naming for GPIOs you still
can provide them in the driver itself (ugly but works). See
Documentation/acpi/gpio-properties.txt chapter "ACPI GPIO Mappings
Provided by Drivers".

> Does it according below DSDT should be "GPO1" or "INT33FC" or something other?

No. The DSDT below does not have any names.

> Kind regards,
>                        Serge Kolotylo.
> 
>  Device (TCS5)
>             {
>                 Name (_ADR, Zero)  // _ADR: Address
>                 Name (_HID, "CHPN0001")  // _HID: Hardware ID
>                 Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus)
> */)  // _CID: Compatible ID
>                 Name (_S0W, Zero)  // _S0W: S0 Device Wake State
>                 Name (_DEP, Package (0x02)  // _DEP: Dependencies
>                 {
>                     GPO1,
>                     I2C5
>                 })
>                 Method (_PS3, 0, Serialized)  // _PS3: Power State 3
>                 {
>                 }
> 
>                 Method (_PS0, 0, Serialized)  // _PS0: Power State 0
>                 {
>                     If ((^^^GPO1.AVBL == One))
>                     {
>                         ^^^GPO1.TCD3 = Zero

Note that all these are part of GPIO Operation Region and not accessible
to the i2c-hid driver. The will be used when the device is powered on
and the pinctrl-baytrail has been loaded (that provides the Operation
Region).

If you need to use GPIOs from driver, they are listed in _CRS of the
device.

>                     }
> 
>                     Sleep (0x05)
>                     If ((^^^I2C5.PMI1.AVBG == One))
>                     {
>                         ^^^I2C5.PMI1.TCON = One
>                     }
> 
>                     Sleep (0x1E)
>                     If ((^^^GPO1.AVBL == One))
>                     {
>                         ^^^GPO1.TCD3 = One
>                     }
> 
>                     Sleep (0x78)
>                 }

Re: [PATCH 3/3] input: gpio_keys: Make use of the device property API

[Linus Walleij]

On Tue, Mar 1, 2016 at 2:52 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Feb 29, 2016 at 04:24:16PM +0000, sergk sergk2mail wrote:

>> But how then to obtain gpio name or if it is possible the list of all
>> available names?
>> For example decoded ACPI DSDT shows the following:
>> how to get gpio name for mentioned in your reply function?
>
> For existing systems that do not provide _DSD naming for GPIOs you still
> can provide them in the driver itself (ugly but works). See
> Documentation/acpi/gpio-properties.txt chapter "ACPI GPIO Mappings
> Provided by Drivers".

I just this merge window added an ABI for userspace to read name
of the GPIO line and also consumer name ("label") by using the
two strings stored in struct gpio_desc.

Currently this will be initialized per-offset from the seldom used
char names[] in struct gpio_chip, if not NULL.

We're working on DT bindings to set this per-line from the DT, and
if ACPI has a mechanism to name individual lines, please submit
patches for assigning this properly! (Hi Mika ;)

For consumers struct gpio_desc is opaque, but if they actually need
to know the name of a line we can add gpiod_get_name(struct gpio_desc *)
but then I want to know a usecase. debugfs and userspace ABI is
already displaying it just fine.

Yours,
Linus Walleij

Re: [PATCH 3/3] input: gpio_keys: Make use of the device property API

[Mika Westerberg]

On Wed, Mar 09, 2016 at 10:36:08AM +0700, Linus Walleij wrote:
> I just this merge window added an ABI for userspace to read name
> of the GPIO line and also consumer name ("label") by using the
> two strings stored in struct gpio_desc.
> 
> Currently this will be initialized per-offset from the seldom used
> char names[] in struct gpio_chip, if not NULL.
> 
> We're working on DT bindings to set this per-line from the DT, and
> if ACPI has a mechanism to name individual lines, please submit
> patches for assigning this properly! (Hi Mika ;)

Hi :)

Yes, ACPI nowadays has mechanism for assigning names to GPIOs (_DSD). We
will look into the DT implementation and try to come up with
corresponding ACPI version.

Thanks for the heads-up.