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
