diff mbox series

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

Message ID 20220819065946.9572-1-Gireesh.Hiremath@in.bosch.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/3] driver: input: matric-keypad: switch to gpiod | expand

Commit Message

Gireesh Hiremath Aug. 19, 2022, 6:59 a.m. UTC
From: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com>

Switch from gpio API to gpiod API

Signed-off-by: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com>

Gbp-Pq: Topic apertis/guardian
Gbp-Pq: Name driver-input-matric-keypad-switch-gpio-to-gpiod.patch
---
 drivers/input/keyboard/matrix_keypad.c | 84 ++++++++++----------------
 include/linux/input/matrix_keypad.h    |  4 +-
 2 files changed, 33 insertions(+), 55 deletions(-)

Comments

Krzysztof Kozlowski Aug. 19, 2022, 12:08 p.m. UTC | #1
On 19/08/2022 09:59, Gireesh.Hiremath@in.bosch.com wrote:
> From: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com>
> 
> Switch from gpio API to gpiod API
> 
> Signed-off-by: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com>
> 
> Gbp-Pq: Topic apertis/guardian
> Gbp-Pq: Name driver-input-matric-keypad-switch-gpio-to-gpiod.patch

What's that? I don't recall such tags in Linux kernel process... Is it
documented anywhere?

Best regards,
Krzysztof
Marco Felsch Aug. 19, 2022, 1:12 p.m. UTC | #2
Hi Gireesh,

On 22-08-19, Gireesh.Hiremath@in.bosch.com wrote:
> From: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com>
> 
> 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@in.bosch.com>
> 
> 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
> 
>
Dmitry Torokhov Aug. 20, 2022, 12:59 a.m. UTC | #3
On Fri, Aug 19, 2022 at 03:12:31PM +0200, Marco Felsch wrote:
> Hi Gireesh,
> 
> On 22-08-19, Gireesh.Hiremath@in.bosch.com wrote:
> > From: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com>
> > 
> > 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@in.bosch.com>
> > 
> > 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.

I do not think there is a reasonable way of keeping the compatibility
while using gpiod API (sans abandoning polarity handling and using
*_raw() versions of API).

I would adjust the DTSes in the kernel and move on, especially given
that the DTSes in the kernel are inconsistent - they specify
gpio-activelow attribute while specifying "action high" in gpio
properties).

Thanks.
Marco Felsch Aug. 20, 2022, 7:36 p.m. UTC | #4
Hi Dmitry,

On 22-08-19, Dmitry Torokhov wrote:
> On Fri, Aug 19, 2022 at 03:12:31PM +0200, Marco Felsch wrote:
> > Hi Gireesh,
> > 
> > On 22-08-19, Gireesh.Hiremath@in.bosch.com wrote:
> > > From: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com>
> > > 
> > > 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@in.bosch.com>
> > > 
> > > 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]);

...

> > > @@ -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.
> 
> I do not think there is a reasonable way of keeping the compatibility
> while using gpiod API (sans abandoning polarity handling and using
> *_raw() versions of API).
> 
> I would adjust the DTSes in the kernel and move on, especially given
> that the DTSes in the kernel are inconsistent - they specify
> gpio-activelow attribute while specifying "action high" in gpio
> properties).

Yes, because the driver didn't respect that by not using the gpiod API.
Got your point for the DTses but what about the boards based on the
platform-data? Those boards must be changed as well.

Also I thought DTB is firmware and we should never break it, now we
doing it by this commit. Just to get your opinion on this.

Regards,
  Marco
Dmitry Torokhov Aug. 21, 2022, 5 a.m. UTC | #5
Hi Marco,

On Sat, Aug 20, 2022 at 09:36:23PM +0200, Marco Felsch wrote:
> Hi Dmitry,
> 
> On 22-08-19, Dmitry Torokhov wrote:
> > On Fri, Aug 19, 2022 at 03:12:31PM +0200, Marco Felsch wrote:
> > > Hi Gireesh,
> > > 
> > > On 22-08-19, Gireesh.Hiremath@in.bosch.com wrote:
> > > > From: Gireesh Hiremath <Gireesh.Hiremath@in.bosch.com>
> > > > 
> > > > 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@in.bosch.com>
> > > > 
> > > > 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]);
> 
> ...
> 
> > > > @@ -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.
> > 
> > I do not think there is a reasonable way of keeping the compatibility
> > while using gpiod API (sans abandoning polarity handling and using
> > *_raw() versions of API).
> > 
> > I would adjust the DTSes in the kernel and move on, especially given
> > that the DTSes in the kernel are inconsistent - they specify
> > gpio-activelow attribute while specifying "action high" in gpio
> > properties).
> 
> Yes, because the driver didn't respect that by not using the gpiod API.
> Got your point for the DTses but what about the boards based on the
> platform-data? Those boards must be changed as well.

