Hi Grant,
Thanks for reacting.
On Thu, 2 Jun 2011 07:30:21 -0600
Grant Likely <grant.likely@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
