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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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.

-- 
Dmitry