[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-tegra
Subject: Re: [PATCH v2] Input: keyboard - add device tree bindings for simple
From: Olof Johansson <olof () lixom ! net>
Date: 2011-12-29 22:26:17
Message-ID: CAOesGMhKjUVJATiD2MQv1OVOWi72Vn6RCtJe=i14JasVV=RLew () mail ! gmail ! com
[Download RAW message or body]
Hi,
On Thu, Dec 29, 2011 at 2:05 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Olof,
>
> On Thu, Dec 29, 2011 at 10:42:26AM -0800, Olof Johansson wrote:
> > This adds a simple device tree binding for simple key matrix data.
> >
> > Changes since v1:
> > * Removed samsung-style binding support
> > * Added linux,fn-keymap and linux,fn-key optional properties
> >
>
> This looks very reasonable to me, a few comments though.
>
> > Signed-off-by: Olof Johansson <olof@lixom.net>
> > ---
> > .../devicetree/bindings/input/matrix-keymap.txt | 25 ++++++
> > drivers/input/keyboard/Makefile | 1 +
> > drivers/input/keyboard/keymap.c | 78 ++++++++++++++++++++
> > include/linux/input/matrix_keypad.h | 34 +++++----
> > 4 files changed, 123 insertions(+), 15 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/input/matrix-keymap.txt
> > create mode 100644 drivers/input/keyboard/keymap.c
> >
> > diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt \
> > b/Documentation/devicetree/bindings/input/matrix-keymap.txt new file mode 100644
> > index 0000000..480ca46
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> > @@ -0,0 +1,25 @@
> > +For matrix keyboards there are two kinds of layout bindings using
> > +linux key codes.
> > +
> > +Required properties:
> > +- compatible: "matrix-keyboard-controller"
> > +- linux,keymap: an array of 3-cell entries containing the equivalent
> > + of the three separate properties above: row, column and linux
> > + key-code.
> > +
> > +Optional properties:
> > +- linux,fn-keymap: A separate array of 3-cell entries similar to
> > + linux,keymap but only to be used when the function key modifier is
> > + active. This is used for keyboards that have a software-based modifier
> > + key for 'Fn' but that need to describe the custom layout that should
> > + be used when said modifier key is active.
> > +
> > +- linux,fn-key: a 2-cell entry containing the < row column > of the
> > + function modifier key used to switch to the above linux,fn-keymap
> > + layout.
> > +
> > +Example:
> > + linux,keymap = < 0 3 2
> > + 0 4 5 >;
> > + linux,fn-key = < 2 1 >;
> > + linux,fn-keymap = < 0 3 1 >;
> > diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> > index df7061f..83e6eed 100644
> > --- a/drivers/input/keyboard/Makefile
> > +++ b/drivers/input/keyboard/Makefile
> > @@ -4,6 +4,7 @@
> >
> > # Each configuration option enables a list of files.
> >
> > +obj-$(CONFIG_INPUT_KEYBOARD) += keymap.o
>
> I do not think we should restrict it keyboard devices only; there could
> be users in input/misc as well, so I'd rather have it as
> drivers/input/matrix-keymap.c
>
> I'd start with just matrix_keyboard_of_build_keymap() there and moved
> matrix_keypad_build_keymap() as a follow-up patch as drivers using it
> should 'select' it.
Hmm. It could be argued that any driver that provides a keyboard
complex enough to need this binding should be under
drivers/input/keyboard (possibly provided MFD-style by the misc
device). But sure, I'll move it and add a slient Kconfig for it. :)
> > obj-$(CONFIG_KEYBOARD_ADP5520) += adp5520-keys.o
> > obj-$(CONFIG_KEYBOARD_ADP5588) += adp5588-keys.o
> > obj-$(CONFIG_KEYBOARD_ADP5589) += adp5589-keys.o
> > diff --git a/drivers/input/keyboard/keymap.c b/drivers/input/keyboard/keymap.c
> > new file mode 100644
> > index 0000000..80d1074
> > --- /dev/null
> > +++ b/drivers/input/keyboard/keymap.c
> > @@ -0,0 +1,78 @@
> > +#include <linux/kernel.h>
> > +#include <linux/types.h>
> > +#include <linux/input.h>
> > +#include <linux/of.h>
> > +#include <linux/input/matrix_keypad.h>
> > +#include <linux/export.h>
> > +#include <linux/gfp.h>
> > +#include <linux/slab.h>
> > +
> > +void matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
> > + unsigned int row_shift,
> > + unsigned short *keymap, unsigned long *keybit)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < keymap_data->keymap_size; i++) {
> > + unsigned int key = keymap_data->keymap[i];
> > + unsigned int row = KEY_ROW(key);
> > + unsigned int col = KEY_COL(key);
> > + unsigned short code = KEY_VAL(key);
> > +
> > + keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
> > + __set_bit(code, keybit);
> > + }
> > + __clear_bit(KEY_RESERVED, keybit);
> > +}
> > +EXPORT_SYMBOL_GPL(matrix_keypad_build_keymap);
> > +
> > +#ifdef CONFIG_OF
> > +struct matrix_keymap_data *
>
> Instead of allocating keymap data why don't we populate keymap and
> keybits directly, like matrix_keypad_build_keymap() does?
The reason I did it this way is the way the drivers are written for
device tree: They use a helper that fills in the platform data, and
then use the same probe flow as for the platform device. If I was to
fill in the keymap/keybits here, then we would have separate code
flows for the two ways to enter the driver probe.
> > +matrix_keyboard_of_fill_keymap(struct device_node *np)
> > +{
> > + struct matrix_keymap_data *kd;
> > + struct device_node *knp;
> > + int proplen = 0, i;
> > + u32 *keymap, row, col, key_code;
>
> Let's fail safely if np is NULL.
Sure.
> > + const __be32 *prop = of_get_property(np, "linux,keymap", &proplen);
> > +
> > + if (!of_device_is_compatible(np, "matrix-keyboard-controller"))
> > + return NULL;
> > +
> > + kd = kmalloc(sizeof(*kd), GFP_KERNEL);
> > + if (!kd)
> > + return NULL;
> > + kd->keymap_size = proplen / 3;
>
> Validate that proplen % 3 == 0?
See discussion on other subthread: I think we want to switch to a
compacter format anyway and just use one cell per key. But if not,
yeah this check is useful for that case.
> > +
> > + keymap = kzalloc(kd->keymap_size * sizeof(u32), GFP_KERNEL);
> > + if (!keymap) {
> > + kfree(kd);
> > + return NULL;
> > + }
> > +
> > + kd->keymap = keymap;
> > +
> > + for (i = 0; i < kd->keymap_size; i++){
> > + /* property format is < row column keycode > */
> > + row = be32_to_cpup(prop++);
> > + col = be32_to_cpup(prop++);
> > + key_code = be32_to_cpup(prop++);
> > + *keymap++ = KEY(row, col, key_code);
> > + }
> > +
> > + /* FIXME: Need to add handling of the linux,fn-keymap property here */
> > +
> > + return kd;
> > +}
> > +EXPORT_SYMBOL_GPL(matrix_keyboard_of_fill_keymap);
> > +
> > +void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
> > +{
> > + if (!kd)
> > + return;
> > + if (kd->keymap)
> > + kfree(kd->keymap);
>
> If we decide to keep allocating kd then checking kd->keymap before
> freeing is not necessary.
Good point, fixing.
-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic