Hi,
On Thu, Dec 29, 2011 at 2:05 PM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> .../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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
