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

From: Marco Felsch <m.felsch@pengutronix.de>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Gireesh.Hiremath@in.bosch.com, linux-omap@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-input@vger.kernel.org, bcousson@baylibre.com,
	tony@atomide.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, mkorpershoek@baylibre.com,
	davidgow@google.com, swboyd@chromium.org,
	fengping.yu@mediatek.com, y.oudjana@protonmail.com,
	rdunlap@infradead.org, colin.king@intel.com,
	sjoerd.simons@collabora.co.uk, VinayKumar.Shettar@in.bosch.com,
	Govindaraji.Sivanantham@in.bosch.com,
	anaclaudia.dias@de.bosch.com
Subject: Re: [PATCH v3 1/3] driver: input: matric-keypad: switch to gpiod
Date: Sat, 20 Aug 2022 21:36:23 +0200	[thread overview]
Message-ID: <20220820193623.bnjgnydu3rkbdtbo@pengutronix.de> (raw)
In-Reply-To: <YwAx38N0ICE37Vu9@google.com>

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