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

[Marco Felsch]

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@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]);

...

> > > @@ -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