[PATCH 0/8] gpio-tqmx86 fixes

[PATCH 0/8] gpio-tqmx86 fixes

[Matthias Schiffer]

This is the first series of improvements to the tqmx86 GPIO driver,
which fixes some long-standing bugs - in particular, IRQ_TYPE_EDGE_BOTH
can never have worked correctly.

Other patches in the series are code cleanup, which is included as it
makes the actual fixes much nicer. I have included the same Fixes tag in
all commits, as they will need to be cherry-picked together.

A second series with new features (changing GPIO directions, support
more GPIOs on SMARC TQMx86 modules) will be submitted when the fixes
have been reviewed and merged.

Gregor Herburger (1):
  gpio: tqmx86: fix typo in Kconfig label

Matthias Schiffer (7):
  gpio: tqmx86: introduce shadow register for GPIO output value
  gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match
    regmap API
  gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper
  gpio: tqmx86: add macros for interrupt configuration
  gpio: tqmx86: store IRQ triggers without offsetting index
  gpio: tqmx86: store IRQ trigger type and unmask status separately
  gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

 drivers/gpio/Kconfig       |   2 +-
 drivers/gpio/gpio-tqmx86.c | 151 ++++++++++++++++++++++++++-----------
 2 files changed, 106 insertions(+), 47 deletions(-)

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 1/8] gpio: tqmx86: fix typo in Kconfig label

[Matthias Schiffer]

From: Gregor Herburger <gregor.herburger@tq-group.com>

Fix description for GPIO_TQMX86 from QTMX86 to TQMx86.

Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
Signed-off-by: Gregor Herburger <gregor.herburger@tq-group.com>
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3dbddec070281..1c28a48915bb2 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1576,7 +1576,7 @@ config GPIO_TPS68470
 	  are "output only" GPIOs.
 
 config GPIO_TQMX86
-	tristate "TQ-Systems QTMX86 GPIO"
+	tristate "TQ-Systems TQMx86 GPIO"
 	depends on MFD_TQMX86 || COMPILE_TEST
 	depends on HAS_IOPORT_MAP
 	select GPIOLIB_IRQCHIP
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 2/8] gpio: tqmx86: introduce shadow register for GPIO output value

[Matthias Schiffer]

The TQMx86 GPIO controller uses the same register address for input and
output data. Reading the register will always return current inputs
rather than the previously set outputs (regardless of the current
direction setting). Therefore, using a RMW pattern does not make sense
when setting output values. Instead, the previously set output register
value needs to be stored as a shadow register.

As there is no reliable way to get the current output values from the
hardware, also initialize all channels to 0, to ensure that stored and
actual output values match. This should usually not have any effect in
practise, as the TQMx86 UEFI sets all outputs to 0 during boot.

Also prepare for extension of the driver to more than 8 GPIOs by using
DECLARE_BITMAP.

Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 3a28c1f273c39..b7e2dbbdc4ebe 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -6,6 +6,7 @@
  *   Vadim V.Vlasov <vvlasov@dev.rtsoft.ru>
  */
 
+#include <linux/bitmap.h>
 #include <linux/bitops.h>
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
@@ -38,6 +39,7 @@ struct tqmx86_gpio_data {
 	void __iomem		*io_base;
 	int			irq;
 	raw_spinlock_t		spinlock;
+	DECLARE_BITMAP(output, TQMX86_NGPIO);
 	u8			irq_type[TQMX86_NGPI];
 };
 
@@ -64,15 +66,10 @@ static void tqmx86_gpio_set(struct gpio_chip *chip, unsigned int offset,
 {
 	struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
 	unsigned long flags;
-	u8 val;
 
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	val = tqmx86_gpio_read(gpio, TQMX86_GPIOD);
-	if (value)
-		val |= BIT(offset);
-	else
-		val &= ~BIT(offset);
-	tqmx86_gpio_write(gpio, val, TQMX86_GPIOD);
+	__assign_bit(offset, gpio->output, value);
+	tqmx86_gpio_write(gpio, bitmap_get_value8(gpio->output, 0), TQMX86_GPIOD);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 }
 
@@ -277,6 +274,13 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
 
 	tqmx86_gpio_write(gpio, (u8)~TQMX86_DIR_INPUT_MASK, TQMX86_GPIODD);
 
+	/*
+	 * Reading the previous output state is not possible with TQMx86 hardware.
+	 * Initialize all outputs to 0 to have a defined state that matches the
+	 * shadow register.
+	 */
+	tqmx86_gpio_write(gpio, 0, TQMX86_GPIOD);
+
 	chip = &gpio->chip;
 	chip->label = "gpio-tqmx86";
 	chip->owner = THIS_MODULE;
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 3/8] gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match regmap API

[Matthias Schiffer]

Conversion to actually use regmap does not seem useful for this driver,
as regmap can't properly represent separate read-only and write-only
registers at the same address, but we can at least match the API to make
the code clearer.

No functional change intended.

Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index b7e2dbbdc4ebe..613ab9ef2e744 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -48,8 +48,8 @@ static u8 tqmx86_gpio_read(struct tqmx86_gpio_data *gd, unsigned int reg)
 	return ioread8(gd->io_base + reg);
 }
 
-static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, u8 val,
-			      unsigned int reg)
+static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, unsigned int reg,
+			      u8 val)
 {
 	iowrite8(val, gd->io_base + reg);
 }
@@ -69,7 +69,7 @@ static void tqmx86_gpio_set(struct gpio_chip *chip, unsigned int offset,
 
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
 	__assign_bit(offset, gpio->output, value);
-	tqmx86_gpio_write(gpio, bitmap_get_value8(gpio->output, 0), TQMX86_GPIOD);
+	tqmx86_gpio_write(gpio, TQMX86_GPIOD, bitmap_get_value8(gpio->output, 0));
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 }
 
@@ -117,7 +117,7 @@ static void tqmx86_gpio_irq_mask(struct irq_data *data)
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
 	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
 	gpiic &= ~mask;
-	tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
+	tqmx86_gpio_write(gpio, TQMX86_GPIIC, gpiic);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 	gpiochip_disable_irq(&gpio->chip, irqd_to_hwirq(data));
 }
@@ -137,7 +137,7 @@ static void tqmx86_gpio_irq_unmask(struct irq_data *data)
 	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
 	gpiic &= ~mask;
 	gpiic |= gpio->irq_type[offset] << (offset * TQMX86_GPII_BITS);
-	tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
+	tqmx86_gpio_write(gpio, TQMX86_GPIIC, gpiic);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 }
 
@@ -170,7 +170,7 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
 	gpiic &= ~((TQMX86_GPII_MASK) << (offset * TQMX86_GPII_BITS));
 	gpiic |= new_type << (offset * TQMX86_GPII_BITS);
-	tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
+	tqmx86_gpio_write(gpio, TQMX86_GPIIC, gpiic);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 
 	return 0;
@@ -188,7 +188,7 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_enter(irq_chip, desc);
 
 	irq_status = tqmx86_gpio_read(gpio, TQMX86_GPIIS);
