Hi Grant,
On Thu, 16 Jun 2011 13:25:59 -0600
Grant Likely <grant.likely@xxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> > .../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@xxxxxxxxxxx>
> > + *
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
