[PATCH 00/10] adp5588-keys refactor and fw properties support

[PATCH 00/10] adp5588-keys refactor and fw properties support

[Nuno Sá]

The main goal of this patchset is to remove platform data and replace it by
firmware properties. Original discussion in [1].

While in here, some refactor was done to the driver. The most noticeable one
is to replace the GPIs events handling by irqchip support so that this gpi
keys can be "consumed" by the gpio-keys driver (also as suggested in [1]).
With this, the gpio-adp5588 can be removed. This change comes first so that
we can already remove some platform data variables making it easier to
completly replace it by firmware properties further down in the series.

As there's no users of the platform data, I just replace it in a single
patch as there's no point in having support for both (even though it might
be harder to review the patch as-is).

Special note to the gpio-adp5588 driver removal. I'm aware of some changes
to the driver in [2]. These changes are in the gpio tree and this patchset
is naturally based on the input tree which means that patch 2 will
not apply. So, I'm not really sure how to handle this. I guess in this
case the conflict is easy to handle :) but just let me know on how to
proceed in here if there's anything for me to do.

[1]: https://lore.kernel.org/linux-input/20220504084617.36844-1-u.kleine-koenig@pengutronix.de/
[2]: https://lore.kernel.org/linux-gpio/20220628193906.36350-3-andriy.shevchenko@linux.intel.com/

Nuno Sá (10):
  input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'
  gpio: gpio-adp5588: drop the driver
  input: keyboard: adp5588-keys: bail out on returned error
  input: keyboard: adp5588-keys: add support for fw properties
  dt-bindings: input: adp5588-keys: add bindings
  input: keyboard: adp5588-keys: do not check for irq presence
  input: keyboard: adp5588-keys: fix coding style warnings
  input: keyboard: adp5588-keys: add optional reset gpio
  input: keyboard: adp5588-keys: add regulator support
  input: keyboard: adp5588-keys: Use new PM macros

 .../bindings/input/adi,adp5588-keys.yaml      | 110 +++
 MAINTAINERS                                   |   2 +-
 drivers/gpio/Kconfig                          |  14 -
 drivers/gpio/Makefile                         |   1 -
 drivers/gpio/gpio-adp5588.c                   | 471 ------------
 drivers/input/keyboard/Kconfig                |   3 +
 drivers/input/keyboard/adp5588-keys.c         | 710 ++++++++++++------
 include/linux/platform_data/adp5588.h         | 171 -----
 8 files changed, 580 insertions(+), 902 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
 delete mode 100644 drivers/gpio/gpio-adp5588.c
 delete mode 100644 include/linux/platform_data/adp5588.h

-- 
2.37.0

[PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[Nuno Sá]

This change replaces the support for GPIs as key event generators.
Instead of reporting the events directly, we add a gpio based irqchip
so that these events can be consumed by keys defined in the gpio-keys
driver (as it's goal is indeed for keys on GPIOs capable of generating
interrupts). With this, the gpio-adp5588 driver can also be dropped.

The basic idea is that all the pins that are not being used as part of
the keymap matrix can be possibly requested as GPIOs by gpio-keys
(it's also fine to use these pins as plain interrupts though that's not
really the point).

Since the gpiochip now also has irqchip capabilities, we should only
remove it after we free the device interrupt (otherwise we could, in
theory, be handling GPIs interrupts while the gpiochip is concurrently
removed). Thus the call 'adp5588_gpio_add()' is moved and since the
setup phase also needs to come before making the gpios visible, we also
need to move 'adp5588_setup()'.

While at it, always select GPIOLIB so that we don't need to use #ifdef
guards.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/input/keyboard/Kconfig        |   2 +
 drivers/input/keyboard/adp5588-keys.c | 262 +++++++++++++-------------
 include/linux/platform_data/adp5588.h |   2 -
 3 files changed, 132 insertions(+), 134 deletions(-)

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index a20ee693b22b..ca5cd5e520a7 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -40,6 +40,8 @@ config KEYBOARD_ADP5520
 config KEYBOARD_ADP5588
 	tristate "ADP5588/87 I2C QWERTY Keypad and IO Expander"
 	depends on I2C
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
 	help
 	  Say Y here if you want to use a ADP5588/87 attached to your
 	  system I2C bus.
diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 1a1a05d7cd42..dad8862f6c76 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -46,15 +46,14 @@ struct adp5588_kpad {
 	ktime_t irq_time;
 	unsigned long delay;
 	unsigned short keycode[ADP5588_KEYMAPSIZE];
-	const struct adp5588_gpi_map *gpimap;
-	unsigned short gpimapsize;
-#ifdef CONFIG_GPIOLIB
 	unsigned char gpiomap[ADP5588_MAXGPIO];
 	struct gpio_chip gc;
+	struct irq_chip irq_chip;
 	struct mutex gpio_lock;	/* Protect cached dir, dat_out */
 	u8 dat_out[3];
 	u8 dir[3];
-#endif
+	u8 int_en[3];
+	u8 irq_mask[3];
 };
 
 static int adp5588_read(struct i2c_client *client, u8 reg)
@@ -72,7 +71,6 @@ static int adp5588_write(struct i2c_client *client, u8 reg, u8 val)
 	return i2c_smbus_write_byte_data(client, reg, val);
 }
 
-#ifdef CONFIG_GPIOLIB
 static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned off)
 {
 	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
@@ -171,9 +169,6 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
 	for (i = 0; i < pdata->cols; i++)
 		pin_used[i + GPI_PIN_COL_BASE - GPI_PIN_BASE] = true;
 
-	for (i = 0; i < kpad->gpimapsize; i++)
-		pin_used[kpad->gpimap[i].pin - GPI_PIN_BASE] = true;
-
 	for (i = 0; i < ADP5588_MAXGPIO; i++)
 		if (!pin_used[i])
 			kpad->gpiomap[n_unused++] = i;
@@ -196,11 +191,63 @@ static void adp5588_gpio_do_teardown(void *_kpad)
 		dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
 }
 
+static void adp5588_irq_bus_lock(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct adp5588_kpad *kpad = gpiochip_get_data(gc);
+
+	mutex_lock(&kpad->gpio_lock);
+}
+
+static void adp5588_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct adp5588_kpad *kpad = gpiochip_get_data(gc);
+	int i;
+
+	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
+		if (kpad->int_en[i] ^ kpad->irq_mask[i]) {
+			kpad->int_en[i] = kpad->irq_mask[i];
+			adp5588_write(kpad->client, GPI_EM1 + i, kpad->int_en[i]);
+		}
+	}
+
+	mutex_unlock(&kpad->gpio_lock);
+}
+
+static void adp5588_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct adp5588_kpad *kpad = gpiochip_get_data(gc);
+	unsigned long real_irq = kpad->gpiomap[d->hwirq];
+
+	kpad->irq_mask[ADP5588_BANK(real_irq)] &= ~ADP5588_BIT(real_irq);
+}
+
+static void adp5588_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct adp5588_kpad *kpad = gpiochip_get_data(gc);
+	unsigned long real_irq = kpad->gpiomap[d->hwirq];
+
+	kpad->irq_mask[ADP5588_BANK(real_irq)] |= ADP5588_BIT(real_irq);
+}
+
+static int adp5588_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	if (!(type & IRQ_TYPE_EDGE_BOTH))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 {
+	struct irq_chip *irq_chip = &kpad->irq_chip;
 	struct device *dev = &kpad->client->dev;
 	const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
 	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
+	struct gpio_irq_chip *girq;
 	int i, error;
 
 	if (!gpio_data)
@@ -212,6 +259,8 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 		return 0;
 	}
 
+	kpad->gc.parent = &kpad->client->dev;
+	kpad->gc.of_node = kpad->client->dev.of_node;
 	kpad->gc.direction_input = adp5588_gpio_direction_input;
 	kpad->gc.direction_output = adp5588_gpio_direction_output;
 	kpad->gc.get = adp5588_gpio_get_value;
@@ -223,6 +272,18 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	kpad->gc.owner = THIS_MODULE;
 	kpad->gc.names = gpio_data->names;
 
+	irq_chip->name = "adp5588";
+	irq_chip->irq_mask = adp5588_irq_mask;
+	irq_chip->irq_unmask = adp5588_irq_unmask;
+	irq_chip->irq_bus_lock = adp5588_irq_bus_lock;
+	irq_chip->irq_bus_sync_unlock = adp5588_irq_bus_sync_unlock;
+	irq_chip->irq_set_type = adp5588_irq_set_type;
+	irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE;
+	girq = &kpad->gc.irq;
+	girq->chip = irq_chip;
+	girq->handler = handle_simple_irq;
+	girq->threaded = true;
+
 	mutex_init(&kpad->gpio_lock);
 
 	error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
@@ -255,35 +316,70 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	return 0;
 }
 
-#else
-static inline int adp5588_gpio_add(struct adp5588_kpad *kpad)
+static int adp5588_gpiomap_get_hwirq(const u8 *map, unsigned int gpio,
+				     unsigned int ngpios)
 {
-	return 0;
+	unsigned int hwirq;
+
+	for (hwirq = 0; hwirq < ngpios; hwirq++)
+		if (map[hwirq] == gpio)
+			return hwirq;
+
+	/* should never happen */
+	WARN_ON_ONCE(hwirq == ngpios);
+
+	return -ENOENT;
+}
+
+static void adp5588_gpio_irq_handle(struct adp5588_kpad *kpad, int key_val,
+				    int key_press)
+{
+	unsigned int irq, gpio = key_val - GPI_PIN_BASE, irq_type, hwirq;
+	struct i2c_client *client = kpad->client;
+	struct irq_data *desc;
+
+	hwirq = adp5588_gpiomap_get_hwirq(kpad->gpiomap, gpio, kpad->gc.ngpio);
+	if (hwirq < 0) {
+		dev_err(&client->dev, "Could not get hwirq for key(%u)\n", key_val);
+		return;
+	}
+
+	irq = irq_find_mapping(kpad->gc.irq.domain, hwirq);
+	if (irq <= 0)
+		return;
+
+	desc = irq_get_irq_data(irq);
+	if (!desc) {
+		dev_err(&client->dev, "Could not get irq(%u) data\n", irq);
+		return;
+	}
+
+	irq_type = irqd_get_trigger_type(desc);
+
+	/*
+	 * Default is active low which means key_press is asserted on
+	 * the falling edge.
+	 */
+	if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press) ||
+	    (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))
+		handle_nested_irq(irq);
 }
-#endif
 
 static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 {
-	int i, j;
+	int i;
 
 	for (i = 0; i < ev_cnt; i++) {
 		int key = adp5588_read(kpad->client, Key_EVENTA + i);
 		int key_val = key & KEY_EV_MASK;
+		int key_press = key & KEY_EV_PRESSED;
 
-		if (key_val >= GPI_PIN_BASE && key_val <= GPI_PIN_END) {
-			for (j = 0; j < kpad->gpimapsize; j++) {
-				if (key_val == kpad->gpimap[j].pin) {
-					input_report_switch(kpad->input,
-							kpad->gpimap[j].sw_evt,
-							key & KEY_EV_PRESSED);
-					break;
-				}
-			}
-		} else {
+		if (key_val >= GPI_PIN_BASE && key_val <= GPI_PIN_END)
+			/* gpio line used as IRQ source */
+			adp5588_gpio_irq_handle(kpad, key_val, key_press);
+		else
 			input_report_key(kpad->input,
-					 kpad->keycode[key_val - 1],
-					 key & KEY_EV_PRESSED);
-		}
+					 kpad->keycode[key_val - 1], key_press);
 	}
 }
 
@@ -341,7 +437,6 @@ static int adp5588_setup(struct i2c_client *client)
 			dev_get_platdata(&client->dev);
 	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
 	int i, ret;
-	unsigned char evt_mode1 = 0, evt_mode2 = 0, evt_mode3 = 0;
 
 	ret = adp5588_write(client, KP_GPIO1, KP_SEL(pdata->rows));
 	ret |= adp5588_write(client, KP_GPIO2, KP_SEL(pdata->cols) & 0xFF);
@@ -356,23 +451,6 @@ static int adp5588_setup(struct i2c_client *client)
 	for (i = 0; i < KEYP_MAX_EVENT; i++)
 		ret |= adp5588_read(client, Key_EVENTA);
 
-	for (i = 0; i < pdata->gpimapsize; i++) {
-		unsigned short pin = pdata->gpimap[i].pin;
-
-		if (pin <= GPI_PIN_ROW_END) {
-			evt_mode1 |= (1 << (pin - GPI_PIN_ROW_BASE));
-		} else {
-			evt_mode2 |= ((1 << (pin - GPI_PIN_COL_BASE)) & 0xFF);
-			evt_mode3 |= ((1 << (pin - GPI_PIN_COL_BASE)) >> 8);
-		}
-	}
-
-	if (pdata->gpimapsize) {
-		ret |= adp5588_write(client, GPI_EM1, evt_mode1);
-		ret |= adp5588_write(client, GPI_EM2, evt_mode2);
-		ret |= adp5588_write(client, GPI_EM3, evt_mode3);
-	}
-
 	if (gpio_data) {
 		for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
 			int pull_mask = gpio_data->pullup_dis_mask;
@@ -399,44 +477,6 @@ static int adp5588_setup(struct i2c_client *client)
 	return 0;
 }
 
-static void adp5588_report_switch_state(struct adp5588_kpad *kpad)
-{
-	int gpi_stat1 = adp5588_read(kpad->client, GPIO_DAT_STAT1);
-	int gpi_stat2 = adp5588_read(kpad->client, GPIO_DAT_STAT2);
-	int gpi_stat3 = adp5588_read(kpad->client, GPIO_DAT_STAT3);
-	int gpi_stat_tmp, pin_loc;
-	int i;
-
-	for (i = 0; i < kpad->gpimapsize; i++) {
-		unsigned short pin = kpad->gpimap[i].pin;
-
-		if (pin <= GPI_PIN_ROW_END) {
-			gpi_stat_tmp = gpi_stat1;
-			pin_loc = pin - GPI_PIN_ROW_BASE;
-		} else if ((pin - GPI_PIN_COL_BASE) < 8) {
-			gpi_stat_tmp = gpi_stat2;
-			pin_loc = pin - GPI_PIN_COL_BASE;
-		} else {
-			gpi_stat_tmp = gpi_stat3;
-			pin_loc = pin - GPI_PIN_COL_BASE - 8;
-		}
-
-		if (gpi_stat_tmp < 0) {
-			dev_err(&kpad->client->dev,
-				"Can't read GPIO_DAT_STAT switch %d default to OFF\n",
-				pin);
-			gpi_stat_tmp = 0;
-		}
-
-		input_report_switch(kpad->input,
-				    kpad->gpimap[i].sw_evt,
-				    !(gpi_stat_tmp & (1 << pin_loc)));
-	}
-
-	input_sync(kpad->input);
-}
-
-
 static int adp5588_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -469,37 +509,6 @@ static int adp5588_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	if (!pdata->gpimap && pdata->gpimapsize) {
-		dev_err(&client->dev, "invalid gpimap from pdata\n");
-		return -EINVAL;
-	}
-
-	if (pdata->gpimapsize > ADP5588_GPIMAPSIZE_MAX) {
-		dev_err(&client->dev, "invalid gpimapsize\n");
-		return -EINVAL;
-	}
-
-	for (i = 0; i < pdata->gpimapsize; i++) {
-		unsigned short pin = pdata->gpimap[i].pin;
-
-		if (pin < GPI_PIN_BASE || pin > GPI_PIN_END) {
-			dev_err(&client->dev, "invalid gpi pin data\n");
-			return -EINVAL;
-		}
-
-		if (pin <= GPI_PIN_ROW_END) {
-			if (pin - GPI_PIN_ROW_BASE + 1 <= pdata->rows) {
-				dev_err(&client->dev, "invalid gpi row data\n");
-				return -EINVAL;
-			}
-		} else {
-			if (pin - GPI_PIN_COL_BASE + 1 <= pdata->cols) {
-				dev_err(&client->dev, "invalid gpi col data\n");
-				return -EINVAL;
-			}
-		}
-	}
-
 	if (!client->irq) {
 		dev_err(&client->dev, "no IRQ?\n");
 		return -EINVAL;
@@ -541,9 +550,6 @@ static int adp5588_probe(struct i2c_client *client,
 	memcpy(kpad->keycode, pdata->keymap,
 		pdata->keymapsize * input->keycodesize);
 
-	kpad->gpimap = pdata->gpimap;
-	kpad->gpimapsize = pdata->gpimapsize;
-
 	/* setup input device */
 	__set_bit(EV_KEY, input->evbit);
 
@@ -555,11 +561,6 @@ static int adp5588_probe(struct i2c_client *client,
 			__set_bit(kpad->keycode[i], input->keybit);
 	__clear_bit(KEY_RESERVED, input->keybit);
 
-	if (kpad->gpimapsize)
-		__set_bit(EV_SW, input->evbit);
-	for (i = 0; i < kpad->gpimapsize; i++)
-		__set_bit(kpad->gpimap[i].sw_evt, input->swbit);
-
 	error = input_register_device(input);
 	if (error) {
 		dev_err(&client->dev, "unable to register input device: %d\n",
@@ -567,6 +568,14 @@ static int adp5588_probe(struct i2c_client *client,
 		return error;
 	}
 
+	error = adp5588_setup(client);
+	if (error)
+		return error;
+
+	error = adp5588_gpio_add(kpad);
+	if (error)
+		return error;
+
 	error = devm_request_threaded_irq(&client->dev, client->irq,
 					  adp5588_hard_irq, adp5588_thread_irq,
 					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
@@ -577,17 +586,6 @@ static int adp5588_probe(struct i2c_client *client,
 		return error;
 	}
 
-	error = adp5588_setup(client);
-	if (error)
-		return error;
-
-	if (kpad->gpimapsize)
-		adp5588_report_switch_state(kpad);
-
-	error = adp5588_gpio_add(kpad);
-	if (error)
-		return error;
-
 	dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
 	return 0;
 }
diff --git a/include/linux/platform_data/adp5588.h b/include/linux/platform_data/adp5588.h
index 6d3f7d911a92..82170ec8c266 100644
--- a/include/linux/platform_data/adp5588.h
+++ b/include/linux/platform_data/adp5588.h
@@ -147,8 +147,6 @@ struct adp5588_kpad_platform_data {
 	unsigned en_keylock:1;		/* Enable Key Lock feature */
 	unsigned short unlock_key1;	/* Unlock Key 1 */
 	unsigned short unlock_key2;	/* Unlock Key 2 */
-	const struct adp5588_gpi_map *gpimap;
-	unsigned short gpimapsize;
 	const struct adp5588_gpio_platform_data *gpio_data;
 };
 
-- 
2.37.0

Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote:
>
> This change replaces the support for GPIs as key event generators.
> Instead of reporting the events directly, we add a gpio based irqchip
> so that these events can be consumed by keys defined in the gpio-keys
> driver (as it's goal is indeed for keys on GPIOs capable of generating
> interrupts). With this, the gpio-adp5588 driver can also be dropped.
>
> The basic idea is that all the pins that are not being used as part of
> the keymap matrix can be possibly requested as GPIOs by gpio-keys
> (it's also fine to use these pins as plain interrupts though that's not
> really the point).
>
> Since the gpiochip now also has irqchip capabilities, we should only
> remove it after we free the device interrupt (otherwise we could, in
> theory, be handling GPIs interrupts while the gpiochip is concurrently
> removed). Thus the call 'adp5588_gpio_add()' is moved and since the
> setup phase also needs to come before making the gpios visible, we also
> need to move 'adp5588_setup()'.
>
> While at it, always select GPIOLIB so that we don't need to use #ifdef
> guards.

...

>         u8 dat_out[3];
>         u8 dir[3];

> +       u8 int_en[3];
> +       u8 irq_mask[3];

Wondering why these can't be converted to natural bitmaps. (See
gpio-pca953x as an example).

...

> +static void adp5588_irq_mask(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct adp5588_kpad *kpad = gpiochip_get_data(gc);
> +       unsigned long real_irq = kpad->gpiomap[d->hwirq];
> +
> +       kpad->irq_mask[ADP5588_BANK(real_irq)] &= ~ADP5588_BIT(real_irq);
> +}
> +
> +static void adp5588_irq_unmask(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct adp5588_kpad *kpad = gpiochip_get_data(gc);
> +       unsigned long real_irq = kpad->gpiomap[d->hwirq];
> +
> +       kpad->irq_mask[ADP5588_BANK(real_irq)] |= ADP5588_BIT(real_irq);
> +}

Please follow the suitable example from here:
https://www.kernel.org/doc/html/latest/driver-api/gpio/driver.html#infrastructure-helpers-for-gpio-irqchips

...

> +       kpad->gc.parent = &kpad->client->dev;

> +       kpad->gc.of_node = kpad->client->dev.of_node;

We are going to remove of_node from GPIO. Moreover the parent device
and its node is a duplication, just drop the latter and GPIO library
will take care of it.

...

> +       irq_chip->name = "adp5588";
> +       irq_chip->irq_mask = adp5588_irq_mask;
> +       irq_chip->irq_unmask = adp5588_irq_unmask;
> +       irq_chip->irq_bus_lock = adp5588_irq_bus_lock;
> +       irq_chip->irq_bus_sync_unlock = adp5588_irq_bus_sync_unlock;
> +       irq_chip->irq_set_type = adp5588_irq_set_type;
> +       irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
> +       girq = &kpad->gc.irq;
> +       girq->chip = irq_chip;

> +       girq->handler = handle_simple_irq;

By default it should be handle_bad_irq() and locked in the ->irq_set_type().

> +       girq->threaded = true;

See documentation above.

...

> +static int adp5588_gpiomap_get_hwirq(const u8 *map, unsigned int gpio,
> +                                    unsigned int ngpios)
>  {
> -       return 0;
> +       unsigned int hwirq;
> +
> +       for (hwirq = 0; hwirq < ngpios; hwirq++)
> +               if (map[hwirq] == gpio)
> +                       return hwirq;

> +       /* should never happen */

Then why it's here?

> +       WARN_ON_ONCE(hwirq == ngpios);
> +
> +       return -ENOENT;
> +}

I don't know this code, can you summarize why this additional mapping is needed?

...

> +static void adp5588_gpio_irq_handle(struct adp5588_kpad *kpad, int key_val,
> +                                   int key_press)
> +{
> +       unsigned int irq, gpio = key_val - GPI_PIN_BASE, irq_type, hwirq;
> +       struct i2c_client *client = kpad->client;
> +       struct irq_data *desc;
> +
> +       hwirq = adp5588_gpiomap_get_hwirq(kpad->gpiomap, gpio, kpad->gc.ngpio);
> +       if (hwirq < 0) {
> +               dev_err(&client->dev, "Could not get hwirq for key(%u)\n", key_val);
> +               return;
> +       }
> +
> +       irq = irq_find_mapping(kpad->gc.irq.domain, hwirq);
> +       if (irq <= 0)
> +               return;
> +
> +       desc = irq_get_irq_data(irq);
> +       if (!desc) {
> +               dev_err(&client->dev, "Could not get irq(%u) data\n", irq);
> +               return;
> +       }
> +
> +       irq_type = irqd_get_trigger_type(desc);
> +
> +       /*
> +        * Default is active low which means key_press is asserted on
> +        * the falling edge.
> +        */
> +       if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press) ||
> +           (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))

This is dup from ->irq_set_type(). Or how this can be not like this?

> +               handle_nested_irq(irq);

There is new helpers I believe for getting domain and handle an IRQ.
Grep for the recent (one-two last cycles) changes in the GPIO drivers.

>  }

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, July 8, 2022 4:18 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO
> SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux-
> input@vger.kernel.org>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>; Bartosz Golaszewski
> <brgl@bgdev.pl>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>
> Subject: Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi
> key events as 'gpio keys'
> 
> [External]
> 
> On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > This change replaces the support for GPIs as key event generators.
> > Instead of reporting the events directly, we add a gpio based irqchip
> > so that these events can be consumed by keys defined in the gpio-
> keys
> > driver (as it's goal is indeed for keys on GPIOs capable of generating
> > interrupts). With this, the gpio-adp5588 driver can also be dropped.
> >
> > The basic idea is that all the pins that are not being used as part of
> > the keymap matrix can be possibly requested as GPIOs by gpio-keys
> > (it's also fine to use these pins as plain interrupts though that's not
> > really the point).
> >
> > Since the gpiochip now also has irqchip capabilities, we should only
> > remove it after we free the device interrupt (otherwise we could, in
> > theory, be handling GPIs interrupts while the gpiochip is concurrently
> > removed). Thus the call 'adp5588_gpio_add()' is moved and since the
> > setup phase also needs to come before making the gpios visible, we
> also
> > need to move 'adp5588_setup()'.
> >
> > While at it, always select GPIOLIB so that we don't need to use #ifdef
> > guards.
> 
> ...
> 
> >         u8 dat_out[3];
> >         u8 dir[3];
> 
> > +       u8 int_en[3];
> > +       u8 irq_mask[3];
> 
> Wondering why these can't be converted to natural bitmaps. (See
> gpio-pca953x as an example).
>

I kind of replied to this in a previous email (to you). It naturally can,
but I would rather prefer to do that in another series... I could have
done it here but it would not be consistent with the rest of the
driver.

> ...
> 
> > +static void adp5588_irq_mask(struct irq_data *d)
> > +{
> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +       struct adp5588_kpad *kpad = gpiochip_get_data(gc);
> > +       unsigned long real_irq = kpad->gpiomap[d->hwirq];
> > +
> > +       kpad->irq_mask[ADP5588_BANK(real_irq)] &=
> ~ADP5588_BIT(real_irq);
> > +}
> > +
> > +static void adp5588_irq_unmask(struct irq_data *d)
> > +{
> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +       struct adp5588_kpad *kpad = gpiochip_get_data(gc);
> > +       unsigned long real_irq = kpad->gpiomap[d->hwirq];
> > +
> > +       kpad->irq_mask[ADP5588_BANK(real_irq)] |=
> ADP5588_BIT(real_irq);
> > +}
> 
> Please follow the suitable example from here:
> https://urldefense.com/v3/__https://www.kernel.org/doc/html/lates
> t/driver-api/gpio/driver.html*infrastructure-helpers-for-gpio-
> irqchips__;Iw!!A3Ni8CS0y2Y!4eMLD5XT2REpOPWl6B9IxYxEgbMfxiW87
> XJu-4KmjeVywSLrobIZqEZpcVJ0dBNbZDGPBpHXvx3xP-HzGS4jIwvu$
> 
> ...
> 
> > +       kpad->gc.parent = &kpad->client->dev;
> 
> > +       kpad->gc.of_node = kpad->client->dev.of_node;
> 
> We are going to remove of_node from GPIO. Moreover the parent
> device
> and its node is a duplication, just drop the latter and GPIO library
> will take care of it.
> 

Well the of_node was set so that I had a proper name in the IRQ domain
IIRC. Will this be handled in the GPIO lib in the future?

The parent assignment was also to make things neater in
/sys/kernel/debug/gpio.

> ...
> 
> > +       irq_chip->name = "adp5588";
> > +       irq_chip->irq_mask = adp5588_irq_mask;
> > +       irq_chip->irq_unmask = adp5588_irq_unmask;
> > +       irq_chip->irq_bus_lock = adp5588_irq_bus_lock;
> > +       irq_chip->irq_bus_sync_unlock =
> adp5588_irq_bus_sync_unlock;
> > +       irq_chip->irq_set_type = adp5588_irq_set_type;
> > +       irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
> > +       girq = &kpad->gc.irq;
> > +       girq->chip = irq_chip;
> 
> > +       girq->handler = handle_simple_irq;
> 
> By default it should be handle_bad_irq() and locked in the -
> >irq_set_type().
> 
> > +       girq->threaded = true;
> 
> See documentation above.

I see... I will look at Docs. In practice I don't think this matters much
since this handler should never really be called (I think) as we just
call handle_nested_irq().

> ...
> 
> > +static int adp5588_gpiomap_get_hwirq(const u8 *map, unsigned int
> gpio,
> > +                                    unsigned int ngpios)
> >  {
> > -       return 0;
> > +       unsigned int hwirq;
> > +
> > +       for (hwirq = 0; hwirq < ngpios; hwirq++)
> > +               if (map[hwirq] == gpio)
> > +                       return hwirq;
> 
> > +       /* should never happen */
> 
> Then why it's here?

because I do not trust the HW to always cooperate :). In theory,
we can get some invalid 'gpio' from it.

> > +       WARN_ON_ONCE(hwirq == ngpios);
> > +
> > +       return -ENOENT;
> > +}
> 
> I don't know this code, can you summarize why this additional mapping
> is needed?
> 

You have 18 possible pins to use as GPIOs (and hence be IRQ sources). Now,
if you just want to use pins 16 and 17 that will map int hwirq 0 and 1. But
what we get from the device in 'key_val - GPI_PIN_BASE' is, for example,
16 and so we need to get the hwirq which will be 0. It's pretty much the
reverse of what it's being done in the GPIOs callbacks.

> ...
> 
> > +static void adp5588_gpio_irq_handle(struct adp5588_kpad *kpad,
> int key_val,
> > +                                   int key_press)
> > +{
> > +       unsigned int irq, gpio = key_val - GPI_PIN_BASE, irq_type,
> hwirq;
> > +       struct i2c_client *client = kpad->client;
> > +       struct irq_data *desc;
> > +
> > +       hwirq = adp5588_gpiomap_get_hwirq(kpad->gpiomap, gpio,
> kpad->gc.ngpio);
> > +       if (hwirq < 0) {
> > +               dev_err(&client->dev, "Could not get hwirq for key(%u)\n",
> key_val);
> > +               return;
> > +       }
> > +
> > +       irq = irq_find_mapping(kpad->gc.irq.domain, hwirq);
> > +       if (irq <= 0)
> > +               return;
> > +
> > +       desc = irq_get_irq_data(irq);
> > +       if (!desc) {
> > +               dev_err(&client->dev, "Could not get irq(%u) data\n", irq);
> > +               return;
> > +       }
> > +
> > +       irq_type = irqd_get_trigger_type(desc);
> > +
> > +       /*
> > +        * Default is active low which means key_press is asserted on
> > +        * the falling edge.
> > +        */
> > +       if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press) ||
> > +           (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))
> 
> This is dup from ->irq_set_type(). Or how this can be not like this?

We get here if we get a key press (falling edge) or a key release (rising
edge). The events are given by the device and it might be that in some
cases we just want to handle/propagate key presses
(not sure if it makes sense). So we need to match it with the
appropriate irq_type requested. Note that this kind of controlling the IRQ
from SW as there's no way from doing it in the device. That is why we don't
do more than just making sure the IRQ types are valid in irq_set_type.

> > +               handle_nested_irq(irq);
> 
> There is new helpers I believe for getting domain and handle an IRQ.
> Grep for the recent (one-two last cycles) changes in the GPIO drivers.
> 

Hmm, I think I saw it but somehow I though I could not use it (because
of the previous calls to get the irq_type). Hmmm...

- Nuno Sá

Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 4:55 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Friday, July 8, 2022 4:18 PM
> > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > > +       kpad->gc.parent = &kpad->client->dev;
> >
> > > +       kpad->gc.of_node = kpad->client->dev.of_node;
> >
> > We are going to remove of_node from GPIO. Moreover the parent
> > device
> > and its node is a duplication, just drop the latter and GPIO library
> > will take care of it.
> >
>
> Well the of_node was set so that I had a proper name in the IRQ domain
> IIRC. Will this be handled in the GPIO lib in the future?

In your case it's a dup. So in _your_ case it will be handled in the
future. For the rest we already have an fwnode member.

> The parent assignment was also to make things neater in
> /sys/kernel/debug/gpio.

Sure.

...

> > > +       girq->handler = handle_simple_irq;
> >
> > By default it should be handle_bad_irq() and locked in the -
> > >irq_set_type().
> >
> > > +       girq->threaded = true;
> >
> > See documentation above.
>
> I see... I will look at Docs. In practice I don't think this matters much
> since this handler should never really be called (I think) as we just
> call handle_nested_irq().

There are two different comments, one about handler, another about how
to properly write IRQ chip data structure and mask()/unmask()
callbacks.

...

> > > +       /* should never happen */
> >
> > Then why it's here?
>
> because I do not trust the HW to always cooperate :). In theory,
> we can get some invalid 'gpio' from it.
>
> > > +       WARN_ON_ONCE(hwirq == ngpios);

On some setups this can lead to panic. Why? Is this so critical error?
hardware can't anymore function?

...

> > I don't know this code, can you summarize why this additional mapping
> > is needed?
>
> You have 18 possible pins to use as GPIOs (and hence be IRQ sources). Now,
> if you just want to use pins 16 and 17 that will map int hwirq 0 and 1. But
> what we get from the device in 'key_val - GPI_PIN_BASE' is, for example,
> 16 and so we need to get the hwirq which will be 0. It's pretty much the
> reverse of what it's being done in the GPIOs callbacks.

Any reason why irq_valid_mask can't be used for that?

...

> > > +       /*
> > > +        * Default is active low which means key_press is asserted on
> > > +        * the falling edge.
> > > +        */
> > > +       if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press) ||
> > > +           (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))
> >
> > This is dup from ->irq_set_type(). Or how this can be not like this?
>
> We get here if we get a key press (falling edge) or a key release (rising
> edge). The events are given by the device and it might be that in some
> cases we just want to handle/propagate key presses
> (not sure if it makes sense). So we need to match it with the
> appropriate irq_type requested. Note that this kind of controlling the IRQ
> from SW as there's no way from doing it in the device. That is why we don't
> do more than just making sure the IRQ types are valid in irq_set_type.

I see, thanks for elaboration.

...

> > > +               handle_nested_irq(irq);
> >
> > There is new helpers I believe for getting domain and handle an IRQ.
> > Grep for the recent (one-two last cycles) changes in the GPIO drivers.
> >
>
> Hmm, I think I saw it but somehow I though I could not use it (because
> of the previous calls to get the irq_type). Hmmm...

Maybe you can double check?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, July 8, 2022 5:05 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO
> SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux-
> input@vger.kernel.org>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>; Bartosz Golaszewski
> <brgl@bgdev.pl>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>
> Subject: Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi
> key events as 'gpio keys'
> 
> [External]
> 
> On Fri, Jul 8, 2022 at 4:55 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Friday, July 8, 2022 4:18 PM
> > > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com>
> wrote:
> 
> ...
> 
> > > > +       kpad->gc.parent = &kpad->client->dev;
> > >
> > > > +       kpad->gc.of_node = kpad->client->dev.of_node;
> > >
> > > We are going to remove of_node from GPIO. Moreover the parent
> > > device
> > > and its node is a duplication, just drop the latter and GPIO library
> > > will take care of it.
> > >
> >
> > Well the of_node was set so that I had a proper name in the IRQ
> domain
> > IIRC. Will this be handled in the GPIO lib in the future?
> 
> In your case it's a dup. So in _your_ case it will be handled in the
> future. For the rest we already have an fwnode member.

ok, I will drop the assignment...

> > The parent assignment was also to make things neater in
> > /sys/kernel/debug/gpio.
> 
> Sure.
> 
> ...
> 
> > > > +       girq->handler = handle_simple_irq;
> > >
> > > By default it should be handle_bad_irq() and locked in the -
> > > >irq_set_type().
> > >
> > > > +       girq->threaded = true;
> > >
> > > See documentation above.
> >
> > I see... I will look at Docs. In practice I don't think this matters much
> > since this handler should never really be called (I think) as we just
> > call handle_nested_irq().
> 
> There are two different comments, one about handler, another about
> how
> to properly write IRQ chip data structure and mask()/unmask()
> callbacks.
> 
> ...
> 
> > > > +       /* should never happen */
> > >
> > > Then why it's here?
> >
> > because I do not trust the HW to always cooperate :). In theory,
> > we can get some invalid 'gpio' from it.
> >
> > > > +       WARN_ON_ONCE(hwirq == ngpios);
> 
> On some setups this can lead to panic. Why? Is this so critical error?

Ahh, you're right. Forgot that in some cases WARN can actually panic.

> hardware can't anymore function?

If we get in here, the device is probably in a very bad state but that
does not mean that the system is...

I will just move to dev_warn(). Thanks for the remainder!

> ...
> 
> > > I don't know this code, can you summarize why this additional
> mapping
> > > is needed?
> >
> > You have 18 possible pins to use as GPIOs (and hence be IRQ
> sources). Now,
> > if you just want to use pins 16 and 17 that will map int hwirq 0 and 1.
> But
> > what we get from the device in 'key_val - GPI_PIN_BASE' is, for
> example,
> > 16 and so we need to get the hwirq which will be 0. It's pretty much
> the
> > reverse of what it's being done in the GPIOs callbacks.
> 
> Any reason why irq_valid_mask can't be used for that?

I will have to look at irq_valid_mask.

> ...
> 
> > > > +       /*
> > > > +        * Default is active low which means key_press is asserted
> on
> > > > +        * the falling edge.
> > > > +        */
> > > > +       if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press) ||
> > > > +           (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))
> > >
> > > This is dup from ->irq_set_type(). Or how this can be not like this?
> >
> > We get here if we get a key press (falling edge) or a key release
> (rising
> > edge). The events are given by the device and it might be that in
> some
> > cases we just want to handle/propagate key presses
> > (not sure if it makes sense). So we need to match it with the
> > appropriate irq_type requested. Note that this kind of controlling the
> IRQ
> > from SW as there's no way from doing it in the device. That is why we
> don't
> > do more than just making sure the IRQ types are valid in
> irq_set_type.
> 
> I see, thanks for elaboration.
> 
> ...
> 
> > > > +               handle_nested_irq(irq);
> > >
> > > There is new helpers I believe for getting domain and handle an
> IRQ.
> > > Grep for the recent (one-two last cycles) changes in the GPIO
> drivers.
> > >
> >
> > Hmm, I think I saw it but somehow I though I could not use it
> (because
> > of the previous calls to get the irq_type). Hmmm...
> 
> Maybe you can double check?

