* [PATCH v4 0/3] Input: gpio_keys.c: Add support for OF and I2C GPIO chips @ 2011-06-14 9:08 David Jander 2011-06-14 9:08 ` [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting David Jander ` (2 more replies) 0 siblings, 3 replies; 44+ messages in thread From: David Jander @ 2011-06-14 9:08 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Grant Likely, linux-input, 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting 2011-06-14 9:08 [PATCH v4 0/3] Input: gpio_keys.c: Add support for OF and I2C GPIO chips David Jander @ 2011-06-14 9:08 ` David Jander 2011-06-16 19:28 ` Grant Likely 2011-06-18 10:19 ` Dmitry Torokhov 2011-06-14 9:08 ` [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data David Jander 2011-06-14 9:08 ` [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips David Jander 2 siblings, 2 replies; 44+ messages in thread From: David Jander @ 2011-06-14 9:08 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Grant Likely, linux-input, 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting 2011-06-14 9:08 ` [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting David Jander @ 2011-06-16 19:28 ` Grant Likely 2011-06-18 10:19 ` Dmitry Torokhov 1 sibling, 0 replies; 44+ messages in thread From: Grant Likely @ 2011-06-16 19:28 UTC (permalink / raw) To: David Jander; +Cc: Dmitry Torokhov, linux-input 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 > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting 2011-06-14 9:08 ` [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting David Jander 2011-06-16 19:28 ` Grant Likely @ 2011-06-18 10:19 ` Dmitry Torokhov 2011-06-20 6:52 ` David Jander 1 sibling, 1 reply; 44+ messages in thread From: Dmitry Torokhov @ 2011-06-18 10:19 UTC (permalink / raw) To: David Jander; +Cc: Grant Likely, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting 2011-06-18 10:19 ` Dmitry Torokhov @ 2011-06-20 6:52 ` David Jander 2011-06-20 8:32 ` Dmitry Torokhov 0 siblings, 1 reply; 44+ messages in thread From: David Jander @ 2011-06-20 6:52 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: David Jander, Grant Likely, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting 2011-06-20 6:52 ` David Jander @ 2011-06-20 8:32 ` Dmitry Torokhov 0 siblings, 0 replies; 44+ messages in thread From: Dmitry Torokhov @ 2011-06-20 8:32 UTC (permalink / raw) To: David Jander; +Cc: David Jander, Grant Likely, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data 2011-06-14 9:08 [PATCH v4 0/3] Input: gpio_keys.c: Add support for OF and I2C GPIO chips David Jander 2011-06-14 9:08 ` [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting David Jander @ 2011-06-14 9:08 ` David Jander 2011-06-16 19:25 ` Grant Likely 2011-06-14 9:08 ` [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips David Jander 2 siblings, 1 reply; 44+ messages in thread From: David Jander @ 2011-06-14 9:08 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Grant Likely, linux-input, 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data 2011-06-14 9:08 ` [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data David Jander @ 2011-06-16 19:25 ` Grant Likely 2011-06-17 8:58 ` David Jander 0 siblings, 1 reply; 44+ messages in thread From: Grant Likely @ 2011-06-16 19:25 UTC (permalink / raw) To: David Jander; +Cc: Dmitry Torokhov, linux-input 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 > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data 2011-06-16 19:25 ` Grant Likely @ 2011-06-17 8:58 ` David Jander 2011-06-17 12:54 ` Grant Likely 0 siblings, 1 reply; 44+ messages in thread From: David Jander @ 2011-06-17 8:58 UTC (permalink / raw) To: Grant Likely; +Cc: David Jander, Dmitry Torokhov, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data 2011-06-17 8:58 ` David Jander @ 2011-06-17 12:54 ` Grant Likely 2011-06-23 8:24 ` Dmitry Torokhov 0 siblings, 1 reply; 44+ messages in thread From: Grant Likely @ 2011-06-17 12:54 UTC (permalink / raw) To: David Jander; +Cc: David Jander, Dmitry Torokhov, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data 2011-06-17 12:54 ` Grant Likely @ 2011-06-23 8:24 ` Dmitry Torokhov 2011-06-23 8:55 ` David Jander 0 siblings, 1 reply; 44+ messages in thread From: Dmitry Torokhov @ 2011-06-23 8:24 UTC (permalink / raw) To: Grant Likely; +Cc: David Jander, David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data 2011-06-23 8:24 ` Dmitry Torokhov @ 2011-06-23 8:55 ` David Jander 0 siblings, 0 replies; 44+ messages in thread From: David Jander @ 2011-06-23 8:55 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Grant Likely, David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-14 9:08 [PATCH v4 0/3] Input: gpio_keys.c: Add support for OF and I2C GPIO chips David Jander 2011-06-14 9:08 ` [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting David Jander 2011-06-14 9:08 ` [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data David Jander @ 2011-06-14 9:08 ` David Jander 2011-06-16 19:27 ` Grant Likely 2012-03-16 7:20 ` Dmitry Torokhov 2 siblings, 2 replies; 44+ messages in thread From: David Jander @ 2011-06-14 9:08 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Grant Likely, linux-input, 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-14 9:08 ` [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips David Jander @ 2011-06-16 19:27 ` Grant Likely 2011-06-18 10:17 ` Dmitry Torokhov 2012-03-16 7:20 ` Dmitry Torokhov 1 sibling, 1 reply; 44+ messages in thread From: Grant Likely @ 2011-06-16 19:27 UTC (permalink / raw) To: David Jander; +Cc: Dmitry Torokhov, linux-input 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 > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-16 19:27 ` Grant Likely @ 2011-06-18 10:17 ` Dmitry Torokhov 2011-06-18 13:18 ` Grant Likely 0 siblings, 1 reply; 44+ messages in thread From: Dmitry Torokhov @ 2011-06-18 10:17 UTC (permalink / raw) To: Grant Likely; +Cc: David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-18 10:17 ` Dmitry Torokhov @ 2011-06-18 13:18 ` Grant Likely 2011-06-18 14:51 ` Dmitry Torokhov 2011-06-20 17:03 ` H Hartley Sweeten 0 siblings, 2 replies; 44+ messages in thread From: Grant Likely @ 2011-06-18 13:18 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: David Jander, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-18 13:18 ` Grant Likely @ 2011-06-18 14:51 ` Dmitry Torokhov 2011-06-18 15:16 ` Grant Likely 2011-06-20 17:03 ` H Hartley Sweeten 1 sibling, 1 reply; 44+ messages in thread From: Dmitry Torokhov @ 2011-06-18 14:51 UTC (permalink / raw) To: Grant Likely; +Cc: David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-18 14:51 ` Dmitry Torokhov @ 2011-06-18 15:16 ` Grant Likely 2011-06-20 7:48 ` David Jander 0 siblings, 1 reply; 44+ messages in thread From: Grant Likely @ 2011-06-18 15:16 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-18 15:16 ` Grant Likely @ 2011-06-20 7:48 ` David Jander 2011-06-20 8:45 ` Dmitry Torokhov 0 siblings, 1 reply; 44+ messages in thread From: David Jander @ 2011-06-20 7:48 UTC (permalink / raw) To: Grant Likely; +Cc: Dmitry Torokhov, David Jander, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-20 7:48 ` David Jander @ 2011-06-20 8:45 ` Dmitry Torokhov 2011-06-20 9:33 ` David Jander ` (2 more replies) 0 siblings, 3 replies; 44+ messages in thread From: Dmitry Torokhov @ 2011-06-20 8:45 UTC (permalink / raw) To: David Jander; +Cc: Grant Likely, David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-20 8:45 ` Dmitry Torokhov @ 2011-06-20 9:33 ` David Jander 2011-06-20 18:49 ` Grant Likely 2011-06-20 18:13 ` Grant Likely 2011-06-21 11:46 ` Mark Brown 2 siblings, 1 reply; 44+ messages in thread From: David Jander @ 2011-06-20 9:33 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Grant Likely, David Jander, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-20 9:33 ` David Jander @ 2011-06-20 18:49 ` Grant Likely 0 siblings, 0 replies; 44+ messages in thread From: Grant Likely @ 2011-06-20 18:49 UTC (permalink / raw) To: David Jander; +Cc: Dmitry Torokhov, David Jander, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-20 8:45 ` Dmitry Torokhov 2011-06-20 9:33 ` David Jander @ 2011-06-20 18:13 ` Grant Likely 2011-06-21 11:46 ` Mark Brown 2 siblings, 0 replies; 44+ messages in thread From: Grant Likely @ 2011-06-20 18:13 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: David Jander, David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-20 8:45 ` Dmitry Torokhov 2011-06-20 9:33 ` David Jander 2011-06-20 18:13 ` Grant Likely @ 2011-06-21 11:46 ` Mark Brown [not found] ` <BANLkTikjUR_9wq_tGfomLZNdurvmEH1Jxw@mail.gmail.com> 2 siblings, 1 reply; 44+ messages in thread From: Mark Brown @ 2011-06-21 11:46 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: David Jander, Grant Likely, David Jander, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
[parent not found: <BANLkTikjUR_9wq_tGfomLZNdurvmEH1Jxw@mail.gmail.com>]
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. [not found] ` <BANLkTikjUR_9wq_tGfomLZNdurvmEH1Jxw@mail.gmail.com> @ 2011-06-21 14:36 ` David Jander 2011-06-21 17:27 ` Mark Brown 1 sibling, 0 replies; 44+ messages in thread From: David Jander @ 2011-06-21 14:36 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Mark Brown, Grant Likely, linux-input, 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. [not found] ` <BANLkTikjUR_9wq_tGfomLZNdurvmEH1Jxw@mail.gmail.com> 2011-06-21 14:36 ` David Jander @ 2011-06-21 17:27 ` Mark Brown 2011-06-21 20:48 ` Dmitry Torokhov 1 sibling, 1 reply; 44+ messages in thread From: Mark Brown @ 2011-06-21 17:27 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Grant Likely, linux-input, David Jander, David Jander 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-21 17:27 ` Mark Brown @ 2011-06-21 20:48 ` Dmitry Torokhov 2011-06-21 23:02 ` Mark Brown 0 siblings, 1 reply; 44+ messages in thread From: Dmitry Torokhov @ 2011-06-21 20:48 UTC (permalink / raw) To: Mark Brown; +Cc: Grant Likely, linux-input, David Jander, David Jander 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-21 20:48 ` Dmitry Torokhov @ 2011-06-21 23:02 ` Mark Brown 2011-06-22 6:11 ` David Jander 2011-06-22 7:00 ` Dmitry Torokhov 0 siblings, 2 replies; 44+ messages in thread From: Mark Brown @ 2011-06-21 23:02 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Grant Likely, linux-input, David Jander, David Jander 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-21 23:02 ` Mark Brown @ 2011-06-22 6:11 ` David Jander 2011-06-22 7:00 ` Dmitry Torokhov 1 sibling, 0 replies; 44+ messages in thread From: David Jander @ 2011-06-22 6:11 UTC (permalink / raw) To: Mark Brown; +Cc: Dmitry Torokhov, Grant Likely, linux-input, 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-21 23:02 ` Mark Brown 2011-06-22 6:11 ` David Jander @ 2011-06-22 7:00 ` Dmitry Torokhov 2011-06-22 11:38 ` Mark Brown 1 sibling, 1 reply; 44+ messages in thread From: Dmitry Torokhov @ 2011-06-22 7:00 UTC (permalink / raw) To: Mark Brown; +Cc: Grant Likely, linux-input, David Jander, David Jander 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-22 7:00 ` Dmitry Torokhov @ 2011-06-22 11:38 ` Mark Brown 2011-06-22 14:58 ` Grant Likely 0 siblings, 1 reply; 44+ messages in thread From: Mark Brown @ 2011-06-22 11:38 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Grant Likely, linux-input, David Jander, David Jander 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-22 11:38 ` Mark Brown @ 2011-06-22 14:58 ` Grant Likely 2011-06-22 21:43 ` Dmitry Torokhov 0 siblings, 1 reply; 44+ messages in thread From: Grant Likely @ 2011-06-22 14:58 UTC (permalink / raw) To: Mark Brown; +Cc: Dmitry Torokhov, linux-input, David Jander, David Jander 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-22 14:58 ` Grant Likely @ 2011-06-22 21:43 ` Dmitry Torokhov 0 siblings, 0 replies; 44+ messages in thread From: Dmitry Torokhov @ 2011-06-22 21:43 UTC (permalink / raw) To: Grant Likely; +Cc: Mark Brown, linux-input, David Jander, David Jander 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-18 13:18 ` Grant Likely 2011-06-18 14:51 ` Dmitry Torokhov @ 2011-06-20 17:03 ` H Hartley Sweeten 2011-06-20 18:20 ` Grant Likely 1 sibling, 1 reply; 44+ messages in thread From: H Hartley Sweeten @ 2011-06-20 17:03 UTC (permalink / raw) To: Grant Likely, Dmitry Torokhov; +Cc: David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-20 17:03 ` H Hartley Sweeten @ 2011-06-20 18:20 ` Grant Likely 2011-06-21 6:55 ` David Jander 0 siblings, 1 reply; 44+ messages in thread From: Grant Likely @ 2011-06-20 18:20 UTC (permalink / raw) To: H Hartley Sweeten; +Cc: Dmitry Torokhov, David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-20 18:20 ` Grant Likely @ 2011-06-21 6:55 ` David Jander 2011-06-21 7:04 ` Grant Likely 0 siblings, 1 reply; 44+ messages in thread From: David Jander @ 2011-06-21 6:55 UTC (permalink / raw) To: Grant Likely Cc: H Hartley Sweeten, Dmitry Torokhov, David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-21 6:55 ` David Jander @ 2011-06-21 7:04 ` Grant Likely 0 siblings, 0 replies; 44+ messages in thread From: Grant Likely @ 2011-06-21 7:04 UTC (permalink / raw) To: David Jander Cc: H Hartley Sweeten, Dmitry Torokhov, David Jander, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2011-06-14 9:08 ` [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips David Jander 2011-06-16 19:27 ` Grant Likely @ 2012-03-16 7:20 ` Dmitry Torokhov 2012-03-16 8:17 ` David Jander 2012-03-16 10:18 ` Ben Dooks 1 sibling, 2 replies; 44+ messages in thread From: Dmitry Torokhov @ 2012-03-16 7:20 UTC (permalink / raw) To: David Jander; +Cc: Grant Likely, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2012-03-16 7:20 ` Dmitry Torokhov @ 2012-03-16 8:17 ` David Jander 2012-03-16 8:32 ` Dmitry Torokhov 2012-03-16 10:18 ` Ben Dooks 1 sibling, 1 reply; 44+ messages in thread From: David Jander @ 2012-03-16 8:17 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: David Jander, Grant Likely, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2012-03-16 8:17 ` David Jander @ 2012-03-16 8:32 ` Dmitry Torokhov 2012-03-16 8:48 ` David Jander 0 siblings, 1 reply; 44+ messages in thread From: Dmitry Torokhov @ 2012-03-16 8:32 UTC (permalink / raw) To: David Jander; +Cc: David Jander, Grant Likely, linux-input 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2012-03-16 8:32 ` Dmitry Torokhov @ 2012-03-16 8:48 ` David Jander 2012-03-16 10:19 ` Ben Dooks 0 siblings, 1 reply; 44+ messages in thread From: David Jander @ 2012-03-16 8:48 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: David Jander, Grant Likely, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2012-03-16 8:48 ` David Jander @ 2012-03-16 10:19 ` Ben Dooks 0 siblings, 0 replies; 44+ messages in thread From: Ben Dooks @ 2012-03-16 10:19 UTC (permalink / raw) To: David Jander; +Cc: Dmitry Torokhov, David Jander, Grant Likely, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2012-03-16 7:20 ` Dmitry Torokhov 2012-03-16 8:17 ` David Jander @ 2012-03-16 10:18 ` Ben Dooks 2012-03-16 11:08 ` David Jander 1 sibling, 1 reply; 44+ messages in thread From: Ben Dooks @ 2012-03-16 10:18 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: David Jander, Grant Likely, linux-input 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips. 2012-03-16 10:18 ` Ben Dooks @ 2012-03-16 11:08 ` David Jander 0 siblings, 0 replies; 44+ messages in thread From: David Jander @ 2012-03-16 11:08 UTC (permalink / raw) To: Ben Dooks Cc: Dmitry Torokhov, David Jander, Grant Likely, linux-input, Uwe Kleine-König 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2012-03-16 11:20 UTC | newest] Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-06-14 9:08 [PATCH v4 0/3] Input: gpio_keys.c: Add support for OF and I2C GPIO chips David Jander 2011-06-14 9:08 ` [PATCH v4 1/3] Input: gpio_keys.c: Simplify platform_device -> device casting David Jander 2011-06-16 19:28 ` Grant Likely 2011-06-18 10:19 ` Dmitry Torokhov 2011-06-20 6:52 ` David Jander 2011-06-20 8:32 ` Dmitry Torokhov 2011-06-14 9:08 ` [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data David Jander 2011-06-16 19:25 ` Grant Likely 2011-06-17 8:58 ` David Jander 2011-06-17 12:54 ` Grant Likely 2011-06-23 8:24 ` Dmitry Torokhov 2011-06-23 8:55 ` David Jander 2011-06-14 9:08 ` [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips David Jander 2011-06-16 19:27 ` Grant Likely 2011-06-18 10:17 ` Dmitry Torokhov 2011-06-18 13:18 ` Grant Likely 2011-06-18 14:51 ` Dmitry Torokhov 2011-06-18 15:16 ` Grant Likely 2011-06-20 7:48 ` David Jander 2011-06-20 8:45 ` Dmitry Torokhov 2011-06-20 9:33 ` David Jander 2011-06-20 18:49 ` Grant Likely 2011-06-20 18:13 ` Grant Likely 2011-06-21 11:46 ` Mark Brown [not found] ` <BANLkTikjUR_9wq_tGfomLZNdurvmEH1Jxw@mail.gmail.com> 2011-06-21 14:36 ` David Jander 2011-06-21 17:27 ` Mark Brown 2011-06-21 20:48 ` Dmitry Torokhov 2011-06-21 23:02 ` Mark Brown 2011-06-22 6:11 ` David Jander 2011-06-22 7:00 ` Dmitry Torokhov 2011-06-22 11:38 ` Mark Brown 2011-06-22 14:58 ` Grant Likely 2011-06-22 21:43 ` Dmitry Torokhov 2011-06-20 17:03 ` H Hartley Sweeten 2011-06-20 18:20 ` Grant Likely 2011-06-21 6:55 ` David Jander 2011-06-21 7:04 ` Grant Likely 2012-03-16 7:20 ` Dmitry Torokhov 2012-03-16 8:17 ` David Jander 2012-03-16 8:32 ` Dmitry Torokhov 2012-03-16 8:48 ` David Jander 2012-03-16 10:19 ` Ben Dooks 2012-03-16 10:18 ` Ben Dooks 2012-03-16 11:08 ` David Jander
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.