[PATCH v4 0/3] Input: gpio_keys.c: Add support for OF and I2C GPIO chips

[PATCH v4 0/3] Input: gpio_keys.c: Add support for OF and I2C GPIO chips

[David Jander]

This patch series adds support for OF device-tree platform data and non-local
GPIO chips, such as I2C I/O expanders.
This is version 4 of the patches.

Changed in this version:
 - Corrected naming of DT properties.
 - Fixed a few errors in the way dynamic pdata is handled.
 - Added support for device-tree gpios specifier.

David Jander (3):
  Input: gpio_keys.c: Simplify platform_device -> device casting
  Input: gpio_keys.c: Added support for device-tree platform data
  Input: gpio_keys.c: Enable use with non-local GPIO chips.

 .../devicetree/bindings/gpio/gpio_keys.txt         |   49 +++++
 drivers/input/keyboard/gpio_keys.c                 |  193 ++++++++++++++++----
 2 files changed, 207 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt

-- 
1.7.4.1

[PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting

[David Jander]

This patch factors out the use of struct platform_device *pdev in most
places.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/input/keyboard/gpio_keys.c |   46 ++++++++++++++++-------------------
 1 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 6e6145b..987498e 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -251,8 +251,7 @@ static ssize_t gpio_keys_show_##name(struct device *dev,		\
 				     struct device_attribute *attr,	\
 				     char *buf)				\
 {									\
-	struct platform_device *pdev = to_platform_device(dev);		\
-	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);	\
+	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);		\
 									\
 	return gpio_keys_attr_show_helper(ddata, buf,			\
 					  type, only_disabled);		\
@@ -278,8 +277,7 @@ static ssize_t gpio_keys_store_##name(struct device *dev,		\
 				      const char *buf,			\
 				      size_t count)			\
 {									\
-	struct platform_device *pdev = to_platform_device(dev);		\
-	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);	\
+	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);		\
 	ssize_t error;							\
 									\
 	error = gpio_keys_attr_store_helper(ddata, buf, type);		\
