| Message ID | 20230113062538.1537-1-Gireesh.Hiremath@in.bosch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | driver: input: matric-keypad: switch to gpiod API | expand |
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; > }
From: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com> Hi Andy Shevchenko, I will correct it as >Thank you for the patch, my comments below. > >> switch to new gpio descriptor based API Switch to 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. and template as Input: matrix_keypad - switch to gpiod API > >... > >> 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; > if GPIO from I2C or SPI IO expander, which may sleep, so it is safer to use the gpiod_set_value_cansleep() and gpiod_get_value_cansleep(). >... > >> - 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. gpio_request() equalent api gpiod_request() is alredy called inside gpiod_get_index(...). gpiod_put() is required to free GPIO. > >... > >> 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? shall I update it as return ERR_CAST(desc); > >> + 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; >> } Thanks, Gireesh Hiremath
On Thu, Jan 19, 2023 at 11:47:36AM +0000, Gireesh.Hiremath@in.bosch.com wrote: > I will correct it as > >Thank you for the patch, my comments below. > > > >> switch to new gpio descriptor based API > Switch to GPIO descriptor based API. ...to the GPIO... > >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. > and template as > Input: matrix_keypad - switch to gpiod API OK! ... > >> 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; > > > if GPIO from I2C or SPI IO expander, which may sleep, > so it is safer to use the gpiod_set_value_cansleep() and > gpiod_get_value_cansleep(). No, my point is about active level (LOW or HIGH). It's encoded into the descriptor in opposite to the plain GPIO number. This change needs very careful understanding of the active level. ... > >> - 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. > gpio_request() equalent api gpiod_request() is alredy called inside gpiod_get_index(...). > gpiod_put() is required to free GPIO. Yes, but you removed request call, so should remove the free. The gpiod_put() should be at the same function as gpiod_get(). ... > >> 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? > shall I update it as > return ERR_CAST(desc); 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; > >> }
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c index 203310727d88..3697f203b3cc 100644 --- a/drivers/input/keyboard/matrix_keypad.c +++ b/drivers/input/keyboard/matrix_keypad.c @@ -50,11 +50,11 @@ static void __activate_col(const struct matrix_keypad_platform_data *pdata, 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); if (!pdata->drive_inactive_cols) - gpio_direction_input(pdata->col_gpios[col]); + gpiod_direction_input(pdata->col_gpios[col]); } } @@ -79,7 +79,7 @@ static void activate_all_cols(const struct matrix_keypad_platform_data *pdata, static bool row_asserted(const struct matrix_keypad_platform_data *pdata, int row) { - return gpio_get_value_cansleep(pdata->row_gpios[row]) ? + return gpiod_get_value_cansleep(pdata->row_gpios[row]) ? !pdata->active_low : pdata->active_low; } @@ -92,7 +92,7 @@ static void enable_row_irqs(struct matrix_keypad *keypad) enable_irq(pdata->clustered_irq); else { for (i = 0; i < pdata->num_row_gpios; i++) - enable_irq(gpio_to_irq(pdata->row_gpios[i])); + enable_irq(gpiod_to_irq(pdata->row_gpios[i])); } } @@ -105,7 +105,7 @@ static void disable_row_irqs(struct matrix_keypad *keypad) disable_irq_nosync(pdata->clustered_irq); else { for (i = 0; i < pdata->num_row_gpios; i++) - disable_irq_nosync(gpio_to_irq(pdata->row_gpios[i])); + disable_irq_nosync(gpiod_to_irq(pdata->row_gpios[i])); } } @@ -128,7 +128,7 @@ static void matrix_keypad_scan(struct work_struct *work) memset(new_state, 0, sizeof(new_state)); for (row = 0; row < pdata->num_row_gpios; row++) - gpio_direction_input(pdata->row_gpios[row]); + gpiod_direction_input(pdata->row_gpios[row]); /* assert each column and read the row status out */ for (col = 0; col < pdata->num_col_gpios; col++) { @@ -233,7 +233,7 @@ static void matrix_keypad_stop(struct input_dev *dev) static void matrix_keypad_enable_wakeup(struct matrix_keypad *keypad) { const struct matrix_keypad_platform_data *pdata = keypad->pdata; - unsigned int gpio; + struct gpio_desc *gpio; int i; if (pdata->clustered_irq > 0) { @@ -245,7 +245,7 @@ static void matrix_keypad_enable_wakeup(struct matrix_keypad *keypad) if (!test_bit(i, keypad->disabled_gpios)) { gpio = pdata->row_gpios[i]; - if (enable_irq_wake(gpio_to_irq(gpio)) == 0) + if (enable_irq_wake(gpiod_to_irq(gpio)) == 0) __set_bit(i, keypad->disabled_gpios); } } @@ -255,7 +255,7 @@ static void matrix_keypad_enable_wakeup(struct matrix_keypad *keypad) static void matrix_keypad_disable_wakeup(struct matrix_keypad *keypad) { const struct matrix_keypad_platform_data *pdata = keypad->pdata; - unsigned int gpio; + struct gpio_desc *gpio; int i; if (pdata->clustered_irq > 0) { @@ -267,7 +267,7 @@ static void matrix_keypad_disable_wakeup(struct matrix_keypad *keypad) for (i = 0; i < pdata->num_row_gpios; i++) { if (test_and_clear_bit(i, keypad->disabled_gpios)) { gpio = pdata->row_gpios[i]; - disable_irq_wake(gpio_to_irq(gpio)); + disable_irq_wake(gpiod_to_irq(gpio)); } } } @@ -310,27 +310,21 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev, /* initialized strobe lines as outputs, activated */ for (i = 0; i < pdata->num_col_gpios; i++) { - err = gpio_request(pdata->col_gpios[i], "matrix_kbd_col"); + err = gpiod_direction_output(pdata->col_gpios[i], !pdata->active_low); if (err) { dev_err(&pdev->dev, - "failed to request GPIO%d for COL%d\n", - pdata->col_gpios[i], i); + "Unable to set direction for COL GPIO line %i\n", i); goto err_free_cols; } - - gpio_direction_output(pdata->col_gpios[i], !pdata->active_low); } for (i = 0; i < pdata->num_row_gpios; i++) { - err = gpio_request(pdata->row_gpios[i], "matrix_kbd_row"); + err = gpiod_direction_input(pdata->row_gpios[i]); if (err) { dev_err(&pdev->dev, - "failed to request GPIO%d for ROW%d\n", - pdata->row_gpios[i], i); + "Unable to set direction for ROW GPIO line %i\n", i); goto err_free_rows; } - - gpio_direction_input(pdata->row_gpios[i]); } if (pdata->clustered_irq > 0) { @@ -346,15 +340,15 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev, } else { for (i = 0; i < pdata->num_row_gpios; i++) { err = request_any_context_irq( - gpio_to_irq(pdata->row_gpios[i]), + gpiod_to_irq(pdata->row_gpios[i]), matrix_keypad_interrupt, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, "matrix-keypad", keypad); if (err < 0) { dev_err(&pdev->dev, - "Unable to acquire interrupt for GPIO line %i\n", - pdata->row_gpios[i]); + "Unable to acquire interrupt for ROW GPIO line %i\n", + i); goto err_free_irqs; } } @@ -366,15 +360,15 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev, err_free_irqs: while (--i >= 0) - free_irq(gpio_to_irq(pdata->row_gpios[i]), keypad); + free_irq(gpiod_to_irq(pdata->row_gpios[i]), keypad); i = pdata->num_row_gpios; err_free_rows: while (--i >= 0) - gpio_free(pdata->row_gpios[i]); + gpiod_put(pdata->row_gpios[i]); i = pdata->num_col_gpios; err_free_cols: while (--i >= 0) - gpio_free(pdata->col_gpios[i]); + gpiod_put(pdata->col_gpios[i]); return err; } @@ -388,14 +382,14 @@ static void matrix_keypad_free_gpio(struct matrix_keypad *keypad) free_irq(pdata->clustered_irq, keypad); } else { for (i = 0; i < pdata->num_row_gpios; i++) - free_irq(gpio_to_irq(pdata->row_gpios[i]), keypad); + free_irq(gpiod_to_irq(pdata->row_gpios[i]), keypad); } for (i = 0; i < pdata->num_row_gpios; i++) - gpio_free(pdata->row_gpios[i]); + gpiod_put(pdata->row_gpios[i]); for (i = 0; i < pdata->num_col_gpios; i++) - gpio_free(pdata->col_gpios[i]); + gpiod_put(pdata->col_gpios[i]); } #ifdef CONFIG_OF @@ -404,8 +398,9 @@ matrix_keypad_parse_dt(struct device *dev) { struct matrix_keypad_platform_data *pdata; struct device_node *np = dev->of_node; - unsigned int *gpios; - int ret, i, nrow, ncol; + struct gpio_desc **gpios; + struct gpio_desc *desc; + int i, nrow, ncol; if (!np) { dev_err(dev, "device lacks DT data\n"); @@ -443,7 +438,7 @@ matrix_keypad_parse_dt(struct device *dev) gpios = devm_kcalloc(dev, pdata->num_row_gpios + pdata->num_col_gpios, - sizeof(unsigned int), + sizeof(*gpios), GFP_KERNEL); if (!gpios) { dev_err(dev, "could not allocate memory for gpios\n"); @@ -451,17 +446,17 @@ matrix_keypad_parse_dt(struct device *dev) } 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; + desc = gpiod_get_index(dev, "row", i, GPIOD_IN); + if (IS_ERR(desc)) + return ERR_PTR(-EINVAL); + 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); + gpios[nrow + i] = desc; } pdata->row_gpios = gpios; diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h index 9476768c3b90..8ad7d4626e62 100644 --- a/include/linux/input/matrix_keypad.h +++ b/include/linux/input/matrix_keypad.h @@ -59,8 +59,8 @@ struct matrix_keymap_data { struct matrix_keypad_platform_data { const struct matrix_keymap_data *keymap_data; - const unsigned int *row_gpios; - const unsigned int *col_gpios; + struct gpio_desc **row_gpios; + struct gpio_desc **col_gpios; unsigned int num_row_gpios; unsigned int num_col_gpios;