Re: [PATCH v2 5/6] Input: matrix-keymap: Add function to read the new DT binding

[a0131647 <sourav.poddar@ti.com>]

Hi,
On Wednesday 13 February 2013 08:12 AM, 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@chromium.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) {
>   		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)
>    * @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);
> +
>   #endif /* _MATRIX_KEYPAD_H */
Looks good to me.You can add
Reviewed-by: Sourav Poddar <sourav.poddar@ti.com>

Also tested this patch on omap4430 sdp, keypad works fine. You can also 
add a
Tested-by: Sourav Poddar <sourav.poddar@ti.com>

~Sourav