Hi Sebastian,
On Sun, Nov 11, 2018 at 08:47:53PM +0100, Sebastian Reichel wrote:
> Hi Christian,
>
> On Sun, Nov 11, 2018 at 07:58:19PM +0100, Christian Hoff wrote:
> >
> > 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.
I have updated the commit message accordingly.
>
> > 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>
I have done that now.
>
> > 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.
Alright, I have thrown this line out.
>
> > 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.
Done.
>
> > } else if (!pdata->keymap_data) {
> > --
> > 2.16.1.2.g9d582f143
> >
>
> Otherwise looks good to me!#
I am happy to hear that! I have just submitted a new second version of this patch for review based on your suggestions.
>
> 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.
I fully agree. I hope that I will find time to do that in the next months. I already had the same idea, but somehow
there were always more urgent things to be done...
Best regards,
Christian