Sure, I think the helper can be used...

- Nuno Sá

Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[Nuno Sá]

On Fri, 2022-07-08 at 15:24 +0000, Sa, Nuno wrote:
> 
> 
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Friday, July 8, 2022 5:05 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO
> > SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux-
> > input@vger.kernel.org>; Dmitry Torokhov
> > <dmitry.torokhov@gmail.com>; Bartosz Golaszewski
> > <brgl@bgdev.pl>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Rob Herring
> > <robh+dt@kernel.org>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij
> > <linus.walleij@linaro.org>
> > Subject: Re: [PATCH 01/10] input: keyboard: adp5588-keys: support
> > gpi
> > key events as 'gpio keys'
> > 
> > [External]
> > 
> > On Fri, Jul 8, 2022 at 4:55 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Sent: Friday, July 8, 2022 4:18 PM
> > > > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com>
> > wrote:
> > 
> > ...
> > 
> > > > > +       kpad->gc.parent = &kpad->client->dev;
> > > > 
> > > > > +       kpad->gc.of_node = kpad->client->dev.of_node;
> > > > 
> > > > We are going to remove of_node from GPIO. Moreover the parent
> > > > device
> > > > and its node is a duplication, just drop the latter and GPIO
> > > > library
> > > > will take care of it.
> > > > 
> > > 
> > > Well the of_node was set so that I had a proper name in the IRQ
> > domain
> > > IIRC. Will this be handled in the GPIO lib in the future?
> > 
> > In your case it's a dup. So in _your_ case it will be handled in
> > the
> > future. For the rest we already have an fwnode member.
> 
> ok, I will drop the assignment...
> 
> > > The parent assignment was also to make things neater in
> > > /sys/kernel/debug/gpio.
> > 
> > Sure.
> > 
> > ...
> > 
> > > > > +       girq->handler = handle_simple_irq;
> > > > 
> > > > By default it should be handle_bad_irq() and locked in the -
> > > > > irq_set_type().
> > > > 
> > > > > +       girq->threaded = true;
> > > > 
> > > > See documentation above.
> > > 
> > > I see... I will look at Docs. In practice I don't think this
> > > matters much
> > > since this handler should never really be called (I think) as we
> > > just
> > > call handle_nested_irq().
> > 
> > There are two different comments, one about handler, another about
> > how
> > to properly write IRQ chip data structure and mask()/unmask()
> > callbacks.
> > 

So I think I have most of stuff understood for v2. About the handler, I
don't think we really need to set 'handle_edge_irq()' in
'irq_set_type()' as this is a nested threaded interrupt and so, the
desc->handle_irq() should never be called (I think, not 100% if there's
any strange case where it might).

That said, if you still think that I should do it (for correctness),
fine by me.

> > ...
> > 
> > > > > +       /* should never happen */
> > > > 
> > > > Then why it's here?
> > > 
> > > because I do not trust the HW to always cooperate :). In theory,
> > > we can get some invalid 'gpio' from it.
> > > 
> > > > > +       WARN_ON_ONCE(hwirq == ngpios);
> > 
> > On some setups this can lead to panic. Why? Is this so critical
> > error?
> 
> Ahh, you're right. Forgot that in some cases WARN can actually panic.
> 
> > hardware can't anymore function?
> 
> If we get in here, the device is probably in a very bad state but
> that
> does not mean that the system is...
> 
> I will just move to dev_warn(). Thanks for the remainder!
> 
> > ...
> > 
> > > > I don't know this code, can you summarize why this additional
> > mapping
> > > > is needed?
> > > 
> > > You have 18 possible pins to use as GPIOs (and hence be IRQ
> > sources). Now,
> > > if you just want to use pins 16 and 17 that will map int hwirq 0
> > > and 1.
> > But
> > > what we get from the device in 'key_val - GPI_PIN_BASE' is, for
> > example,
> > > 16 and so we need to get the hwirq which will be 0. It's pretty
> > > much
> > the
> > > reverse of what it's being done in the GPIOs callbacks.
> > 
> > Any reason why irq_valid_mask can't be used for that?
> 
> I will have to look at irq_valid_mask.
> 
> > ...
> > 
> > > > > +       /*
> > > > > +        * Default is active low which means key_press is
> > > > > asserted
> > on
> > > > > +        * the falling edge.
> > > > > +        */
> > > > > +       if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press)
> > > > > ||
> > > > > +           (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))
> > > > 
> > > > This is dup from ->irq_set_type(). Or how this can be not like
> > > > this?
> > > 
> > > We get here if we get a key press (falling edge) or a key release
> > (rising
> > > edge). The events are given by the device and it might be that in
> > some
> > > cases we just want to handle/propagate key presses
> > > (not sure if it makes sense). So we need to match it with the
> > > appropriate irq_type requested. Note that this kind of
> > > controlling the
> > IRQ
> > > from SW as there's no way from doing it in the device. That is
> > > why we
> > don't
> > > do more than just making sure the IRQ types are valid in
> > irq_set_type.
> > 
> > I see, thanks for elaboration.
> > 
> > ...
> > 
> > > > > +               handle_nested_irq(irq);
> > > > 
> > > > There is new helpers I believe for getting domain and handle an
> > IRQ.
> > > > Grep for the recent (one-two last cycles) changes in the GPIO
> > drivers.
> > > > 
> > > 
> > > Hmm, I think I saw it but somehow I though I could not use it
> > (because
> > > of the previous calls to get the irq_type). Hmmm...
> > 
> > Maybe you can double check?
> 
> Sure, I think the helper can be used...
> 

So I did looked and I think you are thinking about
'generic_handle_domain_irq()'. For nested threaded I could not find a
similar one (maybe a new helper to be added).

- Nuno Sá

Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[kernel test robot]

Hi "Nuno,

I love your patch! Yet something to improve:

[auto build test ERROR on dtor-input/next]
[also build test ERROR on next-20220708]
[cannot apply to brgl/gpio/for-next hid/for-next linus/master v5.19-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220708-173730
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: x86_64-randconfig-a013 (https://download.01.org/0day-ci/archive/20220709/202207091223.nBzeL6dk-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/64267ff775fd4b945fb916a10187be1c15faa165
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220708-173730
        git checkout 64267ff775fd4b945fb916a10187be1c15faa165
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/input/keyboard/adp5588-keys.c: In function 'adp5588_gpio_add':
>> drivers/input/keyboard/adp5588-keys.c:263:18: error: 'struct gpio_chip' has no member named 'of_node'; did you mean 'fwnode'?
     263 |         kpad->gc.of_node = kpad->client->dev.of_node;
         |                  ^~~~~~~
         |                  fwnode


vim +263 drivers/input/keyboard/adp5588-keys.c

   243	
   244	static int adp5588_gpio_add(struct adp5588_kpad *kpad)
   245	{
   246		struct irq_chip *irq_chip = &kpad->irq_chip;
   247		struct device *dev = &kpad->client->dev;
   248		const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
   249		const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
   250		struct gpio_irq_chip *girq;
   251		int i, error;
   252	
   253		if (!gpio_data)
   254			return 0;
   255	
   256		kpad->gc.ngpio = adp5588_build_gpiomap(kpad, pdata);
   257		if (kpad->gc.ngpio == 0) {
   258			dev_info(dev, "No unused gpios left to export\n");
   259			return 0;
   260		}
   261	
   262		kpad->gc.parent = &kpad->client->dev;
 > 263		kpad->gc.of_node = kpad->client->dev.of_node;
   264		kpad->gc.direction_input = adp5588_gpio_direction_input;
   265		kpad->gc.direction_output = adp5588_gpio_direction_output;
   266		kpad->gc.get = adp5588_gpio_get_value;
   267		kpad->gc.set = adp5588_gpio_set_value;
   268		kpad->gc.can_sleep = 1;
   269	
   270		kpad->gc.base = gpio_data->gpio_start;
   271		kpad->gc.label = kpad->client->name;
   272		kpad->gc.owner = THIS_MODULE;
   273		kpad->gc.names = gpio_data->names;
   274	
   275		irq_chip->name = "adp5588";
   276		irq_chip->irq_mask = adp5588_irq_mask;
   277		irq_chip->irq_unmask = adp5588_irq_unmask;
   278		irq_chip->irq_bus_lock = adp5588_irq_bus_lock;
   279		irq_chip->irq_bus_sync_unlock = adp5588_irq_bus_sync_unlock;
   280		irq_chip->irq_set_type = adp5588_irq_set_type;
   281		irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE;
   282		girq = &kpad->gc.irq;
   283		girq->chip = irq_chip;
   284		girq->handler = handle_simple_irq;
   285		girq->threaded = true;
   286	
   287		mutex_init(&kpad->gpio_lock);
   288	
   289		error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
   290		if (error) {
   291			dev_err(dev, "gpiochip_add failed: %d\n", error);
   292			return error;
   293		}
   294	
   295		for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
   296			kpad->dat_out[i] = adp5588_read(kpad->client,
   297							GPIO_DAT_OUT1 + i);
   298			kpad->dir[i] = adp5588_read(kpad->client, GPIO_DIR1 + i);
   299		}
   300	
   301		if (gpio_data->setup) {
   302			error = gpio_data->setup(kpad->client,
   303						 kpad->gc.base, kpad->gc.ngpio,
   304						 gpio_data->context);
   305			if (error)
   306				dev_warn(dev, "setup failed: %d\n", error);
   307		}
   308	
   309		if (gpio_data->teardown) {
   310			error = devm_add_action(dev, adp5588_gpio_do_teardown, kpad);
   311			if (error)
   312				dev_warn(dev, "failed to schedule teardown: %d\n",
   313					 error);
   314		}
   315	
   316		return 0;
   317	}
   318	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[Andy Shevchenko]

On Sat, Jul 9, 2022 at 6:22 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi "Nuno,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on dtor-input/next]
> [also build test ERROR on next-20220708]
> [cannot apply to brgl/gpio/for-next hid/for-next linus/master v5.19-rc5]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220708-173730
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
> config: x86_64-randconfig-a013 (https://download.01.org/0day-ci/archive/20220709/202207091223.nBzeL6dk-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
> reproduce (this is a W=1 build):
>         # https://github.com/intel-lab-lkp/linux/commit/64267ff775fd4b945fb916a10187be1c15faa165
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220708-173730
>         git checkout 64267ff775fd4b945fb916a10187be1c15faa165
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>    drivers/input/keyboard/adp5588-keys.c: In function 'adp5588_gpio_add':
> >> drivers/input/keyboard/adp5588-keys.c:263:18: error: 'struct gpio_chip' has no member named 'of_node'; did you mean 'fwnode'?
>      263 |         kpad->gc.of_node = kpad->client->dev.of_node;
>          |                  ^~~~~~~
>          |                  fwnode

Yes, exactly the point why of_node is bad to have. In legacy code like
this you need to guard access to it with #ifdef CONFIG_OF_GPIO IIRC.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]

On Sat, Jul 9, 2022 at 6:22 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi "Nuno,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on dtor-input/next]
> [also build test ERROR on next-20220708]
> [cannot apply to brgl/gpio/for-next hid/for-next linus/master v5.19-rc5]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220708-173730
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
> config: x86_64-randconfig-a013 (https://download.01.org/0day-ci/archive/20220709/202207091223.nBzeL6dk-lkp(a)intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
> reproduce (this is a W=1 build):
>         # https://github.com/intel-lab-lkp/linux/commit/64267ff775fd4b945fb916a10187be1c15faa165
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220708-173730
>         git checkout 64267ff775fd4b945fb916a10187be1c15faa165
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>    drivers/input/keyboard/adp5588-keys.c: In function 'adp5588_gpio_add':
> >> drivers/input/keyboard/adp5588-keys.c:263:18: error: 'struct gpio_chip' has no member named 'of_node'; did you mean 'fwnode'?
>      263 |         kpad->gc.of_node = kpad->client->dev.of_node;
>          |                  ^~~~~~~
>          |                  fwnode

Yes, exactly the point why of_node is bad to have. In legacy code like
this you need to guard access to it with #ifdef CONFIG_OF_GPIO IIRC.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[kernel test robot]

Hi "Nuno,

I love your patch! Perhaps something to improve:

[auto build test WARNING on dtor-input/next]
[also build test WARNING on next-20220711]
[cannot apply to brgl/gpio/for-next hid/for-next linus/master v5.19-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220708-173730
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20220712/202207121357.JpS5DGdP-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

smatch warnings:
drivers/input/keyboard/adp5588-keys.c:342 adp5588_gpio_irq_handle() warn: unsigned 'hwirq' is never less than zero.

vim +/hwirq +342 drivers/input/keyboard/adp5588-keys.c

   333	
   334	static void adp5588_gpio_irq_handle(struct adp5588_kpad *kpad, int key_val,
   335					    int key_press)
   336	{
   337		unsigned int irq, gpio = key_val - GPI_PIN_BASE, irq_type, hwirq;
   338		struct i2c_client *client = kpad->client;
   339		struct irq_data *desc;
   340	
   341		hwirq = adp5588_gpiomap_get_hwirq(kpad->gpiomap, gpio, kpad->gc.ngpio);
 > 342		if (hwirq < 0) {
   343			dev_err(&client->dev, "Could not get hwirq for key(%u)\n", key_val);
   344			return;
   345		}
   346	
   347		irq = irq_find_mapping(kpad->gc.irq.domain, hwirq);
   348		if (irq <= 0)
   349			return;
   350	
   351		desc = irq_get_irq_data(irq);
   352		if (!desc) {
   353			dev_err(&client->dev, "Could not get irq(%u) data\n", irq);
   354			return;
   355		}
   356	
   357		irq_type = irqd_get_trigger_type(desc);
   358	
   359		/*
   360		 * Default is active low which means key_press is asserted on
   361		 * the falling edge.
   362		 */
   363		if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press) ||
   364		    (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))
   365			handle_nested_irq(irq);
   366	}
   367	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH 02/10] gpio: gpio-adp5588: drop the driver

[Nuno Sá]

With commit 1c18af5f21cc
("input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'"),
the irchip functionality is directly supported in the input driver as
the main goal of these pins is to be used as gpio keys. Hence, this
driver can be removed.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 MAINTAINERS                 |   1 -
 drivers/gpio/Kconfig        |  14 --
 drivers/gpio/Makefile       |   1 -
 drivers/gpio/gpio-adp5588.c | 471 ------------------------------------
 4 files changed, 487 deletions(-)
 delete mode 100644 drivers/gpio/gpio-adp5588.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f468864fd268..41037cfd75fe 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -548,7 +548,6 @@ M:	Michael Hennerich <michael.hennerich@analog.com>
 S:	Supported
 W:	http://wiki.analog.com/ADP5588
 W:	https://ez.analog.com/linux-software-drivers
-F:	drivers/gpio/gpio-adp5588.c
 F:	drivers/input/keyboard/adp5588-keys.c
 
 ADP8860 BACKLIGHT DRIVER (ADP8860/ADP8861/ADP8863)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 45764ec3b2eb..aba99bbe209e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -974,20 +974,6 @@ endmenu
 menu "I2C GPIO expanders"
 	depends on I2C
 
-config GPIO_ADP5588
-	tristate "ADP5588 I2C GPIO expander"
-	help
-	  This option enables support for 18 GPIOs found
-	  on Analog Devices ADP5588 GPIO Expanders.
-
-config GPIO_ADP5588_IRQ
-	bool "Interrupt controller support for ADP5588"
-	depends on GPIO_ADP5588=y
-	select GPIOLIB_IRQCHIP
-	help
-	  Say yes here to enable the adp5588 to be used as an interrupt
-	  controller. It requires the driver to be built in the kernel.
-
 config GPIO_ADNP
 	tristate "Avionic Design N-bit GPIO expander"
 	depends on OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 14352f6dfe8e..225d97015d8f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -25,7 +25,6 @@ obj-$(CONFIG_GPIO_74X164)		+= gpio-74x164.o
 obj-$(CONFIG_GPIO_74XX_MMIO)		+= gpio-74xx-mmio.o
 obj-$(CONFIG_GPIO_ADNP)			+= gpio-adnp.o
 obj-$(CONFIG_GPIO_ADP5520)		+= gpio-adp5520.o
-obj-$(CONFIG_GPIO_ADP5588)		+= gpio-adp5588.o
 obj-$(CONFIG_GPIO_AGGREGATOR)		+= gpio-aggregator.o
 obj-$(CONFIG_GPIO_ALTERA_A10SR)		+= gpio-altera-a10sr.o
 obj-$(CONFIG_GPIO_ALTERA)  		+= gpio-altera.o
diff --git a/drivers/gpio/gpio-adp5588.c b/drivers/gpio/gpio-adp5588.c
deleted file mode 100644
index f1e4ac90e7d3..000000000000
--- a/drivers/gpio/gpio-adp5588.c
+++ /dev/null
@@ -1,471 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * GPIO Chip driver for Analog Devices
- * ADP5588/ADP5587 I/O Expander and QWERTY Keypad Controller
- *
- * Copyright 2009-2010 Analog Devices Inc.
- */
-
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/init.h>
-#include <linux/i2c.h>
-#include <linux/gpio/driver.h>
-#include <linux/interrupt.h>
-#include <linux/irq.h>
-#include <linux/of_device.h>
-
-#include <linux/platform_data/adp5588.h>
-
-#define DRV_NAME	"adp5588-gpio"
-
-/*
- * Early pre 4.0 Silicon required to delay readout by at least 25ms,
- * since the Event Counter Register updated 25ms after the interrupt
- * asserted.
- */
-#define WA_DELAYED_READOUT_REVID(rev)	((rev) < 4)
-
-struct adp5588_gpio {
-	struct i2c_client *client;
-	struct gpio_chip gpio_chip;
-	struct mutex lock;	/* protect cached dir, dat_out */
-	/* protect serialized access to the interrupt controller bus */
-	struct mutex irq_lock;
-	uint8_t dat_out[3];
-	uint8_t dir[3];
-	uint8_t int_lvl_low[3];
-	uint8_t int_lvl_high[3];
-	uint8_t int_en[3];
-	uint8_t irq_mask[3];
-	uint8_t int_input_en[3];
-};
-
-static int adp5588_gpio_read(struct i2c_client *client, u8 reg)
-{
-	int ret = i2c_smbus_read_byte_data(client, reg);
-
-	if (ret < 0)
-		dev_err(&client->dev, "Read Error\n");
-
-	return ret;
-}
-
-static int adp5588_gpio_write(struct i2c_client *client, u8 reg, u8 val)
-{
-	int ret = i2c_smbus_write_byte_data(client, reg, val);
-
-	if (ret < 0)
-		dev_err(&client->dev, "Write Error\n");
-
-	return ret;
-}
-
-static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned off)
-{
-	struct adp5588_gpio *dev = gpiochip_get_data(chip);
-	unsigned bank = ADP5588_BANK(off);
-	unsigned bit = ADP5588_BIT(off);
-	int val;
-
-	mutex_lock(&dev->lock);
-
-	if (dev->dir[bank] & bit)
-		val = dev->dat_out[bank];
-	else
-		val = adp5588_gpio_read(dev->client, GPIO_DAT_STAT1 + bank);
-
-	mutex_unlock(&dev->lock);
-
-	return !!(val & bit);
-}
-
-static void adp5588_gpio_set_value(struct gpio_chip *chip,
-				   unsigned off, int val)
-{
-	unsigned bank, bit;
-	struct adp5588_gpio *dev = gpiochip_get_data(chip);
-
-	bank = ADP5588_BANK(off);
-	bit = ADP5588_BIT(off);
-
-	mutex_lock(&dev->lock);
-	if (val)
-		dev->dat_out[bank] |= bit;
-	else
-		dev->dat_out[bank] &= ~bit;
-
-	adp5588_gpio_write(dev->client, GPIO_DAT_OUT1 + bank,
-			   dev->dat_out[bank]);
-	mutex_unlock(&dev->lock);
-}
-
-static int adp5588_gpio_direction_input(struct gpio_chip *chip, unsigned off)
-{
-	int ret;
-	unsigned bank;
-	struct adp5588_gpio *dev = gpiochip_get_data(chip);
-
-	bank = ADP5588_BANK(off);
-
-	mutex_lock(&dev->lock);
-	dev->dir[bank] &= ~ADP5588_BIT(off);
-	ret = adp5588_gpio_write(dev->client, GPIO_DIR1 + bank, dev->dir[bank]);
-	mutex_unlock(&dev->lock);
-
-	return ret;
-}
-
-static int adp5588_gpio_direction_output(struct gpio_chip *chip,
-					 unsigned off, int val)
-{
-	int ret;
-	unsigned bank, bit;
-	struct adp5588_gpio *dev = gpiochip_get_data(chip);
-
-	bank = ADP5588_BANK(off);
-	bit = ADP5588_BIT(off);
-
-	mutex_lock(&dev->lock);
-	dev->dir[bank] |= bit;
-
-	if (val)
-		dev->dat_out[bank] |= bit;
-	else
-		dev->dat_out[bank] &= ~bit;
-
-	ret = adp5588_gpio_write(dev->client, GPIO_DAT_OUT1 + bank,
-				 dev->dat_out[bank]);
-	ret |= adp5588_gpio_write(dev->client, GPIO_DIR1 + bank,
-				 dev->dir[bank]);
-	mutex_unlock(&dev->lock);
-
-	return ret;
-}
-
-#ifdef CONFIG_GPIO_ADP5588_IRQ
-
-static void adp5588_irq_bus_lock(struct irq_data *d)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct adp5588_gpio *dev = gpiochip_get_data(gc);
-
-	mutex_lock(&dev->irq_lock);
-}
-
- /*
-  * genirq core code can issue chip->mask/unmask from atomic context.
-  * This doesn't work for slow busses where an access needs to sleep.
-  * bus_sync_unlock() is therefore called outside the atomic context,
-  * syncs the current irq mask state with the slow external controller
-  * and unlocks the bus.
-  */
-
-static void adp5588_irq_bus_sync_unlock(struct irq_data *d)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct adp5588_gpio *dev = gpiochip_get_data(gc);
-	int i;
-
-	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
-		if (dev->int_input_en[i]) {
-			mutex_lock(&dev->lock);
-			dev->dir[i] &= ~dev->int_input_en[i];
-			dev->int_input_en[i] = 0;
-			adp5588_gpio_write(dev->client, GPIO_DIR1 + i,
-					   dev->dir[i]);
-			mutex_unlock(&dev->lock);
-		}
-
-		if (dev->int_en[i] ^ dev->irq_mask[i]) {
-			dev->int_en[i] = dev->irq_mask[i];
-			adp5588_gpio_write(dev->client, GPI_EM1 + i,
-					   dev->int_en[i]);
-		}
-	}
-
-	mutex_unlock(&dev->irq_lock);
-}
-
-static void adp5588_irq_mask(struct irq_data *d)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct adp5588_gpio *dev = gpiochip_get_data(gc);
-
-	dev->irq_mask[ADP5588_BANK(d->hwirq)] &= ~ADP5588_BIT(d->hwirq);
-}
-
-static void adp5588_irq_unmask(struct irq_data *d)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct adp5588_gpio *dev = gpiochip_get_data(gc);
-
-	dev->irq_mask[ADP5588_BANK(d->hwirq)] |= ADP5588_BIT(d->hwirq);
-}
-
-static int adp5588_irq_set_type(struct irq_data *d, unsigned int type)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct adp5588_gpio *dev = gpiochip_get_data(gc);
-	uint16_t gpio = d->hwirq;
-	unsigned bank, bit;
-
-	bank = ADP5588_BANK(gpio);
-	bit = ADP5588_BIT(gpio);
-
-	dev->int_lvl_low[bank] &= ~bit;
-	dev->int_lvl_high[bank] &= ~bit;
-
-	if (type & IRQ_TYPE_EDGE_BOTH || type & IRQ_TYPE_LEVEL_HIGH)
-		dev->int_lvl_high[bank] |= bit;
-
-	if (type & IRQ_TYPE_EDGE_BOTH || type & IRQ_TYPE_LEVEL_LOW)
-		dev->int_lvl_low[bank] |= bit;
-
-	dev->int_input_en[bank] |= bit;
-
-	return 0;
-}
-
-static struct irq_chip adp5588_irq_chip = {
-	.name			= "adp5588",
-	.irq_mask		= adp5588_irq_mask,
-	.irq_unmask		= adp5588_irq_unmask,
-	.irq_bus_lock		= adp5588_irq_bus_lock,
-	.irq_bus_sync_unlock	= adp5588_irq_bus_sync_unlock,
-	.irq_set_type		= adp5588_irq_set_type,
-};
-
-static irqreturn_t adp5588_irq_handler(int irq, void *devid)
-{
-	struct adp5588_gpio *dev = devid;
-	int status = adp5588_gpio_read(dev->client, INT_STAT);
-
-	if (status & ADP5588_KE_INT) {
-		int ev_cnt = adp5588_gpio_read(dev->client, KEY_LCK_EC_STAT);
-
-		if (ev_cnt > 0) {
-			int i;
-
-			for (i = 0; i < (ev_cnt & ADP5588_KEC); i++) {
-				int key = adp5588_gpio_read(dev->client,
-							    Key_EVENTA + i);
-				/* GPIN events begin at 97,
-				 * bit 7 indicates logic level
-				 */
-				int gpio = (key & 0x7f) - 97;
-				int lvl = key & (1 << 7);
-				int bank = ADP5588_BANK(gpio);
-				int bit = ADP5588_BIT(gpio);
-
-				if ((lvl && dev->int_lvl_high[bank] & bit) ||
-				    (!lvl && dev->int_lvl_low[bank] & bit))
-					handle_nested_irq(irq_find_mapping(
-					      dev->gpio_chip.irq.domain, gpio));
-			}
-		}
-	}
-
-	adp5588_gpio_write(dev->client, INT_STAT, status); /* Status is W1C */
-
-	return IRQ_HANDLED;
-}
-
-
-static int adp5588_irq_init_hw(struct gpio_chip *gc)
-{
-	struct adp5588_gpio *dev = gpiochip_get_data(gc);
-	/* Enable IRQs after registering chip */
-	adp5588_gpio_write(dev->client, CFG,
-			   ADP5588_AUTO_INC | ADP5588_INT_CFG | ADP5588_KE_IEN);
-
-	return 0;
-}
-
-static int adp5588_irq_setup(struct adp5588_gpio *dev)
-{
-	struct i2c_client *client = dev->client;
-	int ret;
-	struct adp5588_gpio_platform_data *pdata =
-			dev_get_platdata(&client->dev);
-	struct gpio_irq_chip *girq;
-
-	adp5588_gpio_write(client, CFG, ADP5588_AUTO_INC);
-	adp5588_gpio_write(client, INT_STAT, -1); /* status is W1C */
-
-	mutex_init(&dev->irq_lock);
-
-	ret = devm_request_threaded_irq(&client->dev, client->irq,
-					NULL, adp5588_irq_handler, IRQF_ONESHOT
-					| IRQF_TRIGGER_FALLING | IRQF_SHARED,
-					dev_name(&client->dev), dev);
-	if (ret) {
-		dev_err(&client->dev, "failed to request irq %d\n",
-			client->irq);
-		return ret;
-	}
-
-	/* This will be registered in the call to devm_gpiochip_add_data() */
-	girq = &dev->gpio_chip.irq;
-	girq->chip = &adp5588_irq_chip;
-	/* This will let us handle the parent IRQ in the driver */
-	girq->parent_handler = NULL;
-	girq->num_parents = 0;
-	girq->parents = NULL;
-	girq->first = pdata ? pdata->irq_base : 0;
-	girq->default_type = IRQ_TYPE_NONE;
-	girq->handler = handle_simple_irq;
-	girq->init_hw = adp5588_irq_init_hw;
-	girq->threaded = true;
-
-	return 0;
-}
-
-#else
-static int adp5588_irq_setup(struct adp5588_gpio *dev)
-{
-	struct i2c_client *client = dev->client;
-	dev_warn(&client->dev, "interrupt support not compiled in\n");
-
-	return 0;
-}
-
-#endif /* CONFIG_GPIO_ADP5588_IRQ */
-
-static int adp5588_gpio_probe(struct i2c_client *client)
-{
-	struct adp5588_gpio_platform_data *pdata =
-			dev_get_platdata(&client->dev);
-	struct adp5588_gpio *dev;
-	struct gpio_chip *gc;
-	int ret, i, revid;
-	unsigned int pullup_dis_mask = 0;
-
-	if (!i2c_check_functionality(client->adapter,
-					I2C_FUNC_SMBUS_BYTE_DATA)) {
-		dev_err(&client->dev, "SMBUS Byte Data not Supported\n");
-		return -EIO;
-	}
-
-	dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL);
-	if (!dev)
-		return -ENOMEM;
-
-	dev->client = client;
-
-	gc = &dev->gpio_chip;
-	gc->direction_input = adp5588_gpio_direction_input;
-	gc->direction_output = adp5588_gpio_direction_output;
-	gc->get = adp5588_gpio_get_value;
-	gc->set = adp5588_gpio_set_value;
-	gc->can_sleep = true;
-	gc->base = -1;
-	gc->parent = &client->dev;
-
-	if (pdata) {
-		gc->base = pdata->gpio_start;
-		gc->names = pdata->names;
-		pullup_dis_mask = pdata->pullup_dis_mask;
-	}
-
-	gc->ngpio = ADP5588_MAXGPIO;
-	gc->label = client->name;
-	gc->owner = THIS_MODULE;
-
-	mutex_init(&dev->lock);
-
-	ret = adp5588_gpio_read(dev->client, DEV_ID);
-	if (ret < 0)
-		return ret;
-
-	revid = ret & ADP5588_DEVICE_ID_MASK;
-
-	for (i = 0, ret = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
-		dev->dat_out[i] = adp5588_gpio_read(client, GPIO_DAT_OUT1 + i);
-		dev->dir[i] = adp5588_gpio_read(client, GPIO_DIR1 + i);
-		ret |= adp5588_gpio_write(client, KP_GPIO1 + i, 0);
-		ret |= adp5588_gpio_write(client, GPIO_PULL1 + i,
-				(pullup_dis_mask >> (8 * i)) & 0xFF);
-		ret |= adp5588_gpio_write(client, GPIO_INT_EN1 + i, 0);
-		if (ret)
-			return ret;
-	}
-
-	if (client->irq) {
-		if (WA_DELAYED_READOUT_REVID(revid)) {
-			dev_warn(&client->dev, "GPIO int not supported\n");
-		} else {
-			ret = adp5588_irq_setup(dev);
-			if (ret)
-				return ret;
-		}
-	}
-
-	ret = devm_gpiochip_add_data(&client->dev, &dev->gpio_chip, dev);
-	if (ret)
-		return ret;
-
-	if (pdata && pdata->setup) {
-		ret = pdata->setup(client, gc->base, gc->ngpio, pdata->context);
-		if (ret < 0)
-			dev_warn(&client->dev, "setup failed, %d\n", ret);
-	}
-
-	i2c_set_clientdata(client, dev);
-
-	return 0;
-}
-
-static int adp5588_gpio_remove(struct i2c_client *client)
-{
-	struct adp5588_gpio_platform_data *pdata =
-			dev_get_platdata(&client->dev);
-	struct adp5588_gpio *dev = i2c_get_clientdata(client);
-	int ret;
-
-	if (pdata && pdata->teardown) {
-		ret = pdata->teardown(client,
-				      dev->gpio_chip.base, dev->gpio_chip.ngpio,
-				      pdata->context);
-		if (ret < 0) {
-			dev_err(&client->dev, "teardown failed %d\n", ret);
-			return ret;
-		}
-	}
-
-	if (dev->client->irq)
-		free_irq(dev->client->irq, dev);
-
-	return 0;
-}
-
-static const struct i2c_device_id adp5588_gpio_id[] = {
-	{DRV_NAME, 0},
-	{}
-};
-MODULE_DEVICE_TABLE(i2c, adp5588_gpio_id);
-
-#ifdef CONFIG_OF
-static const struct of_device_id adp5588_gpio_of_id[] = {
-	{ .compatible = "adi," DRV_NAME, },
-	{},
-};
-MODULE_DEVICE_TABLE(of, adp5588_gpio_of_id);
-#endif
-
-static struct i2c_driver adp5588_gpio_driver = {
-	.driver = {
-		.name = DRV_NAME,
-		.of_match_table = of_match_ptr(adp5588_gpio_of_id),
-	},
-	.probe_new = adp5588_gpio_probe,
-	.remove = adp5588_gpio_remove,
-	.id_table = adp5588_gpio_id,
-};
-
-module_i2c_driver(adp5588_gpio_driver);
-
-MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
-MODULE_DESCRIPTION("GPIO ADP5588 Driver");
-MODULE_LICENSE("GPL");
-- 
2.37.0

Re: [PATCH 02/10] gpio: gpio-adp5588: drop the driver

[Bartosz Golaszewski]

On Fri, Jul 8, 2022 at 11:34 AM Nuno Sá <nuno.sa@analog.com> wrote:
>
> With commit 1c18af5f21cc
> ("input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'"),
> the irchip functionality is directly supported in the input driver as
> the main goal of these pins is to be used as gpio keys. Hence, this
> driver can be removed.
>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---

Acked-by: Bartosz Golaszewski <brgl@bgdev.pl>

[PATCH 03/10] input: keyboard: adp5588-keys: bail out on returned error

[Nuno Sá]

