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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220820193623.bnjgnydu3rkbdtbo@pengutronix.de>
Date:   Sat, 20 Aug 2022 21:36:23 +0200
From:   Marco Felsch <m.felsch@...gutronix.de>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Gireesh.Hiremath@...bosch.com, linux-omap@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-input@...r.kernel.org, bcousson@...libre.com,
        tony@...mide.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, mkorpershoek@...libre.com,
        davidgow@...gle.com, swboyd@...omium.org, fengping.yu@...iatek.com,
        y.oudjana@...tonmail.com, rdunlap@...radead.org,
        colin.king@...el.com, sjoerd.simons@...labora.co.uk,
        VinayKumar.Shettar@...bosch.com,
        Govindaraji.Sivanantham@...bosch.com, anaclaudia.dias@...bosch.com
Subject: Re: [PATCH v3 1/3] driver: input: matric-keypad: switch to gpiod

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@...bosch.com wrote:
> > > From: Gireesh Hiremath <Gireesh.Hiremath@...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@...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