Yes, that is correct.

> 
> Also I thought DTB is firmware and we should never break it, now we
> doing it by this commit. Just to get your opinion on this.

Well, this is true in theory, however from the practical POV the boards
that use this driver do not store DTB in firmware, but rather distribute
it with the kernel. So while we normally try to keep compatibility, in
cases like this I feel we can be practical and instead of trying to
handle a pure theoretical case simply fix up DTSes and move on with our
lives.

Thanks.
Dan Carpenter Aug. 22, 2022, 7:36 a.m. UTC | #6
Hi,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Gireesh-Hiremath-in-bosch-com/driver-input-matric-keypad-switch-to-gpiod/20220819-151155
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20220819/202208192340.m1XMlTj5-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
drivers/input/keyboard/matrix_keypad.c:432 matrix_keypad_parse_dt() error: uninitialized symbol 'ret'.

Old smatch warnings:
drivers/input/keyboard/matrix_keypad.c:439 matrix_keypad_parse_dt() error: uninitialized symbol 'ret'.

vim +/ret +432 drivers/input/keyboard/matrix_keypad.c

5298cc4cc753bb Bill Pemberton   2012-11-23  380  static struct matrix_keypad_platform_data *
4a83eecff65bd3 AnilKumar Ch     2012-11-20  381  matrix_keypad_parse_dt(struct device *dev)
4a83eecff65bd3 AnilKumar Ch     2012-11-20  382  {
4a83eecff65bd3 AnilKumar Ch     2012-11-20  383  	struct matrix_keypad_platform_data *pdata;
4a83eecff65bd3 AnilKumar Ch     2012-11-20  384  	struct device_node *np = dev->of_node;
90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  385  	struct gpio_desc **gpios;
90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  386  	struct gpio_desc *desc;
d55bda1b3e7c5a Christian Hoff   2018-11-12  387  	int ret, i, nrow, ncol;
4a83eecff65bd3 AnilKumar Ch     2012-11-20  388  
4a83eecff65bd3 AnilKumar Ch     2012-11-20  389  	if (!np) {
4a83eecff65bd3 AnilKumar Ch     2012-11-20  390  		dev_err(dev, "device lacks DT data\n");
4a83eecff65bd3 AnilKumar Ch     2012-11-20  391  		return ERR_PTR(-ENODEV);
4a83eecff65bd3 AnilKumar Ch     2012-11-20  392  	}
4a83eecff65bd3 AnilKumar Ch     2012-11-20  393  
4a83eecff65bd3 AnilKumar Ch     2012-11-20  394  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
4a83eecff65bd3 AnilKumar Ch     2012-11-20  395  	if (!pdata) {
4a83eecff65bd3 AnilKumar Ch     2012-11-20  396  		dev_err(dev, "could not allocate memory for platform data\n");
4a83eecff65bd3 AnilKumar Ch     2012-11-20  397  		return ERR_PTR(-ENOMEM);
4a83eecff65bd3 AnilKumar Ch     2012-11-20  398  	}
4a83eecff65bd3 AnilKumar Ch     2012-11-20  399  
90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  400  	pdata->num_row_gpios = nrow = gpiod_count(dev, "row");
90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  401  	pdata->num_col_gpios = ncol = gpiod_count(dev, "col");
e80beb27d2f81a Grant Likely     2013-02-12  402  	if (nrow <= 0 || ncol <= 0) {
4a83eecff65bd3 AnilKumar Ch     2012-11-20  403  		dev_err(dev, "number of keypad rows/columns not specified\n");
4a83eecff65bd3 AnilKumar Ch     2012-11-20  404  		return ERR_PTR(-EINVAL);
4a83eecff65bd3 AnilKumar Ch     2012-11-20  405  	}
4a83eecff65bd3 AnilKumar Ch     2012-11-20  406  
4a83eecff65bd3 AnilKumar Ch     2012-11-20  407  	if (of_get_property(np, "linux,no-autorepeat", NULL))
4a83eecff65bd3 AnilKumar Ch     2012-11-20  408  		pdata->no_autorepeat = true;
aeda5003d0b987 Dmitry Torokhov  2015-07-16  409  
aeda5003d0b987 Dmitry Torokhov  2015-07-16  410  	pdata->wakeup = of_property_read_bool(np, "wakeup-source") ||
aeda5003d0b987 Dmitry Torokhov  2015-07-16  411  			of_property_read_bool(np, "linux,wakeup"); /* legacy */
aeda5003d0b987 Dmitry Torokhov  2015-07-16  412  
aa0e26bb786b00 David Rivshin    2017-03-29  413  	pdata->drive_inactive_cols =
aa0e26bb786b00 David Rivshin    2017-03-29  414  		of_property_read_bool(np, "drive-inactive-cols");
aa0e26bb786b00 David Rivshin    2017-03-29  415  
4a83eecff65bd3 AnilKumar Ch     2012-11-20  416  	of_property_read_u32(np, "debounce-delay-ms", &pdata->debounce_ms);
4a83eecff65bd3 AnilKumar Ch     2012-11-20  417  	of_property_read_u32(np, "col-scan-delay-us",
4a83eecff65bd3 AnilKumar Ch     2012-11-20  418  						&pdata->col_scan_delay_us);
4a83eecff65bd3 AnilKumar Ch     2012-11-20  419  
a86854d0c599b3 Kees Cook        2018-06-12  420  	gpios = devm_kcalloc(dev,
a86854d0c599b3 Kees Cook        2018-06-12  421  			     pdata->num_row_gpios + pdata->num_col_gpios,
90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  422  			     sizeof(*gpios),
4a83eecff65bd3 AnilKumar Ch     2012-11-20  423  			     GFP_KERNEL);
4a83eecff65bd3 AnilKumar Ch     2012-11-20  424  	if (!gpios) {
4a83eecff65bd3 AnilKumar Ch     2012-11-20  425  		dev_err(dev, "could not allocate memory for gpios\n");
4a83eecff65bd3 AnilKumar Ch     2012-11-20  426  		return ERR_PTR(-ENOMEM);
4a83eecff65bd3 AnilKumar Ch     2012-11-20  427  	}
4a83eecff65bd3 AnilKumar Ch     2012-11-20  428  
d55bda1b3e7c5a Christian Hoff   2018-11-12  429  	for (i = 0; i < nrow; i++) {
90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  430  		desc = devm_gpiod_get_index(dev, "row", i, GPIOD_IN);
90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  431  		if (IS_ERR(desc))
d55bda1b3e7c5a Christian Hoff   2018-11-12 @432  			return ERR_PTR(ret);

s/ret/desc/

90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  433  		gpios[i] = desc;
d55bda1b3e7c5a Christian Hoff   2018-11-12  434  	}
4a83eecff65bd3 AnilKumar Ch     2012-11-20  435  
d55bda1b3e7c5a Christian Hoff   2018-11-12  436  	for (i = 0; i < ncol; i++) {
90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  437  		desc = devm_gpiod_get_index(dev, "col", i, GPIOD_IN);
90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  438  		if (IS_ERR(desc))
d55bda1b3e7c5a Christian Hoff   2018-11-12  439  			return ERR_PTR(ret);
90bb4ee8dc3c27 Gireesh Hiremath 2022-08-19  440  		gpios[nrow + i] = desc;
d55bda1b3e7c5a Christian Hoff   2018-11-12  441  	}
4a83eecff65bd3 AnilKumar Ch     2012-11-20  442  
4a83eecff65bd3 AnilKumar Ch     2012-11-20  443  	pdata->row_gpios = gpios;
4a83eecff65bd3 AnilKumar Ch     2012-11-20  444  	pdata->col_gpios = &gpios[pdata->num_row_gpios];
4a83eecff65bd3 AnilKumar Ch     2012-11-20  445  
4a83eecff65bd3 AnilKumar Ch     2012-11-20  446  	return pdata;
4a83eecff65bd3 AnilKumar Ch     2012-11-20  447  }
diff mbox series

Patch

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);
 	} 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;
-
 	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;