-	tqmx86_gpio_write(gpio, irq_status, TQMX86_GPIIS);
+	tqmx86_gpio_write(gpio, TQMX86_GPIIS, irq_status);
 
 	irq_bits = irq_status;
 	for_each_set_bit(i, &irq_bits, TQMX86_NGPI)
@@ -272,14 +272,14 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
 	raw_spin_lock_init(&gpio->spinlock);
 	gpio->io_base = io_base;
 
-	tqmx86_gpio_write(gpio, (u8)~TQMX86_DIR_INPUT_MASK, TQMX86_GPIODD);
+	tqmx86_gpio_write(gpio, TQMX86_GPIODD, (u8)~TQMX86_DIR_INPUT_MASK);
 
 	/*
 	 * Reading the previous output state is not possible with TQMx86 hardware.
 	 * Initialize all outputs to 0 to have a defined state that matches the
 	 * shadow register.
 	 */
-	tqmx86_gpio_write(gpio, 0, TQMX86_GPIOD);
+	tqmx86_gpio_write(gpio, TQMX86_GPIOD, 0);
 
 	chip = &gpio->chip;
 	chip->label = "gpio-tqmx86";
@@ -300,11 +300,11 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
 		u8 irq_status;
 
 		/* Mask all interrupts */
-		tqmx86_gpio_write(gpio, 0, TQMX86_GPIIC);
+		tqmx86_gpio_write(gpio, TQMX86_GPIIC, 0);
 
 		/* Clear all pending interrupts */
 		irq_status = tqmx86_gpio_read(gpio, TQMX86_GPIIS);
-		tqmx86_gpio_write(gpio, irq_status, TQMX86_GPIIS);
+		tqmx86_gpio_write(gpio, TQMX86_GPIIS, irq_status);
 
 		girq = &chip->irq;
 		gpio_irq_chip_set_chip(girq, &tqmx86_gpio_irq_chip);
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 4/8] gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper

[Matthias Schiffer]

Simplify a lot of code in the driver by introducing helpers for the
common RMW pattern. No tqmx86_gpio_update_bits() function with builtin
locking is added, as it would become redundant with the following fixes,
which further consolidate interrupt configuration register setup.

No functional change intended.

Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 40 ++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 613ab9ef2e744..7a851e1730dd1 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -54,6 +54,17 @@ static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, unsigned int reg,
 	iowrite8(val, gd->io_base + reg);
 }
 
+static void _tqmx86_gpio_update_bits(struct tqmx86_gpio_data *gd,
+				     unsigned int reg, u8 mask, u8 val)
+{
+	u8 tmp = tqmx86_gpio_read(gd, reg);
+
+	tmp &= ~mask;
+	tmp |= val & mask;
+
+	tqmx86_gpio_write(gd, reg, tmp);
+}
+
 static int tqmx86_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
@@ -110,15 +121,13 @@ static void tqmx86_gpio_irq_mask(struct irq_data *data)
 	struct tqmx86_gpio_data *gpio = gpiochip_get_data(
 		irq_data_get_irq_chip_data(data));
 	unsigned long flags;
-	u8 gpiic, mask;
+	u8 mask;
 
 	mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS);
-
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
-	gpiic &= ~mask;
-	tqmx86_gpio_write(gpio, TQMX86_GPIIC, gpiic);
+	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, 0);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
+
 	gpiochip_disable_irq(&gpio->chip, irqd_to_hwirq(data));
 }
 
@@ -128,16 +137,14 @@ static void tqmx86_gpio_irq_unmask(struct irq_data *data)
 	struct tqmx86_gpio_data *gpio = gpiochip_get_data(
 		irq_data_get_irq_chip_data(data));
 	unsigned long flags;
-	u8 gpiic, mask;
-
-	mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS);
+	u8 mask, val;
 
 	gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(data));
+
+	mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS);
+	val = gpio->irq_type[offset] << (offset * TQMX86_GPII_BITS);
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
-	gpiic &= ~mask;
-	gpiic |= gpio->irq_type[offset] << (offset * TQMX86_GPII_BITS);
-	tqmx86_gpio_write(gpio, TQMX86_GPIIC, gpiic);
+	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 }
 
@@ -148,7 +155,7 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 	unsigned int offset = (data->hwirq - TQMX86_NGPO);
 	unsigned int edge_type = type & IRQF_TRIGGER_MASK;
 	unsigned long flags;
-	u8 new_type, gpiic;
+	u8 new_type, mask, val;
 
 	switch (edge_type) {
 	case IRQ_TYPE_EDGE_RISING:
@@ -166,11 +173,10 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 
 	gpio->irq_type[offset] = new_type;
 
+	mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS);
+	val = new_type << (offset * TQMX86_GPII_BITS);
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
-	gpiic &= ~((TQMX86_GPII_MASK) << (offset * TQMX86_GPII_BITS));
-	gpiic |= new_type << (offset * TQMX86_GPII_BITS);
-	tqmx86_gpio_write(gpio, TQMX86_GPIIC, gpiic);
+	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 
 	return 0;
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 5/8] gpio: tqmx86: add macros for interrupt configuration

[Matthias Schiffer]

The new macros introduce a distinction between flags used by the driver
to represent interrupt configuration (TQMX86_INT_ prefix) and the flags
actually written to the hardware (TQMX86_GPII_ prefix). The
TQMX86_INT_TRIG_ values are chosen such that they can be converted to
register values by a simple shift (in the TQMX86_GPII_CONFIG() macro),
at least for the NONE/FALLING/RISING triggers.

No functional change intended.

Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 7a851e1730dd1..d6e77f604f4df 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -29,10 +29,14 @@
 #define TQMX86_GPIIC	3	/* GPI Interrupt Configuration Register */
 #define TQMX86_GPIIS	4	/* GPI Interrupt Status Register */
 
-#define TQMX86_GPII_FALLING	BIT(0)
-#define TQMX86_GPII_RISING	BIT(1)
-#define TQMX86_GPII_MASK	(BIT(0) | BIT(1))
-#define TQMX86_GPII_BITS	2
+#define TQMX86_INT_TRIG_NONE	0x0
+#define TQMX86_INT_TRIG_FALLING	0x1
+#define TQMX86_INT_TRIG_RISING	0x2
+#define TQMX86_INT_TRIG_BOTH	0x3
+#define TQMX86_INT_TRIG_MASK	0x3
+
+#define TQMX86_GPII_CONFIG(i, v)	((v) << (2 * (i)))
+#define TQMX86_GPII_MASK(i)		TQMX86_GPII_CONFIG(i, TQMX86_INT_TRIG_MASK)
 
 struct tqmx86_gpio_data {
 	struct gpio_chip	chip;
@@ -123,7 +127,7 @@ static void tqmx86_gpio_irq_mask(struct irq_data *data)
 	unsigned long flags;
 	u8 mask;
 
-	mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS);
+	mask = TQMX86_GPII_MASK(offset);
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
 	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, 0);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
@@ -141,8 +145,8 @@ static void tqmx86_gpio_irq_unmask(struct irq_data *data)
 
 	gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(data));
 
