Re: [PATCH v4 2/3] Input: gpio_keys.c: Added support for device-tree platform data

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

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