From: Gireesh Hiremath <Gireesh.Hiremath@xxxxxxxxxxxx>
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