-	mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS);
-	val = gpio->irq_type[offset] << (offset * TQMX86_GPII_BITS);
+	mask = TQMX86_GPII_MASK(offset);
+	val = TQMX86_GPII_CONFIG(offset, gpio->irq_type[offset]);
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
 	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
@@ -159,13 +163,13 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 
 	switch (edge_type) {
 	case IRQ_TYPE_EDGE_RISING:
-		new_type = TQMX86_GPII_RISING;
+		new_type = TQMX86_INT_TRIG_RISING;
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		new_type = TQMX86_GPII_FALLING;
+		new_type = TQMX86_INT_TRIG_FALLING;
 		break;
 	case IRQ_TYPE_EDGE_BOTH:
-		new_type = TQMX86_GPII_FALLING | TQMX86_GPII_RISING;
+		new_type = TQMX86_INT_TRIG_BOTH;
 		break;
 	default:
 		return -EINVAL; /* not supported */
@@ -173,8 +177,8 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 
 	gpio->irq_type[offset] = new_type;
 
-	mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS);
-	val = new_type << (offset * TQMX86_GPII_BITS);
+	mask = TQMX86_GPII_MASK(offset);
+	val = TQMX86_GPII_CONFIG(offset, new_type);
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
 	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 6/8] gpio: tqmx86: store IRQ triggers without offsetting index

[Matthias Schiffer]

This will allow us to move all offset handling into a single place in
the following commits. The additional irq_type indices remain unused,
but the tqmx86_gpio_data size increase is insignificant.

No functional change intended.

Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index d6e77f604f4df..4b37cc3bdd455 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -44,7 +44,7 @@ struct tqmx86_gpio_data {
 	int			irq;
 	raw_spinlock_t		spinlock;
 	DECLARE_BITMAP(output, TQMX86_NGPIO);
-	u8			irq_type[TQMX86_NGPI];
+	u8			irq_type[TQMX86_NGPIO];
 };
 
 static u8 tqmx86_gpio_read(struct tqmx86_gpio_data *gd, unsigned int reg)
@@ -146,7 +146,7 @@ static void tqmx86_gpio_irq_unmask(struct irq_data *data)
 	gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(data));
 
 	mask = TQMX86_GPII_MASK(offset);
-	val = TQMX86_GPII_CONFIG(offset, gpio->irq_type[offset]);
+	val = TQMX86_GPII_CONFIG(offset, gpio->irq_type[data->hwirq]);
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
 	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
@@ -175,7 +175,7 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 		return -EINVAL; /* not supported */
 	}
 
-	gpio->irq_type[offset] = new_type;
+	gpio->irq_type[data->hwirq] = new_type;
 
 	mask = TQMX86_GPII_MASK(offset);
 	val = TQMX86_GPII_CONFIG(offset, new_type);
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 7/8] gpio: tqmx86: store IRQ trigger type and unmask status separately

[Matthias Schiffer]

irq_set_type() should not implicitly unmask the IRQ.

All accesses to the interrupt configuration register are moved to a new
helper _tqmx86_gpio_irq_config(). We also introduce the new rule that
accessing irq_type must happen while locked, which will become
significant for fixing EDGE_BOTH handling.

Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 41 +++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 4b37cc3bdd455..c957be3341774 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -34,6 +34,7 @@
 #define TQMX86_INT_TRIG_RISING	0x2
 #define TQMX86_INT_TRIG_BOTH	0x3
 #define TQMX86_INT_TRIG_MASK	0x3
+#define TQMX86_INT_UNMASKED	BIT(2)
 
 #define TQMX86_GPII_CONFIG(i, v)	((v) << (2 * (i)))
 #define TQMX86_GPII_MASK(i)		TQMX86_GPII_CONFIG(i, TQMX86_INT_TRIG_MASK)
@@ -42,6 +43,7 @@ struct tqmx86_gpio_data {
 	struct gpio_chip	chip;
 	void __iomem		*io_base;
 	int			irq;
+	/* Lock must be held for accessing output and irq_type fields */
 	raw_spinlock_t		spinlock;
 	DECLARE_BITMAP(output, TQMX86_NGPIO);
 	u8			irq_type[TQMX86_NGPIO];
@@ -119,17 +121,28 @@ static int tqmx86_gpio_get_direction(struct gpio_chip *chip,
 	return GPIO_LINE_DIRECTION_OUT;
 }
 
+static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
+{
+	unsigned int offset = hwirq - TQMX86_NGPO;
+	u8 type = TQMX86_INT_TRIG_NONE, mask, val;
+
+	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED)
+		type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK;
+
+	mask = TQMX86_GPII_MASK(offset);
+	val = TQMX86_GPII_CONFIG(offset, type);
+	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
+}
+
 static void tqmx86_gpio_irq_mask(struct irq_data *data)
 {
-	unsigned int offset = (data->hwirq - TQMX86_NGPO);
 	struct tqmx86_gpio_data *gpio = gpiochip_get_data(
 		irq_data_get_irq_chip_data(data));
 	unsigned long flags;
-	u8 mask;
 
-	mask = TQMX86_GPII_MASK(offset);
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, 0);
+	gpio->irq_type[data->hwirq] &= ~TQMX86_INT_UNMASKED;
+	_tqmx86_gpio_irq_config(gpio, data->hwirq);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 
 	gpiochip_disable_irq(&gpio->chip, irqd_to_hwirq(data));
@@ -137,18 +150,15 @@ static void tqmx86_gpio_irq_mask(struct irq_data *data)
 
 static void tqmx86_gpio_irq_unmask(struct irq_data *data)
 {
-	unsigned int offset = (data->hwirq - TQMX86_NGPO);
 	struct tqmx86_gpio_data *gpio = gpiochip_get_data(
 		irq_data_get_irq_chip_data(data));
 	unsigned long flags;
-	u8 mask, val;
 
 	gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(data));
 
-	mask = TQMX86_GPII_MASK(offset);
-	val = TQMX86_GPII_CONFIG(offset, gpio->irq_type[data->hwirq]);
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
+	gpio->irq_type[data->hwirq] |= TQMX86_INT_UNMASKED;
+	_tqmx86_gpio_irq_config(gpio, data->hwirq);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 }
 
