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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 17, 2011 at 2:58 AM, David Jander <david.jander@xxxxxxxxxxx> wrote:
>
> 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...?

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.

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


[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux