Hi Dmitry,

On 22-08-19, Dmitry Torokhov wrote:

On Fri, Aug 19, 2022 at 03:12:31PM +0200, Marco Felsch wrote:

quoted

Hi Gireesh,

On 22-08-19, Gireesh.Hiremath@in.bosch.com wrote:

quoted

From: Gireesh Hiremath <redacted>

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.

quoted

Signed-off-by: Gireesh Hiremath <redacted>

Gbp-Pq: Topic apertis/guardian
Gbp-Pq: Name driver-input-matric-keypad-switch-gpio-to-gpiod.patch

Please drop this internal tags.

quoted

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

quoted

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

...

quoted

quoted

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