lkml.org 
[lkml]   [2023]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2023-03-26 23:41    [W:0.059 / U:1.780 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site