@@ -156,10 +166,9 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 {
 	struct tqmx86_gpio_data *gpio = gpiochip_get_data(
 		irq_data_get_irq_chip_data(data));
-	unsigned int offset = (data->hwirq - TQMX86_NGPO);
 	unsigned int edge_type = type & IRQF_TRIGGER_MASK;
 	unsigned long flags;
-	u8 new_type, mask, val;
+	u8 new_type;
 
 	switch (edge_type) {
 	case IRQ_TYPE_EDGE_RISING:
@@ -175,12 +184,12 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 		return -EINVAL; /* not supported */
 	}
 
-	gpio->irq_type[data->hwirq] = new_type;
-
-	mask = TQMX86_GPII_MASK(offset);
-	val = TQMX86_GPII_CONFIG(offset, new_type);
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
+
+	gpio->irq_type[data->hwirq] &= ~TQMX86_INT_TRIG_MASK;
+	gpio->irq_type[data->hwirq] |= new_type;
+	_tqmx86_gpio_irq_config(gpio, data->hwirq);
+
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 
 	return 0;
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

[Matthias Schiffer]

The TQMx86 GPIO controller only supports falling and rising edge
triggers, but not both. Fix this by implementing a software both-edge
mode that toggles the edge type after every interrupt.

Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
Co-developed-by: Gregor Herburger <gregor.herburger@tq-group.com>
Signed-off-by: Gregor Herburger <gregor.herburger@tq-group.com>
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 42 +++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index c957be3341774..400415676ad5d 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
 	unsigned int offset = hwirq - TQMX86_NGPO;
 	u8 type = TQMX86_INT_TRIG_NONE, mask, val;
 
-	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED)
+	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) {
 		type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK;
 
+		if (type == TQMX86_INT_TRIG_BOTH)
+			type = tqmx86_gpio_get(&gpio->chip, hwirq)
+				? TQMX86_INT_TRIG_FALLING
+				: TQMX86_INT_TRIG_RISING;
+	}
+
 	mask = TQMX86_GPII_MASK(offset);
 	val = TQMX86_GPII_CONFIG(offset, type);
 	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
@@ -200,8 +206,8 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc)
 	struct gpio_chip *chip = irq_desc_get_handler_data(desc);
 	struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
 	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
-	unsigned long irq_bits;
-	int i = 0;
+	unsigned long irq_bits, flags;
+	int i, hwirq;
 	u8 irq_status;
 
 	chained_irq_enter(irq_chip, desc);
@@ -210,6 +216,36 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc)
 	tqmx86_gpio_write(gpio, TQMX86_GPIIS, irq_status);
 
 	irq_bits = irq_status;
+
+	raw_spin_lock_irqsave(&gpio->spinlock, flags);
+	for_each_set_bit(i, &irq_bits, TQMX86_NGPI) {
+		hwirq = i + TQMX86_NGPO;
+
+		/*
+		 * Edge-both triggers are implemented by flipping the edge
+		 * trigger after each interrupt, as the controller only supports
+		 * either rising or falling edge triggers, but not both.
+		 *
+		 * Internally, the TQMx86 GPIO controller has separate status
+		 * registers for rising and falling edge interrupts. GPIIC
+		 * configures which bits from which register are visible in the
+		 * interrupt status register GPIIS and defines what triggers the
+		 * parent IRQ line. Writing to GPIIS always clears both rising
+		 * and falling interrupt flags internally, regardless of the
+		 * currently configured trigger.
+		 *
+		 * In consequence, we can cleanly implement the edge-both
+		 * trigger in software by first clearing the interrupt and then
+		 * setting the new trigger based on the current GPIO input in
+		 * _tqmx86_gpio_irq_config() - even if an edge arrives between
+		 * reading the input and setting the trigger, we will have a new
+		 * interrupt pending.
+		 */
+		if ((gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK) == TQMX86_INT_TRIG_BOTH)
+			_tqmx86_gpio_irq_config(gpio, hwirq);
+	}
+	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
+
 	for_each_set_bit(i, &irq_bits, TQMX86_NGPI)
 		generic_handle_domain_irq(gpio->chip.irq.domain,
 					  i + TQMX86_NGPO);
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 1/8] gpio: tqmx86: fix typo in Kconfig label

[Andrew Lunn]

On Wed, May 29, 2024 at 09:45:13AM +0200, Matthias Schiffer wrote:
> From: Gregor Herburger <gregor.herburger@tq-group.com>
> 
> Fix description for GPIO_TQMX86 from QTMX86 to TQMx86.
> 
> Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
> Signed-off-by: Gregor Herburger <gregor.herburger@tq-group.com>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 2/8] gpio: tqmx86: introduce shadow register for GPIO output value

[Andrew Lunn]

On Wed, May 29, 2024 at 09:45:14AM +0200, Matthias Schiffer wrote:
> The TQMx86 GPIO controller uses the same register address for input and
> output data. Reading the register will always return current inputs
> rather than the previously set outputs (regardless of the current
> direction setting). Therefore, using a RMW pattern does not make sense
> when setting output values. Instead, the previously set output register
> value needs to be stored as a shadow register.
> 
> As there is no reliable way to get the current output values from the
> hardware, also initialize all channels to 0, to ensure that stored and
> actual output values match. This should usually not have any effect in
> practise, as the TQMx86 UEFI sets all outputs to 0 during boot.
> 
> Also prepare for extension of the driver to more than 8 GPIOs by using
> DECLARE_BITMAP.
> 
> Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 3/8] gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match regmap API

[Bartosz Golaszewski]

On Wed, May 29, 2024 at 9:46 AM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:
>
> Conversion to actually use regmap does not seem useful for this driver,
> as regmap can't properly represent separate read-only and write-only
> registers at the same address, but we can at least match the API to make
> the code clearer.
>
> No functional change intended.
>
> Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")

This is not a fix.

Bart

Re: [PATCH 0/8] gpio-tqmx86 fixes

[Bartosz Golaszewski]

On Wed, May 29, 2024 at 9:46 AM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:
>
> This is the first series of improvements to the tqmx86 GPIO driver,
> which fixes some long-standing bugs - in particular, IRQ_TYPE_EDGE_BOTH
> can never have worked correctly.
>
> Other patches in the series are code cleanup, which is included as it
> makes the actual fixes much nicer. I have included the same Fixes tag in
> all commits, as they will need to be cherry-picked together.
>
> A second series with new features (changing GPIO directions, support
> more GPIOs on SMARC TQMx86 modules) will be submitted when the fixes
> have been reviewed and merged.
>
> Gregor Herburger (1):
>   gpio: tqmx86: fix typo in Kconfig label
>
> Matthias Schiffer (7):
>   gpio: tqmx86: introduce shadow register for GPIO output value
>   gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match
>     regmap API
>   gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper
>   gpio: tqmx86: add macros for interrupt configuration
>   gpio: tqmx86: store IRQ triggers without offsetting index
>   gpio: tqmx86: store IRQ trigger type and unmask status separately
>   gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type
>
>  drivers/gpio/Kconfig       |   2 +-
>  drivers/gpio/gpio-tqmx86.c | 151 ++++++++++++++++++++++++++-----------
>  2 files changed, 106 insertions(+), 47 deletions(-)
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> https://www.tq-group.com/
>

Hi Matthias!

Not all patches in this series are fixes (as in: warrant being sent
upstream outside of the merge window). Please split the series into
two with the first one containing actual fixes to real bugs and the
second for any refactoring and improvements.

Bart

Re: [PATCH 3/8] gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match regmap API

[Andrew Lunn]

On Wed, May 29, 2024 at 02:03:35PM +0200, Bartosz Golaszewski wrote:
> On Wed, May 29, 2024 at 9:46 AM Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com> wrote:
> >
> > Conversion to actually use regmap does not seem useful for this driver,
> > as regmap can't properly represent separate read-only and write-only
> > registers at the same address, but we can at least match the API to make
> > the code clearer.
> >
> > No functional change intended.
> >
> > Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
> 
> This is not a fix.

Agreed.

I'm somewhat conflicted by this patch. It is a step towards using
regmap, but then says regmap does not make sense. So why make that
step?

Changing the order of parameters like this seems like it is will make
back porting bug fixes harder, unless all supported versions are
changed, which is why fixes make sense. Does the compiler at least
issue a warning if the parameters are used the wrong way around?

Overall, i'm leaning towards just dropping it.

	 Andrew

Re: [PATCH 4/8] gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper

[Andrew Lunn]

On Wed, May 29, 2024 at 09:45:16AM +0200, Matthias Schiffer wrote:
> Simplify a lot of code in the driver by introducing helpers for the
> common RMW pattern. No tqmx86_gpio_update_bits() function with builtin
> locking is added, as it would become redundant with the following fixes,
> which further consolidate interrupt configuration register setup.
> 
> No functional change intended.
> 
> Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/gpio/gpio-tqmx86.c | 40 ++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> index 613ab9ef2e744..7a851e1730dd1 100644
> --- a/drivers/gpio/gpio-tqmx86.c
> +++ b/drivers/gpio/gpio-tqmx86.c
> @@ -54,6 +54,17 @@ static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, unsigned int reg,
>  	iowrite8(val, gd->io_base + reg);
>  }
>  
> +static void _tqmx86_gpio_update_bits(struct tqmx86_gpio_data *gd,
> +				     unsigned int reg, u8 mask, u8 val)

Why the _ prefix? This is a local function, it is static, so you don't
have name space issues. Functions starting with _ are those you should
not call without a good reason, there is generally a version without
the _ prefix which is the real function to use. So i would drop the _.

This is also not a fix. Please read:

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

and stick to the rules described there.

I don't know how the GPIO tree works, but for netdev, about a week
after fixes are merged, they appear in net-next. So you can then build
on top of them for development work.

	Andrew

Re: [PATCH 3/8] gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match regmap API

[Matthias Schiffer]

On Wed, 2024-05-29 at 14:11 +0200, Andrew Lunn wrote:
> 
> On Wed, May 29, 2024 at 02:03:35PM +0200, Bartosz Golaszewski wrote:
> > On Wed, May 29, 2024 at 9:46 AM Matthias Schiffer
> > <matthias.schiffer@ew.tq-group.com> wrote:
> > > 
> > > Conversion to actually use regmap does not seem useful for this driver,
> > > as regmap can't properly represent separate read-only and write-only
> > > registers at the same address, but we can at least match the API to make
> > > the code clearer.
> > > 
> > > No functional change intended.
> > > 
> > > Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
> > 
> > This is not a fix.
> 
> Agreed.
> 
> I'm somewhat conflicted by this patch. It is a step towards using
> regmap, but then says regmap does not make sense. So why make that
> step?
> 
> Changing the order of parameters like this seems like it is will make
> back porting bug fixes harder, unless all supported versions are
> changed, which is why fixes make sense. Does the compiler at least
> issue a warning if the parameters are used the wrong way around?
> 
> Overall, i'm leaning towards just dropping it.
> 
> 	 Andrew



Okay, will drop this patch.

Matthias

Re: [PATCH 4/8] gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper

[Matthias Schiffer]

On Wed, 2024-05-29 at 14:19 +0200, Andrew Lunn wrote:
> 
> On Wed, May 29, 2024 at 09:45:16AM +0200, Matthias Schiffer wrote:
> > Simplify a lot of code in the driver by introducing helpers for the
> > common RMW pattern. No tqmx86_gpio_update_bits() function with builtin
> > locking is added, as it would become redundant with the following fixes,
> > which further consolidate interrupt configuration register setup.
> > 
> > No functional change intended.
> > 
> > Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  drivers/gpio/gpio-tqmx86.c | 40 ++++++++++++++++++++++----------------
> >  1 file changed, 23 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> > index 613ab9ef2e744..7a851e1730dd1 100644
> > --- a/drivers/gpio/gpio-tqmx86.c
> > +++ b/drivers/gpio/gpio-tqmx86.c
> > @@ -54,6 +54,17 @@ static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, unsigned int reg,
> >  	iowrite8(val, gd->io_base + reg);
> >  }
> >  
> > +static void _tqmx86_gpio_update_bits(struct tqmx86_gpio_data *gd,
> > +				     unsigned int reg, u8 mask, u8 val)
> 
> Why the _ prefix? This is a local function, it is static, so you don't
> have name space issues. Functions starting with _ are those you should
> not call without a good reason, there is generally a version without
> the _ prefix which is the real function to use. So i would drop the _.

My intention was to mark functions that need to be called while holding the spinlock with a _
prefix. Should I just remove the prefix and add a comment instead?

Matthias


> 
> This is also not a fix. Please read:
> 
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> 
> and stick to the rules described there.
> 
> I don't know how the GPIO tree works, but for netdev, about a week
> after fixes are merged, they appear in net-next. So you can then build
> on top of them for development work.
> 
> 	Andrew

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 4/8] gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper

[Andrew Lunn]

> My intention was to mark functions that need to be called while holding the spinlock with a _
> prefix. Should I just remove the prefix and add a comment instead?

Yes.

You could also add sparse markup of the locks, or add an
assert_spin_locked(lock);

	Andrew

Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

[Andrew Lunn]

On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote:
> The TQMx86 GPIO controller only supports falling and rising edge
> triggers, but not both. Fix this by implementing a software both-edge
> mode that toggles the edge type after every interrupt.

Do you have a real use case for this, one that will handle lost
interrupts because it cannot swap edge quick enough?

I personally would not do this, because it is dangerous, it gives the
impression the device can do both, when in fact it cannot reliably.

For me, the correct fix is to return EOPNOTSUPP or EINVAL for BOTH.

	   Andrew

Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

[Matthias Schiffer]

On Wed, 2024-05-29 at 14:37 +0200, Andrew Lunn wrote:
> On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote:
> > The TQMx86 GPIO controller only supports falling and rising edge
> > triggers, but not both. Fix this by implementing a software both-edge
> > mode that toggles the edge type after every interrupt.
> 
> Do you have a real use case for this, one that will handle lost
> interrupts because it cannot swap edge quick enough?
> 
> I personally would not do this, because it is dangerous, it gives the
> impression the device can do both, when in fact it cannot reliably.
> 
> For me, the correct fix is to return EOPNOTSUPP or EINVAL for BOTH.
> 

This was the first thing I tried as well, but it seems that supporting IRQ_TYPE_EDGE_BOTH is
mandatory for all GPIO drivers (not doing so results in various error messages when attemting to use
*any* type of interrupt for the GPIO; I don't remember the exact errors). For this reason, several
drivers implement a similar software solution when the hardware doesn't support it.

Many drivers implement this in a fragile way that will easily break when an edge is missed. On the
TQMx86 we are lucky, and this software implementation is actually robust and will not stop reporting
edges when one is missed. The reason is explained in detail in the long comment added by this patch.

Matthias

Re: [PATCH 0/8] gpio-tqmx86 fixes

[Matthias Schiffer]

On Wed, 2024-05-29 at 14:08 +0200, Bartosz Golaszewski wrote:
> On Wed, May 29, 2024 at 9:46 AM Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com> wrote:
> > 
> > This is the first series of improvements to the tqmx86 GPIO driver,
> > which fixes some long-standing bugs - in particular, IRQ_TYPE_EDGE_BOTH
> > can never have worked correctly.
> > 
> > Other patches in the series are code cleanup, which is included as it
> > makes the actual fixes much nicer. I have included the same Fixes tag in
> > all commits, as they will need to be cherry-picked together.
> > 
> > A second series with new features (changing GPIO directions, support
> > more GPIOs on SMARC TQMx86 modules) will be submitted when the fixes
> > have been reviewed and merged.
> > 
> > Gregor Herburger (1):
> >   gpio: tqmx86: fix typo in Kconfig label
> > 
> > Matthias Schiffer (7):
> >   gpio: tqmx86: introduce shadow register for GPIO output value
> >   gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match
> >     regmap API
> >   gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper
> >   gpio: tqmx86: add macros for interrupt configuration
> >   gpio: tqmx86: store IRQ triggers without offsetting index
> >   gpio: tqmx86: store IRQ trigger type and unmask status separately
> >   gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type
> > 
> >  drivers/gpio/Kconfig       |   2 +-
> >  drivers/gpio/gpio-tqmx86.c | 151 ++++++++++++++++++++++++++-----------
> >  2 files changed, 106 insertions(+), 47 deletions(-)
> > 
> > --
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > https://www.tq-group.com/
> > 
> 
> Hi Matthias!
> 
> Not all patches in this series are fixes (as in: warrant being sent
> upstream outside of the merge window). Please split the series into
> two with the first one containing actual fixes to real bugs and the
> second for any refactoring and improvements.
> 
> Bart


Hi Bartosz,

as explained in the cover letter, I've found that the fixes become much nicer to implement (and to
review) if they are based on the refactoring. I can leave out _tqmx86_gpio_update_bits for now, but
removing "add macros for interrupt configuration" and "store IRQ triggers without offsetting index"
does make the actual fixes "store IRQ trigger type and unmask status separately" and "fix broken
IRQ_TYPE_EDGE_BOTH interrupt type" somewhat uglier.

That being said, you're the maintainer, and I will structure this series in any way you prefer. I
can remove the mentioned refactoring, even though it makes the fixes less pleasant. Another option
would be that I can submit just the refactoring for -next for now, and leave the fixes for a future
series. Let me know how you want to proceed.

Thanks,
Matthias

Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

[Dan Carpenter]

On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote:
> diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> index c957be3341774..400415676ad5d 100644
> --- a/drivers/gpio/gpio-tqmx86.c
> +++ b/drivers/gpio/gpio-tqmx86.c
> @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
>  	unsigned int offset = hwirq - TQMX86_NGPO;
>  	u8 type = TQMX86_INT_TRIG_NONE, mask, val;
>  
> -	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED)
> +	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) {
>  		type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK;
>  
> +		if (type == TQMX86_INT_TRIG_BOTH)
> +			type = tqmx86_gpio_get(&gpio->chip, hwirq)
                                                            ^^^^^

> +				? TQMX86_INT_TRIG_FALLING
> +				: TQMX86_INT_TRIG_RISING;
> +	}
> +
>  	mask = TQMX86_GPII_MASK(offset);
                                ^^^^^^
>  	val = TQMX86_GPII_CONFIG(offset, type);
                                 ^^^^^^
>  	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);

The offset stuff wasn't beautiful and I'm glad you are deleting it.  My
understanding is that a hwirq is 0-3 for output or 4-7 input.  An offset
is "hwirq % 4"?

There are a bunch of places which are still marked as taking an offset
but they all actually take a hwirq.  For example, tqmx86_gpio_get()
above.  The only things which still actually take an offset are the
TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros.

Could you:
1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq?
2) Rename all the "offset" variables to "hwirq"?

regards,
dan carpenter

Re: [PATCH 0/8] gpio-tqmx86 fixes

[Bartosz Golaszewski]

On Wed, May 29, 2024 at 2:55 PM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:
>
> On Wed, 2024-05-29 at 14:08 +0200, Bartosz Golaszewski wrote:
> > On Wed, May 29, 2024 at 9:46 AM Matthias Schiffer
> > <matthias.schiffer@ew.tq-group.com> wrote:
> > >
> > > This is the first series of improvements to the tqmx86 GPIO driver,
> > > which fixes some long-standing bugs - in particular, IRQ_TYPE_EDGE_BOTH
> > > can never have worked correctly.
> > >
> > > Other patches in the series are code cleanup, which is included as it
> > > makes the actual fixes much nicer. I have included the same Fixes tag in
> > > all commits, as they will need to be cherry-picked together.
> > >
> > > A second series with new features (changing GPIO directions, support
> > > more GPIOs on SMARC TQMx86 modules) will be submitted when the fixes
> > > have been reviewed and merged.
> > >
> > > Gregor Herburger (1):
> > >   gpio: tqmx86: fix typo in Kconfig label
> > >
> > > Matthias Schiffer (7):
> > >   gpio: tqmx86: introduce shadow register for GPIO output value
> > >   gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match
> > >     regmap API
> > >   gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper
> > >   gpio: tqmx86: add macros for interrupt configuration
> > >   gpio: tqmx86: store IRQ triggers without offsetting index
> > >   gpio: tqmx86: store IRQ trigger type and unmask status separately
> > >   gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type
> > >
> > >  drivers/gpio/Kconfig       |   2 +-
> > >  drivers/gpio/gpio-tqmx86.c | 151 ++++++++++++++++++++++++++-----------
> > >  2 files changed, 106 insertions(+), 47 deletions(-)
> > >
> > > --
> > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > Amtsgericht München, HRB 105018
> > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > > https://www.tq-group.com/
> > >
> >
> > Hi Matthias!
> >
> > Not all patches in this series are fixes (as in: warrant being sent
> > upstream outside of the merge window). Please split the series into
> > two with the first one containing actual fixes to real bugs and the
> > second for any refactoring and improvements.
> >
> > Bart
>
>
> Hi Bartosz,
>
> as explained in the cover letter, I've found that the fixes become much nicer to implement (and to
> review) if they are based on the refactoring. I can leave out _tqmx86_gpio_update_bits for now, but
> removing "add macros for interrupt configuration" and "store IRQ triggers without offsetting index"
> does make the actual fixes "store IRQ trigger type and unmask status separately" and "fix broken
> IRQ_TYPE_EDGE_BOTH interrupt type" somewhat uglier.
>
> That being said, you're the maintainer, and I will structure this series in any way you prefer. I
> can remove the mentioned refactoring, even though it makes the fixes less pleasant. Another option
> would be that I can submit just the refactoring for -next for now, and leave the fixes for a future
> series. Let me know how you want to proceed.
>
> Thanks,
> Matthias