@@ -364,12 +362,11 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int __devinit gpio_keys_setup_key(struct platform_device *pdev,
+static int __devinit gpio_keys_setup_key(struct device *dev,
 					 struct gpio_button_data *bdata,
 					 struct gpio_keys_button *button)
 {
 	const char *desc = button->desc ? button->desc : "gpio_keys";
-	struct device *dev = &pdev->dev;
 	unsigned long irqflags;
 	int irq, error;
 
@@ -447,9 +444,9 @@ static void gpio_keys_close(struct input_dev *input)
 
 static int __devinit gpio_keys_probe(struct platform_device *pdev)
 {
-	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
 	struct gpio_keys_drvdata *ddata;
 	struct device *dev = &pdev->dev;
+	struct gpio_keys_platform_data *pdata = dev->platform_data;
 	struct input_dev *input;
 	int i, error;
 	int wakeup = 0;
@@ -470,12 +467,12 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 	ddata->disable = pdata->disable;
 	mutex_init(&ddata->disable_lock);
 
-	platform_set_drvdata(pdev, ddata);
+	dev_set_drvdata(dev, ddata);
 	input_set_drvdata(input, ddata);
 
 	input->name = pdata->name ? : pdev->name;
 	input->phys = "gpio-keys/input0";
-	input->dev.parent = &pdev->dev;
+	input->dev.parent = dev;
 	input->open = gpio_keys_open;
 	input->close = gpio_keys_close;
 
@@ -496,7 +493,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		bdata->input = input;
 		bdata->button = button;
 
-		error = gpio_keys_setup_key(pdev, bdata, button);
+		error = gpio_keys_setup_key(dev, bdata, button);
 		if (error)
 			goto fail2;
 
@@ -506,7 +503,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		input_set_capability(input, type, button->code);
 	}
 
-	error = sysfs_create_group(&pdev->dev.kobj, &gpio_keys_attr_group);
+	error = sysfs_create_group(&dev->kobj, &gpio_keys_attr_group);
 	if (error) {
 		dev_err(dev, "Unable to export keys/switches, error: %d\n",
 			error);
@@ -525,12 +522,12 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		gpio_keys_report_event(&ddata->data[i]);
 	input_sync(input);
 
-	device_init_wakeup(&pdev->dev, wakeup);
+	device_init_wakeup(dev, wakeup);
 
 	return 0;
 
  fail3:
-	sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
+	sysfs_remove_group(&dev->kobj, &gpio_keys_attr_group);
  fail2:
 	while (--i >= 0) {
 		free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
@@ -540,7 +537,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		gpio_free(pdata->buttons[i].gpio);
 	}
 
-	platform_set_drvdata(pdev, NULL);
+	dev_set_drvdata(dev, NULL);
  fail1:
 	input_free_device(input);
 	kfree(ddata);
@@ -550,14 +547,15 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 
 static int __devexit gpio_keys_remove(struct platform_device *pdev)
 {
-	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
-	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	struct gpio_keys_platform_data *pdata = dev->platform_data;
+	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
 	struct input_dev *input = ddata->input;
 	int i;
 
-	sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
+	sysfs_remove_group(&dev->kobj, &gpio_keys_attr_group);
 
-	device_init_wakeup(&pdev->dev, 0);
+	device_init_wakeup(dev, 0);
 
 	for (i = 0; i < pdata->nbuttons; i++) {
 		int irq = gpio_to_irq(pdata->buttons[i].gpio);
@@ -577,11 +575,10 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM
 static int gpio_keys_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
+	struct gpio_keys_platform_data *pdata = dev->platform_data;
 	int i;
 
-	if (device_may_wakeup(&pdev->dev)) {
+	if (device_may_wakeup(dev)) {
 		for (i = 0; i < pdata->nbuttons; i++) {
 			struct gpio_keys_button *button = &pdata->buttons[i];
 			if (button->wakeup) {
@@ -596,15 +593,14 @@ static int gpio_keys_suspend(struct device *dev)
 
 static int gpio_keys_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
-	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
+	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
+	struct gpio_keys_platform_data *pdata = dev->platform_data;
 	int i;
 
 	for (i = 0; i < pdata->nbuttons; i++) {
 
 		struct gpio_keys_button *button = &pdata->buttons[i];
-		if (button->wakeup && device_may_wakeup(&pdev->dev)) {
+		if (button->wakeup && device_may_wakeup(dev)) {
 			int irq = gpio_to_irq(button->gpio);
 			disable_irq_wake(irq);
 		}
-- 
1.7.4.1

[PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data

[David Jander]

This patch enables fetching configuration data which is normally provided via
platform_data from the device-tree instead.
If the device is configured from device-tree data, the platform_data struct
is not used, and button data needs to be allocated dynamically.
Big part of this patch deals with confining pdata usage to the probe function,
to make this possible.

Signed-off-by: David Jander <david@protonic.nl>
---
 .../devicetree/bindings/gpio/gpio_keys.txt         |   49 +++++++
 drivers/input/keyboard/gpio_keys.c                 |  147 ++++++++++++++++++--
 2 files changed, 186 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio_keys.txt b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
new file mode 100644
index 0000000..60a4d8e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
@@ -0,0 +1,49 @@
+Device-Tree bindings for input/gpio_keys.c keyboard driver
+
+Required properties:
+	- compatible = "gpio-keys";
+
+Optional properties:
+	- autorepeat: Boolean, Enable auto repeat feature of Linux input
+	  subsystem.
+
+Each button (key) is represented as a sub-node of "gpio-keys":
+Subnode properties:
+
+	- reg: GPIO number the key is bound to if linux GPIO numbering is used.
+	- gpios: OF devcie-tree gpio specification, can be used alternatively
+	  if 'reg' is not specified, to use device-tree GPIOs.
+	- label: Descriptive name of the key.
+	- linux,code: Keycode to emit.
+
+Optional subnode-properties:
+	- active-low: Boolean flag to specify active-low GPIO input. Not used
+	  if device-tree gpios property is used.
+	- linux,input-type: Specify event type this button/key generates.
+	  Default if unspecified is <1> == EV_KEY.
+	- debounce-interval: Debouncing interval time in milliseconds.
+	  Default if unspecified is 5.
+	- wakeup: Boolean, button can wake-up the system.
+
+Example nodes:
+
+	gpio_keys {
+			compatible = "gpio-keys";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			autorepeat;
+			button@20 {
+				label = "GPIO Key ESC";
+				linux,code = <1>;
+				reg = <0x20>;
+				key-active-low;
+				linux,input-type = <1>;
+			};
+			button@21 {
+				label = "GPIO Key UP";
+				linux,code = <103>;
+				gpios = <&gpio1 0 1>;
+				linux,input-type = <1>;
+			};
+			...
+
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 987498e..78aeeaa 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -3,6 +3,9 @@
  *
  * Copyright 2005 Phil Blundell
  *
+ * Added OF support:
+ * Copyright 2010 David Jander <david@protonic.nl>
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
@@ -25,6 +28,8 @@
 #include <linux/gpio_keys.h>
 #include <linux/workqueue.h>
 #include <linux/gpio.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
 
 struct gpio_button_data {
 	struct gpio_keys_button *button;
@@ -442,15 +447,124 @@ static void gpio_keys_close(struct input_dev *input)
 		ddata->disable(input->dev.parent);
 }
 
+/*
+ * Handlers for alternative sources of platform_data
+ */
+#ifdef CONFIG_OF
+/*
+ * Translate OpenFirmware node properties into platform_data
+ */
+static struct gpio_keys_platform_data *
+gpio_keys_get_devtree_pdata(struct device *dev,
+			    struct gpio_keys_platform_data *altp)
+{
+	struct gpio_keys_platform_data *pdata = altp;
+	struct device_node *node, *pp;
+	int i;
+	struct gpio_keys_button *buttons;
+	const u32 *reg;
+	int len;
+
+	node = dev->of_node;
+	if (node == NULL)
+		return NULL;
+
+	memset(pdata, 0, sizeof *pdata);
+
+	pdata->rep = !!of_get_property(node, "autorepeat", &len);
+
+	/* First count the subnodes */
+	pdata->nbuttons = 0;
+	pp = NULL;
+	while ((pp = of_get_next_child(node, pp)))
+		pdata->nbuttons++;
+
+	if (pdata->nbuttons == 0)
+		return NULL;
+
+	buttons = kzalloc(pdata->nbuttons * (sizeof *buttons), GFP_KERNEL);
+	if (!buttons)
+		return NULL;
+
+	pp = NULL;
+	i = 0;
+	while ((pp = of_get_next_child(node, pp))) {
+		const char *lbl;
+		enum of_gpio_flags flags;
+
+		reg = of_get_property(pp, "reg", &len);
+		if (!reg && !of_find_property(pp, "gpios", NULL)) {
+			pdata->nbuttons--;
+			dev_warn(dev, "Found button without reg and without gpios\n");
+			continue;
+		}
+		if (reg) {
+			buttons[i].gpio = be32_to_cpup(reg);
+			buttons[i].active_low = !!of_get_property(pp, "key-active-low", NULL);
+		} else {
+			buttons[i].gpio = of_get_gpio_flags(pp, 0, &flags);
+			buttons[i].active_low = !!(flags & OF_GPIO_ACTIVE_LOW);
+		}
+
+		reg = of_get_property(pp, "linux,code", &len);
+		if (!reg) {
+			dev_err(dev, "Button without keycode: 0x%x\n", buttons[i].gpio);
+			goto out_fail;
+		}
+		buttons[i].code = be32_to_cpup(reg);
+
+		lbl = of_get_property(pp, "label", &len);
+		buttons[i].desc = (char *)lbl;
+
+		reg = of_get_property(pp, "linux,input-type", &len);
+		if (reg)
+			buttons[i].type = be32_to_cpup(reg);
+		else
+			buttons[i].type = EV_KEY;
+
+		buttons[i].wakeup = !!of_get_property(pp, "wakeup", NULL);
+
+		reg = of_get_property(pp, "debounce-interval", &len);
+		if (reg)
+			buttons[i].debounce_interval = be32_to_cpup(reg);
+		else
+			buttons[i].debounce_interval = 5;
+		i++;
+	}
+
+	pdata->buttons = buttons;
+
+	return pdata;
+
+out_fail:
+	kfree(buttons);
+	return NULL;
+}
+#else
+static struct gpio_keys_platform_data *
+gpio_keys_get_devtree_pdata(struct device *dev,
+			    struct gpio_keys_platform_data *altp)
+{
+	return NULL;
+}
+#endif
+
 static int __devinit gpio_keys_probe(struct platform_device *pdev)
 {
 	struct gpio_keys_drvdata *ddata;
 	struct device *dev = &pdev->dev;
 	struct gpio_keys_platform_data *pdata = dev->platform_data;
+	struct gpio_keys_platform_data alt_pdata;
 	struct input_dev *input;
 	int i, error;
 	int wakeup = 0;
 
+	if (!pdata) {
+		pdata = gpio_keys_get_devtree_pdata(dev, &alt_pdata);
+		if (!pdata)
+			return -ENODEV;
+	}
+
 	ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
 			pdata->nbuttons * sizeof(struct gpio_button_data),
 			GFP_KERNEL);
@@ -548,7 +662,6 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 static int __devexit gpio_keys_remove(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct gpio_keys_platform_data *pdata = dev->platform_data;
 	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
 	struct input_dev *input = ddata->input;
 	int i;
@@ -557,30 +670,42 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 
 	device_init_wakeup(dev, 0);
 
-	for (i = 0; i < pdata->nbuttons; i++) {
-		int irq = gpio_to_irq(pdata->buttons[i].gpio);
+	for (i = 0; i < ddata->n_buttons; i++) {
+		int irq = gpio_to_irq(ddata->data[i].button->gpio);
 		free_irq(irq, &ddata->data[i]);
 		if (ddata->data[i].timer_debounce)
 			del_timer_sync(&ddata->data[i].timer);
 		cancel_work_sync(&ddata->data[i].work);
-		gpio_free(pdata->buttons[i].gpio);
+		gpio_free(ddata->data[i].button->gpio);
 	}
 
+	/*
+	 * If we had no platform_data, we allocated buttons dynamically, and
+	 * must free them here. ddata->data[0].button is the pointer to the
+	 * beginning of the allocated array.
+	 */
+	if (!dev->platform_data)
+		kfree(ddata->data[0].button);
+
 	input_unregister_device(input);
 
 	return 0;
 }
 
+static struct of_device_id gpio_keys_of_match[] = {
+	{ .compatible = "gpio-keys", },
+	{},
+};
 
 #ifdef CONFIG_PM
 static int gpio_keys_suspend(struct device *dev)
 {
-	struct gpio_keys_platform_data *pdata = dev->platform_data;
+	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
 	int i;
 
 	if (device_may_wakeup(dev)) {
-		for (i = 0; i < pdata->nbuttons; i++) {
-			struct gpio_keys_button *button = &pdata->buttons[i];
+		for (i = 0; i < ddata->n_buttons; i++) {
+			struct gpio_keys_button *button = ddata->data[i].button;
 			if (button->wakeup) {
 				int irq = gpio_to_irq(button->gpio);
 				enable_irq_wake(irq);
@@ -594,12 +719,11 @@ static int gpio_keys_suspend(struct device *dev)
 static int gpio_keys_resume(struct device *dev)
 {
 	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
-	struct gpio_keys_platform_data *pdata = dev->platform_data;
 	int i;
 
-	for (i = 0; i < pdata->nbuttons; i++) {
+	for (i = 0; i < ddata->n_buttons; i++) {
 
-		struct gpio_keys_button *button = &pdata->buttons[i];
+		struct gpio_keys_button *button = ddata->data[i].button;
 		if (button->wakeup && device_may_wakeup(dev)) {
 			int irq = gpio_to_irq(button->gpio);
 			disable_irq_wake(irq);
@@ -618,6 +742,8 @@ static const struct dev_pm_ops gpio_keys_pm_ops = {
 };
 #endif
 
+MODULE_DEVICE_TABLE(of, gpio_keys_of_match);
+
 static struct platform_driver gpio_keys_device_driver = {
 	.probe		= gpio_keys_probe,
 	.remove		= __devexit_p(gpio_keys_remove),
@@ -627,6 +753,7 @@ static struct platform_driver gpio_keys_device_driver = {
 #ifdef CONFIG_PM
 		.pm	= &gpio_keys_pm_ops,
 #endif
+		.of_match_table = gpio_keys_of_match,
 	}
 };
 
-- 
1.7.4.1

[PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[David Jander]

Use a threaded interrupt handler in order to permit the handler to use
a GPIO driver that causes things like I2C transactions being done inside
the handler context.
Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
all needed GPIO drivers have been loaded if the drivers are built into the
kernel.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/input/keyboard/gpio_keys.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 78aeeaa..d179861 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -3,7 +3,7 @@
  *
  * Copyright 2005 Phil Blundell
  *
- * Added OF support:
+ * Added OF support and enabled use with I2C GPIO expanders:
  * Copyright 2010 David Jander <david@protonic.nl>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -417,7 +417,7 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
 	if (!button->can_disable)
 		irqflags |= IRQF_SHARED;
 
-	error = request_any_context_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
+	error = request_threaded_irq(irq, NULL, gpio_keys_isr, irqflags, desc, bdata);
 	if (error < 0) {
 		dev_err(dev, "Unable to claim irq %d; error %d\n",
 			irq, error);
@@ -767,10 +767,10 @@ static void __exit gpio_keys_exit(void)
 	platform_driver_unregister(&gpio_keys_device_driver);
 }
 
-module_init(gpio_keys_init);
+late_initcall(gpio_keys_init);
 module_exit(gpio_keys_exit);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Phil Blundell <pb@handhelds.org>");
-MODULE_DESCRIPTION("Keyboard driver for CPU GPIOs");
+MODULE_DESCRIPTION("Keyboard driver for GPIOs");
 MODULE_ALIAS("platform:gpio-keys");
-- 
1.7.4.1

Re: [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data

[Grant Likely]

On Tue, Jun 14, 2011 at 11:08:10AM +0200, David Jander wrote:
> This patch enables fetching configuration data which is normally provided via
> platform_data from the device-tree instead.
> If the device is configured from device-tree data, the platform_data struct
> is not used, and button data needs to be allocated dynamically.
> Big part of this patch deals with confining pdata usage to the probe function,
> to make this possible.
> 
> Signed-off-by: David Jander <david@protonic.nl>
> ---
>  .../devicetree/bindings/gpio/gpio_keys.txt         |   49 +++++++
>  drivers/input/keyboard/gpio_keys.c                 |  147 ++++++++++++++++++--
>  2 files changed, 186 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio_keys.txt b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> new file mode 100644
> index 0000000..60a4d8e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> @@ -0,0 +1,49 @@
> +Device-Tree bindings for input/gpio_keys.c keyboard driver
> +
> +Required properties:
> +	- compatible = "gpio-keys";
> +
> +Optional properties:
> +	- autorepeat: Boolean, Enable auto repeat feature of Linux input
> +	  subsystem.
> +
> +Each button (key) is represented as a sub-node of "gpio-keys":
> +Subnode properties:
> +
> +	- reg: GPIO number the key is bound to if linux GPIO numbering is used.

Wait.  That won't work.  There is no concept of "linux gpio numbering"
in the device tree data.  When using device tree, the gpio numbers
usually get dynamically assigned.

> +	- gpios: OF devcie-tree gpio specification, can be used alternatively
> +	  if 'reg' is not specified, to use device-tree GPIOs.
> +	- label: Descriptive name of the key.
> +	- linux,code: Keycode to emit.
> +
> +Optional subnode-properties:
> +	- active-low: Boolean flag to specify active-low GPIO input. Not used
> +	  if device-tree gpios property is used.
> +	- linux,input-type: Specify event type this button/key generates.
> +	  Default if unspecified is <1> == EV_KEY.
> +	- debounce-interval: Debouncing interval time in milliseconds.
> +	  Default if unspecified is 5.
> +	- wakeup: Boolean, button can wake-up the system.

"wakeup" is potentially too generic a property name (potential to
conflict with a generic wakeup binding if one ever exists).  Just to
be defencive, I'd suggest prefixing these custom gpio keys properties
with something like "gpio-key,".

> +
> +Example nodes:
> +
> +	gpio_keys {
> +			compatible = "gpio-keys";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			autorepeat;
> +			button@20 {
> +				label = "GPIO Key ESC";
> +				linux,code = <1>;
> +				reg = <0x20>;
> +				key-active-low;
> +				linux,input-type = <1>;
> +			};
> +			button@21 {
> +				label = "GPIO Key UP";
> +				linux,code = <103>;
> +				gpios = <&gpio1 0 1>;
> +				linux,input-type = <1>;
> +			};
> +			...
> +
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 987498e..78aeeaa 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -3,6 +3,9 @@
>   *
>   * Copyright 2005 Phil Blundell
>   *
> + * Added OF support:
> + * Copyright 2010 David Jander <david@protonic.nl>
> + *

But it's 2011!  :-)

>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
> @@ -25,6 +28,8 @@
>  #include <linux/gpio_keys.h>
>  #include <linux/workqueue.h>
>  #include <linux/gpio.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
>  
>  struct gpio_button_data {
>  	struct gpio_keys_button *button;
> @@ -442,15 +447,124 @@ static void gpio_keys_close(struct input_dev *input)
>  		ddata->disable(input->dev.parent);
>  }
>  
> +/*
> + * Handlers for alternative sources of platform_data
> + */
> +#ifdef CONFIG_OF
> +/*
> + * Translate OpenFirmware node properties into platform_data
> + */
> +static struct gpio_keys_platform_data *
> +gpio_keys_get_devtree_pdata(struct device *dev,
> +			    struct gpio_keys_platform_data *altp)
> +{
> +	struct gpio_keys_platform_data *pdata = altp;

pdata is always the same as altp.  You don't need this on the stack,
and the return value should be an error code instead of a pointer
because the pointer is already passed in!

> +	struct device_node *node, *pp;
> +	int i;
> +	struct gpio_keys_button *buttons;
> +	const u32 *reg;
> +	int len;
> +
> +	node = dev->of_node;
> +	if (node == NULL)
> +		return NULL;
> +
> +	memset(pdata, 0, sizeof *pdata);
> +
> +	pdata->rep = !!of_get_property(node, "autorepeat", &len);
> +
> +	/* First count the subnodes */
> +	pdata->nbuttons = 0;
> +	pp = NULL;
> +	while ((pp = of_get_next_child(node, pp)))
> +		pdata->nbuttons++;
> +
> +	if (pdata->nbuttons == 0)
> +		return NULL;
> +
> +	buttons = kzalloc(pdata->nbuttons * (sizeof *buttons), GFP_KERNEL);
> +	if (!buttons)
> +		return NULL;
> +
> +	pp = NULL;
> +	i = 0;
> +	while ((pp = of_get_next_child(node, pp))) {
> +		const char *lbl;
> +		enum of_gpio_flags flags;
> +
> +		reg = of_get_property(pp, "reg", &len);
> +		if (!reg && !of_find_property(pp, "gpios", NULL)) {
> +			pdata->nbuttons--;
> +			dev_warn(dev, "Found button without reg and without gpios\n");
> +			continue;
> +		}
> +		if (reg) {
> +			buttons[i].gpio = be32_to_cpup(reg);

As mentioned above, this won't work.  Linux gpio numbers cannot be
encoded into the DT.

> +			buttons[i].active_low = !!of_get_property(pp, "key-active-low", NULL);
> +		} else {
> +			buttons[i].gpio = of_get_gpio_flags(pp, 0, &flags);
> +			buttons[i].active_low = !!(flags & OF_GPIO_ACTIVE_LOW);
> +		}
> +
> +		reg = of_get_property(pp, "linux,code", &len);
> +		if (!reg) {
> +			dev_err(dev, "Button without keycode: 0x%x\n", buttons[i].gpio);
> +			goto out_fail;
> +		}
> +		buttons[i].code = be32_to_cpup(reg);
> +
> +		lbl = of_get_property(pp, "label", &len);
> +		buttons[i].desc = (char *)lbl;
> +
> +		reg = of_get_property(pp, "linux,input-type", &len);
> +		if (reg)
> +			buttons[i].type = be32_to_cpup(reg);
> +		else
> +			buttons[i].type = EV_KEY;
how about:
		buttons[i].type = reg ? be32_to_cpup(reg) : EV_KEY;

> +
> +		buttons[i].wakeup = !!of_get_property(pp, "wakeup", NULL);
> +
> +		reg = of_get_property(pp, "debounce-interval", &len);
> +		if (reg)
> +			buttons[i].debounce_interval = be32_to_cpup(reg);
> +		else
> +			buttons[i].debounce_interval = 5;

Ditto here.

> +		i++;
> +	}
> +
> +	pdata->buttons = buttons;
> +
> +	return pdata;
> +
> +out_fail:
> +	kfree(buttons);
> +	return NULL;
> +}
> +#else
> +static struct gpio_keys_platform_data *
> +gpio_keys_get_devtree_pdata(struct device *dev,
> +			    struct gpio_keys_platform_data *altp)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  {
>  	struct gpio_keys_drvdata *ddata;
>  	struct device *dev = &pdev->dev;
>  	struct gpio_keys_platform_data *pdata = dev->platform_data;
> +	struct gpio_keys_platform_data alt_pdata;
>  	struct input_dev *input;
>  	int i, error;
>  	int wakeup = 0;
>  
> +	if (!pdata) {
> +		pdata = gpio_keys_get_devtree_pdata(dev, &alt_pdata);
> +		if (!pdata)
> +			return -ENODEV;
> +	}

then this would become:

	if (!pdata) {
		rc = gpio_keys_get_devtree_pdata(dev, &alt_pdata);
		if (rc)
			return rc;
		pdata = &alt_pdata;
	}

> +
>  	ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
>  			pdata->nbuttons * sizeof(struct gpio_button_data),
>  			GFP_KERNEL);
> @@ -548,7 +662,6 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  static int __devexit gpio_keys_remove(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct gpio_keys_platform_data *pdata = dev->platform_data;
>  	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
>  	struct input_dev *input = ddata->input;
>  	int i;
> @@ -557,30 +670,42 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
>  
>  	device_init_wakeup(dev, 0);
>  
> -	for (i = 0; i < pdata->nbuttons; i++) {
> -		int irq = gpio_to_irq(pdata->buttons[i].gpio);
> +	for (i = 0; i < ddata->n_buttons; i++) {
> +		int irq = gpio_to_irq(ddata->data[i].button->gpio);
>  		free_irq(irq, &ddata->data[i]);
>  		if (ddata->data[i].timer_debounce)
>  			del_timer_sync(&ddata->data[i].timer);
>  		cancel_work_sync(&ddata->data[i].work);
> -		gpio_free(pdata->buttons[i].gpio);
> +		gpio_free(ddata->data[i].button->gpio);
>  	}
>  
> +	/*
> +	 * If we had no platform_data, we allocated buttons dynamically, and
> +	 * must free them here. ddata->data[0].button is the pointer to the
> +	 * beginning of the allocated array.
> +	 */
> +	if (!dev->platform_data)
> +		kfree(ddata->data[0].button);
> +
>  	input_unregister_device(input);
>  
>  	return 0;
>  }
>  
> +static struct of_device_id gpio_keys_of_match[] = {
> +	{ .compatible = "gpio-keys", },
> +	{},
> +};
>  
>  #ifdef CONFIG_PM
>  static int gpio_keys_suspend(struct device *dev)
>  {
> -	struct gpio_keys_platform_data *pdata = dev->platform_data;
> +	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
>  	int i;
>  
>  	if (device_may_wakeup(dev)) {
> -		for (i = 0; i < pdata->nbuttons; i++) {
> -			struct gpio_keys_button *button = &pdata->buttons[i];
> +		for (i = 0; i < ddata->n_buttons; i++) {
> +			struct gpio_keys_button *button = ddata->data[i].button;
>  			if (button->wakeup) {
>  				int irq = gpio_to_irq(button->gpio);
>  				enable_irq_wake(irq);
> @@ -594,12 +719,11 @@ static int gpio_keys_suspend(struct device *dev)
>  static int gpio_keys_resume(struct device *dev)
>  {
>  	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
> -	struct gpio_keys_platform_data *pdata = dev->platform_data;
>  	int i;
>  
> -	for (i = 0; i < pdata->nbuttons; i++) {
> +	for (i = 0; i < ddata->n_buttons; i++) {
>  
> -		struct gpio_keys_button *button = &pdata->buttons[i];
> +		struct gpio_keys_button *button = ddata->data[i].button;
>  		if (button->wakeup && device_may_wakeup(dev)) {
>  			int irq = gpio_to_irq(button->gpio);
>  			disable_irq_wake(irq);
> @@ -618,6 +742,8 @@ static const struct dev_pm_ops gpio_keys_pm_ops = {
>  };
>  #endif
>  
> +MODULE_DEVICE_TABLE(of, gpio_keys_of_match);
> +

Modules device table needs to be #ifdef CONFIG_OF protected.
Otherwise the driver advertises behaviour that it cannot provide.

>  static struct platform_driver gpio_keys_device_driver = {
>  	.probe		= gpio_keys_probe,
>  	.remove		= __devexit_p(gpio_keys_remove),
> @@ -627,6 +753,7 @@ static struct platform_driver gpio_keys_device_driver = {
>  #ifdef CONFIG_PM
>  		.pm	= &gpio_keys_pm_ops,
>  #endif
> +		.of_match_table = gpio_keys_of_match,
>  	}
>  };
>  
> -- 
> 1.7.4.1
> 

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Grant Likely]

On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> Use a threaded interrupt handler in order to permit the handler to use
> a GPIO driver that causes things like I2C transactions being done inside
> the handler context.
> Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> all needed GPIO drivers have been loaded if the drivers are built into the
> kernel.

...which is a horrid hack, but until device dependencies can be
described, it isn't one that can be solved easily.

This patch looks okay to me.

Acked-by: Grant Likely <grant.likely@secretlab.ca>

g.

> 
> Signed-off-by: David Jander <david@protonic.nl>
> ---
>  drivers/input/keyboard/gpio_keys.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 78aeeaa..d179861 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -3,7 +3,7 @@
>   *
>   * Copyright 2005 Phil Blundell
>   *
> - * Added OF support:
> + * Added OF support and enabled use with I2C GPIO expanders:
>   * Copyright 2010 David Jander <david@protonic.nl>
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -417,7 +417,7 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
>  	if (!button->can_disable)
>  		irqflags |= IRQF_SHARED;
>  
> -	error = request_any_context_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
> +	error = request_threaded_irq(irq, NULL, gpio_keys_isr, irqflags, desc, bdata);
>  	if (error < 0) {
>  		dev_err(dev, "Unable to claim irq %d; error %d\n",
>  			irq, error);
> @@ -767,10 +767,10 @@ static void __exit gpio_keys_exit(void)
>  	platform_driver_unregister(&gpio_keys_device_driver);
>  }
>  
> -module_init(gpio_keys_init);
> +late_initcall(gpio_keys_init);
>  module_exit(gpio_keys_exit);
>  
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Phil Blundell <pb@handhelds.org>");
> -MODULE_DESCRIPTION("Keyboard driver for CPU GPIOs");
> +MODULE_DESCRIPTION("Keyboard driver for GPIOs");
>  MODULE_ALIAS("platform:gpio-keys");
> -- 
> 1.7.4.1
> 

Re: [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting

[Grant Likely]

On Tue, Jun 14, 2011 at 11:08:09AM +0200, David Jander wrote:
> This patch factors out the use of struct platform_device *pdev in most
> places.
> 
> Signed-off-by: David Jander <david@protonic.nl>

Okay by me.  I assume this one isn't needed unless patch 2 is also merged.

Acked-by: Grant Likely <grant.likely@secretlab.ca>
g.

> ---
>  drivers/input/keyboard/gpio_keys.c |   46 ++++++++++++++++-------------------
>  1 files changed, 21 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 6e6145b..987498e 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -251,8 +251,7 @@ static ssize_t gpio_keys_show_##name(struct device *dev,		\
>  				     struct device_attribute *attr,	\
>  				     char *buf)				\
>  {									\
> -	struct platform_device *pdev = to_platform_device(dev);		\
> -	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);	\
> +	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);		\
>  									\
>  	return gpio_keys_attr_show_helper(ddata, buf,			\
>  					  type, only_disabled);		\
> @@ -278,8 +277,7 @@ static ssize_t gpio_keys_store_##name(struct device *dev,		\
>  				      const char *buf,			\
>  				      size_t count)			\
>  {									\
> -	struct platform_device *pdev = to_platform_device(dev);		\
> -	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);	\
> +	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);		\
>  	ssize_t error;							\
>  									\
>  	error = gpio_keys_attr_store_helper(ddata, buf, type);		\
> @@ -364,12 +362,11 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static int __devinit gpio_keys_setup_key(struct platform_device *pdev,
> +static int __devinit gpio_keys_setup_key(struct device *dev,
>  					 struct gpio_button_data *bdata,
>  					 struct gpio_keys_button *button)
>  {
>  	const char *desc = button->desc ? button->desc : "gpio_keys";
> -	struct device *dev = &pdev->dev;
>  	unsigned long irqflags;
>  	int irq, error;
>  
> @@ -447,9 +444,9 @@ static void gpio_keys_close(struct input_dev *input)
>  
>  static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  {
> -	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
>  	struct gpio_keys_drvdata *ddata;
>  	struct device *dev = &pdev->dev;
> +	struct gpio_keys_platform_data *pdata = dev->platform_data;
>  	struct input_dev *input;
>  	int i, error;
>  	int wakeup = 0;
> @@ -470,12 +467,12 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  	ddata->disable = pdata->disable;
>  	mutex_init(&ddata->disable_lock);
>  
> -	platform_set_drvdata(pdev, ddata);
> +	dev_set_drvdata(dev, ddata);
>  	input_set_drvdata(input, ddata);
>  
>  	input->name = pdata->name ? : pdev->name;
>  	input->phys = "gpio-keys/input0";
> -	input->dev.parent = &pdev->dev;
> +	input->dev.parent = dev;
>  	input->open = gpio_keys_open;
>  	input->close = gpio_keys_close;
>  
> @@ -496,7 +493,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  		bdata->input = input;
>  		bdata->button = button;
>  
> -		error = gpio_keys_setup_key(pdev, bdata, button);
> +		error = gpio_keys_setup_key(dev, bdata, button);
>  		if (error)
>  			goto fail2;
>  
> @@ -506,7 +503,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  		input_set_capability(input, type, button->code);
>  	}
>  
> -	error = sysfs_create_group(&pdev->dev.kobj, &gpio_keys_attr_group);
> +	error = sysfs_create_group(&dev->kobj, &gpio_keys_attr_group);
>  	if (error) {
>  		dev_err(dev, "Unable to export keys/switches, error: %d\n",
>  			error);
> @@ -525,12 +522,12 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  		gpio_keys_report_event(&ddata->data[i]);
>  	input_sync(input);
>  
> -	device_init_wakeup(&pdev->dev, wakeup);
> +	device_init_wakeup(dev, wakeup);
>  
>  	return 0;
>  
>   fail3:
> -	sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
> +	sysfs_remove_group(&dev->kobj, &gpio_keys_attr_group);
>   fail2:
>  	while (--i >= 0) {
>  		free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
> @@ -540,7 +537,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  		gpio_free(pdata->buttons[i].gpio);
>  	}
>  
> -	platform_set_drvdata(pdev, NULL);
> +	dev_set_drvdata(dev, NULL);
>   fail1:
>  	input_free_device(input);
>  	kfree(ddata);
> @@ -550,14 +547,15 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  
>  static int __devexit gpio_keys_remove(struct platform_device *pdev)
>  {
> -	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
> -	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +	struct gpio_keys_platform_data *pdata = dev->platform_data;
> +	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
>  	struct input_dev *input = ddata->input;
>  	int i;
>  
> -	sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
> +	sysfs_remove_group(&dev->kobj, &gpio_keys_attr_group);
>  
> -	device_init_wakeup(&pdev->dev, 0);
> +	device_init_wakeup(dev, 0);
>  
>  	for (i = 0; i < pdata->nbuttons; i++) {
>  		int irq = gpio_to_irq(pdata->buttons[i].gpio);
> @@ -577,11 +575,10 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
>  #ifdef CONFIG_PM
>  static int gpio_keys_suspend(struct device *dev)
>  {
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
> +	struct gpio_keys_platform_data *pdata = dev->platform_data;
>  	int i;
>  
> -	if (device_may_wakeup(&pdev->dev)) {
> +	if (device_may_wakeup(dev)) {
>  		for (i = 0; i < pdata->nbuttons; i++) {
>  			struct gpio_keys_button *button = &pdata->buttons[i];
>  			if (button->wakeup) {
> @@ -596,15 +593,14 @@ static int gpio_keys_suspend(struct device *dev)
>  
>  static int gpio_keys_resume(struct device *dev)
>  {
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
> -	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
> +	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
> +	struct gpio_keys_platform_data *pdata = dev->platform_data;
>  	int i;
>  
>  	for (i = 0; i < pdata->nbuttons; i++) {
>  
>  		struct gpio_keys_button *button = &pdata->buttons[i];
> -		if (button->wakeup && device_may_wakeup(&pdev->dev)) {
> +		if (button->wakeup && device_may_wakeup(dev)) {
>  			int irq = gpio_to_irq(button->gpio);
>  			disable_irq_wake(irq);
>  		}
> -- 
> 1.7.4.1
> 

Re: [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data

[David Jander]

Hi Grant,

On Thu, 16 Jun 2011 13:25:59 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> On Tue, Jun 14, 2011 at 11:08:10AM +0200, David Jander wrote:
> > This patch enables fetching configuration data which is normally provided
> > via platform_data from the device-tree instead.
> > If the device is configured from device-tree data, the platform_data struct
> > is not used, and button data needs to be allocated dynamically.
> > Big part of this patch deals with confining pdata usage to the probe
> > function, to make this possible.
> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> > ---
> >  .../devicetree/bindings/gpio/gpio_keys.txt         |   49 +++++++
> >  drivers/input/keyboard/gpio_keys.c                 |  147
> > ++++++++++++++++++-- 2 files changed, 186 insertions(+), 10 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> > b/Documentation/devicetree/bindings/gpio/gpio_keys.txt new file mode 100644
> > index 0000000..60a4d8e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> > @@ -0,0 +1,49 @@
> > +Device-Tree bindings for input/gpio_keys.c keyboard driver
> > +
> > +Required properties:
> > +	- compatible = "gpio-keys";
> > +
> > +Optional properties:
> > +	- autorepeat: Boolean, Enable auto repeat feature of Linux input
> > +	  subsystem.
> > +
> > +Each button (key) is represented as a sub-node of "gpio-keys":
> > +Subnode properties:
> > +
> > +	- reg: GPIO number the key is bound to if linux GPIO numbering is
> > used.
> 
> Wait.  That won't work.  There is no concept of "linux gpio numbering"
> in the device tree data.  When using device tree, the gpio numbers
> usually get dynamically assigned.

Yes I know, but suppose you want to use this driver with a GPIO-driver that
does not have devaice-tree support yet? I need a way of binding the button to
a GPIO pin that does not have a device-tree definition. How should I do this
otherwise?
I am using this driver with a pca963x, as you might have suspected already,
and I just added OF device-tree support to it, so that will work, but
others...?
Before "fixing" pca963x, I used this property and it worked perfectly well, so
please explain what is not supposed to work. Please keep in mind that this
is meant as more of a backwards-compatibility feature. If you think that being
backwards compatible with non-OF GPIO drivers is a bad idea, I'll remove this.

> > +	- gpios: OF devcie-tree gpio specification, can be used
> > alternatively
> > +	  if 'reg' is not specified, to use device-tree GPIOs.
> > +	- label: Descriptive name of the key.
> > +	- linux,code: Keycode to emit.
> > +
> > +Optional subnode-properties:
> > +	- active-low: Boolean flag to specify active-low GPIO input. Not
> > used
> > +	  if device-tree gpios property is used.
> > +	- linux,input-type: Specify event type this button/key generates.
> > +	  Default if unspecified is <1> == EV_KEY.
> > +	- debounce-interval: Debouncing interval time in milliseconds.
> > +	  Default if unspecified is 5.
> > +	- wakeup: Boolean, button can wake-up the system.
> 
> "wakeup" is potentially too generic a property name (potential to
> conflict with a generic wakeup binding if one ever exists).  Just to
> be defencive, I'd suggest prefixing these custom gpio keys properties
> with something like "gpio-key,".

Ok, "gpio-key,wakeup" it will be then? But isn't this function something
potentially other IO-pins/keys/buttons/interrupts/etc... could have to be able
to wake up the system?

> > +
> > +Example nodes:
> > +
> > +	gpio_keys {
> > +			compatible = "gpio-keys";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			autorepeat;
> > +			button@20 {
> > +				label = "GPIO Key ESC";
> > +				linux,code = <1>;
> > +				reg = <0x20>;
> > +				key-active-low;
> > +				linux,input-type = <1>;
> > +			};
> > +			button@21 {
> > +				label = "GPIO Key UP";
> > +				linux,code = <103>;
> > +				gpios = <&gpio1 0 1>;
> > +				linux,input-type = <1>;
> > +			};
> > +			...
> > +
> > diff --git a/drivers/input/keyboard/gpio_keys.c
> > b/drivers/input/keyboard/gpio_keys.c index 987498e..78aeeaa 100644
> > --- a/drivers/input/keyboard/gpio_keys.c
> > +++ b/drivers/input/keyboard/gpio_keys.c
> > @@ -3,6 +3,9 @@
> >   *
> >   * Copyright 2005 Phil Blundell
> >   *
> > + * Added OF support:
> > + * Copyright 2010 David Jander <david@protonic.nl>
> > + *
> 
> But it's 2011!  :-)

Ooops :-) You see... this patch is rather old already (in my tree). I actually
wrote it in 2010, but I am submitting it now. I guess it should be "2010, 2011"
then?

> >   * This program is free software; you can redistribute it and/or modify
> >   * it under the terms of the GNU General Public License version 2 as
> >   * published by the Free Software Foundation.
> > @@ -25,6 +28,8 @@
> >  #include <linux/gpio_keys.h>
> >  #include <linux/workqueue.h>
> >  #include <linux/gpio.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_gpio.h>
> >  
> >  struct gpio_button_data {
> >  	struct gpio_keys_button *button;
> > @@ -442,15 +447,124 @@ static void gpio_keys_close(struct input_dev *input)
> >  		ddata->disable(input->dev.parent);
> >  }
> >  
> > +/*
> > + * Handlers for alternative sources of platform_data
> > + */
> > +#ifdef CONFIG_OF
> > +/*
> > + * Translate OpenFirmware node properties into platform_data
> > + */
> > +static struct gpio_keys_platform_data *
> > +gpio_keys_get_devtree_pdata(struct device *dev,
> > +			    struct gpio_keys_platform_data *altp)
> > +{
> > +	struct gpio_keys_platform_data *pdata = altp;
> 
> pdata is always the same as altp.

Ok, right.

> You don't need this on the stack, and the return value should be an error
> code instead of a pointer because the pointer is already passed in!

Hmm... I was (mis-)using the returned pointer as an error code. Will try to
come up with something more sensible.

> > +	struct device_node *node, *pp;
> > +	int i;
> > +	struct gpio_keys_button *buttons;
> > +	const u32 *reg;
> > +	int len;
> > +
> > +	node = dev->of_node;
> > +	if (node == NULL)
> > +		return NULL;
> > +
> > +	memset(pdata, 0, sizeof *pdata);
> > +
> > +	pdata->rep = !!of_get_property(node, "autorepeat", &len);
> > +
> > +	/* First count the subnodes */
> > +	pdata->nbuttons = 0;
> > +	pp = NULL;
> > +	while ((pp = of_get_next_child(node, pp)))
> > +		pdata->nbuttons++;
> > +
> > +	if (pdata->nbuttons == 0)
> > +		return NULL;
> > +
> > +	buttons = kzalloc(pdata->nbuttons * (sizeof *buttons),
> > GFP_KERNEL);
> > +	if (!buttons)
> > +		return NULL;
> > +
> > +	pp = NULL;
> > +	i = 0;
> > +	while ((pp = of_get_next_child(node, pp))) {
> > +		const char *lbl;
> > +		enum of_gpio_flags flags;
> > +
> > +		reg = of_get_property(pp, "reg", &len);
> > +		if (!reg && !of_find_property(pp, "gpios", NULL)) {
> > +			pdata->nbuttons--;
> > +			dev_warn(dev, "Found button without reg and
> > without gpios\n");
> > +			continue;
> > +		}
> > +		if (reg) {
> > +			buttons[i].gpio = be32_to_cpup(reg);
> 
> As mentioned above, this won't work.  Linux gpio numbers cannot be
> encoded into the DT.

Why not? It worked fine for me before I "fixed" pca963x.
If you have a non-OF GPIO controller, that one will need a numeric range of
GPIO numbers. If that range is fixed, I can perfectly well give this number to
the driver here.... again, this is only used if the GPIO driver does not
have a device-tree node.

> > +			buttons[i].active_low = !!of_get_property(pp,
> > "key-active-low", NULL);
> > +		} else {
> > +			buttons[i].gpio = of_get_gpio_flags(pp, 0,
> > &flags);
> > +			buttons[i].active_low = !!(flags &
> > OF_GPIO_ACTIVE_LOW);
> > +		}
> > +
> > +		reg = of_get_property(pp, "linux,code", &len);
> > +		if (!reg) {
> > +			dev_err(dev, "Button without keycode: 0x%x\n",
> > buttons[i].gpio);
> > +			goto out_fail;
> > +		}
> > +		buttons[i].code = be32_to_cpup(reg);
> > +
> > +		lbl = of_get_property(pp, "label", &len);
> > +		buttons[i].desc = (char *)lbl;
> > +
> > +		reg = of_get_property(pp, "linux,input-type", &len);
> > +		if (reg)
> > +			buttons[i].type = be32_to_cpup(reg);
> > +		else
> > +			buttons[i].type = EV_KEY;
> how about:
> 		buttons[i].type = reg ? be32_to_cpup(reg) : EV_KEY;

Ok, if you prefer this notation.... just an "if...else" in another dress ;-)

> > +
> > +		buttons[i].wakeup = !!of_get_property(pp, "wakeup", NULL);
> > +
> > +		reg = of_get_property(pp, "debounce-interval", &len);
> > +		if (reg)
> > +			buttons[i].debounce_interval = be32_to_cpup(reg);
> > +		else
> > +			buttons[i].debounce_interval = 5;
> 
> Ditto here.

Ok, as you wish.

> > +		i++;
> > +	}
> > +
> > +	pdata->buttons = buttons;
> > +
> > +	return pdata;
> > +
> > +out_fail:
> > +	kfree(buttons);
> > +	return NULL;
> > +}
> > +#else
> > +static struct gpio_keys_platform_data *
> > +gpio_keys_get_devtree_pdata(struct device *dev,
> > +			    struct gpio_keys_platform_data *altp)
> > +{
> > +	return NULL;
> > +}
> > +#endif
> > +
> >  static int __devinit gpio_keys_probe(struct platform_device *pdev)
> >  {
> >  	struct gpio_keys_drvdata *ddata;
> >  	struct device *dev = &pdev->dev;
> >  	struct gpio_keys_platform_data *pdata = dev->platform_data;
> > +	struct gpio_keys_platform_data alt_pdata;
> >  	struct input_dev *input;
> >  	int i, error;
> >  	int wakeup = 0;
> >  
> > +	if (!pdata) {
> > +		pdata = gpio_keys_get_devtree_pdata(dev, &alt_pdata);
> > +		if (!pdata)
> > +			return -ENODEV;
> > +	}
> 
> then this would become:
> 
> 	if (!pdata) {
> 		rc = gpio_keys_get_devtree_pdata(dev, &alt_pdata);
> 		if (rc)
> 			return rc;
> 		pdata = &alt_pdata;
> 	}

Yes, of course. I just need to "invent" which error codes to use for the
different failure cases.... no problem.

> > +
> >  	ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
> >  			pdata->nbuttons * sizeof(struct gpio_button_data),
> >  			GFP_KERNEL);
> > @@ -548,7 +662,6 @@ static int __devinit gpio_keys_probe(struct
> > platform_device *pdev) static int __devexit gpio_keys_remove(struct
> > platform_device *pdev) {
> >  	struct device *dev = &pdev->dev;
> > -	struct gpio_keys_platform_data *pdata = dev->platform_data;
> >  	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
> >  	struct input_dev *input = ddata->input;
> >  	int i;
> > @@ -557,30 +670,42 @@ static int __devexit gpio_keys_remove(struct
> > platform_device *pdev) 
> >  	device_init_wakeup(dev, 0);
> >  
> > -	for (i = 0; i < pdata->nbuttons; i++) {
> > -		int irq = gpio_to_irq(pdata->buttons[i].gpio);
> > +	for (i = 0; i < ddata->n_buttons; i++) {
> > +		int irq = gpio_to_irq(ddata->data[i].button->gpio);
> >  		free_irq(irq, &ddata->data[i]);
> >  		if (ddata->data[i].timer_debounce)
> >  			del_timer_sync(&ddata->data[i].timer);
> >  		cancel_work_sync(&ddata->data[i].work);
> > -		gpio_free(pdata->buttons[i].gpio);
> > +		gpio_free(ddata->data[i].button->gpio);
> >  	}
> >  
> > +	/*
> > +	 * If we had no platform_data, we allocated buttons dynamically,
> > and
> > +	 * must free them here. ddata->data[0].button is the pointer to
> > the
> > +	 * beginning of the allocated array.
> > +	 */
> > +	if (!dev->platform_data)
> > +		kfree(ddata->data[0].button);
> > +
> >  	input_unregister_device(input);
> >  
> >  	return 0;
> >  }
> >  
> > +static struct of_device_id gpio_keys_of_match[] = {
> > +	{ .compatible = "gpio-keys", },
> > +	{},
> > +};
> >  
> >  #ifdef CONFIG_PM
> >  static int gpio_keys_suspend(struct device *dev)
> >  {
> > -	struct gpio_keys_platform_data *pdata = dev->platform_data;
> > +	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
> >  	int i;
> >  
> >  	if (device_may_wakeup(dev)) {
> > -		for (i = 0; i < pdata->nbuttons; i++) {
> > -			struct gpio_keys_button *button =
> > &pdata->buttons[i];
> > +		for (i = 0; i < ddata->n_buttons; i++) {
> > +			struct gpio_keys_button *button =
> > ddata->data[i].button; if (button->wakeup) {
> >  				int irq = gpio_to_irq(button->gpio);
> >  				enable_irq_wake(irq);
> > @@ -594,12 +719,11 @@ static int gpio_keys_suspend(struct device *dev)
> >  static int gpio_keys_resume(struct device *dev)
> >  {
> >  	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
> > -	struct gpio_keys_platform_data *pdata = dev->platform_data;
> >  	int i;
> >  
> > -	for (i = 0; i < pdata->nbuttons; i++) {
> > +	for (i = 0; i < ddata->n_buttons; i++) {
> >  
> > -		struct gpio_keys_button *button = &pdata->buttons[i];
> > +		struct gpio_keys_button *button = ddata->data[i].button;
> >  		if (button->wakeup && device_may_wakeup(dev)) {
> >  			int irq = gpio_to_irq(button->gpio);
> >  			disable_irq_wake(irq);
> > @@ -618,6 +742,8 @@ static const struct dev_pm_ops gpio_keys_pm_ops = {
> >  };
> >  #endif
> >  
> > +MODULE_DEVICE_TABLE(of, gpio_keys_of_match);
> > +
> 
> Modules device table needs to be #ifdef CONFIG_OF protected.
> Otherwise the driver advertises behaviour that it cannot provide.

Ah, ok. Will add an #ifdef. Thanks for pointing out.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data

[Grant Likely]

On Fri, Jun 17, 2011 at 2:58 AM, David Jander <david.jander@protonic.nl> wrote:
>
> Hi Grant,
>
> On Thu, 16 Jun 2011 13:25:59 -0600
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
>> On Tue, Jun 14, 2011 at 11:08:10AM +0200, David Jander wrote:
>> > This patch enables fetching configuration data which is normally provided
>> > via platform_data from the device-tree instead.
>> > If the device is configured from device-tree data, the platform_data struct
>> > is not used, and button data needs to be allocated dynamically.
>> > Big part of this patch deals with confining pdata usage to the probe
>> > function, to make this possible.
>> >
>> > Signed-off-by: David Jander <david@protonic.nl>
>> > ---
>> >  .../devicetree/bindings/gpio/gpio_keys.txt         |   49 +++++++
>> >  drivers/input/keyboard/gpio_keys.c                 |  147
>> > ++++++++++++++++++-- 2 files changed, 186 insertions(+), 10 deletions(-)
>> >  create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/gpio/gpio_keys.txt
>> > b/Documentation/devicetree/bindings/gpio/gpio_keys.txt new file mode 100644
>> > index 0000000..60a4d8e
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
>> > @@ -0,0 +1,49 @@
>> > +Device-Tree bindings for input/gpio_keys.c keyboard driver
>> > +
>> > +Required properties:
>> > +   - compatible = "gpio-keys";
>> > +
>> > +Optional properties:
>> > +   - autorepeat: Boolean, Enable auto repeat feature of Linux input
>> > +     subsystem.
>> > +
>> > +Each button (key) is represented as a sub-node of "gpio-keys":
>> > +Subnode properties:
>> > +
>> > +   - reg: GPIO number the key is bound to if linux GPIO numbering is
>> > used.
>>
>> Wait.  That won't work.  There is no concept of "linux gpio numbering"
>> in the device tree data.  When using device tree, the gpio numbers
>> usually get dynamically assigned.
>
> Yes I know, but suppose you want to use this driver with a GPIO-driver that
> does not have devaice-tree support yet? I need a way of binding the button to
> a GPIO pin that does not have a device-tree definition. How should I do this
> otherwise?
> I am using this driver with a pca963x, as you might have suspected already,
> and I just added OF device-tree support to it, so that will work, but
> others...?

The solution is to add OF support to the GPIO driver being used.

> Before "fixing" pca963x, I used this property and it worked perfectly well, so
> please explain what is not supposed to work. Please keep in mind that this
> is meant as more of a backwards-compatibility feature. If you think that being
> backwards compatible with non-OF GPIO drivers is a bad idea, I'll remove this.

It is.  Something that we've been very careful about is to avoid
encoding Linux-specific implementation details into the device tree
bindings.  The implementation details, such as how gpio controllers
are enumerated, are subject to change just in the normal progress of
development.  By focusing the DT bindings on HW description, the DT
data is insulated to a degree from kernel churn.

>> > +   - wakeup: Boolean, button can wake-up the system.
>>
>> "wakeup" is potentially too generic a property name (potential to
>> conflict with a generic wakeup binding if one ever exists).  Just to
>> be defencive, I'd suggest prefixing these custom gpio keys properties
>> with something like "gpio-key,".
>
> Ok, "gpio-key,wakeup" it will be then? But isn't this function something
> potentially other IO-pins/keys/buttons/interrupts/etc... could have to be able
> to wake up the system?

Can you foresee how all bindings would potentially use a 'wakeup'
property?  It's really hard to define a common binding without first
having several use cases ready to use it.  It's better to start being
cautious, and then create a common binding at some point in the
future.


>> > +           reg = of_get_property(pp, "linux,input-type", &len);
>> > +           if (reg)
>> > +                   buttons[i].type = be32_to_cpup(reg);
>> > +           else
>> > +                   buttons[i].type = EV_KEY;
>> how about:
>>               buttons[i].type = reg ? be32_to_cpup(reg) : EV_KEY;
>
> Ok, if you prefer this notation.... just an "if...else" in another dress ;-)

Yup, but it's shorter, and I like painting bike sheds.

g.
--
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 v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Dmitry Torokhov]

On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > Use a threaded interrupt handler in order to permit the handler to use
> > a GPIO driver that causes things like I2C transactions being done inside
> > the handler context.
> > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> > all needed GPIO drivers have been loaded if the drivers are built into the
> > kernel.
> 
> ...which is a horrid hack, but until device dependencies can be
> described, it isn't one that can be solved easily.
> 

I really do not want to apply this... Currently the order of
initialization does not matter since nothing actually happens until
corresponding device appears on the bus. Does the OF code creates
devices before all resources are ready?

Thanks.

-- 
Dmitry

Re: [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting

[Dmitry Torokhov]

On Tue, Jun 14, 2011 at 11:08:09AM +0200, David Jander wrote:
> This patch factors out the use of struct platform_device *pdev in most
> places.
> 

Why? We are dealing with a platform device so why would we switch to
generic device?

I also think that we should not be mixing dev_get/set_drvdata() and
<bus>_get/set_drvdata() calls but rather use appropriate bus-specific
version to access data on given layer.

Thanks.

-- 
Dmitry

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Grant Likely]

On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
>> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
>> > Use a threaded interrupt handler in order to permit the handler to use
>> > a GPIO driver that causes things like I2C transactions being done inside
>> > the handler context.
>> > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
>> > all needed GPIO drivers have been loaded if the drivers are built into the
>> > kernel.
>>
>> ...which is a horrid hack, but until device dependencies can be
>> described, it isn't one that can be solved easily.
>>
>
> I really do not want to apply this... Currently the order of
> initialization does not matter since nothing actually happens until
> corresponding device appears on the bus. Does the OF code creates
> devices before all resources are ready?

It's not an OF problem.  The problem is that all the platform_devices
typically get registered all at once at machine_init time (on arm),
and if the gpio expander isn't a platform_device, (like an i2c gpio
expander which would end up being a child of a platform_device), then
it won't be ready.  The real problem is that we have no mechanism for
holding off or deferring a driver probe if it depends on an
asynchronous resource.

g.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Dmitry Torokhov]

On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
> On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
> >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> >> > Use a threaded interrupt handler in order to permit the handler to use
> >> > a GPIO driver that causes things like I2C transactions being done inside
> >> > the handler context.
> >> > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> >> > all needed GPIO drivers have been loaded if the drivers are built into the
> >> > kernel.
> >>
> >> ...which is a horrid hack, but until device dependencies can be
> >> described, it isn't one that can be solved easily.
> >>
> >
> > I really do not want to apply this... Currently the order of
> > initialization does not matter since nothing actually happens until
> > corresponding device appears on the bus. Does the OF code creates
> > devices before all resources are ready?
> 
> It's not an OF problem.  The problem is that all the platform_devices
> typically get registered all at once at machine_init time (on arm),
> and if the gpio expander isn't a platform_device, (like an i2c gpio
> expander which would end up being a child of a platform_device), then
> it won't be ready.

Ah, I see. But that can be handled in board code that should ensure that
it registers devices in correct order.

>  The real problem is that we have no mechanism for
> holding off or deferring a driver probe if it depends on an
> asynchronous resource.

The mechanism we do have - we should not be creating the device for the
driver to bind to unless all resources that are needed by that device
are ready.

Just shuffling the initcall order is not maintanable. Next there will be
GPIO expander that is for some reason registered as late_initcall and
we'll be back to square one. I am going to take the threaded IRQ bit but
will drop the initcall bit from the patch.

Thanks.

-- 
Dmitry

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Grant Likely]

On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote:
> On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
> > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
> > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > >> > Use a threaded interrupt handler in order to permit the handler to use
> > >> > a GPIO driver that causes things like I2C transactions being done inside
> > >> > the handler context.
> > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> > >> > all needed GPIO drivers have been loaded if the drivers are built into the
> > >> > kernel.
> > >>
> > >> ...which is a horrid hack, but until device dependencies can be
> > >> described, it isn't one that can be solved easily.
> > >>
> > >
> > > I really do not want to apply this... Currently the order of
> > > initialization does not matter since nothing actually happens until
> > > corresponding device appears on the bus. Does the OF code creates
> > > devices before all resources are ready?
> > 
> > It's not an OF problem.  The problem is that all the platform_devices
> > typically get registered all at once at machine_init time (on arm),
> > and if the gpio expander isn't a platform_device, (like an i2c gpio
> > expander which would end up being a child of a platform_device), then
> > it won't be ready.
> 
> Ah, I see. But that can be handled in board code that should ensure that
> it registers devices in correct order.

Unfortunately, handling it in board code doesn't really work either.
It just shuffles the complexity to the board code to implement some
kind of deferred mechanism for registering devices, and it has to take
into account that it may be a long time before the device actually
appears, such as when the driver is configured as a module.

I completely agree that shuffling initcall order isn't maintainable
though.

A related concern is that changing the device registration order, or
the initcall order, does absolutely nothing to tell runtime PM about
the dependencies between devices.  For instance, how does runtime PM
know when it is safe to PM a gpio controller, when it has no reference
to devices depending on it, like gpio-keys?  (although gpio-keys isn't
a great example because it doesn't really have any runtime PM states).

I think part of the solution is to give drivers the option of
returning a 'defer' code at probe time if it cannot obtain all it's
resources, and have the driver core re-probe it when more devices
become available, but I haven't had time to prototype it yet.

g.

> 
> >  The real problem is that we have no mechanism for
> > holding off or deferring a driver probe if it depends on an
> > asynchronous resource.
> 
> The mechanism we do have - we should not be creating the device for the
> driver to bind to unless all resources that are needed by that device
> are ready.
> 
> Just shuffling the initcall order is not maintanable. Next there will be
> GPIO expander that is for some reason registered as late_initcall and
> we'll be back to square one. I am going to take the threaded IRQ bit but
> will drop the initcall bit from the patch.
> 
> Thanks.
> 
> -- 
> Dmitry

Re: [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting

[David Jander]

Hi Dmitry,

On Sat, 18 Jun 2011 03:19:25 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Tue, Jun 14, 2011 at 11:08:09AM +0200, David Jander wrote:
> > This patch factors out the use of struct platform_device *pdev in most
> > places.
> > 
> 
> Why? We are dealing with a platform device so why would we switch to
> generic device?

Actually, when I wrote this patch there still was a difference between
the platform bus and the of_platform bus, and this change was necessary. There
also were ifdefs around platform_driver_register and
of_platform_driver_register. Now it seems this has been merged, and I am
not sure it is necessary anymore, but I still think it simplifies the code
quite a bit. Also, why should the driver be bus-dependent, when it doesn't
even need a real "bus" (it talks to an abstract device through another driver,
potentially connected to any bus), besides due to how linux views devices and
drivers.

> I also think that we should not be mixing dev_get/set_drvdata() and
> <bus>_get/set_drvdata() calls

AFAICS, we are not mixing.... it is dev_*_drvdata() only.

> but rather use appropriate bus-specific version to access data on given
> layer.

Doesn't that make the driver much too complex? And why would that be
necessary? The driver isn't bus-specific anymore... except for the binding and
probing part.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[David Jander]

On Sat, 18 Jun 2011 09:16:45 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote:
> > On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
> > > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
> > > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > > >> > Use a threaded interrupt handler in order to permit the handler to
> > > >> > use a GPIO driver that causes things like I2C transactions being
> > > >> > done inside the handler context.
> > > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to
> > > >> > make sure all needed GPIO drivers have been loaded if the drivers
> > > >> > are built into the kernel.
> > > >>
> > > >> ...which is a horrid hack, but until device dependencies can be
> > > >> described, it isn't one that can be solved easily.
> > > >>
> > > >
> > > > I really do not want to apply this... Currently the order of
> > > > initialization does not matter since nothing actually happens until
> > > > corresponding device appears on the bus. Does the OF code creates
> > > > devices before all resources are ready?
> > > 
> > > It's not an OF problem.  The problem is that all the platform_devices
> > > typically get registered all at once at machine_init time (on arm),
> > > and if the gpio expander isn't a platform_device, (like an i2c gpio
> > > expander which would end up being a child of a platform_device), then
> > > it won't be ready.
> > 
> > Ah, I see. But that can be handled in board code that should ensure that
> > it registers devices in correct order.
> 
> Unfortunately, handling it in board code doesn't really work either.
> It just shuffles the complexity to the board code to implement some
> kind of deferred mechanism for registering devices, and it has to take
> into account that it may be a long time before the device actually
> appears, such as when the driver is configured as a module.

Besides... we don't want anymore board-code, do we? I mean, if a board can use
a generic board configuration and specify all it needs in the device-tree, why
should something as trivial as connecting a gpio_keys device to a I2C GPIO
expander force us to do special board setup all of a sudden?
IMHO specifying I2C-gpios to be used for gpio_keys should "just work", even if
declared in a device-tree.

> I completely agree that shuffling initcall order isn't maintainable
> though.

I also agree, and if there is a better solution to make this work without
additional board-support code, please tell me.
I just think that this patch makes the already cool gpio_keys driver quite a
bit more awesome. IMO, being able to just hook it all up in the device-tree is
just fantastic, and we should make it possible.

> A related concern is that changing the device registration order, or
> the initcall order, does absolutely nothing to tell runtime PM about
> the dependencies between devices.  For instance, how does runtime PM
> know when it is safe to PM a gpio controller, when it has no reference
> to devices depending on it, like gpio-keys?  (although gpio-keys isn't
> a great example because it doesn't really have any runtime PM states).
> 
> I think part of the solution is to give drivers the option of
> returning a 'defer' code at probe time if it cannot obtain all it's
> resources, and have the driver core re-probe it when more devices
> become available, but I haven't had time to prototype it yet.

Sounds interesting. So the probe function could return some sort of -ENOTYET
or -EAGAIN and have it called again later?

But, does that mean that we really need to miss this use-case until something
like this gets approved and merged? Can't we just declare this late_initcall
for now and fix it later? Please!

> > >  The real problem is that we have no mechanism for
> > > holding off or deferring a driver probe if it depends on an
> > > asynchronous resource.
> > 
> > The mechanism we do have - we should not be creating the device for the
> > driver to bind to unless all resources that are needed by that device
> > are ready.

How would we do that in a device-tree?

> > Just shuffling the initcall order is not maintanable. Next there will be
> > GPIO expander that is for some reason registered as late_initcall and
> > we'll be back to square one. I am going to take the threaded IRQ bit but
> > will drop the initcall bit from the patch.

That would destroy the whole purpose of this patch. Do you mean to say, what I
want to do has no acceptable implementation? That would be a pity, since IMHO
it is a very cool feature, and quite trivial to implement this way.
Our boards do not need any board setup code. Actually just adding one
line of code in arch/powerpc/platforms/512x/mpc5121_generic.c or
arch/powerpc/platforms/52xx/mpc5200_simple.c is enough to support any of our
boards that need this driver... the rest is done in the device-tree. Don't you
think this is worth that little bit of (temporary) ugliness?

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting

[Dmitry Torokhov]

Hi David,

On Mon, Jun 20, 2011 at 08:52:13AM +0200, David Jander wrote:
> 
> Hi Dmitry,
> 
> On Sat, 18 Jun 2011 03:19:25 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Tue, Jun 14, 2011 at 11:08:09AM +0200, David Jander wrote:
> > > This patch factors out the use of struct platform_device *pdev in most
> > > places.
> > > 
> > 
> > Why? We are dealing with a platform device so why would we switch to
> > generic device?
> 
> Actually, when I wrote this patch there still was a difference between
> the platform bus and the of_platform bus, and this change was necessary. There
> also were ifdefs around platform_driver_register and
> of_platform_driver_register. Now it seems this has been merged, and I am
> not sure it is necessary anymore, but I still think it simplifies the code
> quite a bit. Also, why should the driver be bus-dependent, when it doesn't
> even need a real "bus" (it talks to an abstract device through another driver,
> potentially connected to any bus), besides due to how linux views devices and
> drivers.

While there isn't real hardware bus the driver is fitted into platform
device framework and so we should use platform device API unless there
is compelling reason for using another API.

> 
> > I also think that we should not be mixing dev_get/set_drvdata() and
> > <bus>_get/set_drvdata() calls
> 
> AFAICS, we are not mixing.... it is dev_*_drvdata() only.

"Mixing" was probably not the best word. "Using API from a different
layer" would probably be better.

> 
> > but rather use appropriate bus-specific version to access data on given
> > layer.
> 
> Doesn't that make the driver much too complex? And why would that be
> necessary? The driver isn't bus-specific anymore... except for the binding and
> probing part.

Why would it make the driver more complex? Aside from a couple of
PM methods coming from the driver core and therefore operating on
"struct device *" the rest is using platform device directly.

Thanks.

-- 
Dmitry

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Dmitry Torokhov]

On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote:
> On Sat, 18 Jun 2011 09:16:45 -0600
> Grant Likely <grant.likely@secretlab.ca> wrote:
> 
> > On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote:
> > > On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
> > > > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
> > > > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > > > >> > Use a threaded interrupt handler in order to permit the handler to
> > > > >> > use a GPIO driver that causes things like I2C transactions being
> > > > >> > done inside the handler context.
> > > > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to
> > > > >> > make sure all needed GPIO drivers have been loaded if the drivers
> > > > >> > are built into the kernel.
> > > > >>
> > > > >> ...which is a horrid hack, but until device dependencies can be
> > > > >> described, it isn't one that can be solved easily.
> > > > >>
> > > > >
> > > > > I really do not want to apply this... Currently the order of
> > > > > initialization does not matter since nothing actually happens until
> > > > > corresponding device appears on the bus. Does the OF code creates
> > > > > devices before all resources are ready?
> > > > 
> > > > It's not an OF problem.  The problem is that all the platform_devices
> > > > typically get registered all at once at machine_init time (on arm),
> > > > and if the gpio expander isn't a platform_device, (like an i2c gpio
> > > > expander which would end up being a child of a platform_device), then
> > > > it won't be ready.
> > > 
> > > Ah, I see. But that can be handled in board code that should ensure that
> > > it registers devices in correct order.
> > 
> > Unfortunately, handling it in board code doesn't really work either.
> > It just shuffles the complexity to the board code to implement some
> > kind of deferred mechanism for registering devices, and it has to take
> > into account that it may be a long time before the device actually
> > appears, such as when the driver is configured as a module.
> 
> Besides... we don't want anymore board-code, do we? I mean, if a board can use
> a generic board configuration and specify all it needs in the device-tree, why
> should something as trivial as connecting a gpio_keys device to a I2C GPIO
> expander force us to do special board setup all of a sudden?
> IMHO specifying I2C-gpios to be used for gpio_keys should "just work", even if
> declared in a device-tree.

This is a laudable goal, but then device-tree needs to be able to
express device dependencies better. Until then board-specific code is
needed to register devices in proper order.

> 
> > I completely agree that shuffling initcall order isn't maintainable
> > though.
> 
> I also agree, and if there is a better solution to make this work without
> additional board-support code, please tell me.
> I just think that this patch makes the already cool gpio_keys driver quite a
> bit more awesome. IMO, being able to just hook it all up in the device-tree is
> just fantastic, and we should make it possible.
> 
> > A related concern is that changing the device registration order, or
> > the initcall order, does absolutely nothing to tell runtime PM about
> > the dependencies between devices.  For instance, how does runtime PM
> > know when it is safe to PM a gpio controller, when it has no reference
> > to devices depending on it, like gpio-keys?  (although gpio-keys isn't
> > a great example because it doesn't really have any runtime PM states).
> > 
> > I think part of the solution is to give drivers the option of
> > returning a 'defer' code at probe time if it cannot obtain all it's
> > resources, and have the driver core re-probe it when more devices
> > become available, but I haven't had time to prototype it yet.
> 
> Sounds interesting. So the probe function could return some sort of -ENOTYET
> or -EAGAIN and have it called again later?

How about we do not register device until all resources are ready? This
is pretty simple concept - do not create an object until it is usable. Then
nobody needs to bother with -EAGAIN or -ENOTYET or any other similar
garbage.

> 
> But, does that mean that we really need to miss this use-case until something
> like this gets approved and merged? Can't we just declare this late_initcall
> for now and fix it later? Please!
> 
> > > >  The real problem is that we have no mechanism for
> > > > holding off or deferring a driver probe if it depends on an
> > > > asynchronous resource.
> > > 
> > > The mechanism we do have - we should not be creating the device for the
> > > driver to bind to unless all resources that are needed by that device
> > > are ready.
> 
> How would we do that in a device-tree?
> 
> > > Just shuffling the initcall order is not maintanable. Next there will be
> > > GPIO expander that is for some reason registered as late_initcall and
> > > we'll be back to square one. I am going to take the threaded IRQ bit but
> > > will drop the initcall bit from the patch.
> 
> That would destroy the whole purpose of this patch.

No, it is still useful as it will allow using the driver with GPIOs
accessed over a slow bus.

> Do you mean to say, what I
> want to do has no acceptable implementation? That would be a pity, since IMHO
> it is a very cool feature, and quite trivial to implement this way.
> Our boards do not need any board setup code. Actually just adding one
> line of code in arch/powerpc/platforms/512x/mpc5121_generic.c or
> arch/powerpc/platforms/52xx/mpc5200_simple.c is enough to support any of our
> boards that need this driver... the rest is done in the device-tree. Don't you
> think this is worth that little bit of (temporary) ugliness?

Turning the question around, can you add secondary device tree traversal
for gpio_keys to your board code and keep the ugliness there until
device tree can better express dependencies between resources?

Thanks.

-- 
Dmitry

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[David Jander]

On Mon, 20 Jun 2011 01:45:12 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote:
> > On Sat, 18 Jun 2011 09:16:45 -0600
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> > 
> > > On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote:
> > > > On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
> > > > > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
> > > > > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > > > > >> > Use a threaded interrupt handler in order to permit the handler
> > > > > >> > to use a GPIO driver that causes things like I2C transactions
> > > > > >> > being done inside the handler context.
> > > > > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to
> > > > > >> > make sure all needed GPIO drivers have been loaded if the
> > > > > >> > drivers are built into the kernel.
> > > > > >>
> > > > > >> ...which is a horrid hack, but until device dependencies can be
> > > > > >> described, it isn't one that can be solved easily.
> > > > > >>
> > > > > >
> > > > > > I really do not want to apply this... Currently the order of
> > > > > > initialization does not matter since nothing actually happens until
> > > > > > corresponding device appears on the bus. Does the OF code creates
> > > > > > devices before all resources are ready?
> > > > > 
> > > > > It's not an OF problem.  The problem is that all the platform_devices
> > > > > typically get registered all at once at machine_init time (on arm),
> > > > > and if the gpio expander isn't a platform_device, (like an i2c gpio
> > > > > expander which would end up being a child of a platform_device), then
> > > > > it won't be ready.
> > > > 
> > > > Ah, I see. But that can be handled in board code that should ensure
> > > > that it registers devices in correct order.
> > > 
> > > Unfortunately, handling it in board code doesn't really work either.
> > > It just shuffles the complexity to the board code to implement some
> > > kind of deferred mechanism for registering devices, and it has to take
> > > into account that it may be a long time before the device actually
> > > appears, such as when the driver is configured as a module.
> > 
> > Besides... we don't want anymore board-code, do we? I mean, if a board can
> > use a generic board configuration and specify all it needs in the
> > device-tree, why should something as trivial as connecting a gpio_keys
> > device to a I2C GPIO expander force us to do special board setup all of a
> > sudden? IMHO specifying I2C-gpios to be used for gpio_keys should "just
> > work", even if declared in a device-tree.
> 
> This is a laudable goal, but then device-tree needs to be able to
> express device dependencies better. Until then board-specific code is
> needed to register devices in proper order.

Hmmm, I am not an expert in OF/DT stuff, but I think that while it would
theoretically be possible to add extra properties to the tree, that are
handled by the of_platform code, I am not sure if that is an option, since
that would be pretty much linux-specific, and could never work on another OS.
Grant?

> > > I completely agree that shuffling initcall order isn't maintainable
> > > though.
> > 
> > I also agree, and if there is a better solution to make this work without
> > additional board-support code, please tell me.
> > I just think that this patch makes the already cool gpio_keys driver quite
> > a bit more awesome. IMO, being able to just hook it all up in the
> > device-tree is just fantastic, and we should make it possible.
> > 
> > > A related concern is that changing the device registration order, or
> > > the initcall order, does absolutely nothing to tell runtime PM about
> > > the dependencies between devices.  For instance, how does runtime PM
> > > know when it is safe to PM a gpio controller, when it has no reference
> > > to devices depending on it, like gpio-keys?  (although gpio-keys isn't
> > > a great example because it doesn't really have any runtime PM states).
> > > 
> > > I think part of the solution is to give drivers the option of
> > > returning a 'defer' code at probe time if it cannot obtain all it's
> > > resources, and have the driver core re-probe it when more devices
> > > become available, but I haven't had time to prototype it yet.
> > 
> > Sounds interesting. So the probe function could return some sort of
> > -ENOTYET or -EAGAIN and have it called again later?
> 
> How about we do not register device until all resources are ready? This
> is pretty simple concept - do not create an object until it is usable. Then
> nobody needs to bother with -EAGAIN or -ENOTYET or any other similar
> garbage.

I agree, but DT doesn't permit that (yet).

> > But, does that mean that we really need to miss this use-case until
> > something like this gets approved and merged? Can't we just declare this
> > late_initcall for now and fix it later? Please!
> > 
> > > > >  The real problem is that we have no mechanism for
> > > > > holding off or deferring a driver probe if it depends on an
> > > > > asynchronous resource.
> > > > 
> > > > The mechanism we do have - we should not be creating the device for the
> > > > driver to bind to unless all resources that are needed by that device
> > > > are ready.
> > 
> > How would we do that in a device-tree?
> > 
> > > > Just shuffling the initcall order is not maintanable. Next there will
> > > > be GPIO expander that is for some reason registered as late_initcall
> > > > and we'll be back to square one. I am going to take the threaded IRQ
> > > > bit but will drop the initcall bit from the patch.
> > 
> > That would destroy the whole purpose of this patch.
> 
> No, it is still useful as it will allow using the driver with GPIOs
> accessed over a slow bus.

Ok, that's true. Problem is that such slow busses (usually I2C or SPI) most
probably have this dependency problem also, so it is a general problem that
needs a solution.

> > Do you mean to say, what I
> > want to do has no acceptable implementation? That would be a pity, since
> > IMHO it is a very cool feature, and quite trivial to implement this way.
> > Our boards do not need any board setup code. Actually just adding one
> > line of code in arch/powerpc/platforms/512x/mpc5121_generic.c or
> > arch/powerpc/platforms/52xx/mpc5200_simple.c is enough to support any of
> > our boards that need this driver... the rest is done in the device-tree.
> > Don't you think this is worth that little bit of (temporary) ugliness?
> 
> Turning the question around, can you add secondary device tree traversal
> for gpio_keys to your board code and keep the ugliness there until
> device tree can better express dependencies between resources?

What do you think, Grant? Would it be possible/acceptable to add some special
property to devices that could make them be added in a second round by
of_platform code (until there are _real_ dependencies)?
Or could the of_platform code be smart and just notice that gpio_keys needs
"gpios" (or other resources for that matter) that are depending on another
node in the tree, and make sure it gets probed before adding this one?
I just don't want to give up on that feature now... besides, now that ARM will
hopefully also adopt OF/DT, more and more drivers will need to be adapted, and
this problem will repeat more sooner than later I guess.

Thanks a lot.

Best regards,

-- 
David Jander
Protonic Holland.

RE: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[H Hartley Sweeten]

On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote:
> On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote:
>> On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
>>> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
>>>> Use a threaded interrupt handler in order to permit the handler to use
>>>> a GPIO driver that causes things like I2C transactions being done inside
>>>> the handler context.
>>>> Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
>>>> all needed GPIO drivers have been loaded if the drivers are built into the
>>>> kernel.
>>>
>>> ...which is a horrid hack, but until device dependencies can be
>>> described, it isn't one that can be solved easily.
>>>
>>
>> I really do not want to apply this... Currently the order of
>> initialization does not matter since nothing actually happens until
>> corresponding device appears on the bus. Does the OF code creates
>> devices before all resources are ready?
>
> It's not an OF problem.  The problem is that all the platform_devices
> typically get registered all at once at machine_init time (on arm),
> and if the gpio expander isn't a platform_device, (like an i2c gpio
> expander which would end up being a child of a platform_device), then
> it won't be ready.  The real problem is that we have no mechanism for
> holding off or deferring a driver probe if it depends on an
> asynchronous resource.

To avoid the registration order issue, isn't the proper fix to use a "setup"
method in the gpio expander driver?  The gpio-pca953x.c driver has this and
I use it to hook the gpio_keys driver to those gpios.  The registration order
ends up being:

i2c-gpio as a platform_device in the machine init code
pca9539 as part of the i2c_board_info for the i2c-gpio device
gpio-keys as a platform_device via the .setup callback of pca9539

Regards,
Hartley

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Grant Likely]

On Mon, Jun 20, 2011 at 2:45 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote:
>> On Sat, 18 Jun 2011 09:16:45 -0600
>> Grant Likely <grant.likely@secretlab.ca> wrote:
>>
>> > On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote:
>> > > On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
>> > > > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
>> > > > <dmitry.torokhov@gmail.com> wrote:
>> > > > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
>> > > > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
>> > > > >> > Use a threaded interrupt handler in order to permit the handler to
>> > > > >> > use a GPIO driver that causes things like I2C transactions being
>> > > > >> > done inside the handler context.
>> > > > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to
>> > > > >> > make sure all needed GPIO drivers have been loaded if the drivers
>> > > > >> > are built into the kernel.
>> > > > >>
>> > > > >> ...which is a horrid hack, but until device dependencies can be
>> > > > >> described, it isn't one that can be solved easily.
>> > > > >>
>> > > > >
>> > > > > I really do not want to apply this... Currently the order of
>> > > > > initialization does not matter since nothing actually happens until
>> > > > > corresponding device appears on the bus. Does the OF code creates
>> > > > > devices before all resources are ready?
>> > > >
>> > > > It's not an OF problem.  The problem is that all the platform_devices
>> > > > typically get registered all at once at machine_init time (on arm),
>> > > > and if the gpio expander isn't a platform_device, (like an i2c gpio
>> > > > expander which would end up being a child of a platform_device), then
>> > > > it won't be ready.
>> > >
>> > > Ah, I see. But that can be handled in board code that should ensure that
>> > > it registers devices in correct order.
>> >
>> > Unfortunately, handling it in board code doesn't really work either.
>> > It just shuffles the complexity to the board code to implement some
>> > kind of deferred mechanism for registering devices, and it has to take
>> > into account that it may be a long time before the device actually
>> > appears, such as when the driver is configured as a module.
>>
>> Besides... we don't want anymore board-code, do we? I mean, if a board can use
>> a generic board configuration and specify all it needs in the device-tree, why
>> should something as trivial as connecting a gpio_keys device to a I2C GPIO
>> expander force us to do special board setup all of a sudden?
>> IMHO specifying I2C-gpios to be used for gpio_keys should "just work", even if
>> declared in a device-tree.
>
> This is a laudable goal, but then device-tree needs to be able to
> express device dependencies better. Until then board-specific code is
> needed to register devices in proper order.

Do:
$ git grep _initcall drivers/gpio
and
$ git grep module_init drivers/gpio

I curse and hold my nose every time I have to apply one of those
initcall patches, but I have to anyway since there isn't even a good
way in board-specific code to control the device registration order.
Everything gets registered at machine_init time, and the /order/ that
things get registered has barely any effect.  It all ends up hanging
on the initcall order.  The only way for board code to have a
meaningful impact on initialization order is to wait for a driver to
get probed on a prerequisite device, probably by using a notifier, and
then register the device at that point.

As far as I can tell, no board code does that.  As ugly as fiddling
with initcall levels is, it has been sufficient up to this point for
existing (non-DT) board ports.

g.
--
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 v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Grant Likely]

On Mon, Jun 20, 2011 at 11:03 AM, H Hartley Sweeten
<hartleys@visionengravers.com> wrote:
> On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote:
>> On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote:
>>> On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
>>>> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
>>>>> Use a threaded interrupt handler in order to permit the handler to use
>>>>> a GPIO driver that causes things like I2C transactions being done inside
>>>>> the handler context.
>>>>> Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
>>>>> all needed GPIO drivers have been loaded if the drivers are built into the
>>>>> kernel.
>>>>
>>>> ...which is a horrid hack, but until device dependencies can be
>>>> described, it isn't one that can be solved easily.
>>>>
>>>
>>> I really do not want to apply this... Currently the order of
>>> initialization does not matter since nothing actually happens until
>>> corresponding device appears on the bus. Does the OF code creates
>>> devices before all resources are ready?
>>
>> It's not an OF problem.  The problem is that all the platform_devices
>> typically get registered all at once at machine_init time (on arm),
>> and if the gpio expander isn't a platform_device, (like an i2c gpio
>> expander which would end up being a child of a platform_device), then
>> it won't be ready.  The real problem is that we have no mechanism for
>> holding off or deferring a driver probe if it depends on an
>> asynchronous resource.
>
> To avoid the registration order issue, isn't the proper fix to use a "setup"
> method in the gpio expander driver?  The gpio-pca953x.c driver has this and
> I use it to hook the gpio_keys driver to those gpios.  The registration order
> ends up being:
>
> i2c-gpio as a platform_device in the machine init code
> pca9539 as part of the i2c_board_info for the i2c-gpio device
> gpio-keys as a platform_device via the .setup callback of pca9539

Blech!  That approach fell out of the ugly tree and hit every branch
on the way down!  Points for creativity though.  :-)  Essentially that
ends up being a driver-specific notifier implementation.

Yes, that works, but to me it just highlights a deficiency in the
Linux device model.

g.
--
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 v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Grant Likely]

On Mon, Jun 20, 2011 at 3:33 AM, David Jander <david.jander@protonic.nl> wrote:
> On Mon, 20 Jun 2011 01:45:12 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
>> On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote:
>> > On Sat, 18 Jun 2011 09:16:45 -0600
>> > Grant Likely <grant.likely@secretlab.ca> wrote:
>> > > Unfortunately, handling it in board code doesn't really work either.
>> > > It just shuffles the complexity to the board code to implement some
>> > > kind of deferred mechanism for registering devices, and it has to take
>> > > into account that it may be a long time before the device actually
>> > > appears, such as when the driver is configured as a module.
>> >
>> > Besides... we don't want anymore board-code, do we? I mean, if a board can
>> > use a generic board configuration and specify all it needs in the
>> > device-tree, why should something as trivial as connecting a gpio_keys
>> > device to a I2C GPIO expander force us to do special board setup all of a
>> > sudden? IMHO specifying I2C-gpios to be used for gpio_keys should "just
>> > work", even if declared in a device-tree.

No, of course we don't.  It's a big problem.  The "just work" test
oversimplifies what is going on here.  Yes, you need gpio-keys
working, and yes changing the initcall solves the problem for you, and
it may very well be expedient to apply the change for the short term,
but no it doesn't solve the underlying problem.  While it makes it
"just work" for you, there is the potential that it will make it "just
not work" for somebody else.

>>
>> This is a laudable goal, but then device-tree needs to be able to
>> express device dependencies better. Until then board-specific code is
>> needed to register devices in proper order.
>
> Hmmm, I am not an expert in OF/DT stuff, but I think that while it would
> theoretically be possible to add extra properties to the tree, that are
> handled by the of_platform code, I am not sure if that is an option, since
> that would be pretty much linux-specific, and could never work on another OS.
> Grant?

We /could/, but I don't think that it is a good idea.  Dependencies
between devices are already expressed by the device tree in the
domain-specific properties.  A gpio property expresses which gpio
controller it depends on.  Similarly an interrupt-parent property
expresses which interrupt controller it depends on.  Similarly with
phy-device for PHYs, and other bindings for i2s links, clock
connections, etc.  What is not defined, is any kind of global
"depends-on" node that states dependencies on other nodes.  I don't
think that having a depends-on property that duplicates already
present information is a good idea; particularly when having
inaccurate data in the property is very likely to go unnoticed because
it may not break anything.

My opinion is that making decisions about dependencies, and telling
the core kernel about those dependencies, really should be in the
domain of the driver.  The driver has all the information about what a
device needs to operate correctly, and it is the only place that will
be able to describe constraints on other devices to the runtime PM
infrastructure.  Plus, most dependencies aren't necessarily on
devices, but rather on the interfaces that a device driver advertises.
 For instance, a single device may present multiple interfaces (say,
gpio controller with irq function), but depending on the
configuration, the driver may not register one or the other.  It's not
the actual device that a driver like gpio-keys depends on, but rather
whether or not the driver registers the gpio interface when it
initialized the device.

Again, this is a core infrastructure deficiency.  The problem is just
as much present when not using the DT, but it does present
differently.

>> How about we do not register device until all resources are ready? This
>> is pretty simple concept - do not create an object until it is usable. Then
>> nobody needs to bother with -EAGAIN or -ENOTYET or any other similar
>> garbage.
>
> I agree, but DT doesn't permit that (yet).

I don't.  The systems we're working with are sufficiently complex now
that I don't think that solution works in the long term.  Take a look
at the work the ALSA folks needed to do to get the audio complex wired
up.  Three or more devices get registered, a DAI driver, a codec
driver, and a sound device driver, all of which can get registered in
any order.  The sound driver waits for the other devices to show up,
and then does the work to attach them all together.  I think this is a
very credible approach, and I intend to dig into generalizing it.

-EAGAIN might not be the right mechanism, but I do think it is
entirely appropriate for drivers to be able to defer initialization
when waiting for asynchronous events.

> What do you think, Grant? Would it be possible/acceptable to add some special
> property to devices that could make them be added in a second round by
> of_platform code (until there are _real_ dependencies)?

As discussed above, I don't think this is a good idea.

> Or could the of_platform code be smart and just notice that gpio_keys needs
> "gpios" (or other resources for that matter) that are depending on another
> node in the tree, and make sure it gets probed before adding this one?

I'm not going to say no; but I'd like to see a prototype what it would
look like before I say yes.  If it can be done relatively cleanly,
then I'll be okay with it.  I suspect that it will end up being crazy
complex to use global code for tracking dependencies in this manor.

g.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[David Jander]

On Mon, 20 Jun 2011 12:20:30 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> On Mon, Jun 20, 2011 at 11:03 AM, H Hartley Sweeten
> <hartleys@visionengravers.com> wrote:
> > On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote:
> >> On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote:
> >>> On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
> >>>> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> >>>>> Use a threaded interrupt handler in order to permit the handler to use
> >>>>> a GPIO driver that causes things like I2C transactions being done
> >>>>> inside the handler context.
> >>>>> Also, gpio_keys_init needs to be declared as a late_initcall, to make
> >>>>> sure all needed GPIO drivers have been loaded if the drivers are built
> >>>>> into the kernel.
> >>>>
> >>>> ...which is a horrid hack, but until device dependencies can be
> >>>> described, it isn't one that can be solved easily.
> >>>>
> >>>
> >>> I really do not want to apply this... Currently the order of
> >>> initialization does not matter since nothing actually happens until
> >>> corresponding device appears on the bus. Does the OF code creates
> >>> devices before all resources are ready?
> >>
> >> It's not an OF problem.  The problem is that all the platform_devices
> >> typically get registered all at once at machine_init time (on arm),
> >> and if the gpio expander isn't a platform_device, (like an i2c gpio
> >> expander which would end up being a child of a platform_device), then
> >> it won't be ready.  The real problem is that we have no mechanism for
> >> holding off or deferring a driver probe if it depends on an
> >> asynchronous resource.
> >
> > To avoid the registration order issue, isn't the proper fix to use a
> > "setup" method in the gpio expander driver?  The gpio-pca953x.c driver has
> > this and I use it to hook the gpio_keys driver to those gpios.  The
> > registration order ends up being:
> >
> > i2c-gpio as a platform_device in the machine init code
> > pca9539 as part of the i2c_board_info for the i2c-gpio device
> > gpio-keys as a platform_device via the .setup callback of pca9539
> 
> Blech!  That approach fell out of the ugly tree and hit every branch
> on the way down!  Points for creativity though.  :-)  Essentially that
> ends up being a driver-specific notifier implementation.
> 
> Yes, that works, but to me it just highlights a deficiency in the
> Linux device model.

Especially, how would I specify this setup handler in a device-tree? The DT
compiler still has no support for something like embedded DT-script ;-)
Still, an interesting (albeit smelly) idea I hadn't thought of before.

Best regards,

-- 
David Jander
Protonic Holland.
--
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 v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Grant Likely]

On Tue, Jun 21, 2011 at 12:55 AM, David Jander <david.jander@protonic.nl> wrote:
> On Mon, 20 Jun 2011 12:20:30 -0600
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
>> On Mon, Jun 20, 2011 at 11:03 AM, H Hartley Sweeten
>> <hartleys@visionengravers.com> wrote:
>> > On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote:
>> >> On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote:
>> >>> On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
>> >>>> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
>> >>>>> Use a threaded interrupt handler in order to permit the handler to use
>> >>>>> a GPIO driver that causes things like I2C transactions being done
>> >>>>> inside the handler context.
>> >>>>> Also, gpio_keys_init needs to be declared as a late_initcall, to make
>> >>>>> sure all needed GPIO drivers have been loaded if the drivers are built
>> >>>>> into the kernel.
>> >>>>
>> >>>> ...which is a horrid hack, but until device dependencies can be
>> >>>> described, it isn't one that can be solved easily.
>> >>>>
>> >>>
>> >>> I really do not want to apply this... Currently the order of
>> >>> initialization does not matter since nothing actually happens until
>> >>> corresponding device appears on the bus. Does the OF code creates
>> >>> devices before all resources are ready?
>> >>
>> >> It's not an OF problem.  The problem is that all the platform_devices
>> >> typically get registered all at once at machine_init time (on arm),
>> >> and if the gpio expander isn't a platform_device, (like an i2c gpio
>> >> expander which would end up being a child of a platform_device), then
>> >> it won't be ready.  The real problem is that we have no mechanism for
>> >> holding off or deferring a driver probe if it depends on an
>> >> asynchronous resource.
>> >
>> > To avoid the registration order issue, isn't the proper fix to use a
>> > "setup" method in the gpio expander driver?  The gpio-pca953x.c driver has
>> > this and I use it to hook the gpio_keys driver to those gpios.  The
>> > registration order ends up being:
>> >
>> > i2c-gpio as a platform_device in the machine init code
>> > pca9539 as part of the i2c_board_info for the i2c-gpio device
>> > gpio-keys as a platform_device via the .setup callback of pca9539
>>
>> Blech!  That approach fell out of the ugly tree and hit every branch
>> on the way down!  Points for creativity though.  :-)  Essentially that
>> ends up being a driver-specific notifier implementation.
>>
>> Yes, that works, but to me it just highlights a deficiency in the
>> Linux device model.
>
> Especially, how would I specify this setup handler in a device-tree?

You wouldn't.  Notifiers (and the above poor-man's notifier) only
works for specific cases in traditional board support code.  There
isn't a good pattern for using it with DT data.  You'd have to make
of_platform_populate() parse every node for any properties containing
a phandle it recognizes (assuming you can know whether the resource is
actually needed for the device to operate) and keep track of which
devices should not be registered because other devices haven't been
bound to drivers yet.  Then you need to have a notifier that looks for
those resources showing up, and registers the deferred device.

> The DT
> compiler still has no support for something like embedded DT-script ;-)

We're not going there.

g.
--
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 v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Mark Brown]

On Mon, Jun 20, 2011 at 01:45:12AM -0700, Dmitry Torokhov wrote:

> This is a laudable goal, but then device-tree needs to be able to
> express device dependencies better. Until then board-specific code is
> needed to register devices in proper order.

Like Grant says this really isn't terribly sustainable - it's not just
the device registration you need to sort out, it's also the registration
of the drivers so things actually get bound and handing of any delays in
the process of getting things to appear.  It's not trivial to get this
right in the general case and it's not reasonable to expect individual
boards to open code things, we really do need core code to figure things
out in some fashion (ideally data rather than retry driven).

> > Sounds interesting. So the probe function could return some sort of -ENOTYET
> > or -EAGAIN and have it called again later?

> How about we do not register device until all resources are ready? This
> is pretty simple concept - do not create an object until it is usable. Then
> nobody needs to bother with -EAGAIN or -ENOTYET or any other similar
> garbage.

As soon as you let the user build drivers modular this goes out of the
window.  All the faff with initcall ordering that we do at the minute is
essentially trying to implement this mechanism.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[David Jander]

On Tue, 21 Jun 2011 06:34:48 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Jun 21, 2011 3:46 PM, "Mark Brown" <broonie@opensource.wolfsonmicro.com>
> wrote:
> >
> > On Mon, Jun 20, 2011 at 01:45:12AM -0700, Dmitry Torokhov wrote:
> >
> > > This is a laudable goal, but then device-tree needs to be able to
> > > express device dependencies better. Until then board-specific code is
> > > needed to register devices in proper order.
> >
> > Like Grant says this really isn't terribly sustainable - it's not just
> > the device registration you need to sort out, it's also the registration
> > of the drivers so things actually get bound and handing of any delays in
> > the process of getting things to appear.
> 
> If devices are registered only when they are fully usable then driver
> registration does not matter.
> 
> > It's not trivial to get this
> > right in the general case and it's not reasonable to expect individual
> > boards to open code things,
> 
> Board code has the ultimate knowledge about connected devices though.

Ok, let me try this essay:
Board code, if there is any, or device-tree, or any other source of setup
information knows best what needs to be initialized or bound when and in which
order. If there is board code, one could solve this by embedding the logic in
synchronous (initcall-based) or asynchronous (thread) board setup code. It is
done on ARM that way, and IMHO it stinks, and AFAICS even Linus would
probably agree (see
http://thread.gmane.org/gmane.linux.ports.arm.kernel/113483/focus=113895 ).
But we already have one example of non-code based setup information sources,
which is the device-tree. A flexible system should not lock out the
possibility that there are other such sources which favor generic code and
dis-favor specific board setup code (reinventing wheels btw).
So I guess everyone agrees that some core-infrastructure is missing to solve
this problem "correctly". Since requiring board-specific code is not
desirable due to the reasons stated above, what should we do in the meantime?

IMHO, the late_initcall thing is both simple and should increase chances of
success by a reasonable amount, while waiting for the correct solution to be
implemented: An interface in the device/driver core infrastructure to specify
device-dependencies in a generic and flexible way, so it can be sourced from a
device-tree, board setup code (if you must) or any other source for that
matter. At that moment, it is a matter of grepping for late_initcall and
reverting these changes, if needed.

Also, something like a keyboard driver (the part that generates input events,
gpio_keys.c in this case), hardly could be a prerequisite for anything else,
since it is clearly at the end of the driver food-chain. So what could possibly
break by this change?

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Mark Brown]

On Tue, Jun 21, 2011 at 06:34:48AM -0700, Dmitry Torokhov wrote:
> On Jun 21, 2011 3:46 PM, "Mark Brown" <broonie@opensource.wolfsonmicro.com>
> > On Mon, Jun 20, 2011 at 01:45:12AM -0700, Dmitry Torokhov wrote:

> > Like Grant says this really isn't terribly sustainable - it's not just
> > the device registration you need to sort out, it's also the registration
> > of the drivers so things actually get bound and handing of any delays in
> > the process of getting things to appear.

> If devices are registered only when they are fully usable then driver
> registration does not matter.

Right, but this is something that it's not reasonable to implement in
board code - if nothing else implementing it in board code would mean
we'd got lots of repitition of common patterns.

> > It's not trivial to get this
> > right in the general case and it's not reasonable to expect individual
> > boards to open code things,

> Board code has the ultimate knowledge about connected devices though.

Absolutely, board code or data should provide the information about how
things are wired up.  It's the acting on it bit that's the issue.

> > > How about we do not register device until all resources are ready? This
> > > is pretty simple concept - do not create an object until it is usable.
> Then
> > > nobody needs to bother with -EAGAIN or -ENOTYET or any other similar
> > > garbage.

> > As soon as you let the user build drivers modular this goes out of the
> > window.

> Why is that? If device is registered only when it is ready to be bound to
> then it does not matter when the driver is registered and whether it is
> built into the kernel or as a module.

Originally you were talking about registration ordering - solving the
module load issues also requires dynamic delays and rollbacks when
things get unregisterd, something that goes well beyond simple ordering
of the registrations. 

> > All the faff with initcall ordering that we do at the minute is
> > essentially trying to implement this mechanism.

> No, what you are doing is creating devices before they are usable and
> postponing the driver registration in hopes that devices will be ready by
> that time.

Right, which is controlling the ordering of registration so that things
generally work out OK as described above.

Nobody's arguing that we don't want to solve this in a better way, we're
just saying that actually doing that requires improvements in both core
infrastructure and the data we've got available to the infrastructure so
there's no reasonable solutions that we can deploy which are better than
the initcall ordering stuff we're doing at the minute.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Dmitry Torokhov]

On Tue, Jun 21, 2011 at 06:27:45PM +0100, Mark Brown wrote:
> On Tue, Jun 21, 2011 at 06:34:48AM -0700, Dmitry Torokhov wrote:
> > On Jun 21, 2011 3:46 PM, "Mark Brown" <broonie@opensource.wolfsonmicro.com>
> > > On Mon, Jun 20, 2011 at 01:45:12AM -0700, Dmitry Torokhov wrote:
> 
> > > Like Grant says this really isn't terribly sustainable - it's not just
> > > the device registration you need to sort out, it's also the registration
> > > of the drivers so things actually get bound and handing of any delays in
> > > the process of getting things to appear.
> 
> > If devices are registered only when they are fully usable then driver
> > registration does not matter.
> 
> Right, but this is something that it's not reasonable to implement in
> board code - if nothing else implementing it in board code would mean
> we'd got lots of repitition of common patterns.

I agree here. I just disagree that we should be implementing this in
driver core by having special -EAGAIN handling. Having a common
library-like code (probably tied to device-tree) that handles device
dependencies would be great.

> 
> > > It's not trivial to get this
> > > right in the general case and it's not reasonable to expect individual
> > > boards to open code things,
> 
> > Board code has the ultimate knowledge about connected devices though.
> 
> Absolutely, board code or data should provide the information about how
> things are wired up.  It's the acting on it bit that's the issue.
> 
> > > > How about we do not register device until all resources are ready? This
> > > > is pretty simple concept - do not create an object until it is usable.
> > Then
> > > > nobody needs to bother with -EAGAIN or -ENOTYET or any other similar
> > > > garbage.
> 
> > > As soon as you let the user build drivers modular this goes out of the
> > > window.
> 
> > Why is that? If device is registered only when it is ready to be bound to
> > then it does not matter when the driver is registered and whether it is
> > built into the kernel or as a module.
> 
> Originally you were talking about registration ordering - solving the
> module load issues also requires dynamic delays and rollbacks when
> things get unregisterd, something that goes well beyond simple ordering
> of the registrations. 

I always was only saying that devices should be registered when they are
ready. It is my understanding that normally board code tries to register
all devices; drivers may or may not be compiled as modules. Not that we
could not have devices created by modules...

> 
> > > All the faff with initcall ordering that we do at the minute is
> > > essentially trying to implement this mechanism.
> 
> > No, what you are doing is creating devices before they are usable and
> > postponing the driver registration in hopes that devices will be ready by
> > that time.
> 
> Right, which is controlling the ordering of registration so that things
> generally work out OK as described above.
> 
> Nobody's arguing that we don't want to solve this in a better way, we're
> just saying that actually doing that requires improvements in both core
> infrastructure and the data we've got available to the infrastructure so
> there's no reasonable solutions that we can deploy which are better than
> the initcall ordering stuff we're doing at the minute.

Ah, OK, so we basically in agreement here with the exception that I do
not want the band-aid to hit mainline since it takes the heat off people
who need inter-device dependency to actually work.

Can the initcall stuff be kept out of mainline? I'd expect
there exist board-specific trees where such patches could be kept? Or
maybe interested parties could create board-crap tree to store patches
like this one?

Thanks.

-- 
Dmitry

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Mark Brown]

On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote:
> On Tue, Jun 21, 2011 at 06:27:45PM +0100, Mark Brown wrote:

> > Right, but this is something that it's not reasonable to implement in
> > board code - if nothing else implementing it in board code would mean
> > we'd got lots of repitition of common patterns.

> I agree here. I just disagree that we should be implementing this in
> driver core by having special -EAGAIN handling. Having a common
> library-like code (probably tied to device-tree) that handles device
> dependencies would be great.

Ah, that's more OK then.  I'm not entirely sure about the -EAGAIN
proposal but it does seem to have some advantages in terms of
deployment.

> Ah, OK, so we basically in agreement here with the exception that I do
> not want the band-aid to hit mainline since it takes the heat off people
> who need inter-device dependency to actually work.

> Can the initcall stuff be kept out of mainline? I'd expect

The init order stuff is in mainline already, you're far too late to the
party here.

> there exist board-specific trees where such patches could be kept? Or
> maybe interested parties could create board-crap tree to store patches
> like this one?

Keeping things in board trees is exactly the sort of thing we want to
avoid people doing.  That just means people do all sorts of stuff that
wouldn't be acceptable upstream, either out of ignorance or through
knowing that only their systems have to work with what they're doing,
and just don't bother working upstream at all half the time making life
miserable for pretty much everyone.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[David Jander]

On Wed, 22 Jun 2011 00:02:42 +0100
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote:
> > On Tue, Jun 21, 2011 at 06:27:45PM +0100, Mark Brown wrote:
> 
> > > Right, but this is something that it's not reasonable to implement in
> > > board code - if nothing else implementing it in board code would mean
> > > we'd got lots of repitition of common patterns.
> 
> > I agree here. I just disagree that we should be implementing this in
> > driver core by having special -EAGAIN handling. Having a common
> > library-like code (probably tied to device-tree) that handles device
> > dependencies would be great.
> 
> Ah, that's more OK then.  I'm not entirely sure about the -EAGAIN
> proposal but it does seem to have some advantages in terms of
> deployment.
> 
> > Ah, OK, so we basically in agreement here with the exception that I do
> > not want the band-aid to hit mainline since it takes the heat off people
> > who need inter-device dependency to actually work.
> 
> > Can the initcall stuff be kept out of mainline? I'd expect
> 
> The init order stuff is in mainline already, you're far too late to the
> party here.
> 
> > there exist board-specific trees where such patches could be kept? Or
> > maybe interested parties could create board-crap tree to store patches
> > like this one?
> 
> Keeping things in board trees is exactly the sort of thing we want to
> avoid people doing.  That just means people do all sorts of stuff that
> wouldn't be acceptable upstream, either out of ignorance or through
> knowing that only their systems have to work with what they're doing,
> and just don't bother working upstream at all half the time making life
> miserable for pretty much everyone.

Looks like we all agree then?
Dmitry, would you consider the late_initcall() part of the hack now
(temporarily)?

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Dmitry Torokhov]

On Wed, Jun 22, 2011 at 12:02:42AM +0100, Mark Brown wrote:
> On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote:
> > On Tue, Jun 21, 2011 at 06:27:45PM +0100, Mark Brown wrote:
> 
> > > Right, but this is something that it's not reasonable to implement in
> > > board code - if nothing else implementing it in board code would mean
> > > we'd got lots of repitition of common patterns.
> 
> > I agree here. I just disagree that we should be implementing this in
> > driver core by having special -EAGAIN handling. Having a common
> > library-like code (probably tied to device-tree) that handles device
> > dependencies would be great.
> 
> Ah, that's more OK then.  I'm not entirely sure about the -EAGAIN
> proposal but it does seem to have some advantages in terms of
> deployment.
> 
> > Ah, OK, so we basically in agreement here with the exception that I do
> > not want the band-aid to hit mainline since it takes the heat off people
> > who need inter-device dependency to actually work.
> 
> > Can the initcall stuff be kept out of mainline? I'd expect
> 
> The init order stuff is in mainline already, you're far too late to the
> party here.

For some drivers it might be already in mainline, it does not matter
that we should continue adding more.

> 
> > there exist board-specific trees where such patches could be kept? Or
> > maybe interested parties could create board-crap tree to store patches
> > like this one?
> 
> Keeping things in board trees is exactly the sort of thing we want to
> avoid people doing.  That just means people do all sorts of stuff that
> wouldn't be acceptable upstream, either out of ignorance or through
> knowing that only their systems have to work with what they're doing,
> and just don't bother working upstream at all half the time making life
> miserable for pretty much everyone.

So you are saying that we should accept such crap directly into
mainline?

Again, it looks like we agree that shuffling initcalls is not proper
solution for this problem nor it is maintainable, so it is exactly the
kind of patches that should be kept in the board trees and out of
mainline.

-- 
Dmitry

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Mark Brown]

On Wed, Jun 22, 2011 at 12:00:52AM -0700, Dmitry Torokhov wrote:
> On Wed, Jun 22, 2011 at 12:02:42AM +0100, Mark Brown wrote:
> > On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote:

> > > Can the initcall stuff be kept out of mainline? I'd expect

> > The init order stuff is in mainline already, you're far too late to the
> > party here.

> For some drivers it might be already in mainline, it does not matter
> that we should continue adding more.

It's not just a few drivers, there's entire subsystems that are doing
this.

> > Keeping things in board trees is exactly the sort of thing we want to
> > avoid people doing.  That just means people do all sorts of stuff that
> > wouldn't be acceptable upstream, either out of ignorance or through
> > knowing that only their systems have to work with what they're doing,
> > and just don't bother working upstream at all half the time making life
> > miserable for pretty much everyone.

> So you are saying that we should accept such crap directly into
> mainline?

Pretty much, yes.  In code terms it's not really invasive and it doesn't
have any real impact on other systems so it's the sort of thing we can
carry without too much pain.  Pragmatically it's not unreasonable.

> Again, it looks like we agree that shuffling initcalls is not proper
> solution for this problem nor it is maintainable, so it is exactly the
> kind of patches that should be kept in the board trees and out of
> mainline.

On the other hand if we're telling people that they can't run their
system usefully from mainline (in some cases we can't even boot) then
we're sending a bad message about the usefulness of mainline and we're
encouraging a space where non-mainline code is acceptable.

The situation here is similar to what we used to have with interrupt
controllers on slow buses - we spent a while working with open coded
non-genirq implementations confined to particular drivers before genirq
was able to support this sort of hardware because there wasn't a clear
route to getting that done in a reasonable timeframe.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Grant Likely]

On Wed, Jun 22, 2011 at 5:38 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Jun 22, 2011 at 12:00:52AM -0700, Dmitry Torokhov wrote:
>> On Wed, Jun 22, 2011 at 12:02:42AM +0100, Mark Brown wrote:
>> > On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote:
>
>> > > Can the initcall stuff be kept out of mainline? I'd expect
>
>> > The init order stuff is in mainline already, you're far too late to the
>> > party here.
>
>> For some drivers it might be already in mainline, it does not matter
>> that we should continue adding more.
>
> It's not just a few drivers, there's entire subsystems that are doing
> this.
>
>> > Keeping things in board trees is exactly the sort of thing we want to
>> > avoid people doing.  That just means people do all sorts of stuff that
>> > wouldn't be acceptable upstream, either out of ignorance or through
>> > knowing that only their systems have to work with what they're doing,
>> > and just don't bother working upstream at all half the time making life
>> > miserable for pretty much everyone.
>
>> So you are saying that we should accept such crap directly into
>> mainline?
>
> Pretty much, yes.  In code terms it's not really invasive and it doesn't
> have any real impact on other systems so it's the sort of thing we can
> carry without too much pain.  Pragmatically it's not unreasonable.

+1

g.
--
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 v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Dmitry Torokhov]

On Wed, Jun 22, 2011 at 08:58:45AM -0600, Grant Likely wrote:
> On Wed, Jun 22, 2011 at 5:38 AM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> > On Wed, Jun 22, 2011 at 12:00:52AM -0700, Dmitry Torokhov wrote:
> >> On Wed, Jun 22, 2011 at 12:02:42AM +0100, Mark Brown wrote:
> >> > On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote:
> >
> >> > > Can the initcall stuff be kept out of mainline? I'd expect
> >
> >> > The init order stuff is in mainline already, you're far too late to the
> >> > party here.
> >
> >> For some drivers it might be already in mainline, it does not matter
> >> that we should continue adding more.
> >
> > It's not just a few drivers, there's entire subsystems that are doing
> > this.
> >
> >> > Keeping things in board trees is exactly the sort of thing we want to
> >> > avoid people doing.  That just means people do all sorts of stuff that
> >> > wouldn't be acceptable upstream, either out of ignorance or through
> >> > knowing that only their systems have to work with what they're doing,
> >> > and just don't bother working upstream at all half the time making life
> >> > miserable for pretty much everyone.
> >
> >> So you are saying that we should accept such crap directly into
> >> mainline?
> >
> > Pretty much, yes.  In code terms it's not really invasive and it doesn't
> > have any real impact on other systems so it's the sort of thing we can
> > carry without too much pain.  Pragmatically it's not unreasonable.
> 
> +1
> 

OK, you wore me out. I'll apply the initcall change...

-- 
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 v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data

[Dmitry Torokhov]

On Fri, Jun 17, 2011 at 06:54:17AM -0600, Grant Likely wrote:
> On Fri, Jun 17, 2011 at 2:58 AM, David Jander <david.jander@protonic.nl> wrote:
> >
> > Hi Grant,
> >
> > On Thu, 16 Jun 2011 13:25:59 -0600
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> >
> >> On Tue, Jun 14, 2011 at 11:08:10AM +0200, David Jander wrote:
> >> > This patch enables fetching configuration data which is normally provided
> >> > via platform_data from the device-tree instead.
> >> > If the device is configured from device-tree data, the platform_data struct
> >> > is not used, and button data needs to be allocated dynamically.
> >> > Big part of this patch deals with confining pdata usage to the probe
> >> > function, to make this possible.
> >> >
> >> > Signed-off-by: David Jander <david@protonic.nl>
> >> > ---
> >> >  .../devicetree/bindings/gpio/gpio_keys.txt         |   49 +++++++
> >> >  drivers/input/keyboard/gpio_keys.c                 |  147
> >> > ++++++++++++++++++-- 2 files changed, 186 insertions(+), 10 deletions(-)
> >> >  create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> >> > b/Documentation/devicetree/bindings/gpio/gpio_keys.txt new file mode 100644
> >> > index 0000000..60a4d8e
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> >> > @@ -0,0 +1,49 @@
> >> > +Device-Tree bindings for input/gpio_keys.c keyboard driver
> >> > +
> >> > +Required properties:
> >> > +   - compatible = "gpio-keys";
> >> > +
> >> > +Optional properties:
> >> > +   - autorepeat: Boolean, Enable auto repeat feature of Linux input
> >> > +     subsystem.
> >> > +
> >> > +Each button (key) is represented as a sub-node of "gpio-keys":
> >> > +Subnode properties:
> >> > +
> >> > +   - reg: GPIO number the key is bound to if linux GPIO numbering is
> >> > used.
> >>
> >> Wait.  That won't work.  There is no concept of "linux gpio numbering"
> >> in the device tree data.  When using device tree, the gpio numbers
> >> usually get dynamically assigned.
> >
> > Yes I know, but suppose you want to use this driver with a GPIO-driver that
> > does not have devaice-tree support yet? I need a way of binding the button to
> > a GPIO pin that does not have a device-tree definition. How should I do this
> > otherwise?
> > I am using this driver with a pca963x, as you might have suspected already,
> > and I just added OF device-tree support to it, so that will work, but
> > others...?
> 
> The solution is to add OF support to the GPIO driver being used.
> 
> > Before "fixing" pca963x, I used this property and it worked perfectly well, so
> > please explain what is not supposed to work. Please keep in mind that this
> > is meant as more of a backwards-compatibility feature. If you think that being
> > backwards compatible with non-OF GPIO drivers is a bad idea, I'll remove this.
> 
> It is.  Something that we've been very careful about is to avoid
> encoding Linux-specific implementation details into the device tree
> bindings.  The implementation details, such as how gpio controllers
> are enumerated, are subject to change just in the normal progress of
> development.  By focusing the DT bindings on HW description, the DT
> data is insulated to a degree from kernel churn.
> 
> >> > +   - wakeup: Boolean, button can wake-up the system.
> >>
> >> "wakeup" is potentially too generic a property name (potential to
> >> conflict with a generic wakeup binding if one ever exists).  Just to
> >> be defencive, I'd suggest prefixing these custom gpio keys properties
> >> with something like "gpio-key,".
> >
> > Ok, "gpio-key,wakeup" it will be then? But isn't this function something
> > potentially other IO-pins/keys/buttons/interrupts/etc... could have to be able
> > to wake up the system?
> 
> Can you foresee how all bindings would potentially use a 'wakeup'
> property?  It's really hard to define a common binding without first
> having several use cases ready to use it.  It's better to start being
> cautious, and then create a common binding at some point in the
> future.
> 
> 
> >> > +           reg = of_get_property(pp, "linux,input-type", &len);
> >> > +           if (reg)
> >> > +                   buttons[i].type = be32_to_cpup(reg);
> >> > +           else
> >> > +                   buttons[i].type = EV_KEY;
> >> how about:
> >>               buttons[i].type = reg ? be32_to_cpup(reg) : EV_KEY;
> >
> > Ok, if you prefer this notation.... just an "if...else" in another dress ;-)
> 
> Yup, but it's shorter, and I like painting bike sheds.
> 

So is there an updated version of this patch coming?

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 v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data

[David Jander]

On Thu, 23 Jun 2011 01:24:10 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Fri, Jun 17, 2011 at 06:54:17AM -0600, Grant Likely wrote:
> > On Fri, Jun 17, 2011 at 2:58 AM, David Jander <david.jander@protonic.nl>
> > wrote:
> > >
> > > Hi Grant,
> > >
> > > On Thu, 16 Jun 2011 13:25:59 -0600
> > > Grant Likely <grant.likely@secretlab.ca> wrote:
> > >
> > >> On Tue, Jun 14, 2011 at 11:08:10AM +0200, David Jander wrote:
> > >> > This patch enables fetching configuration data which is normally
> > >> > provided via platform_data from the device-tree instead.
> > >> > If the device is configured from device-tree data, the platform_data
> > >> > struct is not used, and button data needs to be allocated dynamically.
> > >> > Big part of this patch deals with confining pdata usage to the probe
> > >> > function, to make this possible.
> > >> >
> > >> > Signed-off-by: David Jander <david@protonic.nl>
> > >> > ---
> > >> >  .../devicetree/bindings/gpio/gpio_keys.txt         |   49 +++++++
> > >> >  drivers/input/keyboard/gpio_keys.c                 |  147
> > >> > ++++++++++++++++++-- 2 files changed, 186 insertions(+), 10
> > >> > deletions(-) create mode 100644
> > >> > Documentation/devicetree/bindings/gpio/gpio_keys.txt
> > >> >
> > >> > diff --git a/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> > >> > b/Documentation/devicetree/bindings/gpio/gpio_keys.txt new file mode
> > >> > 100644 index 0000000..60a4d8e
> > >> > --- /dev/null
> > >> > +++ b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> > >> > @@ -0,0 +1,49 @@
> > >> > +Device-Tree bindings for input/gpio_keys.c keyboard driver
> > >> > +
> > >> > +Required properties:
> > >> > +   - compatible = "gpio-keys";
> > >> > +
> > >> > +Optional properties:
> > >> > +   - autorepeat: Boolean, Enable auto repeat feature of Linux input
> > >> > +     subsystem.
> > >> > +
> > >> > +Each button (key) is represented as a sub-node of "gpio-keys":
> > >> > +Subnode properties:
> > >> > +
> > >> > +   - reg: GPIO number the key is bound to if linux GPIO numbering is
> > >> > used.
> > >>
> > >> Wait.  That won't work.  There is no concept of "linux gpio numbering"
> > >> in the device tree data.  When using device tree, the gpio numbers
> > >> usually get dynamically assigned.
> > >
> > > Yes I know, but suppose you want to use this driver with a GPIO-driver
> > > that does not have devaice-tree support yet? I need a way of binding the
> > > button to a GPIO pin that does not have a device-tree definition. How
> > > should I do this otherwise?
> > > I am using this driver with a pca963x, as you might have suspected
> > > already, and I just added OF device-tree support to it, so that will
> > > work, but others...?
> > 
> > The solution is to add OF support to the GPIO driver being used.
> > 
> > > Before "fixing" pca963x, I used this property and it worked perfectly
> > > well, so please explain what is not supposed to work. Please keep in
> > > mind that this is meant as more of a backwards-compatibility feature. If
> > > you think that being backwards compatible with non-OF GPIO drivers is a
> > > bad idea, I'll remove this.
> > 
> > It is.  Something that we've been very careful about is to avoid
> > encoding Linux-specific implementation details into the device tree
> > bindings.  The implementation details, such as how gpio controllers
> > are enumerated, are subject to change just in the normal progress of
> > development.  By focusing the DT bindings on HW description, the DT
> > data is insulated to a degree from kernel churn.
> > 
> > >> > +   - wakeup: Boolean, button can wake-up the system.
> > >>
> > >> "wakeup" is potentially too generic a property name (potential to
> > >> conflict with a generic wakeup binding if one ever exists).  Just to
> > >> be defencive, I'd suggest prefixing these custom gpio keys properties
> > >> with something like "gpio-key,".
> > >
> > > Ok, "gpio-key,wakeup" it will be then? But isn't this function something
> > > potentially other IO-pins/keys/buttons/interrupts/etc... could have to
> > > be able to wake up the system?
> > 
> > Can you foresee how all bindings would potentially use a 'wakeup'
> > property?  It's really hard to define a common binding without first
> > having several use cases ready to use it.  It's better to start being
> > cautious, and then create a common binding at some point in the
> > future.
> > 
> > 
> > >> > +           reg = of_get_property(pp, "linux,input-type", &len);
> > >> > +           if (reg)
> > >> > +                   buttons[i].type = be32_to_cpup(reg);
> > >> > +           else
> > >> > +                   buttons[i].type = EV_KEY;
> > >> how about:
> > >>               buttons[i].type = reg ? be32_to_cpup(reg) : EV_KEY;
> > >
> > > Ok, if you prefer this notation.... just an "if...else" in another
> > > dress ;-)
> > 
> > Yup, but it's shorter, and I like painting bike sheds.
> > 
> 
> So is there an updated version of this patch coming?

Yes, I'm preparing v5 right now.

Best regards,

-- 
David Jander
Protonic Holland.
--
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 v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Dmitry Torokhov]

Hi David,

On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> Use a threaded interrupt handler in order to permit the handler to use
> a GPIO driver that causes things like I2C transactions being done inside
> the handler context.
> Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> all needed GPIO drivers have been loaded if the drivers are built into the
> kernel.

Don't want to resurrect the whole initcall discussion, but could you
tell me again why the interrup handler needs to be threaded? We do not
access hardware from it, hardware is accessed from workqueue context.
Here is the ISR in its entirety:

static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
        struct gpio_button_data *bdata = dev_id;
        const struct gpio_keys_button *button = bdata->button;

        BUG_ON(irq != gpio_to_irq(button->gpio));

        if (bdata->timer_debounce)
                mod_timer(&bdata->timer,
                        jiffies + msecs_to_jiffies(bdata->timer_debounce));
        else
                schedule_work(&bdata->work);

        return IRQ_HANDLED;
}

It looks to me that non-threaded handler would work as well? Or
gpio_to_irq() can sleep with certain chips?

Thanks.

-- 
Dmitry

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[David Jander]

On Fri, 16 Mar 2012 00:20:04 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Hi David,
> 
> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > Use a threaded interrupt handler in order to permit the handler to use
> > a GPIO driver that causes things like I2C transactions being done inside
> > the handler context.
> > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> > all needed GPIO drivers have been loaded if the drivers are built into the
> > kernel.
> 
> Don't want to resurrect the whole initcall discussion, but could you
> tell me again why the interrup handler needs to be threaded? We do not
> access hardware from it, hardware is accessed from workqueue context.
> Here is the ISR in its entirety:

Sorry, the reason described is apparently not very clear. The real reason seems
to be that I would like this driver to work with I2C GPIO expanders, and its
the GPIO expanders "interrupt controller" which has itself a threaded handler
(due to I2C transfers done in it to ack an IRQ). So this is actually a nested
and threaded interrupt controller (because the IRQ line of the GPIO expander
is connected to a different GPIO acting itself also as interrupt line).
In irq/manage.c, function __setup_irq():

...
	/*
	 * Check whether the interrupt nests into another interrupt
	 * thread.
	 */
	nested = irq_settings_is_nested_thread(desc);
	if (nested) {
		if (!new->thread_fn) {
			ret = -EINVAL;
			goto out_mput;
		}
...

This is were requesting a non-threaded IRQ from this GPIO controller will fail.

I know this is not a trivial setup, but IMHO it is very useful (for
connecting keyboards), and a nice demonstration of the powerful features this
GPIO driver has :-)

> static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> {
>         struct gpio_button_data *bdata = dev_id;
>         const struct gpio_keys_button *button = bdata->button;
> 
>         BUG_ON(irq != gpio_to_irq(button->gpio));
> 
>         if (bdata->timer_debounce)
>                 mod_timer(&bdata->timer,
>                         jiffies + msecs_to_jiffies(bdata->timer_debounce));
>         else
>                 schedule_work(&bdata->work);
> 
>         return IRQ_HANDLED;
> }
> 
> It looks to me that non-threaded handler would work as well? Or
> gpio_to_irq() can sleep with certain chips?

Not in my case. I just checked again. If I change request_threaded_irq() to
request_irq(), I get this:

...
[    6.409810] gpio-keys gpio_keys.0: Unable to claim irq 0; error -22
[    6.416106] gpio-keys: probe of gpio_keys.0 failed with error -22
...

This error -22 (-EINVAL) is returned from __setup_irq() (see above).

BTW: The connections of CPU-GPIO -> IRQ of PCA9539 -> GPIO -> gpio_key is
entirely done in the device tree, which is also sort of cool ;-)

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Dmitry Torokhov]

On Fri, Mar 16, 2012 at 09:17:01AM +0100, David Jander wrote:
> On Fri, 16 Mar 2012 00:20:04 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > Hi David,
> > 
> > On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > > Use a threaded interrupt handler in order to permit the handler to use
> > > a GPIO driver that causes things like I2C transactions being done inside
> > > the handler context.
> > > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> > > all needed GPIO drivers have been loaded if the drivers are built into the
> > > kernel.
> > 
> > Don't want to resurrect the whole initcall discussion, but could you
> > tell me again why the interrup handler needs to be threaded? We do not
> > access hardware from it, hardware is accessed from workqueue context.
> > Here is the ISR in its entirety:
> 
> Sorry, the reason described is apparently not very clear. The real reason seems
> to be that I would like this driver to work with I2C GPIO expanders, and its
> the GPIO expanders "interrupt controller" which has itself a threaded handler
> (due to I2C transfers done in it to ack an IRQ). So this is actually a nested
> and threaded interrupt controller (because the IRQ line of the GPIO expander
> is connected to a different GPIO acting itself also as interrupt line).
> In irq/manage.c, function __setup_irq():
> 
> ...
> 	/*
> 	 * Check whether the interrupt nests into another interrupt
> 	 * thread.
> 	 */
> 	nested = irq_settings_is_nested_thread(desc);
> 	if (nested) {
> 		if (!new->thread_fn) {
> 			ret = -EINVAL;
> 			goto out_mput;
> 		}
> ...
> 
> This is were requesting a non-threaded IRQ from this GPIO controller will fail.
> 
> I know this is not a trivial setup, but IMHO it is very useful (for
> connecting keyboards), and a nice demonstration of the powerful features this
> GPIO driver has :-)

Thanks for the explanation of your setup.

> 
> > static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > {
> >         struct gpio_button_data *bdata = dev_id;
> >         const struct gpio_keys_button *button = bdata->button;
> > 
> >         BUG_ON(irq != gpio_to_irq(button->gpio));
> > 
> >         if (bdata->timer_debounce)
> >                 mod_timer(&bdata->timer,
> >                         jiffies + msecs_to_jiffies(bdata->timer_debounce));
> >         else
> >                 schedule_work(&bdata->work);
> > 
> >         return IRQ_HANDLED;
> > }
> > 
> > It looks to me that non-threaded handler would work as well? Or
> > gpio_to_irq() can sleep with certain chips?
> 
> Not in my case. I just checked again. If I change request_threaded_irq() to
> request_irq(), I get this:
> 
> ...
> [    6.409810] gpio-keys gpio_keys.0: Unable to claim irq 0; error -22
> [    6.416106] gpio-keys: probe of gpio_keys.0 failed with error -22
> ...
> 
> This error -22 (-EINVAL) is returned from __setup_irq() (see above).

But the original code used request_any_context_irq() which should have
taken care of your nested IRQ setup:

int request_any_context_irq(unsigned int irq, irq_handler_t handler,
                            unsigned long flags, const char *name, void *dev_id)
{
        struct irq_desc *desc = irq_to_desc(irq);
        int ret;

        if (!desc)
                return -EINVAL;

        if (irq_settings_is_nested_thread(desc)) {
                ret = request_threaded_irq(irq, NULL, handler,
                                           flags, name, dev_id);
                return !ret ? IRQC_IS_NESTED : ret;
        }

        ret = request_irq(irq, handler, flags, name, dev_id);
        return !ret ? IRQC_IS_HARDIRQ : ret;
}

Thanks.

-- 
Dmitry

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[David Jander]

On Fri, 16 Mar 2012 01:32:01 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Fri, Mar 16, 2012 at 09:17:01AM +0100, David Jander wrote:
> > On Fri, 16 Mar 2012 00:20:04 -0700
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > > Hi David,
> > > 
> > > On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > > > Use a threaded interrupt handler in order to permit the handler to use
> > > > a GPIO driver that causes things like I2C transactions being done inside
> > > > the handler context.
> > > > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> > > > all needed GPIO drivers have been loaded if the drivers are built into the
> > > > kernel.
> > > 
> > > Don't want to resurrect the whole initcall discussion, but could you
> > > tell me again why the interrup handler needs to be threaded? We do not
> > > access hardware from it, hardware is accessed from workqueue context.
> > > Here is the ISR in its entirety:
> > 
> > Sorry, the reason described is apparently not very clear. The real reason seems
> > to be that I would like this driver to work with I2C GPIO expanders, and its
> > the GPIO expanders "interrupt controller" which has itself a threaded handler
> > (due to I2C transfers done in it to ack an IRQ). So this is actually a nested
> > and threaded interrupt controller (because the IRQ line of the GPIO expander
> > is connected to a different GPIO acting itself also as interrupt line).
> > In irq/manage.c, function __setup_irq():
> > 
> > ...
> > 	/*
> > 	 * Check whether the interrupt nests into another interrupt
> > 	 * thread.
> > 	 */
> > 	nested = irq_settings_is_nested_thread(desc);
> > 	if (nested) {
> > 		if (!new->thread_fn) {
> > 			ret = -EINVAL;
> > 			goto out_mput;
> > 		}
> > ...
> > 
> > This is were requesting a non-threaded IRQ from this GPIO controller will fail.
> > 
> > I know this is not a trivial setup, but IMHO it is very useful (for
> > connecting keyboards), and a nice demonstration of the powerful features this
> > GPIO driver has :-)
> 
> Thanks for the explanation of your setup.
> 
> > 
> > > static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > > {
> > >         struct gpio_button_data *bdata = dev_id;
> > >         const struct gpio_keys_button *button = bdata->button;
> > > 
> > >         BUG_ON(irq != gpio_to_irq(button->gpio));
> > > 
> > >         if (bdata->timer_debounce)
> > >                 mod_timer(&bdata->timer,
> > >                         jiffies + msecs_to_jiffies(bdata->timer_debounce));
> > >         else
> > >                 schedule_work(&bdata->work);
> > > 
> > >         return IRQ_HANDLED;
> > > }
> > > 
> > > It looks to me that non-threaded handler would work as well? Or
> > > gpio_to_irq() can sleep with certain chips?
> > 
> > Not in my case. I just checked again. If I change request_threaded_irq() to
> > request_irq(), I get this:
> > 
> > ...
> > [    6.409810] gpio-keys gpio_keys.0: Unable to claim irq 0; error -22
> > [    6.416106] gpio-keys: probe of gpio_keys.0 failed with error -22
> > ...
> > 
> > This error -22 (-EINVAL) is returned from __setup_irq() (see above).
> 
> But the original code used request_any_context_irq() which should have
> taken care of your nested IRQ setup:

Hmm. You are right. Apparently this change was introduced in 2.6.38, and I
must have missed it. Before 2.6.38, this place called request_irq(), which was
broken for my case.

I just checked, and indeed, using request_any_context_irq() seems to work fine
for me.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Ben Dooks]

On Fri, Mar 16, 2012 at 12:20:04AM -0700, Dmitry Torokhov wrote:
> Hi David,
> 
> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > Use a threaded interrupt handler in order to permit the handler to use
> > a GPIO driver that causes things like I2C transactions being done inside
> > the handler context.
> > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> > all needed GPIO drivers have been loaded if the drivers are built into the
> > kernel.
> 
> Don't want to resurrect the whole initcall discussion, but could you
> tell me again why the interrup handler needs to be threaded? We do not
> access hardware from it, hardware is accessed from workqueue context.
> Here is the ISR in its entirety:
> 
> static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> {
>         struct gpio_button_data *bdata = dev_id;
>         const struct gpio_keys_button *button = bdata->button;
> 
>         BUG_ON(irq != gpio_to_irq(button->gpio));

Why on earth do we need this? this looks like something that is not
necessary and in my view a waste of cpu cycles.
 
>         if (bdata->timer_debounce)
>                 mod_timer(&bdata->timer,
>                         jiffies + msecs_to_jiffies(bdata->timer_debounce));
>         else
>                 schedule_work(&bdata->work);
> 
>         return IRQ_HANDLED;
> }
> 
> It looks to me that non-threaded handler would work as well? Or
> gpio_to_irq() can sleep with certain chips?

See above comment, I'd go with just remove it and unthread.

-- 
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Ben Dooks]

On Fri, Mar 16, 2012 at 09:48:03AM +0100, David Jander wrote:
> On Fri, 16 Mar 2012 01:32:01 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Fri, Mar 16, 2012 at 09:17:01AM +0100, David Jander wrote:
> > > On Fri, 16 Mar 2012 00:20:04 -0700
> > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > 
> > > > Hi David,
> > > > 
> > > > On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > > > > Use a threaded interrupt handler in order to permit the handler to use
> > > > > a GPIO driver that causes things like I2C transactions being done inside
> > > > > the handler context.
> > > > > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> > > > > all needed GPIO drivers have been loaded if the drivers are built into the
> > > > > kernel.
> > > > 
> > > > Don't want to resurrect the whole initcall discussion, but could you
> > > > tell me again why the interrup handler needs to be threaded? We do not
> > > > access hardware from it, hardware is accessed from workqueue context.
> > > > Here is the ISR in its entirety:
> > > 
> > > Sorry, the reason described is apparently not very clear. The real reason seems
> > > to be that I would like this driver to work with I2C GPIO expanders, and its
> > > the GPIO expanders "interrupt controller" which has itself a threaded handler
> > > (due to I2C transfers done in it to ack an IRQ). So this is actually a nested
> > > and threaded interrupt controller (because the IRQ line of the GPIO expander
> > > is connected to a different GPIO acting itself also as interrupt line).
> > > In irq/manage.c, function __setup_irq():
> > > 
> > > ...
> > > 	/*
> > > 	 * Check whether the interrupt nests into another interrupt
> > > 	 * thread.
> > > 	 */
> > > 	nested = irq_settings_is_nested_thread(desc);
> > > 	if (nested) {
> > > 		if (!new->thread_fn) {
> > > 			ret = -EINVAL;
> > > 			goto out_mput;
> > > 		}
> > > ...
> > > 
> > > This is were requesting a non-threaded IRQ from this GPIO controller will fail.
> > > 
> > > I know this is not a trivial setup, but IMHO it is very useful (for
> > > connecting keyboards), and a nice demonstration of the powerful features this
> > > GPIO driver has :-)
> > 
> > Thanks for the explanation of your setup.
> > 
> > > 
> > > > static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > > > {
> > > >         struct gpio_button_data *bdata = dev_id;
> > > >         const struct gpio_keys_button *button = bdata->button;
> > > > 
> > > >         BUG_ON(irq != gpio_to_irq(button->gpio));
> > > > 
> > > >         if (bdata->timer_debounce)
> > > >                 mod_timer(&bdata->timer,
> > > >                         jiffies + msecs_to_jiffies(bdata->timer_debounce));
> > > >         else
> > > >                 schedule_work(&bdata->work);
> > > > 
> > > >         return IRQ_HANDLED;
> > > > }
> > > > 
> > > > It looks to me that non-threaded handler would work as well? Or
> > > > gpio_to_irq() can sleep with certain chips?
> > > 
> > > Not in my case. I just checked again. If I change request_threaded_irq() to
> > > request_irq(), I get this:
> > > 
> > > ...
> > > [    6.409810] gpio-keys gpio_keys.0: Unable to claim irq 0; error -22
> > > [    6.416106] gpio-keys: probe of gpio_keys.0 failed with error -22
> > > ...
> > > 
> > > This error -22 (-EINVAL) is returned from __setup_irq() (see above).
> > 
> > But the original code used request_any_context_irq() which should have
> > taken care of your nested IRQ setup:
> 
> Hmm. You are right. Apparently this change was introduced in 2.6.38, and I
> must have missed it. Before 2.6.38, this place called request_irq(), which was
> broken for my case.
> 
> I just checked, and indeed, using request_any_context_irq() seems to work fine
> for me.

I'd say that would be better, seems an un-necesary use of threaded
interrupt work.

-- 
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[David Jander]

On Fri, 16 Mar 2012 10:18:15 +0000
Ben Dooks <ben@trinity.fluff.org> wrote:

> On Fri, Mar 16, 2012 at 12:20:04AM -0700, Dmitry Torokhov wrote:
> > Hi David,
> > 
> > On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > > Use a threaded interrupt handler in order to permit the handler to use
> > > a GPIO driver that causes things like I2C transactions being done inside
> > > the handler context.
> > > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> > > all needed GPIO drivers have been loaded if the drivers are built into the
> > > kernel.
> > 
> > Don't want to resurrect the whole initcall discussion, but could you
> > tell me again why the interrup handler needs to be threaded? We do not
> > access hardware from it, hardware is accessed from workqueue context.
> > Here is the ISR in its entirety:
> > 
> > static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > {
> >         struct gpio_button_data *bdata = dev_id;
> >         const struct gpio_keys_button *button = bdata->button;
> > 
> >         BUG_ON(irq != gpio_to_irq(button->gpio));
> 
> Why on earth do we need this? this looks like something that is not
> necessary and in my view a waste of cpu cycles.

No idea... catch some weird (hardware-/setup-)bug? Not _that_ many CPU cycles
anyway, plus I am not the author of that line.... maybe ask Uwe Kleine-König
(CC'd)?

> >         if (bdata->timer_debounce)
> >                 mod_timer(&bdata->timer,
> >                         jiffies + msecs_to_jiffies(bdata->timer_debounce));
> >         else
> >                 schedule_work(&bdata->work);
> > 
> >         return IRQ_HANDLED;
> > }
> > 
> > It looks to me that non-threaded handler would work as well? Or
> > gpio_to_irq() can sleep with certain chips?
> 
> See above comment, I'd go with just remove it and unthread.

Not unthread, but use request_any_context_irq(), please!

Best regards,

-- 
David Jander
Protonic Holland.
--
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