Hi Christian,
On Sun, Nov 11, 2018 at 07:58:19PM +0100, Christian Hoff wrote:
> "of_get_named_gpio()" returns a negative error value if it fails
> and drivers should check for this. This missing check was now
> added to the matrix_keypad driver.
>
> In my case "of_get_named_gpio()" returned -EPROBE_DEFER because
> the referenced GPIOs belong to an I/O expander, which was not yet
> probed at the point in time when the matrix_keypad driver was
> loading. Because the driver did not check for errors from the
> "of_get_named_gpio()" routine, it was assuming that "-EPROBE_DEFER"
> is actually a GPIO number and continued as usual, which led to further
> errors like this later on:
>
> WARNING: CPU: 3 PID: 167 at drivers/gpio/gpiolib.c:114
> gpio_to_desc+0xc8/0xd0
> invalid GPIO -517
>
> Note that the "GPIO number" -517 in the error message above is
> actually "-EPROBE_DEFER".
>
> As part of the patch a misleading error message "no platform data defined"
> was also removed.
You should mention, that there is no information loss, since the
other error paths in matrix_keypad_parse_dt() already print an
error.
> Thanks a lot to Sebastian Reichl for his analysis of the problem
> and for pointing me into the right direction on how to fix this issue!
Your are welcome! FWIW my lastname is spelled "Reichel" and you can
shorten this to:
Suggested-by: Sebastian Reichel <sre@xxxxxxxxxx>
> Signed-off-by: Christian Hoff <christian_hoff@xxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
>
> ---
> drivers/input/keyboard/matrix_keypad.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
> index f51ae09596ef..8a6be94b7838 100644
> --- a/drivers/input/keyboard/matrix_keypad.c
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -407,7 +407,7 @@ matrix_keypad_parse_dt(struct device *dev)
> struct matrix_keypad_platform_data *pdata;
> struct device_node *np = dev->of_node;
> unsigned int *gpios;
> - int i, nrow, ncol;
> + int ret, i, nrow, ncol;
>
> if (!np) {
> dev_err(dev, "device lacks DT data\n");
> @@ -444,7 +444,7 @@ matrix_keypad_parse_dt(struct device *dev)
> &pdata->col_scan_delay_us);
>
> gpios = devm_kcalloc(dev,
> - pdata->num_row_gpios + pdata->num_col_gpios,
> + nrow + ncol,
Unrelated change. We have a "Solve only one problem per patch"
policy in the kernel.
> sizeof(unsigned int),
> GFP_KERNEL);
> if (!gpios) {
> @@ -452,12 +452,19 @@ matrix_keypad_parse_dt(struct device *dev)
> return ERR_PTR(-ENOMEM);
> }
>
> - for (i = 0; i < pdata->num_row_gpios; i++)
> - gpios[i] = of_get_named_gpio(np, "row-gpios", i);
> + for (i = 0; i < nrow; i++) {
> + ret = of_get_named_gpio(np, "row-gpios", i);
> + if (ret < 0)
> + return ERR_PTR(ret);
> + gpios[i] = ret;
> + }
>
> - for (i = 0; i < pdata->num_col_gpios; i++)
> - gpios[pdata->num_row_gpios + i] =
> - of_get_named_gpio(np, "col-gpios", i);
> + for (i = 0; i < ncol; i++) {
> + ret = of_get_named_gpio(np, "col-gpios", i);
> + if (ret < 0)
> + return ERR_PTR(ret);
> + gpios[nrow + i] = ret;
> + }
>
> pdata->row_gpios = gpios;
> pdata->col_gpios = &gpios[pdata->num_row_gpios];
> @@ -485,7 +492,6 @@ static int matrix_keypad_probe(struct platform_device *pdev)
> if (!pdata) {
> pdata = matrix_keypad_parse_dt(&pdev->dev);
> if (IS_ERR(pdata)) {
> - dev_err(&pdev->dev, "no platform data defined\n");
> return PTR_ERR(pdata);
> }
The curly brackets can be removed.
> } else if (!pdata->keymap_data) {
> --
> 2.16.1.2.g9d582f143
>
Otherwise looks good to me!
P.S.: It would be nice to have this driver converted to gpiod, but
that can be done on top of this and is not suitable for stable.
-- Sebastian
Attachment:
signature.asc
Description: PGP signature
