INPUT: gpio_keys.c: Added OF support and enabled use with I2C GPIO expanders
A message that is not in the archive (sent off-list or not yet received): -3144470533789149426@unknownmsgid
Quoted 3 lines · On Wed, Jun 1, 2011 at 3:07 AM, <[email protected]> wrote:
From: David Jander <[email protected]>
Signed-off-by David Jander <[email protected]>
There are a lot of unrelated changes being made in this patch. Can you split it up please into logical changes. For example, adding device tree support is one discrete change, reworking the handling of struct device pointers is another, changing the initcall is another, etc.
Quoted 19 lines
---
drivers/input/keyboard/gpio_keys.c | 192 ++++++++++++++++++++++++++++++------1 files changed, 161 insertions(+), 31 deletions(-)drivers/input/keyboard/gpio_keys.c +3
-0@@ -3,6 +3,9 @@ * * Copyright 2005 Phil Blundell * + * Added OF support and enabled use with I2C GPIO expanders: + * Copyright 2010 David Jander <[email protected]> + * * 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.
drivers/input/keyboard/gpio_keys.c line 34
Quoted 7 lines
drivers/input/keyboard/gpio_keys.c +4
-0@@ -25,6 +28,10 @@ #include <linux/gpio_keys.h> #include <linux/workqueue.h> #include <linux/gpio.h> +#ifdef CONFIG_OF +#include <linux/of.h> +#include <linux/of_platform.h> +#endif
The #ifdef CONFIG_OF is not required around these headers
drivers/input/keyboard/gpio_keys.c struct gpio_keys_drvdata { · line 52
Quoted 8 lines
drivers/input/keyboard/gpio_keys.c +1
-0struct gpio_button_data { struct gpio_keys_button *button;@@ -42,6 +49,7 @@ struct gpio_keys_drvdata { int (*enable)(struct device *dev); void (*disable)(struct device *dev); struct gpio_button_data data[0]; + struct gpio_keys_platform_data *dyn_pdata;
If you make this a copy of the gpio_keys_platform_data instead of a pointer, then you can avoid kzallocing a pdata structure, and avoid the messy question about when to free it.
Quoted 38 lines
drivers/input/keyboard/gpio_keys.c +3
-6}; /*@@ -251,8 +259,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 +285,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); \@@ -359,12 +365,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) { char *desc = button->desc ? button->desc : "gpio_keys"; - struct device *dev = &pdev->dev; unsigned long irqflags; int irq, error;
drivers/input/keyboard/gpio_keys.c static int __devinit gpio_keys_setup_key(struct platform_device *pdev, · line 419
Quoted 7 lines
drivers/input/keyboard/gpio_keys.c +2
-2@@ -410,8 +415,8 @@ static int __devinit gpio_keys_setup_key(struct platform_device *pdev, if (!button->can_disable) irqflags |= IRQF_SHARED; - error = request_any_context_irq(irq, gpio_keys_isr, irqflags, desc, bdata); - if (error < 0) { + error = request_threaded_irq(irq, NULL, gpio_keys_isr, irqflags, desc, bdata); + if (error) {
You need to explain why you are making this change.
Quoted 6 lines
drivers/input/keyboard/gpio_keys.c +0
-0dev_err(dev, "Unable to claim irq %d; error %d\n", irq, error); goto fail3;@@ -440,18 +445,133 @@ static void gpio_keys_close(struct input_dev *input) ddata->disable(input->dev.parent); }
drivers/input/keyboard/gpio_keys.c static void gpio_keys_close(struct input_dev *input) · line 456
Quoted 10 lines
drivers/input/keyboard/gpio_keys.c +9
-0+/* +* 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_alt_pdata (struct device *dev)
Function name should reflect what it does. In this case, something like gpio_keys_get_devtree_pdata() would be more appropriate.
Quoted 9 lines
drivers/input/keyboard/gpio_keys.c +9
-0+{ + struct gpio_keys_platform_data *pdata; + struct device_node *node, *pp; + int i; + struct gpio_keys_button *buttons; + const u32 *reg; + int len; + + node = dev->of_node;
drivers/input/keyboard/gpio_keys.c static void gpio_keys_close(struct input_dev *input) · line 475
Quoted 10 lines
drivers/input/keyboard/gpio_keys.c +10
-0+ if (node == NULL) + return NULL; + + pdata = kzalloc(sizeof(struct gpio_keys_platform_data), GFP_KERNEL); + if (pdata == NULL) { + dev_err(dev, "Unable to allocate platform_data\n"); + return NULL; + } + + reg = of_get_property(node, "linux,autorepeat", &len);
You're creating a new binding. You must also document it in Documentation/devicetree/bindings.
Quoted 194 lines
drivers/input/keyboard/gpio_keys.c +85
-1+ if (reg) + pdata->rep = reg[0]; + + /* 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; + + reg = of_get_property(pp, "reg", &len); + if (!reg) { + pdata->nbuttons--; + printk("Found button without reg\n"); + continue; + } + buttons[i].gpio = reg[0]; + + reg = of_get_property(pp, "linux,code", &len); + if (!reg) { + dev_err(dev, "Button without keycode: 0x%x\n", buttons[i].gpio); + return NULL; + } + buttons[i].code = reg[0]; + + lbl = of_get_property(pp, "label", &len); + buttons[i].desc = (char *)lbl; + + buttons[i].active_low = !!of_get_property(pp, "linux,active_low", NULL); + + reg = of_get_property(pp, "linux,input_type", &len); + if (reg) + buttons[i].type = reg[0]; + else + buttons[i].type = EV_KEY; + + buttons[i].wakeup = !!of_get_property(pp, "linux,wakeup", NULL); + + reg = of_get_property(pp, "linux,debounc_interval", &len); + if (reg) + buttons[i].debounce_interval = reg[0]; + else + buttons[i].debounce_interval = 5; + + buttons[i].can_disable = (bool)!!of_get_property(pp, "linux,can_disable", NULL); + i++; + } + + pdata->buttons = buttons; + + return pdata; +} +#else +static struct gpio_keys_platform_data * +gpio_keys_get_alt_pdata (struct device *dev) +{ + return NULL; +} +#endif + 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 gpio_keys_platform_data *dyn_pdata = NULL; struct input_dev *input; int i, error; int wakeup = 0; + if (!pdata) { + pdata = gpio_keys_get_alt_pdata(dev); + if (!pdata) + return -ENODEV; + /* + * Unlike normal platform_data, this is allocated + * dynamically and must be freed in the driver + */ + dev->platform_data = pdata; + dyn_pdata = pdata; + } +ddata =
kzalloc(sizeof(struct gpio_keys_drvdata) + pdata->nbuttons * sizeof(struct gpio_button_data), GFP_KERNEL);drivers/input/keyboard/gpio_keys.c +3
-0+ + ddata->dyn_pdata = dyn_pdata; +input =
input_allocate_device();
if (!ddata || !input) {
dev_err(dev, "failed to allocate state\n");drivers/input/keyboard/gpio_keys.c +6
-7@@ -465,12 +585,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 = pdev->name; + input->name = "GPIO_keyboard"; input->phys = "gpio-keys/input0"; - input->dev.parent = &pdev->dev; + input->dev.parent = dev; input->open = gpio_keys_open; input->close = gpio_keys_close;@@ -491,7 +611,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;@@ -501,13 +621,12 @@ 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); goto fail2; } - error = input_register_device(input); if (error) { dev_err(dev, "Unable to register input device, error: %d\n",@@ -520,12 +639,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]);drivers/input/keyboard/gpio_keys.c +1
-1@@ -535,7 +654,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);drivers/input/keyboard/gpio_keys.c +8
-4@@ -545,14 +664,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);@@ -562,21 +682,29 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev) cancel_work_sync(&ddata->data[i].work); gpio_free(pdata->buttons[i].gpio); } + if (ddata->dyn_pdata) { + kfree(pdata->buttons); + kfree(pdata);
drivers/input/keyboard/gpio_keys.c static int __devexit gpio_keys_remove(struct platform_device *pdev) · line 697
Quoted 10 lines
drivers/input/keyboard/gpio_keys.c +4
-0+ } input_unregister_device(input); return 0; } +static struct of_device_id gpio_keys_of_match[] = { + { .compatible = "linux,gpio-keys", .data = NULL, }, + { .compatible = "gpio-keys", .data = NULL, },
Drop the ".data = NULL" bit.
Quoted 69 lines
drivers/input/keyboard/gpio_keys.c +12
-10+ {}, +}; #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) {@@ -591,15 +719,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); }@@ -617,6 +744,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),@@ -626,6 +755,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, } };@@ -639,10 +769,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 <[email protected]>"); -MODULE_DESCRIPTION ("Keyboard driver for CPU GPIOs" ); +MODULE_DESCRIPTION ("Keyboard driver for GPIOs" ); MODULE_ALIAS("platform:gpio-keys"); --1.7.4.1
Signature · 6 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant,
Thanks for reacting.
Quoted 5 lines · On Thu, 2 Jun 2011 07:30:21 -0600 Grant Likely <[email protected]>…
There are a lot of unrelated changes being made in this patch. Can you split it up please into logical changes. For example, adding device tree support is one discrete change, reworking the handling of struct device pointers is another, changing the initcall is another, etc.
Ok, will do.
Quoted 13 lines
[...]
Review struct gpio_keys_drvdata { · line 52
Quoted 8 lines
struct
gpio_button_data{
structgpio_keys_button*button;(quoted diff) +1
-0@@ -42,6 +49,7 @@ struct gpio_keys_drvdata { int (*enable)(struct device *dev); void (*disable)(struct device *dev); struct gpio_button_data data[0]; + struct gpio_keys_platform_data *dyn_pdata;If you make this a copy of the
gpio_keys_platform_datainstead of a pointer, then you can avoid kzallocing a pdata structure, and avoid the messy question about when to free it.
Someone needs to kzalloc it somewhere. Originally it is supposed to be done in platform setup code somewhere in the BSP. Now I am parsing OF Device-Tree data, so I will need to provide this, since pdata is NULL.
I don't know how to deal with it in a simpler way than using an extra member in struct gpio_keys_drvdata to save the allocated pointer and free it if needed....
Quoted 41 lines
Quoted 41 lines
(quoted diff) +3
-6}; /*@@ -251,8 +259,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 +285,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); \@@ -359,12 +365,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) { char *desc = button->desc ? button->desc : "gpio_keys"; - struct device *dev = &pdev->dev; unsigned long irqflags; int irq, error;@@ -410,8 +415,8 @@ static int __devinit gpio_keys_setup_key(struct platform_device *pdev, if (!button->can_disable) irqflags |= IRQF_SHARED;
Quoted 8 lines
Review static int __devinit gpio_keys_setup_key(struct · line 421
Quoted 6 lines
(quoted diff) +2
-2- error = request_any_context_irq(irq, gpio_keys_isr, irqflags, desc, bdata); - if (error < 0) { + error = request_threaded_irq(irq, NULL, gpio_keys_isr, irqflags, desc, bdata); + if (error) {You need to explain why you are making this change.
Will do in next patch version... it amounts to this:request_threaded_irq() permits to execute the IRQ code in a thread, which is better suited if the handler wants to do things like I2C transfers to figure out the cause of the interrupt (like in a PCA953x).
The second line was not intended to be changed... will revert that, sorry.
Quoted 7 lines
Quoted 7 lines
(quoted diff) +0
-0dev_err(dev, "Unable to claim irq %d; error %d\n",irq, error);
goto fail3;(quoted diff) +0
-0@@ -440,18 +445,133 @@ static void gpio_keys_close(struct input_dev *input) ddata->disable(input->dev.parent); }
Quoted 12 lines
Review static void gpio_keys_close(struct input_dev *input) · line 456
Quoted 9 lines
(quoted diff) +9
-0+/* + * 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_alt_pdata(struct device *dev)Function name should reflect what it does. In this case, something like
gpio_keys_get_devtree_pdata()would be more appropriate.
Agreed!
Quoted 15 lines
Quoted 15 lines
(quoted diff) +14
-0+{ + struct gpio_keys_platform_data *pdata; + 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; + + pdata = kzalloc(sizeof(struct gpio_keys_platform_data), GFP_KERNEL); + if (pdata == NULL) {
Quoted 8 lines
Review static void gpio_keys_close(struct input_dev *input) · line 476
Quoted 5 lines
(quoted diff) +5
-0+ dev_err(dev, "Unable to allocate platform_data\n"); + return NULL; + } + + reg = of_get_property(node, "linux,autorepeat", &len);You're creating a new binding. You must also document it in
Documentation/devicetree/bindings.
Ok, will do, thanks for pointing out.
Best regards,
Signature · 6 lines
David Jander Protonic Holland. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html