| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 13 Feb 2013 21:40:21 -0800
From: Simon Glass <sjg@...omium.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
Samuel Ortiz <sameo@...ux.intel.com>,
Bill Pemberton <wfp5p@...ginia.edu>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
Javier Martinez Canillas <javier@...hile0.org>,
Sourav Poddar <sourav.poddar@...com>,
Felipe Balbi <balbi@...com>,
Alban Bedel <alban.bedel@...onic-design.de>,
linux-input@...r.kernel.org
Subject: Re: [PATCH v2 5/6] Input: matrix-keymap: Add function to read the new
DT binding
Hi Dmitry,
On Wed, Feb 13, 2013 at 11:43 AM, Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> Hi Simon,
>
> On Tue, Feb 12, 2013 at 06:42:25PM -0800, Simon Glass wrote:
>> We now have a binding which adds two parameters to the matrix keypad DT
>> node. This is separate from the GPIO-driven matrix keypad binding, and
>> unfortunately incompatible, since that uses row-gpios/col-gpios for the
>> row and column counts.
>>
>> So the easiest option here is to provide a function for non-GPIO drivers
>> to use to decode the binding.
>>
>> Note: We could in fact create an entirely separate structure to hold
>> these two fields, but it does not seem worth it, yet. If we have more
>> parameters then we can add this, and then refactor each driver to hold
>> such a structure.
>>
>> Signed-off-by: Simon Glass <sjg@...omium.org>
>> ---
>> Changes in v2:
>> - Add new patch to decode matrix-keypad DT binding
>>
>> drivers/input/keyboard/lpc32xx-keys.c | 11 ++++++-----
>> drivers/input/keyboard/omap4-keypad.c | 16 +++++-----------
>> drivers/input/keyboard/tca8418_keypad.c | 11 +++++++----
>> drivers/input/matrix-keymap.c | 20 ++++++++++++++++++++
>> include/linux/input/matrix_keypad.h | 11 +++++++++++
>> 5 files changed, 49 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/lpc32xx-keys.c b/drivers/input/keyboard/lpc32xx-keys.c
>> index 1b8add6..4218143 100644
>> --- a/drivers/input/keyboard/lpc32xx-keys.c
>> +++ b/drivers/input/keyboard/lpc32xx-keys.c
>> @@ -144,12 +144,13 @@ static int lpc32xx_parse_dt(struct device *dev,
>> {
>> struct device_node *np = dev->of_node;
>> u32 rows = 0, columns = 0;
>> + int err;
>>
>> - of_property_read_u32(np, "keypad,num-rows", &rows);
>> - of_property_read_u32(np, "keypad,num-columns", &columns);
>> - if (!rows || rows != columns) {
>> - dev_err(dev,
>> - "rows and columns must be specified and be equal!\n");
>> + err = matrix_keypad_parse_of_params(dev, &rows, &columns);
>> + if (err)
>> + return err;
>> + if (rows != columns) {
>> + dev_err(dev, "rows and columns must be equal!\n");
>> return -EINVAL;
>> }
>>
>> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
>> index e25b022..1b28909 100644
>> --- a/drivers/input/keyboard/omap4-keypad.c
>> +++ b/drivers/input/keyboard/omap4-keypad.c
>> @@ -215,18 +215,12 @@ static int omap4_keypad_parse_dt(struct device *dev,
>> struct omap4_keypad *keypad_data)
>> {
>> struct device_node *np = dev->of_node;
>> + int err;
>>
>> - if (!np) {
>> - dev_err(dev, "missing DT data");
>> - return -EINVAL;
>> - }
>> -
>> - of_property_read_u32(np, "keypad,num-rows", &keypad_data->rows);
>> - of_property_read_u32(np, "keypad,num-columns", &keypad_data->cols);
>> - if (!keypad_data->rows || !keypad_data->cols) {
>> - dev_err(dev, "number of keypad rows/columns not specified\n");
>> - return -EINVAL;
>> - }
>> + err = matrix_keypad_parse_of_params(dev, &keypad_data->rows,
>> + &keypad_data->cols);
>> + if (err)
>> + return err;
>>
>> if (of_get_property(np, "linux,input-no-autorepeat", NULL))
>> keypad_data->no_autorepeat = true;
>> diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
>> index a34cc67..4d5a6d5 100644
>> --- a/drivers/input/keyboard/tca8418_keypad.c
>> +++ b/drivers/input/keyboard/tca8418_keypad.c
>> @@ -288,17 +288,20 @@ static int tca8418_keypad_probe(struct i2c_client *client,
>> irq_is_gpio = pdata->irq_is_gpio;
>> } else {
>> struct device_node *np = dev->of_node;
>> - of_property_read_u32(np, "keypad,num-rows", &rows);
>> - of_property_read_u32(np, "keypad,num-columns", &cols);
>> + int err;
>> +
>> + err = matrix_keypad_parse_of_params(dev, &rows, &cols);
>> + if (err)
>> + return err;
>> rep = of_property_read_bool(np, "keypad,autorepeat");
>> }
>>
>> - if (!rows || rows > TCA8418_MAX_ROWS) {
>> + if (rows > TCA8418_MAX_ROWS) {
>
> Why? Your new function only checks rows != 0 for the DT case, but we
> also need to validate platform data supplied values.
Thanks for reviewing.
OK - will fix. It seems odd that that driver calls device tree code
even when CONFIG_OF is not defined.
>
>> dev_err(dev, "invalid rows\n");
>> return -EINVAL;
>> }
>>
>> - if (!cols || cols > TCA8418_MAX_COLS) {
>> + if (cols > TCA8418_MAX_COLS) {
>> dev_err(dev, "invalid columns\n");
>> return -EINVAL;
>> }
>> diff --git a/drivers/input/matrix-keymap.c b/drivers/input/matrix-keymap.c
>> index 3ae496e..64bad81 100644
>> --- a/drivers/input/matrix-keymap.c
>> +++ b/drivers/input/matrix-keymap.c
>> @@ -50,6 +50,25 @@ static bool matrix_keypad_map_key(struct input_dev *input_dev,
>> }
>>
>> #ifdef CONFIG_OF
>> +int matrix_keypad_parse_of_params(struct device *dev,
>> + unsigned int *rows, unsigned int *cols)
>> +{
>> + struct device_node *np = dev->of_node;
>> +
>> + if (!np) {
>> + dev_err(dev, "missing DT data");
>> + return -EINVAL;
>> + }
>> + of_property_read_u32(np, "keypad,num-rows", rows);
>> + of_property_read_u32(np, "keypad,num-columns", cols);
>> + if (!*rows || !*cols) {
>> + dev_err(dev, "number of keypad rows/columns not specified\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int matrix_keypad_parse_of_keymap(const char *propname,
>> unsigned int rows, unsigned int cols,
>> struct input_dev *input_dev)
>> @@ -112,6 +131,7 @@ static int matrix_keypad_parse_of_keymap(const char *propname,
>> * tree support is enabled).
>> * @rows: number of rows in target keymap array
>> * @cols: number of cols in target keymap array
>> + * (if either/both is 0 then they will be read from the FDT if available)
>
> Huh?
Not sure what happened there, some sort of stuff-up, but that code
isn't there now.
>
>> * @keymap: expanded version of keymap that is suitable for use by
>> * matrix keyboard driver
>> * @input_dev: input devices for which we are setting up the keymap
>> diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
>> index 5f3aa6b..01a30cf 100644
>> --- a/include/linux/input/matrix_keypad.h
>> +++ b/include/linux/input/matrix_keypad.h
>> @@ -81,4 +81,15 @@ int matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
>> unsigned short *keymap,
>> struct input_dev *input_dev);
>>
>> +/**
>> + * matrix_keypad_parse_of_params() - Read parameters from matrix-keypad node
>> + *
>> + * @dev: Device containing of_node
>> + * @rows: Returns number of matrix rows
>> + * @cols: Returns number of matrix columns
>> + * @return 0 if OK, <0 on error
>> + */
>> +int matrix_keypad_parse_of_params(struct device *dev,
>> + unsigned int *rows, unsigned int *cols);
>> +
>
> The new function needs a non-CONFIG_OF stub as some drivers
> (tca8418-keypad for example) use it in common code.
OK, not sure whether I am permitted to put a static inline in the
header, but will go with that for now.
>
> Thanks.
>
> --
> Dmitry
Regards,
Simon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists