| 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
| ||
|
Message-ID: <20130213194307.GA22031@core.coreip.homeip.net>
Date: Wed, 13 Feb 2013 11:43:08 -0800
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Simon Glass <sjg@...omium.org>
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 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.
> 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?
> * @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.
Thanks.
--
Dmitry
--
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