Re: [PATCH] INPUT: gpio_keys.c: Added OF support and enabled use with I2C GPIO expanders

[David Jander <david.jander@protonic.nl>]

Hi Grant,

Thanks for reacting.

On Thu, 2 Jun 2011 07:30:21 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:
> 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.

>[...]
> >
> >  struct 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.

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....

> >  };
> >
> >  /*
> > @@ -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;
> >
> > -       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.

> >                dev_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);
> >  }
> >
> > +/*
> > + * 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!

> > +{
> > +       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) {
> > +               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,

-- 
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