Don't continue in code paths after some error is found. It makes no
sense to do any other device configuration if a previous one failed.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 56 ++++++++++++++++++---------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index dad8862f6c76..10f24283d7a3 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -146,9 +146,13 @@ static int adp5588_gpio_direction_output(struct gpio_chip *chip,
 
 	ret = adp5588_write(kpad->client, GPIO_DAT_OUT1 + bank,
 				 kpad->dat_out[bank]);
-	ret |= adp5588_write(kpad->client, GPIO_DIR1 + bank,
+	if (ret)
+		goto out_unlock;
+
+	ret = adp5588_write(kpad->client, GPIO_DIR1 + bank,
 				 kpad->dir[bank]);
 
+out_unlock:
 	mutex_unlock(&kpad->gpio_lock);
 
 	return ret;
@@ -439,42 +443,58 @@ static int adp5588_setup(struct i2c_client *client)
 	int i, ret;
 
 	ret = adp5588_write(client, KP_GPIO1, KP_SEL(pdata->rows));
-	ret |= adp5588_write(client, KP_GPIO2, KP_SEL(pdata->cols) & 0xFF);
-	ret |= adp5588_write(client, KP_GPIO3, KP_SEL(pdata->cols) >> 8);
+	if (ret)
+		return ret;
+
+	ret = adp5588_write(client, KP_GPIO2, KP_SEL(pdata->cols) & 0xFF);
+	if (ret)
+		return ret;
+
+	ret = adp5588_write(client, KP_GPIO3, KP_SEL(pdata->cols) >> 8);
+	if (ret)
+		return ret;
 
 	if (pdata->en_keylock) {
-		ret |= adp5588_write(client, UNLOCK1, pdata->unlock_key1);
-		ret |= adp5588_write(client, UNLOCK2, pdata->unlock_key2);
-		ret |= adp5588_write(client, KEY_LCK_EC_STAT, ADP5588_K_LCK_EN);
+		ret = adp5588_write(client, UNLOCK1, pdata->unlock_key1);
+		if (ret)
+			return ret;
+
+		ret = adp5588_write(client, UNLOCK2, pdata->unlock_key2);
+		if (ret)
+			return ret;
+
+		ret = adp5588_write(client, KEY_LCK_EC_STAT, ADP5588_K_LCK_EN);
+		if (ret)
+			return ret;
 	}
 
-	for (i = 0; i < KEYP_MAX_EVENT; i++)
-		ret |= adp5588_read(client, Key_EVENTA);
+	for (i = 0; i < KEYP_MAX_EVENT; i++) {
+		ret = adp5588_read(client, Key_EVENTA);
+		if (ret)
+			return ret;
+	}
 
 	if (gpio_data) {
 		for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
 			int pull_mask = gpio_data->pullup_dis_mask;
 
-			ret |= adp5588_write(client, GPIO_PULL1 + i,
+			ret = adp5588_write(client, GPIO_PULL1 + i,
 				(pull_mask >> (8 * i)) & 0xFF);
+			if (ret)
+				return ret;
 		}
 	}
 
-	ret |= adp5588_write(client, INT_STAT,
+	ret = adp5588_write(client, INT_STAT,
 				ADP5588_CMP2_INT | ADP5588_CMP1_INT |
 				ADP5588_OVR_FLOW_INT | ADP5588_K_LCK_INT |
 				ADP5588_GPI_INT | ADP5588_KE_INT); /* Status is W1C */
+	if (ret)
+		return ret;
 
-	ret |= adp5588_write(client, CFG, ADP5588_INT_CFG |
+	return adp5588_write(client, CFG, ADP5588_INT_CFG |
 					  ADP5588_OVR_FLOW_IEN |
 					  ADP5588_KE_IEN);
-
-	if (ret < 0) {
-		dev_err(&client->dev, "Write Error\n");
-		return ret;
-	}
-
-	return 0;
 }
 
 static int adp5588_probe(struct i2c_client *client,
-- 
2.37.0

Re: [PATCH 03/10] input: keyboard: adp5588-keys: bail out on returned error

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote:
>
> Don't continue in code paths after some error is found. It makes no
> sense to do any other device configuration if a previous one failed.

...

>                 for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
>                         int pull_mask = gpio_data->pullup_dis_mask;
>
> -                       ret |= adp5588_write(client, GPIO_PULL1 + i,
> +                       ret = adp5588_write(client, GPIO_PULL1 + i,
>                                 (pull_mask >> (8 * i)) & 0xFF);
> +                       if (ret)
> +                               return ret;
>                 }

Looks like a good candidate for bitmap_get_value8(pull_mask).

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 03/10] input: keyboard: adp5588-keys: bail out on returned error

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, July 8, 2022 4:25 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO
> SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux-
> input@vger.kernel.org>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>; Bartosz Golaszewski
> <brgl@bgdev.pl>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>
> Subject: Re: [PATCH 03/10] input: keyboard: adp5588-keys: bail out on
> returned error
> 
> [External]
> 
> On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > Don't continue in code paths after some error is found. It makes no
> > sense to do any other device configuration if a previous one failed.
> 
> ...
> 
> >                 for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
> >                         int pull_mask = gpio_data->pullup_dis_mask;
> >
> > -                       ret |= adp5588_write(client, GPIO_PULL1 + i,
> > +                       ret = adp5588_write(client, GPIO_PULL1 + i,
> >                                 (pull_mask >> (8 * i)) & 0xFF);
> > +                       if (ret)
> > +                               return ret;
> >                 }
> 
> Looks like a good candidate for bitmap_get_value8(pull_mask).
> 

I'm not touching the original way the driver was handling this
kind of stuff. I do have in my mind to just convert this driver to use
regmap and with it (by using the *bits functions) we can get rid of
most of the "plain" bitmaps in the driver.

- Nuno Sá

Re: [PATCH 03/10] input: keyboard: adp5588-keys: bail out on returned error

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 4:35 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Friday, July 8, 2022 4:25 PM
> > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > Looks like a good candidate for bitmap_get_value8(pull_mask).
>
> I'm not touching the original way the driver was handling this
> kind of stuff. I do have in my mind to just convert this driver to use
> regmap and with it (by using the *bits functions) we can get rid of
> most of the "plain" bitmaps in the driver.

Works for me!

-- 
With Best Regards,
Andy Shevchenko

[PATCH 04/10] input: keyboard: adp5588-keys: add support for fw properties

[Nuno Sá]

Use firmware properties (eg: OF) to get the device specific
configuration. This change just replaces the platform data since there
was no platform using it and so, it makes no sense having both.

Special note to the PULL-UP disable setting that is now supported as
part of the gpio subsystem (using 'set_config()' callback).

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/input/keyboard/Kconfig        |   1 +
 drivers/input/keyboard/adp5588-keys.c | 394 +++++++++++++++++++-------
 include/linux/platform_data/adp5588.h | 169 -----------
 3 files changed, 288 insertions(+), 276 deletions(-)
 delete mode 100644 include/linux/platform_data/adp5588.h

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index ca5cd5e520a7..be4a6f8165cd 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -42,6 +42,7 @@ config KEYBOARD_ADP5588
 	depends on I2C
 	select GPIOLIB
 	select GPIOLIB_IRQCHIP
+	select INPUT_MATRIXKMAP
 	help
 	  Say Y here if you want to use a ADP5588/87 attached to your
 	  system I2C bus.
diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 10f24283d7a3..e831b6f66b55 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -13,16 +13,149 @@
 #include <linux/gpio/driver.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
+#include <linux/input/matrix_keypad.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/ktime.h>
 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 #include <linux/timekeeping.h>
 
-#include <linux/platform_data/adp5588.h>
+#define DEV_ID 0x00		/* Device ID */
+#define CFG 0x01		/* Configuration Register1 */
+#define INT_STAT 0x02		/* Interrupt Status Register */
+#define KEY_LCK_EC_STAT 0x03	/* Key Lock and Event Counter Register */
+#define Key_EVENTA 0x04		/* Key Event Register A */
+#define Key_EVENTB 0x05		/* Key Event Register B */
+#define Key_EVENTC 0x06		/* Key Event Register C */
+#define Key_EVENTD 0x07		/* Key Event Register D */
+#define Key_EVENTE 0x08		/* Key Event Register E */
+#define Key_EVENTF 0x09		/* Key Event Register F */
+#define Key_EVENTG 0x0A		/* Key Event Register G */
+#define Key_EVENTH 0x0B		/* Key Event Register H */
+#define Key_EVENTI 0x0C		/* Key Event Register I */
+#define Key_EVENTJ 0x0D		/* Key Event Register J */
+#define KP_LCK_TMR 0x0E		/* Keypad Lock1 to Lock2 Timer */
+#define UNLOCK1 0x0F		/* Unlock Key1 */
+#define UNLOCK2 0x10		/* Unlock Key2 */
+#define GPIO_INT_STAT1 0x11	/* GPIO Interrupt Status */
+#define GPIO_INT_STAT2 0x12	/* GPIO Interrupt Status */
+#define GPIO_INT_STAT3 0x13	/* GPIO Interrupt Status */
+#define GPIO_DAT_STAT1 0x14	/* GPIO Data Status, Read twice to clear */
+#define GPIO_DAT_STAT2 0x15	/* GPIO Data Status, Read twice to clear */
+#define GPIO_DAT_STAT3 0x16	/* GPIO Data Status, Read twice to clear */
+#define GPIO_DAT_OUT1 0x17	/* GPIO DATA OUT */
+#define GPIO_DAT_OUT2 0x18	/* GPIO DATA OUT */
+#define GPIO_DAT_OUT3 0x19	/* GPIO DATA OUT */
+#define GPIO_INT_EN1 0x1A	/* GPIO Interrupt Enable */
+#define GPIO_INT_EN2 0x1B	/* GPIO Interrupt Enable */
+#define GPIO_INT_EN3 0x1C	/* GPIO Interrupt Enable */
+#define KP_GPIO1 0x1D		/* Keypad or GPIO Selection */
+#define KP_GPIO2 0x1E		/* Keypad or GPIO Selection */
+#define KP_GPIO3 0x1F		/* Keypad or GPIO Selection */
+#define GPI_EM1 0x20		/* GPI Event Mode 1 */
+#define GPI_EM2 0x21		/* GPI Event Mode 2 */
+#define GPI_EM3 0x22		/* GPI Event Mode 3 */
+#define GPIO_DIR1 0x23		/* GPIO Data Direction */
+#define GPIO_DIR2 0x24		/* GPIO Data Direction */
+#define GPIO_DIR3 0x25		/* GPIO Data Direction */
+#define GPIO_INT_LVL1 0x26	/* GPIO Edge/Level Detect */
+#define GPIO_INT_LVL2 0x27	/* GPIO Edge/Level Detect */
+#define GPIO_INT_LVL3 0x28	/* GPIO Edge/Level Detect */
+#define Debounce_DIS1 0x29	/* Debounce Disable */
+#define Debounce_DIS2 0x2A	/* Debounce Disable */
+#define Debounce_DIS3 0x2B	/* Debounce Disable */
+#define GPIO_PULL1 0x2C		/* GPIO Pull Disable */
+#define GPIO_PULL2 0x2D		/* GPIO Pull Disable */
+#define GPIO_PULL3 0x2E		/* GPIO Pull Disable */
+#define CMP_CFG_STAT 0x30	/* Comparator Configuration and Status Register */
+#define CMP_CONFG_SENS1 0x31	/* Sensor1 Comparator Configuration Register */
+#define CMP_CONFG_SENS2 0x32	/* L2 Light Sensor Reference Level, Output Falling for Sensor 1 */
+#define CMP1_LVL2_TRIP 0x33	/* L2 Light Sensor Hysteresis (Active when Output Rising) for Sensor 1 */
+#define CMP1_LVL2_HYS 0x34	/* L3 Light Sensor Reference Level, Output Falling For Sensor 1 */
+#define CMP1_LVL3_TRIP 0x35	/* L3 Light Sensor Hysteresis (Active when Output Rising) For Sensor 1 */
+#define CMP1_LVL3_HYS 0x36	/* Sensor 2 Comparator Configuration Register */
+#define CMP2_LVL2_TRIP 0x37	/* L2 Light Sensor Reference Level, Output Falling for Sensor 2 */
+#define CMP2_LVL2_HYS 0x38	/* L2 Light Sensor Hysteresis (Active when Output Rising) for Sensor 2 */
+#define CMP2_LVL3_TRIP 0x39	/* L3 Light Sensor Reference Level, Output Falling For Sensor 2 */
+#define CMP2_LVL3_HYS 0x3A	/* L3 Light Sensor Hysteresis (Active when Output Rising) For Sensor 2 */
+#define CMP1_ADC_DAT_R1 0x3B	/* Comparator 1 ADC data Register1 */
+#define CMP1_ADC_DAT_R2 0x3C	/* Comparator 1 ADC data Register2 */
+#define CMP2_ADC_DAT_R1 0x3D	/* Comparator 2 ADC data Register1 */
+#define CMP2_ADC_DAT_R2 0x3E	/* Comparator 2 ADC data Register2 */
+
+#define ADP5588_DEVICE_ID_MASK	0xF
+
+ /* Configuration Register1 */
+#define ADP5588_AUTO_INC	(1 << 7)
+#define ADP5588_GPIEM_CFG	(1 << 6)
+#define ADP5588_OVR_FLOW_M	(1 << 5)
+#define ADP5588_INT_CFG		(1 << 4)
+#define ADP5588_OVR_FLOW_IEN	(1 << 3)
+#define ADP5588_K_LCK_IM	(1 << 2)
+#define ADP5588_GPI_IEN		(1 << 1)
+#define ADP5588_KE_IEN		(1 << 0)
+
+/* Interrupt Status Register */
+#define ADP5588_CMP2_INT	(1 << 5)
+#define ADP5588_CMP1_INT	(1 << 4)
+#define ADP5588_OVR_FLOW_INT	(1 << 3)
+#define ADP5588_K_LCK_INT	(1 << 2)
+#define ADP5588_GPI_INT		(1 << 1)
+#define ADP5588_KE_INT		(1 << 0)
+
+/* Key Lock and Event Counter Register */
+#define ADP5588_K_LCK_EN	(1 << 6)
+#define ADP5588_LCK21		0x30
+#define ADP5588_KEC		0xF
+
+#define ADP5588_MAXGPIO		18
+#define ADP5588_BANK(offs)	((offs) >> 3)
+#define ADP5588_BIT(offs)	(1u << ((offs) & 0x7))
+
+/* Put one of these structures in i2c_board_info platform_data */
+
+/*
+ * 128 so it fits matrix-keymap maximum number of keys when the full
+ * 10cols * 8rows are used.
+ */
+#define ADP5588_KEYMAPSIZE 128
+
+#define GPI_PIN_ROW0 97
+#define GPI_PIN_ROW1 98
+#define GPI_PIN_ROW2 99
+#define GPI_PIN_ROW3 100
+#define GPI_PIN_ROW4 101
+#define GPI_PIN_ROW5 102
+#define GPI_PIN_ROW6 103
+#define GPI_PIN_ROW7 104
+#define GPI_PIN_COL0 105
+#define GPI_PIN_COL1 106
+#define GPI_PIN_COL2 107
+#define GPI_PIN_COL3 108
+#define GPI_PIN_COL4 109
+#define GPI_PIN_COL5 110
+#define GPI_PIN_COL6 111
+#define GPI_PIN_COL7 112
+#define GPI_PIN_COL8 113
+#define GPI_PIN_COL9 114
+
+#define GPI_PIN_ROW_BASE GPI_PIN_ROW0
+#define GPI_PIN_ROW_END GPI_PIN_ROW7
+#define GPI_PIN_COL_BASE GPI_PIN_COL0
+#define GPI_PIN_COL_END GPI_PIN_COL9
+
+#define GPI_PIN_BASE GPI_PIN_ROW_BASE
+#define GPI_PIN_END GPI_PIN_COL_END
+
+#define ADP5588_ROWS_MAX (GPI_PIN_ROW7 - GPI_PIN_ROW0 + 1)
+#define ADP5588_COLS_MAX (GPI_PIN_COL9 - GPI_PIN_COL0 + 1)
+
+#define ADP5588_GPIMAPSIZE_MAX (GPI_PIN_END - GPI_PIN_BASE + 1)
 
 /* Key Event Register xy */
 #define KEY_EV_PRESSED		(1 << 7)
@@ -45,6 +178,11 @@ struct adp5588_kpad {
 	struct input_dev *input;
 	ktime_t irq_time;
 	unsigned long delay;
+	u32 row_shift;
+	u32 rows;
+	u32 cols;
+	u32 unlock_keys[2];
+	int nkeys_unlock;
 	unsigned short keycode[ADP5588_KEYMAPSIZE];
 	unsigned char gpiomap[ADP5588_MAXGPIO];
 	struct gpio_chip gc;
@@ -54,6 +192,7 @@ struct adp5588_kpad {
 	u8 dir[3];
 	u8 int_en[3];
 	u8 irq_mask[3];
+	u8 pull_dis[3];
 };
 
 static int adp5588_read(struct i2c_client *client, u8 reg)
@@ -110,6 +249,40 @@ static void adp5588_gpio_set_value(struct gpio_chip *chip,
 	mutex_unlock(&kpad->gpio_lock);
 }
 
+static int adp5588_gpio_set_config(struct gpio_chip *chip,  unsigned int off,
+				   unsigned long config)
+{
+	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
+	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
+	unsigned int bit = ADP5588_BIT(kpad->gpiomap[off]), pull_disable;
+	int ret;
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+		pull_disable = 0;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		pull_disable = 1;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	mutex_lock(&kpad->gpio_lock);
+
+	if (pull_disable)
+		kpad->pull_dis[bank] |= bit;
+	else
+		kpad->pull_dis[bank] &= bit;
+
+	ret = adp5588_write(kpad->client, GPIO_PULL1 + bank,
+			    kpad->pull_dis[bank]);
+
+	mutex_unlock(&kpad->gpio_lock);
+
+	return ret;
+}
+
 static int adp5588_gpio_direction_input(struct gpio_chip *chip, unsigned off)
 {
 	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
@@ -158,8 +331,7 @@ static int adp5588_gpio_direction_output(struct gpio_chip *chip,
 	return ret;
 }
 
-static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
-				const struct adp5588_kpad_platform_data *pdata)
+static int adp5588_build_gpiomap(struct adp5588_kpad *kpad)
 {
 	bool pin_used[ADP5588_MAXGPIO];
 	int n_unused = 0;
@@ -167,10 +339,10 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
 
 	memset(pin_used, 0, sizeof(pin_used));
 
-	for (i = 0; i < pdata->rows; i++)
+	for (i = 0; i < kpad->rows; i++)
 		pin_used[i] = true;
 
-	for (i = 0; i < pdata->cols; i++)
+	for (i = 0; i < kpad->cols; i++)
 		pin_used[i + GPI_PIN_COL_BASE - GPI_PIN_BASE] = true;
 
 	for (i = 0; i < ADP5588_MAXGPIO; i++)
@@ -180,21 +352,6 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
 	return n_unused;
 }
 
-static void adp5588_gpio_do_teardown(void *_kpad)
-{
-	struct adp5588_kpad *kpad = _kpad;
-	struct device *dev = &kpad->client->dev;
-	const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
-	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
-	int error;
-
-	error = gpio_data->teardown(kpad->client,
-				    kpad->gc.base, kpad->gc.ngpio,
-				    gpio_data->context);
-	if (error)
-		dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
-}
-
 static void adp5588_irq_bus_lock(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -249,15 +406,10 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 {
 	struct irq_chip *irq_chip = &kpad->irq_chip;
 	struct device *dev = &kpad->client->dev;
-	const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
-	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
 	struct gpio_irq_chip *girq;
 	int i, error;
 
-	if (!gpio_data)
-		return 0;
-
-	kpad->gc.ngpio = adp5588_build_gpiomap(kpad, pdata);
+	kpad->gc.ngpio = adp5588_build_gpiomap(kpad);
 	if (kpad->gc.ngpio == 0) {
 		dev_info(dev, "No unused gpios left to export\n");
 		return 0;
@@ -269,12 +421,12 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	kpad->gc.direction_output = adp5588_gpio_direction_output;
 	kpad->gc.get = adp5588_gpio_get_value;
 	kpad->gc.set = adp5588_gpio_set_value;
+	kpad->gc.set_config = adp5588_gpio_set_config;
 	kpad->gc.can_sleep = 1;
 
-	kpad->gc.base = gpio_data->gpio_start;
+	kpad->gc.base = -1;
 	kpad->gc.label = kpad->client->name;
 	kpad->gc.owner = THIS_MODULE;
-	kpad->gc.names = gpio_data->names;
 
 	irq_chip->name = "adp5588";
 	irq_chip->irq_mask = adp5588_irq_mask;
@@ -300,21 +452,7 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 		kpad->dat_out[i] = adp5588_read(kpad->client,
 						GPIO_DAT_OUT1 + i);
 		kpad->dir[i] = adp5588_read(kpad->client, GPIO_DIR1 + i);
-	}
-
-	if (gpio_data->setup) {
-		error = gpio_data->setup(kpad->client,
-					 kpad->gc.base, kpad->gc.ngpio,
-					 gpio_data->context);
-		if (error)
-			dev_warn(dev, "setup failed: %d\n", error);
-	}
-
-	if (gpio_data->teardown) {
-		error = devm_add_action(dev, adp5588_gpio_do_teardown, kpad);
-		if (error)
-			dev_warn(dev, "failed to schedule teardown: %d\n",
-				 error);
+		kpad->pull_dis[i] = adp5588_read(kpad->client, GPIO_PULL1 + i);
 	}
 
 	return 0;
@@ -378,12 +516,21 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 		int key_val = key & KEY_EV_MASK;
 		int key_press = key & KEY_EV_PRESSED;
 
-		if (key_val >= GPI_PIN_BASE && key_val <= GPI_PIN_END)
+		if (key_val >= GPI_PIN_BASE && key_val <= GPI_PIN_END) {
 			/* gpio line used as IRQ source */
 			adp5588_gpio_irq_handle(kpad, key_val, key_press);
-		else
+		} else {
+			int row = (key_val - 1) / ADP5588_COLS_MAX;
+			int col =  (key_val - 1) % ADP5588_COLS_MAX;
+			int code = MATRIX_SCAN_CODE(row, col, kpad->row_shift);
+
+			dev_dbg_ratelimited(&kpad->client->dev,
+					    "report key(%d) r(%d) c(%d) code(%d)\n",
+					    key_val, row, col, kpad->keycode[code]);
+
 			input_report_key(kpad->input,
-					 kpad->keycode[key_val - 1], key_press);
+					 kpad->keycode[code], key_press);
+		}
 	}
 }
 
@@ -435,34 +582,30 @@ static irqreturn_t adp5588_thread_irq(int irq, void *handle)
 	return IRQ_HANDLED;
 }
 
-static int adp5588_setup(struct i2c_client *client)
+static int adp5588_setup(struct adp5588_kpad *kpad)
 {
-	const struct adp5588_kpad_platform_data *pdata =
-			dev_get_platdata(&client->dev);
-	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
+	struct i2c_client *client = kpad->client;
 	int i, ret;
 
-	ret = adp5588_write(client, KP_GPIO1, KP_SEL(pdata->rows));
+	ret = adp5588_write(client, KP_GPIO1, KP_SEL(kpad->rows));
 	if (ret)
 		return ret;
 
-	ret = adp5588_write(client, KP_GPIO2, KP_SEL(pdata->cols) & 0xFF);
+	ret = adp5588_write(client, KP_GPIO2, KP_SEL(kpad->cols) & 0xFF);
 	if (ret)
 		return ret;
 
-	ret = adp5588_write(client, KP_GPIO3, KP_SEL(pdata->cols) >> 8);
+	ret = adp5588_write(client, KP_GPIO3, KP_SEL(kpad->cols) >> 8);
 	if (ret)
 		return ret;
 
-	if (pdata->en_keylock) {
-		ret = adp5588_write(client, UNLOCK1, pdata->unlock_key1);
-		if (ret)
-			return ret;
-
-		ret = adp5588_write(client, UNLOCK2, pdata->unlock_key2);
+	for (i = 0; i < kpad->nkeys_unlock; i++) {
+		ret = adp5588_write(client, UNLOCK1 + i, kpad->unlock_keys[i]);
 		if (ret)
 			return ret;
+	}
 
+	if (kpad->nkeys_unlock) {
 		ret = adp5588_write(client, KEY_LCK_EC_STAT, ADP5588_K_LCK_EN);
 		if (ret)
 			return ret;
@@ -474,17 +617,6 @@ static int adp5588_setup(struct i2c_client *client)
 			return ret;
 	}
 
-	if (gpio_data) {
-		for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
-			int pull_mask = gpio_data->pullup_dis_mask;
-
-			ret = adp5588_write(client, GPIO_PULL1 + i,
-				(pull_mask >> (8 * i)) & 0xFF);
-			if (ret)
-				return ret;
-		}
-	}
-
 	ret = adp5588_write(client, INT_STAT,
 				ADP5588_CMP2_INT | ADP5588_CMP1_INT |
 				ADP5588_OVR_FLOW_INT | ADP5588_K_LCK_INT |
@@ -497,15 +629,84 @@ static int adp5588_setup(struct i2c_client *client)
 					  ADP5588_KE_IEN);
 }
 
+static int adp5588_fw_parse(struct adp5588_kpad *kpad)
+{
+	struct i2c_client *client = kpad->client;
+	int ret, i;
+
+	ret = matrix_keypad_parse_properties(&client->dev, &kpad->rows,
+					     &kpad->cols);
+	if (ret)
+		return ret;
+
+	if (kpad->rows > ADP5588_ROWS_MAX || kpad->cols > ADP5588_COLS_MAX) {
+		dev_err(&client->dev, "Invalid nr of rows(%u) or cols(%u)\n",
+			kpad->rows, kpad->cols);
+		return -EINVAL;
+	}
+
+	ret = matrix_keypad_build_keymap(NULL, NULL, kpad->rows, kpad->cols,
+					 kpad->keycode, kpad->input);
+	if (ret)
+		return ret;
+
+	kpad->row_shift = get_count_order(kpad->cols);
+
+	if (device_property_read_bool(&client->dev, "autorepeat"))
+		__set_bit(EV_REP, kpad->input->evbit);
+
+	kpad->nkeys_unlock = device_property_count_u32(&client->dev,
+						       "adi,unlock-keys");
+	if (kpad->nkeys_unlock <= 0) {
+		/* so that we don't end up enabling key lock */
+		kpad->nkeys_unlock = 0;
+		return 0;
+	}
+
+	if (kpad->nkeys_unlock > ARRAY_SIZE(kpad->unlock_keys)) {
+		dev_err(&client->dev, "number of unlock keys(%d) > (%d)\n",
+			kpad->nkeys_unlock, ARRAY_SIZE(kpad->unlock_keys));
+		return -EINVAL;
+	}
+
+	ret = device_property_read_u32_array(&client->dev, "adi,unlock-keys",
+					     kpad->unlock_keys,
+					     kpad->nkeys_unlock);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < kpad->nkeys_unlock; i++) {
+		/*
+		 * Even though it should be possible (as stated in the datasheet)
+		 * to use GPIs (which are part of the keys event) as unlock keys,
+		 * it was not working at all and was leading to overflow events
+		 * at some point. Hence, for now, let's just allow keys which are
+		 * part of keypad matrix to be used and if a reliable way of
+		 * using GPIs is found, this condition can be removed/lightened.
+		 */
+		if (kpad->unlock_keys[i] >= kpad->cols * kpad->rows) {
+			dev_err(&client->dev, "Invalid unlock key(%d)\n",
+				kpad->unlock_keys[i]);
+			return -EINVAL;
+		}
+
+		/*
+		 * fw properties keys start from 0 but on the device they
+		 * start from 1.
+		 */
+		kpad->unlock_keys[i] += 1;
+	}
+
+	return 0;
+}
+
 static int adp5588_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct adp5588_kpad *kpad;
-	const struct adp5588_kpad_platform_data *pdata =
-			dev_get_platdata(&client->dev);
 	struct input_dev *input;
 	unsigned int revid;
-	int ret, i;
+	int ret;
 	int error;
 
 	if (!i2c_check_functionality(client->adapter,
@@ -514,21 +715,6 @@ static int adp5588_probe(struct i2c_client *client,
 		return -EIO;
 	}
 
-	if (!pdata) {
-		dev_err(&client->dev, "no platform data?\n");
-		return -EINVAL;
-	}
-
-	if (!pdata->rows || !pdata->cols || !pdata->keymap) {
-		dev_err(&client->dev, "no rows, cols or keymap from pdata\n");
-		return -EINVAL;
-	}
-
-	if (pdata->keymapsize != ADP5588_KEYMAPSIZE) {
-		dev_err(&client->dev, "invalid keymapsize\n");
-		return -EINVAL;
-	}
-
 	if (!client->irq) {
 		dev_err(&client->dev, "no IRQ?\n");
 		return -EINVAL;
@@ -545,6 +731,10 @@ static int adp5588_probe(struct i2c_client *client,
 	kpad->client = client;
 	kpad->input = input;
 
+	error = adp5588_fw_parse(kpad);
+	if (error)
+		return error;
+
 	ret = adp5588_read(client, DEV_ID);
 	if (ret < 0)
 		return ret;
@@ -563,24 +753,6 @@ static int adp5588_probe(struct i2c_client *client,
 	input->id.product = 0x0001;
 	input->id.version = revid;
 
-	input->keycodesize = sizeof(kpad->keycode[0]);
-	input->keycodemax = pdata->keymapsize;
-	input->keycode = kpad->keycode;
-
-	memcpy(kpad->keycode, pdata->keymap,
-		pdata->keymapsize * input->keycodesize);
-
-	/* setup input device */
-	__set_bit(EV_KEY, input->evbit);
-
-	if (pdata->repeat)
-		__set_bit(EV_REP, input->evbit);
-
-	for (i = 0; i < input->keycodemax; i++)
-		if (kpad->keycode[i] <= KEY_MAX)
-			__set_bit(kpad->keycode[i], input->keybit);
-	__clear_bit(KEY_RESERVED, input->keybit);
-
 	error = input_register_device(input);
 	if (error) {
 		dev_err(&client->dev, "unable to register input device: %d\n",
@@ -588,7 +760,7 @@ static int adp5588_probe(struct i2c_client *client,
 		return error;
 	}
 
-	error = adp5588_setup(client);
+	error = adp5588_setup(kpad);
 	if (error)
 		return error;
 
@@ -645,9 +817,17 @@ static const struct i2c_device_id adp5588_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp5588_id);
 
+static const struct of_device_id adp5588_of_match[] = {
+	{ .compatible = "adp5588-keys" },
+	{ .compatible = "adp5587-keys" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, adp5588_of_match);
+
 static struct i2c_driver adp5588_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
+		.of_match_table = adp5588_of_match,
 		.pm   = &adp5588_dev_pm_ops,
 	},
 	.probe    = adp5588_probe,
diff --git a/include/linux/platform_data/adp5588.h b/include/linux/platform_data/adp5588.h
deleted file mode 100644
index 82170ec8c266..000000000000
--- a/include/linux/platform_data/adp5588.h
+++ /dev/null
@@ -1,169 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * Analog Devices ADP5588 I/O Expander and QWERTY Keypad Controller
- *
- * Copyright 2009-2010 Analog Devices Inc.
- */
-
-#ifndef _ADP5588_H
-#define _ADP5588_H
-
-#define DEV_ID 0x00		/* Device ID */
-#define CFG 0x01		/* Configuration Register1 */
-#define INT_STAT 0x02		/* Interrupt Status Register */
-#define KEY_LCK_EC_STAT 0x03	/* Key Lock and Event Counter Register */
-#define Key_EVENTA 0x04		/* Key Event Register A */
-#define Key_EVENTB 0x05		/* Key Event Register B */
-#define Key_EVENTC 0x06		/* Key Event Register C */
-#define Key_EVENTD 0x07		/* Key Event Register D */
-#define Key_EVENTE 0x08		/* Key Event Register E */
-#define Key_EVENTF 0x09		/* Key Event Register F */
-#define Key_EVENTG 0x0A		/* Key Event Register G */
-#define Key_EVENTH 0x0B		/* Key Event Register H */
-#define Key_EVENTI 0x0C		/* Key Event Register I */
-#define Key_EVENTJ 0x0D		/* Key Event Register J */
-#define KP_LCK_TMR 0x0E		/* Keypad Lock1 to Lock2 Timer */
-#define UNLOCK1 0x0F		/* Unlock Key1 */
-#define UNLOCK2 0x10		/* Unlock Key2 */
-#define GPIO_INT_STAT1 0x11	/* GPIO Interrupt Status */
-#define GPIO_INT_STAT2 0x12	/* GPIO Interrupt Status */
-#define GPIO_INT_STAT3 0x13	/* GPIO Interrupt Status */
-#define GPIO_DAT_STAT1 0x14	/* GPIO Data Status, Read twice to clear */
-#define GPIO_DAT_STAT2 0x15	/* GPIO Data Status, Read twice to clear */
-#define GPIO_DAT_STAT3 0x16	/* GPIO Data Status, Read twice to clear */
-#define GPIO_DAT_OUT1 0x17	/* GPIO DATA OUT */
-#define GPIO_DAT_OUT2 0x18	/* GPIO DATA OUT */
-#define GPIO_DAT_OUT3 0x19	/* GPIO DATA OUT */
-#define GPIO_INT_EN1 0x1A	/* GPIO Interrupt Enable */
-#define GPIO_INT_EN2 0x1B	/* GPIO Interrupt Enable */
-#define GPIO_INT_EN3 0x1C	/* GPIO Interrupt Enable */
-#define KP_GPIO1 0x1D		/* Keypad or GPIO Selection */
-#define KP_GPIO2 0x1E		/* Keypad or GPIO Selection */
-#define KP_GPIO3 0x1F		/* Keypad or GPIO Selection */
-#define GPI_EM1 0x20		/* GPI Event Mode 1 */
-#define GPI_EM2 0x21		/* GPI Event Mode 2 */
-#define GPI_EM3 0x22		/* GPI Event Mode 3 */
-#define GPIO_DIR1 0x23		/* GPIO Data Direction */
-#define GPIO_DIR2 0x24		/* GPIO Data Direction */
-#define GPIO_DIR3 0x25		/* GPIO Data Direction */
-#define GPIO_INT_LVL1 0x26	/* GPIO Edge/Level Detect */
-#define GPIO_INT_LVL2 0x27	/* GPIO Edge/Level Detect */
-#define GPIO_INT_LVL3 0x28	/* GPIO Edge/Level Detect */
-#define Debounce_DIS1 0x29	/* Debounce Disable */
-#define Debounce_DIS2 0x2A	/* Debounce Disable */
-#define Debounce_DIS3 0x2B	/* Debounce Disable */
-#define GPIO_PULL1 0x2C		/* GPIO Pull Disable */
-#define GPIO_PULL2 0x2D		/* GPIO Pull Disable */
-#define GPIO_PULL3 0x2E		/* GPIO Pull Disable */
-#define CMP_CFG_STAT 0x30	/* Comparator Configuration and Status Register */
-#define CMP_CONFG_SENS1 0x31	/* Sensor1 Comparator Configuration Register */
-#define CMP_CONFG_SENS2 0x32	/* L2 Light Sensor Reference Level, Output Falling for Sensor 1 */
-#define CMP1_LVL2_TRIP 0x33	/* L2 Light Sensor Hysteresis (Active when Output Rising) for Sensor 1 */
-#define CMP1_LVL2_HYS 0x34	/* L3 Light Sensor Reference Level, Output Falling For Sensor 1 */
-#define CMP1_LVL3_TRIP 0x35	/* L3 Light Sensor Hysteresis (Active when Output Rising) For Sensor 1 */
-#define CMP1_LVL3_HYS 0x36	/* Sensor 2 Comparator Configuration Register */
-#define CMP2_LVL2_TRIP 0x37	/* L2 Light Sensor Reference Level, Output Falling for Sensor 2 */
-#define CMP2_LVL2_HYS 0x38	/* L2 Light Sensor Hysteresis (Active when Output Rising) for Sensor 2 */
-#define CMP2_LVL3_TRIP 0x39	/* L3 Light Sensor Reference Level, Output Falling For Sensor 2 */
-#define CMP2_LVL3_HYS 0x3A	/* L3 Light Sensor Hysteresis (Active when Output Rising) For Sensor 2 */
-#define CMP1_ADC_DAT_R1 0x3B	/* Comparator 1 ADC data Register1 */
-#define CMP1_ADC_DAT_R2 0x3C	/* Comparator 1 ADC data Register2 */
-#define CMP2_ADC_DAT_R1 0x3D	/* Comparator 2 ADC data Register1 */
-#define CMP2_ADC_DAT_R2 0x3E	/* Comparator 2 ADC data Register2 */
-
-#define ADP5588_DEVICE_ID_MASK	0xF
-
- /* Configuration Register1 */
-#define ADP5588_AUTO_INC	(1 << 7)
-#define ADP5588_GPIEM_CFG	(1 << 6)
-#define ADP5588_OVR_FLOW_M	(1 << 5)
-#define ADP5588_INT_CFG		(1 << 4)
-#define ADP5588_OVR_FLOW_IEN	(1 << 3)
-#define ADP5588_K_LCK_IM	(1 << 2)
-#define ADP5588_GPI_IEN		(1 << 1)
-#define ADP5588_KE_IEN		(1 << 0)
-
-/* Interrupt Status Register */
-#define ADP5588_CMP2_INT	(1 << 5)
-#define ADP5588_CMP1_INT	(1 << 4)
-#define ADP5588_OVR_FLOW_INT	(1 << 3)
-#define ADP5588_K_LCK_INT	(1 << 2)
-#define ADP5588_GPI_INT		(1 << 1)
-#define ADP5588_KE_INT		(1 << 0)
-
-/* Key Lock and Event Counter Register */
-#define ADP5588_K_LCK_EN	(1 << 6)
-#define ADP5588_LCK21		0x30
-#define ADP5588_KEC		0xF
-
-#define ADP5588_MAXGPIO		18
-#define ADP5588_BANK(offs)	((offs) >> 3)
-#define ADP5588_BIT(offs)	(1u << ((offs) & 0x7))
-
-/* Put one of these structures in i2c_board_info platform_data */
-
-#define ADP5588_KEYMAPSIZE	80
-
-#define GPI_PIN_ROW0 97
-#define GPI_PIN_ROW1 98
-#define GPI_PIN_ROW2 99
-#define GPI_PIN_ROW3 100
-#define GPI_PIN_ROW4 101
-#define GPI_PIN_ROW5 102
-#define GPI_PIN_ROW6 103
-#define GPI_PIN_ROW7 104
-#define GPI_PIN_COL0 105
-#define GPI_PIN_COL1 106
-#define GPI_PIN_COL2 107
-#define GPI_PIN_COL3 108
-#define GPI_PIN_COL4 109
-#define GPI_PIN_COL5 110
-#define GPI_PIN_COL6 111
-#define GPI_PIN_COL7 112
-#define GPI_PIN_COL8 113
-#define GPI_PIN_COL9 114
-
-#define GPI_PIN_ROW_BASE GPI_PIN_ROW0
-#define GPI_PIN_ROW_END GPI_PIN_ROW7
-#define GPI_PIN_COL_BASE GPI_PIN_COL0
-#define GPI_PIN_COL_END GPI_PIN_COL9
-
-#define GPI_PIN_BASE GPI_PIN_ROW_BASE
-#define GPI_PIN_END GPI_PIN_COL_END
-
-#define ADP5588_GPIMAPSIZE_MAX (GPI_PIN_END - GPI_PIN_BASE + 1)
-
-struct adp5588_gpi_map {
-	unsigned short pin;
-	unsigned short sw_evt;
-};
-
-struct adp5588_kpad_platform_data {
-	int rows;			/* Number of rows */
-	int cols;			/* Number of columns */
-	const unsigned short *keymap;	/* Pointer to keymap */
-	unsigned short keymapsize;	/* Keymap size */
-	unsigned repeat:1;		/* Enable key repeat */
-	unsigned en_keylock:1;		/* Enable Key Lock feature */
-	unsigned short unlock_key1;	/* Unlock Key 1 */
-	unsigned short unlock_key2;	/* Unlock Key 2 */
-	const struct adp5588_gpio_platform_data *gpio_data;
-};
-
-struct i2c_client; /* forward declaration */
-
-struct adp5588_gpio_platform_data {
-	int gpio_start;		/* GPIO Chip base # */
-	const char *const *names;
-	unsigned irq_base;	/* interrupt base # */
-	unsigned pullup_dis_mask; /* Pull-Up Disable Mask */
-	int	(*setup)(struct i2c_client *client,
-				unsigned gpio, unsigned ngpio,
-				void *context);
-	int	(*teardown)(struct i2c_client *client,
-				unsigned gpio, unsigned ngpio,
-				void *context);
-	void	*context;
-};
-
-#endif
-- 
2.37.0

Re: [PATCH 04/10] input: keyboard: adp5588-keys: add support for fw properties

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 11:37 AM Nuno Sá <nuno.sa@analog.com> wrote:
>
> Use firmware properties (eg: OF) to get the device specific
> configuration. This change just replaces the platform data since there
> was no platform using it and so, it makes no sense having both.
>
> Special note to the PULL-UP disable setting that is now supported as
> part of the gpio subsystem (using 'set_config()' callback).

...

> +#define ADP5588_DEVICE_ID_MASK 0xF
> +
> + /* Configuration Register1 */
> +#define ADP5588_AUTO_INC       (1 << 7)
> +#define ADP5588_GPIEM_CFG      (1 << 6)
> +#define ADP5588_OVR_FLOW_M     (1 << 5)
> +#define ADP5588_INT_CFG                (1 << 4)
> +#define ADP5588_OVR_FLOW_IEN   (1 << 3)
> +#define ADP5588_K_LCK_IM       (1 << 2)
> +#define ADP5588_GPI_IEN                (1 << 1)
> +#define ADP5588_KE_IEN         (1 << 0)

Okay, you add something in the wrong form and then fix it in the other
patch in the very same series? Please no ping-pong type of changes.
Squash / rebase your series accordingly.

...

> -       ret = adp5588_write(client, KP_GPIO2, KP_SEL(pdata->cols) & 0xFF);
> +       ret = adp5588_write(client, KP_GPIO2, KP_SEL(kpad->cols) & 0xFF);

Do you need these " & 0xFF" parts?

...

> +               /*
> +                * fw properties keys start from 0 but on the device they

Firmware

> +                * start from 1.
> +                */

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 04/10] input: keyboard: adp5588-keys: add support for fw properties

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, July 8, 2022 4:56 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO
> SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux-
> input@vger.kernel.org>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>; Bartosz Golaszewski
> <brgl@bgdev.pl>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>
> Subject: Re: [PATCH 04/10] input: keyboard: adp5588-keys: add
> support for fw properties
> 
> [External]
> 
> On Fri, Jul 8, 2022 at 11:37 AM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > Use firmware properties (eg: OF) to get the device specific
> > configuration. This change just replaces the platform data since there
> > was no platform using it and so, it makes no sense having both.
> >
> > Special note to the PULL-UP disable setting that is now supported as
> > part of the gpio subsystem (using 'set_config()' callback).
> 
> ...
> 
> > +#define ADP5588_DEVICE_ID_MASK 0xF
> > +
> > + /* Configuration Register1 */
> > +#define ADP5588_AUTO_INC       (1 << 7)
> > +#define ADP5588_GPIEM_CFG      (1 << 6)
> > +#define ADP5588_OVR_FLOW_M     (1 << 5)
> > +#define ADP5588_INT_CFG                (1 << 4)
> > +#define ADP5588_OVR_FLOW_IEN   (1 << 3)
> > +#define ADP5588_K_LCK_IM       (1 << 2)
> > +#define ADP5588_GPI_IEN                (1 << 1)
> > +#define ADP5588_KE_IEN         (1 << 0)
> 
> Okay, you add something in the wrong form and then fix it in the other
> patch in the very same series? Please no ping-pong type of changes.
> Squash / rebase your series accordingly.

Well, I thought to just copy it as it was on the platform file and then just fix
it with the rest of the coding styles changes. But I'm fine in fixing it already
in this patch. In fact, there's a lot of defines that are not used (it's just
defining the complete register map) so I can as well get rid of all the stuff 
that is not used anywhere in the driver.

> ...
> 
> > -       ret = adp5588_write(client, KP_GPIO2, KP_SEL(pdata->cols) &
> 0xFF);
> > +       ret = adp5588_write(client, KP_GPIO2, KP_SEL(kpad->cols) &
> 0xFF);
> 
> Do you need these " & 0xFF" parts?

Not sure but probably not. I just kept as it was...

> ...
> 
> > +               /*
> > +                * fw properties keys start from 0 but on the device they
> 
> Firmware
> 

ack...

- Nuno Sá

Re: [PATCH 04/10] input: keyboard: adp5588-keys: add support for fw properties

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 5:04 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Friday, July 8, 2022 4:56 PM
> > On Fri, Jul 8, 2022 at 11:37 AM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > Okay, you add something in the wrong form and then fix it in the other
> > patch in the very same series? Please no ping-pong type of changes.
> > Squash / rebase your series accordingly.
>
> Well, I thought to just copy it as it was on the platform file and then just fix
> it with the rest of the coding styles changes. But I'm fine in fixing it already
> in this patch. In fact, there's a lot of defines that are not used (it's just
> defining the complete register map) so I can as well get rid of all the stuff
> that is not used anywhere in the driver.

This needs to be split to:

1) fix existing
2) move data
3) use that data

Or

1) move data (no other changes, if possible)
2) fix data
3) use it

...

> > > +       ret = adp5588_write(client, KP_GPIO2, KP_SEL(kpad->cols) &
> > 0xFF);
> >
> > Do you need these " & 0xFF" parts?
>
> Not sure but probably not. I just kept as it was...

It's rather for a separate change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 04/10] input: keyboard: adp5588-keys: add support for fw properties

[Nuno Sá]

On Fri, 2022-07-08 at 17:07 +0200, Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 5:04 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Friday, July 8, 2022 4:56 PM
> > > On Fri, Jul 8, 2022 at 11:37 AM Nuno Sá <nuno.sa@analog.com>
> > > wrote:
> 
> ...
> 
> > > Okay, you add something in the wrong form and then fix it in the
> > > other
> > > patch in the very same series? Please no ping-pong type of
> > > changes.
> > > Squash / rebase your series accordingly.
> > 
> > Well, I thought to just copy it as it was on the platform file and
> > then just fix
> > it with the rest of the coding styles changes. But I'm fine in
> > fixing it already
> > in this patch. In fact, there's a lot of defines that are not used
> > (it's just
> > defining the complete register map) so I can as well get rid of all
> > the stuff
> > that is not used anywhere in the driver.
> 
> This needs to be split to:
> 
> 1) fix existing
> 2) move data
> 3) use that data
> 
> Or
> 
> 1) move data (no other changes, if possible)
> 2) fix data
> 3) use it
> 
> ...

Well, I think in the current form is already your option 2... And fix
is a strong word in here, these are coding style changes :)

- Nuno Sá

Re: [PATCH 04/10] input: keyboard: adp5588-keys: add support for fw properties

[kernel test robot]

Hi "Nuno,

I love your patch! Perhaps something to improve:

[auto build test WARNING on dtor-input/next]
[cannot apply to brgl/gpio/for-next hid/for-next linus/master v5.19-rc5 next-20220708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220708-173730
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220709/202207090942.hoWXaKCu-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/75ce2e5c9e3267912dc4bc6773869d135a753e35
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220708-173730
        git checkout 75ce2e5c9e3267912dc4bc6773869d135a753e35
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/input/keyboard/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/gpio/driver.h:5,
                    from drivers/input/keyboard/adp5588-keys.c:13:
   drivers/input/keyboard/adp5588-keys.c: In function 'adp5588_fw_parse':
>> drivers/input/keyboard/adp5588-keys.c:667:39: warning: format '%d' expects argument of type 'int', but argument 4 has type 'long unsigned int' [-Wformat=]
     667 |                 dev_err(&client->dev, "number of unlock keys(%d) > (%d)\n",
         |                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ^~~
   include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
     144 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                        ^~~~~~~
   drivers/input/keyboard/adp5588-keys.c:667:17: note: in expansion of macro 'dev_err'
     667 |                 dev_err(&client->dev, "number of unlock keys(%d) > (%d)\n",
         |                 ^~~~~~~
   drivers/input/keyboard/adp5588-keys.c:667:70: note: format string is defined here
     667 |                 dev_err(&client->dev, "number of unlock keys(%d) > (%d)\n",
         |                                                                     ~^
         |                                                                      |
         |                                                                      int
         |                                                                     %ld


vim +667 drivers/input/keyboard/adp5588-keys.c

   631	
   632	static int adp5588_fw_parse(struct adp5588_kpad *kpad)
   633	{
   634		struct i2c_client *client = kpad->client;
   635		int ret, i;
   636	
   637		ret = matrix_keypad_parse_properties(&client->dev, &kpad->rows,
   638						     &kpad->cols);
   639		if (ret)
   640			return ret;
   641	
   642		if (kpad->rows > ADP5588_ROWS_MAX || kpad->cols > ADP5588_COLS_MAX) {
   643			dev_err(&client->dev, "Invalid nr of rows(%u) or cols(%u)\n",
   644				kpad->rows, kpad->cols);
   645			return -EINVAL;
   646		}
   647	
   648		ret = matrix_keypad_build_keymap(NULL, NULL, kpad->rows, kpad->cols,
   649						 kpad->keycode, kpad->input);
   650		if (ret)
   651			return ret;
   652	
   653		kpad->row_shift = get_count_order(kpad->cols);
   654	
   655		if (device_property_read_bool(&client->dev, "autorepeat"))
   656			__set_bit(EV_REP, kpad->input->evbit);
   657	
   658		kpad->nkeys_unlock = device_property_count_u32(&client->dev,
   659							       "adi,unlock-keys");
   660		if (kpad->nkeys_unlock <= 0) {
   661			/* so that we don't end up enabling key lock */
   662			kpad->nkeys_unlock = 0;
   663			return 0;
   664		}
   665	
   666		if (kpad->nkeys_unlock > ARRAY_SIZE(kpad->unlock_keys)) {
 > 667			dev_err(&client->dev, "number of unlock keys(%d) > (%d)\n",
   668				kpad->nkeys_unlock, ARRAY_SIZE(kpad->unlock_keys));
   669			return -EINVAL;
   670		}
   671	
   672		ret = device_property_read_u32_array(&client->dev, "adi,unlock-keys",
   673						     kpad->unlock_keys,
   674						     kpad->nkeys_unlock);
   675		if (ret)
   676			return ret;
   677	
   678		for (i = 0; i < kpad->nkeys_unlock; i++) {
   679			/*
   680			 * Even though it should be possible (as stated in the datasheet)
   681			 * to use GPIs (which are part of the keys event) as unlock keys,
   682			 * it was not working at all and was leading to overflow events
   683			 * at some point. Hence, for now, let's just allow keys which are
   684			 * part of keypad matrix to be used and if a reliable way of
   685			 * using GPIs is found, this condition can be removed/lightened.
   686			 */
   687			if (kpad->unlock_keys[i] >= kpad->cols * kpad->rows) {
   688				dev_err(&client->dev, "Invalid unlock key(%d)\n",
   689					kpad->unlock_keys[i]);
   690				return -EINVAL;
   691			}
   692	
   693			/*
   694			 * fw properties keys start from 0 but on the device they
   695			 * start from 1.
   696			 */
   697			kpad->unlock_keys[i] += 1;
   698		}
   699	
   700		return 0;
   701	}
   702	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH 05/10] dt-bindings: input: adp5588-keys: add bindings

[Nuno Sá]

Add device tree bindings for the adp5588-keys driver.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 .../bindings/input/adi,adp5588-keys.yaml      | 110 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 111 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml

diff --git a/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml b/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
new file mode 100644
index 000000000000..c079af8a063d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/adi,adp5588-keys.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADP5588 Keypad Controller
+
+maintainers:
+  - Nuno Sá <nuno.sa@analog.com>
+
+description: |
+  Analog Devices Mobile I/O Expander and QWERTY Keypad Controller
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ADP5588.pdf
+
+allOf:
+  - $ref: "/schemas/input/matrix-keymap.yaml#"
+  - $ref: input.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,adp5588-keys
+      - adi,adp5587-keys
+
+  reg:
+    maxItems: 1
+
+  vcc-supply:
+    description: Supply Voltage Input
+
+  reset-gpios:
+    description:
+      If specified, it will be asserted during driver probe. As the line is
+      active low, it should be marked GPIO_ACTIVE_LOW.
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  gpio-controller:
+    description:
+      This property applies if either keypad,num-rows lower than 8 or
+      keypad,num-columns lower than 10.
+
+  '#gpio-cells':
+    const: 2
+
+  interrupt-controller:
+    description:
+      This property applies if either keypad,num-rows lower than 8 or
+      keypad,num-columns lower than 10.
+
+  '#interrupt-cells':
+    const: 2
+
+  adi,unlock-keys:
+    description:
+      Specifies a maximum of 2 keys that can be used to unlock the keypad.
+      If this property is set, the keyboard will be locked and only unlocked
+      after these keys are pressed. If only one key is set, a double click is
+      needed to unlock the keypad.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - keypad,num-rows
+  - keypad,num-columns
+  - linux,keymap
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/input/input.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          adp5588@34 {
+                  compatible = "adi,adp5588-keys";
+                  reg = <0x34>;
+
+                  vcc-supply = <&vcc>;
+                  interrupts = <21 IRQ_TYPE_EDGE_FALLING>;
+                  interrupt-parent = <&gpio>;
+                  reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
+
+                  keypad,num-rows = <1>;
+                  keypad,num-columns = <9>;
+                  linux,keymap = <
+                        MATRIX_KEY(0x00, 0x00, KEY_1)
+                        MATRIX_KEY(0x00, 0x01, KEY_2)
+                        MATRIX_KEY(0x00, 0x02, KEY_3)
+                        MATRIX_KEY(0x00, 0x03, KEY_4)
+                        MATRIX_KEY(0x00, 0x04, KEY_5)
+                        MATRIX_KEY(0x00, 0x05, KEY_6)
+                        MATRIX_KEY(0x00, 0x06, KEY_7)
+                        MATRIX_KEY(0x00, 0x07, KEY_8)
+                        MATRIX_KEY(0x00, 0x08, KEY_9)
+                 >;
+          };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 41037cfd75fe..f9c09f0ed0d4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -548,6 +548,7 @@ M:	Michael Hennerich <michael.hennerich@analog.com>
 S:	Supported
 W:	http://wiki.analog.com/ADP5588
 W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
 F:	drivers/input/keyboard/adp5588-keys.c
 
 ADP8860 BACKLIGHT DRIVER (ADP8860/ADP8861/ADP8863)
-- 
2.37.0

Re: [PATCH 05/10] dt-bindings: input: adp5588-keys: add bindings

[Rob Herring]

On Fri, Jul 08, 2022 at 11:34:43AM +0200, Nuno Sá wrote:
> Add device tree bindings for the adp5588-keys driver.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  .../bindings/input/adi,adp5588-keys.yaml      | 110 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml b/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
> new file mode 100644
> index 000000000000..c079af8a063d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/adi,adp5588-keys.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADP5588 Keypad Controller
> +
> +maintainers:
> +  - Nuno Sá <nuno.sa@analog.com>
> +
> +description: |
> +  Analog Devices Mobile I/O Expander and QWERTY Keypad Controller
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ADP5588.pdf
> +
> +allOf:
> +  - $ref: "/schemas/input/matrix-keymap.yaml#"
> +  - $ref: input.yaml#

Be consistent with paths and quotes. Don't need quotes. Use either full 
path or no path.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adp5588-keys
> +      - adi,adp5587-keys

'-keys' is redundant. The device doesn't do anything else. IOW, it's not 
sub-block of an SoC.

> +
> +  reg:
> +    maxItems: 1
> +
> +  vcc-supply:
> +    description: Supply Voltage Input
> +
> +  reset-gpios:
> +    description:
> +      If specified, it will be asserted during driver probe. As the line is
> +      active low, it should be marked GPIO_ACTIVE_LOW.
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  gpio-controller:
> +    description:
> +      This property applies if either keypad,num-rows lower than 8 or
> +      keypad,num-columns lower than 10.
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  interrupt-controller:
> +    description:
> +      This property applies if either keypad,num-rows lower than 8 or
> +      keypad,num-columns lower than 10.
> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  adi,unlock-keys:
> +    description:
> +      Specifies a maximum of 2 keys that can be used to unlock the keypad.
> +      If this property is set, the keyboard will be locked and only unlocked
> +      after these keys are pressed. If only one key is set, a double click is
> +      needed to unlock the keypad.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    maxItems: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - keypad,num-rows
> +  - keypad,num-columns
> +  - linux,keymap
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/input/input.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          adp5588@34 {
> +                  compatible = "adi,adp5588-keys";
> +                  reg = <0x34>;
> +
> +                  vcc-supply = <&vcc>;
> +                  interrupts = <21 IRQ_TYPE_EDGE_FALLING>;
> +                  interrupt-parent = <&gpio>;
> +                  reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
> +
> +                  keypad,num-rows = <1>;
> +                  keypad,num-columns = <9>;
> +                  linux,keymap = <
> +                        MATRIX_KEY(0x00, 0x00, KEY_1)
> +                        MATRIX_KEY(0x00, 0x01, KEY_2)
> +                        MATRIX_KEY(0x00, 0x02, KEY_3)
> +                        MATRIX_KEY(0x00, 0x03, KEY_4)
> +                        MATRIX_KEY(0x00, 0x04, KEY_5)
> +                        MATRIX_KEY(0x00, 0x05, KEY_6)
> +                        MATRIX_KEY(0x00, 0x06, KEY_7)
> +                        MATRIX_KEY(0x00, 0x07, KEY_8)
> +                        MATRIX_KEY(0x00, 0x08, KEY_9)
> +                 >;
> +          };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 41037cfd75fe..f9c09f0ed0d4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -548,6 +548,7 @@ M:	Michael Hennerich <michael.hennerich@analog.com>
>  S:	Supported
>  W:	http://wiki.analog.com/ADP5588
>  W:	https://ez.analog.com/linux-software-drivers
> +F:	Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
>  F:	drivers/input/keyboard/adp5588-keys.c
>  
>  ADP8860 BACKLIGHT DRIVER (ADP8860/ADP8861/ADP8863)
> -- 
> 2.37.0
> 
> 

[PATCH 06/10] input: keyboard: adp5588-keys: do not check for irq presence

[Nuno Sá]

There's no need for an extra check for 'client-irq'. Just let it fail when
calling 'request_irq()'.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index e831b6f66b55..1395d2449ec0 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -715,11 +715,6 @@ static int adp5588_probe(struct i2c_client *client,
 		return -EIO;
 	}
 
-	if (!client->irq) {
-		dev_err(&client->dev, "no IRQ?\n");
-		return -EINVAL;
-	}
-
 	kpad = devm_kzalloc(&client->dev, sizeof(*kpad), GFP_KERNEL);
 	if (!kpad)
 		return -ENOMEM;
-- 
2.37.0

[PATCH 07/10] input: keyboard: adp5588-keys: fix coding style warnings

[Nuno Sá]

Just some code cleanup regarding coding style. No functional changes
intended.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 93 +++++++++++++--------------
 1 file changed, 45 insertions(+), 48 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 1395d2449ec0..32c023dde7a6 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -29,16 +29,16 @@
 #define CFG 0x01		/* Configuration Register1 */
 #define INT_STAT 0x02		/* Interrupt Status Register */
 #define KEY_LCK_EC_STAT 0x03	/* Key Lock and Event Counter Register */
-#define Key_EVENTA 0x04		/* Key Event Register A */
-#define Key_EVENTB 0x05		/* Key Event Register B */
-#define Key_EVENTC 0x06		/* Key Event Register C */
-#define Key_EVENTD 0x07		/* Key Event Register D */
-#define Key_EVENTE 0x08		/* Key Event Register E */
-#define Key_EVENTF 0x09		/* Key Event Register F */
-#define Key_EVENTG 0x0A		/* Key Event Register G */
-#define Key_EVENTH 0x0B		/* Key Event Register H */
-#define Key_EVENTI 0x0C		/* Key Event Register I */
-#define Key_EVENTJ 0x0D		/* Key Event Register J */
+#define KEY_EVENTA 0x04		/* Key Event Register A */
+#define KEY_EVENTB 0x05		/* Key Event Register B */
+#define KEY_EVENTC 0x06		/* Key Event Register C */
+#define KEY_EVENTD 0x07		/* Key Event Register D */
+#define KEY_EVENTE 0x08		/* Key Event Register E */
+#define KEY_EVENTF 0x09		/* Key Event Register F */
+#define KEY_EVENTG 0x0A		/* Key Event Register G */
+#define KEY_EVENTH 0x0B		/* Key Event Register H */
+#define KEY_EVENTI 0x0C		/* Key Event Register I */
+#define KEY_EVENTJ 0x0D		/* Key Event Register J */
 #define KP_LCK_TMR 0x0E		/* Keypad Lock1 to Lock2 Timer */
 #define UNLOCK1 0x0F		/* Unlock Key1 */
 #define UNLOCK2 0x10		/* Unlock Key2 */
@@ -66,9 +66,9 @@
 #define GPIO_INT_LVL1 0x26	/* GPIO Edge/Level Detect */
 #define GPIO_INT_LVL2 0x27	/* GPIO Edge/Level Detect */
 #define GPIO_INT_LVL3 0x28	/* GPIO Edge/Level Detect */
-#define Debounce_DIS1 0x29	/* Debounce Disable */
-#define Debounce_DIS2 0x2A	/* Debounce Disable */
-#define Debounce_DIS3 0x2B	/* Debounce Disable */
+#define DEBOUNCE_DIS1 0x29	/* Debounce Disable */
+#define DEBOUNCE_DIS2 0x2A	/* Debounce Disable */
+#define DEBOUNCE_DIS3 0x2B	/* Debounce Disable */
 #define GPIO_PULL1 0x2C		/* GPIO Pull Disable */
 #define GPIO_PULL2 0x2D		/* GPIO Pull Disable */
 #define GPIO_PULL3 0x2E		/* GPIO Pull Disable */
@@ -91,25 +91,25 @@
 #define ADP5588_DEVICE_ID_MASK	0xF
 
  /* Configuration Register1 */
-#define ADP5588_AUTO_INC	(1 << 7)
-#define ADP5588_GPIEM_CFG	(1 << 6)
-#define ADP5588_OVR_FLOW_M	(1 << 5)
-#define ADP5588_INT_CFG		(1 << 4)
-#define ADP5588_OVR_FLOW_IEN	(1 << 3)
-#define ADP5588_K_LCK_IM	(1 << 2)
-#define ADP5588_GPI_IEN		(1 << 1)
-#define ADP5588_KE_IEN		(1 << 0)
+#define ADP5588_AUTO_INC	BIT(7)
+#define ADP5588_GPIEM_CFG	BIT(6)
+#define ADP5588_OVR_FLOW_M	BIT(5)
+#define ADP5588_INT_CFG		BIT(4)
+#define ADP5588_OVR_FLOW_IEN	BIT(3)
+#define ADP5588_K_LCK_IM	BIT(2)
+#define ADP5588_GPI_IEN		BIT(1)
+#define ADP5588_KE_IEN		BIT(0)
 
 /* Interrupt Status Register */
-#define ADP5588_CMP2_INT	(1 << 5)
-#define ADP5588_CMP1_INT	(1 << 4)
-#define ADP5588_OVR_FLOW_INT	(1 << 3)
-#define ADP5588_K_LCK_INT	(1 << 2)
-#define ADP5588_GPI_INT		(1 << 1)
-#define ADP5588_KE_INT		(1 << 0)
+#define ADP5588_CMP2_INT	BIT(5)
+#define ADP5588_CMP1_INT	BIT(4)
+#define ADP5588_OVR_FLOW_INT	BIT(3)
+#define ADP5588_K_LCK_INT	BIT(2)
+#define ADP5588_GPI_INT		BIT(1)
+#define ADP5588_KE_INT		BIT(0)
 
 /* Key Lock and Event Counter Register */
-#define ADP5588_K_LCK_EN	(1 << 6)
+#define ADP5588_K_LCK_EN	BIT(6)
 #define ADP5588_LCK21		0x30
 #define ADP5588_KEC		0xF
 
@@ -158,10 +158,10 @@
 #define ADP5588_GPIMAPSIZE_MAX (GPI_PIN_END - GPI_PIN_BASE + 1)
 
 /* Key Event Register xy */
-#define KEY_EV_PRESSED		(1 << 7)
+#define KEY_EV_PRESSED		BIT(7)
 #define KEY_EV_MASK		(0x7F)
 
-#define KP_SEL(x)		(0xFFFF >> (16 - x))	/* 2^x-1 */
+#define KP_SEL(x)		(0xFFFF >> (16 - (x)))	/* 2^x-1 */
 
 #define KEYP_MAX_EVENT		10
 
@@ -210,7 +210,7 @@ static int adp5588_write(struct i2c_client *client, u8 reg, u8 val)
 	return i2c_smbus_write_byte_data(client, reg, val);
 }
 
-static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned off)
+static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned int off)
 {
 	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
 	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
@@ -230,7 +230,7 @@ static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned off)
 }
 
 static void adp5588_gpio_set_value(struct gpio_chip *chip,
-				   unsigned off, int val)
+				   unsigned int off, int val)
 {
 	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
 	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
@@ -243,8 +243,7 @@ static void adp5588_gpio_set_value(struct gpio_chip *chip,
 	else
 		kpad->dat_out[bank] &= ~bit;
 
-	adp5588_write(kpad->client, GPIO_DAT_OUT1 + bank,
-			   kpad->dat_out[bank]);
+	adp5588_write(kpad->client, GPIO_DAT_OUT1 + bank, kpad->dat_out[bank]);
 
 	mutex_unlock(&kpad->gpio_lock);
 }
@@ -283,7 +282,7 @@ static int adp5588_gpio_set_config(struct gpio_chip *chip,  unsigned int off,
 	return ret;
 }
 
-static int adp5588_gpio_direction_input(struct gpio_chip *chip, unsigned off)
+static int adp5588_gpio_direction_input(struct gpio_chip *chip, unsigned int off)
 {
 	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
 	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
@@ -301,7 +300,7 @@ static int adp5588_gpio_direction_input(struct gpio_chip *chip, unsigned off)
 }
 
 static int adp5588_gpio_direction_output(struct gpio_chip *chip,
-					 unsigned off, int val)
+					 unsigned int off, int val)
 {
 	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
 	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
@@ -318,12 +317,11 @@ static int adp5588_gpio_direction_output(struct gpio_chip *chip,
 		kpad->dat_out[bank] &= ~bit;
 
 	ret = adp5588_write(kpad->client, GPIO_DAT_OUT1 + bank,
-				 kpad->dat_out[bank]);
+			    kpad->dat_out[bank]);
 	if (ret)
 		goto out_unlock;
 
-	ret = adp5588_write(kpad->client, GPIO_DIR1 + bank,
-				 kpad->dir[bank]);
+	ret = adp5588_write(kpad->client, GPIO_DIR1 + bank, kpad->dir[bank]);
 
 out_unlock:
 	mutex_unlock(&kpad->gpio_lock);
@@ -512,7 +510,7 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 	int i;
 
 	for (i = 0; i < ev_cnt; i++) {
-		int key = adp5588_read(kpad->client, Key_EVENTA + i);
+		int key = adp5588_read(kpad->client, KEY_EVENTA + i);
 		int key_val = key & KEY_EV_MASK;
 		int key_press = key & KEY_EV_PRESSED;
 
@@ -612,21 +610,20 @@ static int adp5588_setup(struct adp5588_kpad *kpad)
 	}
 
 	for (i = 0; i < KEYP_MAX_EVENT; i++) {
-		ret = adp5588_read(client, Key_EVENTA);
+		ret = adp5588_read(client, KEY_EVENTA);
 		if (ret)
 			return ret;
 	}
 
 	ret = adp5588_write(client, INT_STAT,
-				ADP5588_CMP2_INT | ADP5588_CMP1_INT |
-				ADP5588_OVR_FLOW_INT | ADP5588_K_LCK_INT |
-				ADP5588_GPI_INT | ADP5588_KE_INT); /* Status is W1C */
+			    ADP5588_CMP2_INT | ADP5588_CMP1_INT |
+			    ADP5588_OVR_FLOW_INT | ADP5588_K_LCK_INT |
+			    ADP5588_GPI_INT | ADP5588_KE_INT); /* Status is W1C */
 	if (ret)
 		return ret;
 
 	return adp5588_write(client, CFG, ADP5588_INT_CFG |
-					  ADP5588_OVR_FLOW_IEN |
-					  ADP5588_KE_IEN);
+			     ADP5588_OVR_FLOW_IEN | ADP5588_KE_IEN);
 }
 
 static int adp5588_fw_parse(struct adp5588_kpad *kpad)
