Re: [PATCH v3 1/3] driver: input: matric-keypad: switch to gpiod

From: Marco Felsch
Date: Fri Aug 19 2022 - 09:12:49 EST

Next message: Krzysztof Kozlowski: "Re: [PATCH v2 2/4] arm64: dts: apple: t8103: Add MCA and its support"
Previous message: Taehee Yoo: "Re: [PATCH -next] amt: remove unneccessary skb pointer check"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v3 1/3] driver: input: matric-keypad: switch to gpiod"
Next in thread: Dmitry Torokhov: "Re: [PATCH v3 1/3] driver: input: matric-keypad: switch to gpiod"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Gireesh,

On 22-08-19, Gireesh.Hiremath@xxxxxxxxxxxx wrote:
> From: Gireesh Hiremath <Gireesh.Hiremath@xxxxxxxxxxxx>
>
> Switch from gpio API to gpiod API

Nice change.

Did you checked the users of this driver? This change changes the
behaviour for actice_low GPIOs. A quick check showed that at least on
upstream board: arch/arm/mach-pxa/palmtc.c uses active low GPIOs.

> Signed-off-by: Gireesh Hiremath <Gireesh.Hiremath@xxxxxxxxxxxx>
>
> Gbp-Pq: Topic apertis/guardian
> Gbp-Pq: Name driver-input-matric-keypad-switch-gpio-to-gpiod.patch

Please drop this internal tags.

> ---
> drivers/input/keyboard/matrix_keypad.c | 84 ++++++++++----------------
> include/linux/input/matrix_keypad.h | 4 +-
> 2 files changed, 33 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
> index 30924b57058f..b99574edad9c 100644
> --- a/drivers/input/keyboard/matrix_keypad.c
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -15,11 +15,10 @@
> #include <linux/interrupt.h>
> #include <linux/jiffies.h>
> #include <linux/module.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/input/matrix_keypad.h>
> #include <linux/slab.h>
> #include <linux/of.h>
> -#include <linux/of_gpio.h>
> #include <linux/of_platform.h>
>
> struct matrix_keypad {
> @@ -49,11 +48,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);

Now strange things can happen, if active_low is set and you specified
GPIO_ACTIVE_LOW within the device-tree. We need a way to move to gpiod
and keep the current behaviour.

> } 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]);
> }
> }
>
> @@ -78,7 +77,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;
> }
>
> @@ -91,7 +90,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]));
> }
> }
>
> @@ -104,7 +103,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]));
> }
> }
>
> @@ -230,7 +229,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) {
> @@ -242,7 +241,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);
> }
> }
> @@ -252,7 +251,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) {
> @@ -264,7 +263,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));
> }
> }
> }
> @@ -308,27 +307,11 @@ 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");
> - if (err) {
> - dev_err(&pdev->dev,
> - "failed to request GPIO%d for COL%d\n",
> - pdata->col_gpios[i], i);
> - goto err_free_cols;
> - }
> -
> - gpio_direction_output(pdata->col_gpios[i], !pdata->active_low);
> + gpiod_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");
> - if (err) {
> - dev_err(&pdev->dev,
> - "failed to request GPIO%d for ROW%d\n",
> - pdata->row_gpios[i], i);
> - goto err_free_rows;
> - }
> -
> - gpio_direction_input(pdata->row_gpios[i]);
> + gpiod_direction_input(pdata->row_gpios[i]);
> }
>
> if (pdata->clustered_irq > 0) {
> @@ -344,7 +327,7 @@ 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,
> @@ -352,7 +335,7 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev,
> if (err < 0) {
> dev_err(&pdev->dev,
> "Unable to acquire interrupt for GPIO line %i\n",
> - pdata->row_gpios[i]);
> + i);
> goto err_free_irqs;
> }
> }
> @@ -364,15 +347,12 @@ 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]);
>
> return err;
> }
> @@ -386,14 +366,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
> @@ -402,7 +382,8 @@ matrix_keypad_parse_dt(struct device *dev)
> {
> struct matrix_keypad_platform_data *pdata;
> struct device_node *np = dev->of_node;
> - unsigned int *gpios;
> + struct gpio_desc **gpios;
> + struct gpio_desc *desc;
> int ret, i, nrow, ncol;
>
> if (!np) {
> @@ -416,8 +397,8 @@ matrix_keypad_parse_dt(struct device *dev)
> return ERR_PTR(-ENOMEM);
> }
>
> - pdata->num_row_gpios = nrow = of_gpio_named_count(np, "row-gpios");
> - pdata->num_col_gpios = ncol = of_gpio_named_count(np, "col-gpios");
> + pdata->num_row_gpios = nrow = gpiod_count(dev, "row");
> + pdata->num_col_gpios = ncol = gpiod_count(dev, "col");
> if (nrow <= 0 || ncol <= 0) {
> dev_err(dev, "number of keypad rows/columns not specified\n");
> return ERR_PTR(-EINVAL);
> @@ -429,9 +410,6 @@ matrix_keypad_parse_dt(struct device *dev)
> pdata->wakeup = of_property_read_bool(np, "wakeup-source") ||
> of_property_read_bool(np, "linux,wakeup"); /* legacy */
>
> - if (of_get_property(np, "gpio-activelow", NULL))
> - pdata->active_low = true;
> -

This removes backward compatibility, please don't do that.

Regards,
Marco

> pdata->drive_inactive_cols =
> of_property_read_bool(np, "drive-inactive-cols");
>
> @@ -441,7 +419,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");
> @@ -449,17 +427,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)
> + desc = devm_gpiod_get_index(dev, "row", i, GPIOD_IN);
> + if (IS_ERR(desc))
> return ERR_PTR(ret);
> - gpios[i] = ret;
> + gpios[i] = desc;
> }
>
> for (i = 0; i < ncol; i++) {
> - ret = of_get_named_gpio(np, "col-gpios", i);
> - if (ret < 0)
> + desc = devm_gpiod_get_index(dev, "col", i, GPIOD_IN);
> + if (IS_ERR(desc))
> return ERR_PTR(ret);
> - gpios[nrow + i] = ret;
> + 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;
> --
> 2.20.1
>
>

Next message: Krzysztof Kozlowski: "Re: [PATCH v2 2/4] arm64: dts: apple: t8103: Add MCA and its support"
Previous message: Taehee Yoo: "Re: [PATCH -next] amt: remove unneccessary skb pointer check"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v3 1/3] driver: input: matric-keypad: switch to gpiod"
Next in thread: Dmitry Torokhov: "Re: [PATCH v3 1/3] driver: input: matric-keypad: switch to gpiod"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]