The question is: do you want these fixes to be backported into stable
branches? Because if not then it's true that the ordering doesn't
matter. But if you do, then it makes more sense to put fixes first,
send them to Torvalds, get them backported and then add refactoring
changes on top.

Bart

Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

[Matthias Schiffer]

On Wed, 2024-05-29 at 17:38 +0300, Dan Carpenter wrote:
> 
> On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote:
> > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> > index c957be3341774..400415676ad5d 100644
> > --- a/drivers/gpio/gpio-tqmx86.c
> > +++ b/drivers/gpio/gpio-tqmx86.c
> > @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
> >  	unsigned int offset = hwirq - TQMX86_NGPO;
> >  	u8 type = TQMX86_INT_TRIG_NONE, mask, val;
> >  
> > -	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED)
> > +	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) {
> >  		type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK;
> >  
> > +		if (type == TQMX86_INT_TRIG_BOTH)
> > +			type = tqmx86_gpio_get(&gpio->chip, hwirq)
>                                                             ^^^^^
> 
> > +				? TQMX86_INT_TRIG_FALLING
> > +				: TQMX86_INT_TRIG_RISING;
> > +	}
> > +
> >  	mask = TQMX86_GPII_MASK(offset);
>                                 ^^^^^^
> >  	val = TQMX86_GPII_CONFIG(offset, type);
>                                  ^^^^^^
> >  	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
> 
> The offset stuff wasn't beautiful and I'm glad you are deleting it.  My
> understanding is that a hwirq is 0-3 for output or 4-7 input.  An offset
> is "hwirq % 4"?
> 
> There are a bunch of places which are still marked as taking an offset
> but they all actually take a hwirq.  For example, tqmx86_gpio_get()
> above.  The only things which still actually take an offset are the
> TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros.
> 
> Could you:
> 1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq?
> 2) Rename all the "offset" variables to "hwirq"?

Unfortunately, the TQMx86 GPIO is a huge mess, and the mapping between GPIO numbers and IRQ numbers
depends on the hardware generation/variant. I don't think it is possible to have GPIO numbers and
hwirq numbers differ, is it?

Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7,
while GPIOs 0-3 don't have interrupt support. We will soon be mainlining support for our SMARC
modules, which have up to 14 GPIOs, and (on some families) IRQ support for all GPIOs (IRQs 0-13
correspond to GPIOs 0-13).

New interrupt config and status registers have been introduced to support more IRQs - up to 4 config
registers (2 bits for each IRQ) and 3 status registers (IRQs 0-3 in the first one, 4-11 in the
second one, 12-13 in the third one... so this part is a bit more convoluted than just "hwirq % 4") 

As the mapping between GPIOs and IRQs will become dynamic with these changes, I'd rather keep
TQMX86_GPII_* using IRQ numbers instead of GPIO numbers. We will be introducing helpers for
accessing the interrupt registers; the macros deal with individual register bits, and I think they
should be agnostic of the mapping to GPIO/hwirq numbers.

Matthias



> 
> regards,
> dan carpenter
> 

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

[Dan Carpenter]

On Thu, May 30, 2024 at 10:39:25AM +0200, Matthias Schiffer wrote:
> On Wed, 2024-05-29 at 17:38 +0300, Dan Carpenter wrote:
> > 
> > On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote:
> > > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> > > index c957be3341774..400415676ad5d 100644
> > > --- a/drivers/gpio/gpio-tqmx86.c
> > > +++ b/drivers/gpio/gpio-tqmx86.c
> > > @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
> > >  	unsigned int offset = hwirq - TQMX86_NGPO;
> > >  	u8 type = TQMX86_INT_TRIG_NONE, mask, val;
> > >  
> > > -	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED)
> > > +	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) {
> > >  		type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK;
> > >  
> > > +		if (type == TQMX86_INT_TRIG_BOTH)
> > > +			type = tqmx86_gpio_get(&gpio->chip, hwirq)
> >                                                             ^^^^^
> > 
> > > +				? TQMX86_INT_TRIG_FALLING
> > > +				: TQMX86_INT_TRIG_RISING;
> > > +	}
> > > +
> > >  	mask = TQMX86_GPII_MASK(offset);
> >                                 ^^^^^^
> > >  	val = TQMX86_GPII_CONFIG(offset, type);
> >                                  ^^^^^^
> > >  	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
> > 
> > The offset stuff wasn't beautiful and I'm glad you are deleting it.  My
> > understanding is that a hwirq is 0-3 for output or 4-7 input.  An offset
> > is "hwirq % 4"?
> > 
> > There are a bunch of places which are still marked as taking an offset
> > but they all actually take a hwirq.  For example, tqmx86_gpio_get()
> > above.  The only things which still actually take an offset are the
> > TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros.
> > 
> > Could you:
> > 1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq?
> > 2) Rename all the "offset" variables to "hwirq"?
> 
> Unfortunately, the TQMx86 GPIO is a huge mess, and the mapping between GPIO numbers and IRQ numbers
> depends on the hardware generation/variant. I don't think it is possible to have GPIO numbers and
> hwirq numbers differ, is it?
> 
> Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7,
> while GPIOs 0-3 don't have interrupt support.

I'm so confused.

So "offset" is the GPIO number and "hwirq" is the IRQ number?  If the
IRQ numbers are 0-3 then why do we subtract 4 to get the GPIO number in
_tqmx86_gpio_irq_config()?

	unsigned int offset = hwirq - TQMX86_NGPO;

And again, it's just weird to call:

		type = tqmx86_gpio_get(&gpio->chip, hwirq);

where we're passing "hwirq" when tqmx86_gpio_get() takes an "offset" as
an argument.

regards,
dan carpenter

Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

[Matthias Schiffer]

On Thu, 2024-05-30 at 13:22 +0300, Dan Carpenter wrote:
> 
> On Thu, May 30, 2024 at 10:39:25AM +0200, Matthias Schiffer wrote:
> > On Wed, 2024-05-29 at 17:38 +0300, Dan Carpenter wrote:
> > > 
> > > On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote:
> > > > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> > > > index c957be3341774..400415676ad5d 100644
> > > > --- a/drivers/gpio/gpio-tqmx86.c
> > > > +++ b/drivers/gpio/gpio-tqmx86.c
> > > > @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
> > > >  	unsigned int offset = hwirq - TQMX86_NGPO;
> > > >  	u8 type = TQMX86_INT_TRIG_NONE, mask, val;
> > > >  
> > > > -	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED)
> > > > +	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) {
> > > >  		type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK;
> > > >  
> > > > +		if (type == TQMX86_INT_TRIG_BOTH)
> > > > +			type = tqmx86_gpio_get(&gpio->chip, hwirq)
> > >                                                             ^^^^^
> > > 
> > > > +				? TQMX86_INT_TRIG_FALLING
> > > > +				: TQMX86_INT_TRIG_RISING;
> > > > +	}
> > > > +
> > > >  	mask = TQMX86_GPII_MASK(offset);
> > >                                 ^^^^^^
> > > >  	val = TQMX86_GPII_CONFIG(offset, type);
> > >                                  ^^^^^^
> > > >  	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
> > > 
> > > The offset stuff wasn't beautiful and I'm glad you are deleting it.  My
> > > understanding is that a hwirq is 0-3 for output or 4-7 input.  An offset
> > > is "hwirq % 4"?
> > > 
> > > There are a bunch of places which are still marked as taking an offset
> > > but they all actually take a hwirq.  For example, tqmx86_gpio_get()
> > > above.  The only things which still actually take an offset are the
> > > TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros.
> > > 
> > > Could you:
> > > 1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq?
> > > 2) Rename all the "offset" variables to "hwirq"?
> > 
> > Unfortunately, the TQMx86 GPIO is a huge mess, and the mapping between GPIO numbers and IRQ numbers
> > depends on the hardware generation/variant. I don't think it is possible to have GPIO numbers and
> > hwirq numbers differ, is it?
> > 
> > Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7,
> > while GPIOs 0-3 don't have interrupt support.
> 
> I'm so confused.
> 
> So "offset" is the GPIO number and "hwirq" is the IRQ number?  If the
> IRQ numbers are 0-3 then why do we subtract 4 to get the GPIO number in