@@ -710,7 +707,7 @@ static int adp5588_probe(struct i2c_client *client,
 	int error;
 
 	if (!i2c_check_functionality(client->adapter,
-					I2C_FUNC_SMBUS_BYTE_DATA)) {
+				     I2C_FUNC_SMBUS_BYTE_DATA)) {
 		dev_err(&client->dev, "SMBUS Byte Data not Supported\n");
 		return -EIO;
 	}
@@ -734,7 +731,7 @@ static int adp5588_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	revid = (u8) ret & ADP5588_DEVICE_ID_MASK;
+	revid = ret & ADP5588_DEVICE_ID_MASK;
 	if (WA_DELAYED_READOUT_REVID(revid))
 		kpad->delay = msecs_to_jiffies(WA_DELAYED_READOUT_TIME);
 
-- 
2.37.0

Re: [PATCH 07/10] input: keyboard: adp5588-keys: fix coding style warnings

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote:
>
> Just some code cleanup regarding coding style. No functional changes
> intended.

...

> -#define ADP5588_KE_IEN         (1 << 0)

> +#define ADP5588_KE_IEN         BIT(0)

This is actually a change. And if there wasn't bits.h included, you
would do it as well.

...

>  #define ADP5588_KEC            0xF

Probably then GENMASK() ?

