Messages in this thread | | | Date | Fri, 13 Jan 2023 20:43:13 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH] driver: input: matric-keypad: switch to gpiod API |
| |
On Fri, Jan 13, 2023 at 06:25:38AM +0000, Gireesh.Hiremath@in.bosch.com wrote: > From: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com>
Thank you for the patch, my comments below.
> switch to new gpio descriptor based API
Please, respect English grammar and punctuation.
Also, you have a typo in the Subject besides the fact that the template for Input subsystem is different. So prefix has to be changed as well.
...
> bool level_on = !pdata->active_low; > > if (on) { > - gpio_direction_output(pdata->col_gpios[col], level_on); > + gpiod_direction_output(pdata->col_gpios[col], level_on); > } else { > - gpio_set_value_cansleep(pdata->col_gpios[col], !level_on); > + gpiod_set_value_cansleep(pdata->col_gpios[col], !level_on); > }
I believe it's not so trivial. The GPIO descriptor already has encoded the level information and above one as below are not clear now.
> - return gpio_get_value_cansleep(pdata->row_gpios[row]) ? > + return gpiod_get_value_cansleep(pdata->row_gpios[row]) ? > !pdata->active_low : pdata->active_low;
...
> - err = gpio_request(pdata->col_gpios[i], "matrix_kbd_col"); > + err = gpiod_direction_output(pdata->col_gpios[i], !pdata->active_low);
> while (--i >= 0) > - gpio_free(pdata->row_gpios[i]); > + gpiod_put(pdata->row_gpios[i]);
This looks like an incorrect change.
> while (--i >= 0) > - gpio_free(pdata->col_gpios[i]); > + gpiod_put(pdata->col_gpios[i]);
So does this.
Since you dropped gpio_request() you need to drop gpio_free() as well, and not replace it.
...
> for (i = 0; i < nrow; i++) { > - ret = of_get_named_gpio(np, "row-gpios", i); > - if (ret < 0)
> - return ERR_PTR(ret);
(1)
> - gpios[i] = ret; > + desc = gpiod_get_index(dev, "row", i, GPIOD_IN); > + if (IS_ERR(desc))
> + return ERR_PTR(-EINVAL);
Why?! How will it handle deferred probe, for example?
> + gpios[i] = desc; > }
...
> 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; > + desc = gpiod_get_index(dev, "col", i, GPIOD_IN); > + if (IS_ERR(desc)) > + return ERR_PTR(-EINVAL);
Ditto.
> + gpios[nrow + i] = desc; > }
-- With Best Regards, Andy Shevchenko
| |