The current naming in the driver is confusing and I'll fix that in the next round of refactoring
patches.

Generally, hwirq == GPIO number (I have not found a way to change this mapping - if there is one,
I'd be interested to try if it makes the code less confusing). "offset" currently always refers to
some shift in a hardware register. In tqmx86_gpio_get and tqmx86_gpio_set, offset is a GPIO number.
In all functions dealing with IRQs, offset is an IRQ number (which is different from the hwirq
number).

Matthias


> _tqmx86_gpio_irq_config()?
> 
> 	unsigned int offset = hwirq - TQMX86_NGPO;
> 
> And again, it's just weird to call:
> 
> 		type = tqmx86_gpio_get(&gpio->chip, hwirq);
> 
> where we're passing "hwirq" when tqmx86_gpio_get() takes an "offset" as
> an argument.
> 
> regards,
> dan carpenter
> 

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

[Matthias Schiffer]

On Thu, 2024-05-30 at 13:15 +0200, Matthias Schiffer wrote:
> On Thu, 2024-05-30 at 13:22 +0300, Dan Carpenter wrote:
> > 
> > On Thu, May 30, 2024 at 10:39:25AM +0200, Matthias Schiffer wrote:
> > > On Wed, 2024-05-29 at 17:38 +0300, Dan Carpenter wrote:
> > > > 
> > > > On Wed, May 29, 2024 at 09:45:20AM +0200, Matthias Schiffer wrote:
> > > > > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> > > > > index c957be3341774..400415676ad5d 100644
> > > > > --- a/drivers/gpio/gpio-tqmx86.c
> > > > > +++ b/drivers/gpio/gpio-tqmx86.c
> > > > > @@ -126,9 +126,15 @@ static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
> > > > >  	unsigned int offset = hwirq - TQMX86_NGPO;
> > > > >  	u8 type = TQMX86_INT_TRIG_NONE, mask, val;
> > > > >  
> > > > > -	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED)
> > > > > +	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) {
> > > > >  		type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK;
> > > > >  
> > > > > +		if (type == TQMX86_INT_TRIG_BOTH)
> > > > > +			type = tqmx86_gpio_get(&gpio->chip, hwirq)
> > > >                                                             ^^^^^
> > > > 
> > > > > +				? TQMX86_INT_TRIG_FALLING
> > > > > +				: TQMX86_INT_TRIG_RISING;
> > > > > +	}
> > > > > +
> > > > >  	mask = TQMX86_GPII_MASK(offset);
> > > >                                 ^^^^^^
> > > > >  	val = TQMX86_GPII_CONFIG(offset, type);
> > > >                                  ^^^^^^
> > > > >  	_tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
> > > > 
> > > > The offset stuff wasn't beautiful and I'm glad you are deleting it.  My
> > > > understanding is that a hwirq is 0-3 for output or 4-7 input.  An offset
> > > > is "hwirq % 4"?
> > > > 
> > > > There are a bunch of places which are still marked as taking an offset
> > > > but they all actually take a hwirq.  For example, tqmx86_gpio_get()
> > > > above.  The only things which still actually take an offset are the
> > > > TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() macros.
> > > > 
> > > > Could you:
> > > > 1) Modify TQMX86_GPII_MASK() and TQMX86_GPII_CONFIG() to take a hwirq?
> > > > 2) Rename all the "offset" variables to "hwirq"?
> > > 
> > > Unfortunately, the TQMx86 GPIO is a huge mess, and the mapping between GPIO numbers and IRQ numbers
> > > depends on the hardware generation/variant. I don't think it is possible to have GPIO numbers and
> > > hwirq numbers differ, is it?
> > > 
> > > Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7,
> > > while GPIOs 0-3 don't have interrupt support.
> > 
> > I'm so confused.
> > 
> > So "offset" is the GPIO number and "hwirq" is the IRQ number?  If the
> > IRQ numbers are 0-3 then why do we subtract 4 to get the GPIO number in
> 
> The current naming in the driver is confusing and I'll fix that in the next round of refactoring
> patches.
> 
> Generally, hwirq == GPIO number (I have not found a way to change this mapping - if there is one,
> I'd be interested to try if it makes the code less confusing). "offset" currently always refers to
> some shift in a hardware register. In tqmx86_gpio_get and tqmx86_gpio_set, offset is a GPIO number.
> In all functions dealing with IRQs, offset is an IRQ number (which is different from the hwirq
> number).
> 
> Matthias

Ah, I just noticed the "to_irq" function in struct gpio_chip, I guess I simply didn't look in the
right place before. Will have a closer look when I rebase the refactoring patches.

Matthias



> 
> 
> > _tqmx86_gpio_irq_config()?
> > 
> > 	unsigned int offset = hwirq - TQMX86_NGPO;
> > 
> > And again, it's just weird to call:
> > 
> > 		type = tqmx86_gpio_get(&gpio->chip, hwirq);
> > 
> > where we're passing "hwirq" when tqmx86_gpio_get() takes an "offset" as
> > an argument.
> > 
> > regards,
> > dan carpenter
> > 
> 

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 8/8] gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type

[Andrew Lunn]

> Currently, the driver only supports COM Express modules, where IRQs 0-3 correspond to GPIOs 4-7,
> while GPIOs 0-3 don't have interrupt support. We will soon be mainlining support for our SMARC
> modules, which have up to 14 GPIOs, and (on some families) IRQ support for all GPIOs (IRQs 0-13
> correspond to GPIOs 0-13).
> 
> New interrupt config and status registers have been introduced to support more IRQs - up to 4 config
> registers (2 bits for each IRQ) and 3 status registers (IRQs 0-3 in the first one, 4-11 in the
> second one, 12-13 in the third one... so this part is a bit more convoluted than just "hwirq % 4") 

Depending on how different it is, you could consider a second driver,
rather than make this driver more complex.

	Andrew