...

>  #define KEY_EV_MASK            (0x7F)

GENMASK()

...

> -#define KP_SEL(x)              (0xFFFF >> (16 - x))    /* 2^x-1 */
> +#define KP_SEL(x)              (0xFFFF >> (16 - (x)))  /* 2^x-1 */

Ditto.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 07/10] input: keyboard: adp5588-keys: fix coding style warnings

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, July 8, 2022 4:50 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO
> SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux-
> input@vger.kernel.org>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>; Bartosz Golaszewski
> <brgl@bgdev.pl>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>
> Subject: Re: [PATCH 07/10] input: keyboard: adp5588-keys: fix coding
> style warnings
> 
> [External]
> 
> On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > Just some code cleanup regarding coding style. No functional
> changes
> > intended.
> 
> ...
> 
> > -#define ADP5588_KE_IEN         (1 << 0)
> 
> > +#define ADP5588_KE_IEN         BIT(0)
> 
> This is actually a change. And if there wasn't bits.h included, you
> would do it as well.
> 

You mean because of it being unsigned now? Well, I guess it's true
but in practice I don't think it will have any side effect..

> ...
> 
> >  #define ADP5588_KEC            0xF
> 
> Probably then GENMASK() ?

Makes sense yes...

> ...
> 
> >  #define KEY_EV_MASK            (0x7F)
> 
> GENMASK()
> 
> ...
> 
> > -#define KP_SEL(x)              (0xFFFF >> (16 - x))    /* 2^x-1 */
> > +#define KP_SEL(x)              (0xFFFF >> (16 - (x)))  /* 2^x-1 */
> 
> Ditto.
> 

- Nuno Sá

Re: [PATCH 07/10] input: keyboard: adp5588-keys: fix coding style warnings

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 5:05 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Friday, July 8, 2022 4:50 PM
> > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > > -#define ADP5588_KE_IEN         (1 << 0)
> >
> > > +#define ADP5588_KE_IEN         BIT(0)
> >
> > This is actually a change. And if there wasn't bits.h included, you
> > would do it as well.
> >
>
> You mean because of it being unsigned now? Well, I guess it's true
> but in practice I don't think it will have any side effect..

int --> unsigned long

FYI: We used to have bugs with this conversation in the past.

So, please amend the commit message accordingly. And as I mentioned,
this should probably be done before the rest of this patch.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 08/10] input: keyboard: adp5588-keys: add optional reset gpio

[Nuno Sá]

Optionally reset the device during probe.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 32c023dde7a6..57bc4e52ec21 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -10,6 +10,7 @@
 
 #include <linux/delay.h>
 #include <linux/errno.h>
+#include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
@@ -702,6 +703,7 @@ static int adp5588_probe(struct i2c_client *client,
 {
 	struct adp5588_kpad *kpad;
 	struct input_dev *input;
+	struct gpio_desc *gpio;
 	unsigned int revid;
 	int ret;
 	int error;
@@ -727,6 +729,16 @@ static int adp5588_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
+	gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+
+	if (gpio) {
+		fsleep(30);
+		gpiod_set_value_cansleep(gpio, 0);
+		fsleep(60);
+	}
+
 	ret = adp5588_read(client, DEV_ID);
 	if (ret < 0)
 		return ret;
-- 
2.37.0

Re: [PATCH 08/10] input: keyboard: adp5588-keys: add optional reset gpio

[Linus Walleij]

On Fri, Jul 8, 2022 at 11:34 AM Nuno Sá <nuno.sa@analog.com> wrote:

> Optionally reset the device during probe.
>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 09/10] input: keyboard: adp5588-keys: add regulator support

[Nuno Sá]

Support feeding VCC through a regulator.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 57bc4e52ec21..b4414f5f3cfb 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -23,6 +23,7 @@
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/timekeeping.h>
 
@@ -698,12 +699,18 @@ static int adp5588_fw_parse(struct adp5588_kpad *kpad)
 	return 0;
 }
 
+static void adp5588_disable_regulator(void *reg)
+{
+	regulator_disable(reg);
+}
+
 static int adp5588_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct adp5588_kpad *kpad;
 	struct input_dev *input;
 	struct gpio_desc *gpio;
+	struct regulator *vcc;
 	unsigned int revid;
 	int ret;
 	int error;
@@ -729,6 +736,19 @@ static int adp5588_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
+	vcc = devm_regulator_get(&client->dev, "vcc");
+	if (IS_ERR(vcc))
+		return PTR_ERR(vcc);
+
+	ret = regulator_enable(vcc);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(&client->dev, adp5588_disable_regulator,
+				       vcc);
+	if (ret)
+		return ret;
+
 	gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(gpio))
 		return PTR_ERR(gpio);
-- 
2.37.0

Re: [PATCH 09/10] input: keyboard: adp5588-keys: add regulator support

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 11:37 AM Nuno Sá <nuno.sa@analog.com> wrote:
>
> Support feeding VCC through a regulator.

...

> +       ret = devm_add_action_or_reset(&client->dev, adp5588_disable_regulator,
> +                                      vcc);

One line?

> +       if (ret)
> +               return ret;


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 09/10] input: keyboard: adp5588-keys: add regulator support

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, July 8, 2022 4:47 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO
> SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux-
> input@vger.kernel.org>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>; Bartosz Golaszewski
> <brgl@bgdev.pl>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>
> Subject: Re: [PATCH 09/10] input: keyboard: adp5588-keys: add
> regulator support
> 
> [External]
> 
> On Fri, Jul 8, 2022 at 11:37 AM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > Support feeding VCC through a regulator.
> 
> ...
> 
> > +       ret = devm_add_action_or_reset(&client->dev,
> adp5588_disable_regulator,
> > +                                      vcc);
> 
> One line?
> 

Happy to do it but I guess it depends on the maintainer... If no one
complains, I will change it on v2

- Nuno Sá
 

[PATCH 10/10] input: keyboard: adp5588-keys: Use new PM macros

[Nuno Sá]

With the new PM macros (DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr()), the
compiler has visibility to see that the functions are not used when
!CONFIG_PM and hence, remove the dead code. As such, there's no need
for '__maybe_unused'.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index b4414f5f3cfb..17360b193e1f 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -814,7 +814,7 @@ static int adp5588_remove(struct i2c_client *client)
 	return 0;
 }
 
-static int __maybe_unused adp5588_suspend(struct device *dev)
+static int adp5588_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 
@@ -823,7 +823,7 @@ static int __maybe_unused adp5588_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused adp5588_resume(struct device *dev)
+static int adp5588_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 
@@ -832,7 +832,7 @@ static int __maybe_unused adp5588_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(adp5588_dev_pm_ops, adp5588_suspend, adp5588_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(adp5588_dev_pm_ops, adp5588_suspend, adp5588_resume);
 
 static const struct i2c_device_id adp5588_id[] = {
 	{ "adp5588-keys", 0 },
@@ -852,7 +852,7 @@ static struct i2c_driver adp5588_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
 		.of_match_table = adp5588_of_match,
-		.pm   = &adp5588_dev_pm_ops,
+		.pm   = pm_sleep_ptr(&adp5588_dev_pm_ops),
 	},
 	.probe    = adp5588_probe,
 	.remove   = adp5588_remove,
-- 
2.37.0

Re: [PATCH 00/10] adp5588-keys refactor and fw properties support

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote:
>
> The main goal of this patchset is to remove platform data and replace it by
>
> firmware properties. Original discussion in [1].
>
>
>
> While in here, some refactor was done to the driver. The most noticeable one
>
> is to replace the GPIs events handling by irqchip support so that this gpi
>
> keys can be "consumed" by the gpio-keys driver (also as suggested in [1]).
>
> With this, the gpio-adp5588 can be removed. This change comes first so that
>
> we can already remove some platform data variables making it easier to
>
> completly replace it by firmware properties further down in the series.
>
>
>
> As there's no users of the platform data, I just replace it in a single
>
> patch as there's no point in having support for both (even though it might
>
> be harder to review the patch as-is).
>
>
>
> Special note to the gpio-adp5588 driver removal. I'm aware of some changes
>
> to the driver in [2]. These changes are in the gpio tree and this patchset
>
> is naturally based on the input tree which means that patch 2 will
>
> not apply. So, I'm not really sure how to handle this. I guess in this
>
> case the conflict is easy to handle :) but just let me know on how to
>
> proceed in here if there's anything for me to do.

Resolving conflict like this is easy, just input subsys maintainer
needs to tell Linus about it to Linus in PR.

-- 
With Best Regards,
Andy Shevchenko