[PATCH 0/4] gpio-tqmx86: cleanup + changing directions

[PATCH 0/4] gpio-tqmx86: cleanup + changing directions

[Matthias Schiffer]

This is the first of two series adding new features to the gpio-tqmx86
driver. The first 3 patches are cleanup/preparation and the last patch
adds support for changing the directions of GPIOs.

Once this is merged, the final series will add support for new TQMx86
variants (SMARC and COM-HPC) that feature up to 14 GPIOs and full IRQ
support on all lines.


Matthias Schiffer (4):
  gpio: tqmx86: add macros for interrupt configuration
  gpio: tqmx86: consistently refer to IRQs by hwirq numbers
  gpio: tqmx86: introduce tqmx86_gpio_clrsetbits() helper
  gpio: tqmx86: add support for changing GPIO directions

 drivers/gpio/gpio-tqmx86.c | 135 +++++++++++++++++++++++--------------
 1 file changed, 84 insertions(+), 51 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/4] gpio: tqmx86: add macros for interrupt configuration

[Matthias Schiffer]

We now consistently use TQMX86_INT_* flags for irq_type values. The
TQMX86_GPII_CONFIG macro is used to convert from TQMX86_INT_TRIG_*
flags to GPII register values. Bit patterns for TQMX86_INT_* are chosen
to make this conversion as simple as possible.

No functional change intended.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 43 ++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 5e26eb3adabbf..667cb34b882f0 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -29,18 +29,21 @@
 #define TQMX86_GPIIC	3	/* GPI Interrupt Configuration Register */
 #define TQMX86_GPIIS	4	/* GPI Interrupt Status Register */
 
-#define TQMX86_GPII_NONE	0
-#define TQMX86_GPII_FALLING	BIT(0)
-#define TQMX86_GPII_RISING	BIT(1)
-/* Stored in irq_type as a trigger type, but not actually valid as a register
- * value, so the name doesn't use "GPII"
+/* NONE, FALLING and RISING use the same bit patterns that can be programmed to
+ * the GPII register (after passing them to the TQMX86_GPII_ macros to shift
+ * them to the right position)
  */
-#define TQMX86_INT_BOTH		(BIT(0) | BIT(1))
-#define TQMX86_GPII_MASK	(BIT(0) | BIT(1))
-#define TQMX86_GPII_BITS	2
+#define TQMX86_INT_TRIG_NONE	0
+#define TQMX86_INT_TRIG_FALLING	BIT(0)
+#define TQMX86_INT_TRIG_RISING	BIT(1)
+#define TQMX86_INT_TRIG_BOTH	(BIT(0) | BIT(1))
+#define TQMX86_INT_TRIG_MASK	(BIT(0) | BIT(1))
 /* Stored in irq_type with GPII bits */
 #define TQMX86_INT_UNMASKED	BIT(2)
 
+#define TQMX86_GPIIC_CONFIG(i, v)	((v) << (2 * (i)))
+#define TQMX86_GPIIC_MASK(i)		TQMX86_GPIIC_CONFIG(i, TQMX86_INT_TRIG_MASK)
+
 struct tqmx86_gpio_data {
 	struct gpio_chip	chip;
 	void __iomem		*io_base;
@@ -115,20 +118,20 @@ static int tqmx86_gpio_get_direction(struct gpio_chip *chip,
 static void tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int offset)
 	__must_hold(&gpio->spinlock)
 {
-	u8 type = TQMX86_GPII_NONE, gpiic;
+	u8 type = TQMX86_INT_TRIG_NONE, gpiic;
 
 	if (gpio->irq_type[offset] & TQMX86_INT_UNMASKED) {
-		type = gpio->irq_type[offset] & TQMX86_GPII_MASK;
+		type = gpio->irq_type[offset] & TQMX86_INT_TRIG_MASK;
 
-		if (type == TQMX86_INT_BOTH)
+		if (type == TQMX86_INT_TRIG_BOTH)
 			type = tqmx86_gpio_get(&gpio->chip, offset + TQMX86_NGPO)
-				? TQMX86_GPII_FALLING
-				: TQMX86_GPII_RISING;
+				? TQMX86_INT_TRIG_FALLING
+				: TQMX86_INT_TRIG_RISING;
 	}
 
 	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
-	gpiic &= ~(TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS));
-	gpiic |= type << (offset * TQMX86_GPII_BITS);
+	gpiic &= ~TQMX86_GPIIC_MASK(offset);
+	gpiic |= TQMX86_GPIIC_CONFIG(offset, type);
 	tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
 }
 
@@ -173,20 +176,20 @@ 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_INT_BOTH;
+		new_type = TQMX86_INT_TRIG_BOTH;
 		break;
 	default:
 		return -EINVAL; /* not supported */
 	}
 
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	gpio->irq_type[offset] &= ~TQMX86_GPII_MASK;
+	gpio->irq_type[offset] &= ~TQMX86_INT_TRIG_MASK;
 	gpio->irq_type[offset] |= new_type;
 	tqmx86_gpio_irq_config(gpio, offset);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
@@ -232,7 +235,7 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc)
 		 * reading the input and setting the trigger, we will have a new
 		 * interrupt pending.
 		 */
-		if ((gpio->irq_type[i] & TQMX86_GPII_MASK) == TQMX86_INT_BOTH)
+		if ((gpio->irq_type[i] & TQMX86_INT_TRIG_MASK) == TQMX86_INT_TRIG_BOTH)
 			tqmx86_gpio_irq_config(gpio, i);
 	}
 	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/

Re: [PATCH 1/4] gpio: tqmx86: add macros for interrupt configuration

[Bartosz Golaszewski]

On Mon, Dec 9, 2024 at 11:36 AM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:
>
> We now consistently use TQMX86_INT_* flags for irq_type values. The
> TQMX86_GPII_CONFIG macro is used to convert from TQMX86_INT_TRIG_*
> flags to GPII register values. Bit patterns for TQMX86_INT_* are chosen
> to make this conversion as simple as possible.
>

Please use imperative clause in commit messages. Prefer "Consistently
use..." over "We now do this".

> No functional change intended.
>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/gpio/gpio-tqmx86.c | 43 ++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> index 5e26eb3adabbf..667cb34b882f0 100644
> --- a/drivers/gpio/gpio-tqmx86.c
> +++ b/drivers/gpio/gpio-tqmx86.c
> @@ -29,18 +29,21 @@
>  #define TQMX86_GPIIC   3       /* GPI Interrupt Configuration Register */
>  #define TQMX86_GPIIS   4       /* GPI Interrupt Status Register */
>
> -#define TQMX86_GPII_NONE       0
> -#define TQMX86_GPII_FALLING    BIT(0)
> -#define TQMX86_GPII_RISING     BIT(1)
> -/* Stored in irq_type as a trigger type, but not actually valid as a register
> - * value, so the name doesn't use "GPII"
> +/* NONE, FALLING and RISING use the same bit patterns that can be programmed to
> + * the GPII register (after passing them to the TQMX86_GPII_ macros to shift
> + * them to the right position)
>   */

If you're changing this, can you switch to using the preferred:

/*
 * foo
 */

pattern? Checkpatch should have warned you about this.

Looks good otherwise.

Bart

[PATCH 2/4] gpio: tqmx86: consistently refer to IRQs by hwirq numbers

[Matthias Schiffer]

On currently supported variants of the TQMx86 GPIO controller, only
GPIOs 4-7 have IRQ support; in the interrupt status and config
registers, position 0 therefore corresponds to GPIO 4, position 1 to
GPIO 5, etc. This was made even more confusing by sometimes using the
term "offset" to refer to GPIO numbers (which are equavalent to hwirq
numbers), and sometimes to bit positions in the hardware registers.

With this change, the whole driver consistently uses hwirq numbers (==
GPIO numbers) when referring to the IRQs, and only the two pieces of
code that interact with the hardware registers (tqmx86_gpio_irq_config()
and tqmx86_gpio_irq_handler()) deal with bit positions. Space for hwirq
numbers 0-3 is reserved in the irq_type array, but remains unused for
existing (COM Express) TQMx86 variants; support for TQMx86 variants that
support IRQs on all GPIO lines will be added in the future.

No functional change intended.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 40 +++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 667cb34b882f0..27f44d112d582 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -51,7 +51,7 @@ struct tqmx86_gpio_data {
 	/* Lock must be held for accessing output and irq_type fields */
 	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)
@@ -115,36 +115,36 @@ 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 offset)
+static void tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
 	__must_hold(&gpio->spinlock)
 {
 	u8 type = TQMX86_INT_TRIG_NONE, gpiic;
+	int gpiic_irq = hwirq - TQMX86_NGPO;
 
-	if (gpio->irq_type[offset] & TQMX86_INT_UNMASKED) {
-		type = gpio->irq_type[offset] & TQMX86_INT_TRIG_MASK;
+	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, offset + TQMX86_NGPO)
+			type = tqmx86_gpio_get(&gpio->chip, hwirq)
 				? TQMX86_INT_TRIG_FALLING
 				: TQMX86_INT_TRIG_RISING;
 	}
 
 	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
-	gpiic &= ~TQMX86_GPIIC_MASK(offset);
-	gpiic |= TQMX86_GPIIC_CONFIG(offset, type);
+	gpiic &= ~TQMX86_GPIIC_MASK(gpiic_irq);
+	gpiic |= TQMX86_GPIIC_CONFIG(gpiic_irq, type);
 	tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
 }
 
 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;
 
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	gpio->irq_type[offset] &= ~TQMX86_INT_UNMASKED;
-	tqmx86_gpio_irq_config(gpio, offset);
+	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));
@@ -152,7 +152,6 @@ 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;
@@ -160,8 +159,8 @@ static void tqmx86_gpio_irq_unmask(struct irq_data *data)
 	gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(data));
 
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	gpio->irq_type[offset] |= TQMX86_INT_UNMASKED;
-	tqmx86_gpio_irq_config(gpio, offset);
+	gpio->irq_type[data->hwirq] |= TQMX86_INT_UNMASKED;
+	tqmx86_gpio_irq_config(gpio, data->hwirq);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 }
 
@@ -169,7 +168,6 @@ 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;
@@ -189,9 +187,9 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 	}
 
 	raw_spin_lock_irqsave(&gpio->spinlock, flags);
-	gpio->irq_type[offset] &= ~TQMX86_INT_TRIG_MASK;
-	gpio->irq_type[offset] |= new_type;
-	tqmx86_gpio_irq_config(gpio, offset);
+	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;
@@ -203,7 +201,7 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc)
 	struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
 	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
 	unsigned long irq_bits, flags;
-	int i;
+	int i, hwirq;
 	u8 irq_status;
 
 	chained_irq_enter(irq_chip, desc);
@@ -215,6 +213,8 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc)
 
 	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
@@ -235,8 +235,8 @@ static void tqmx86_gpio_irq_handler(struct irq_desc *desc)
 		 * reading the input and setting the trigger, we will have a new
 		 * interrupt pending.
 		 */
-		if ((gpio->irq_type[i] & TQMX86_INT_TRIG_MASK) == TQMX86_INT_TRIG_BOTH)
-			tqmx86_gpio_irq_config(gpio, i);
+		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);
 
-- 
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/4] gpio: tqmx86: introduce tqmx86_gpio_clrsetbits() helper

[Matthias Schiffer]

Add a helper for the common read-modify-write pattern (only used in
tqmx86_gpio_irq_config() initially).

No functional change intended.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 27f44d112d582..54e7e193bb209 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -65,6 +65,18 @@ static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, u8 val,
 	iowrite8(val, gd->io_base + reg);
 }
 
+static void tqmx86_gpio_clrsetbits(struct tqmx86_gpio_data *gpio,
+				    u8 clr, u8 set, unsigned int reg)
+	__must_hold(&gpio->spinlock)
+{
+	u8 val = tqmx86_gpio_read(gpio, reg);
+
+	val &= ~clr;
+	val |= set;
+
+	tqmx86_gpio_write(gpio, val, reg);
+}
+
 static int tqmx86_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
@@ -118,7 +130,7 @@ static int tqmx86_gpio_get_direction(struct gpio_chip *chip,
 static void tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
 	__must_hold(&gpio->spinlock)
 {
-	u8 type = TQMX86_INT_TRIG_NONE, gpiic;
+	u8 type = TQMX86_INT_TRIG_NONE;
 	int gpiic_irq = hwirq - TQMX86_NGPO;
 
 	if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED) {
@@ -130,10 +142,10 @@ static void tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
 				: TQMX86_INT_TRIG_RISING;
 	}
 
-	gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
-	gpiic &= ~TQMX86_GPIIC_MASK(gpiic_irq);
-	gpiic |= TQMX86_GPIIC_CONFIG(gpiic_irq, type);
-	tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
+	tqmx86_gpio_clrsetbits(gpio,
+			       TQMX86_GPIIC_MASK(gpiic_irq),
+			       TQMX86_GPIIC_CONFIG(gpiic_irq, type),
+			       TQMX86_GPIIC);
 }
 
 static void tqmx86_gpio_irq_mask(struct irq_data *data)
-- 
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 3/4] gpio: tqmx86: introduce tqmx86_gpio_clrsetbits() helper

[Linus Walleij]

On Mon, Dec 9, 2024 at 11:36 AM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:

> +static void tqmx86_gpio_clrsetbits(struct tqmx86_gpio_data *gpio,
> +                                   u8 clr, u8 set, unsigned int reg)
> +       __must_hold(&gpio->spinlock)
> +{
> +       u8 val = tqmx86_gpio_read(gpio, reg);
> +
> +       val &= ~clr;
> +       val |= set;
> +
> +       tqmx86_gpio_write(gpio, val, reg);
> +}

Maybe a question that has been asked before but why are we rolling
a set of tqmx86_* wrappers that start to look like regmap-mmio
instead of just using regmap-mmio?

tqmx86_gpio_[read|write|get|set] and now clrsetbits can all
be handled by corresponding regmap_* calls (in this case
regmap_update_bits().

Sure, this driver is using a raq spinlock but regmap-mmio supports
raw spinlocks too.

Yours,
Linus Walleij

Re: [PATCH 3/4] gpio: tqmx86: introduce tqmx86_gpio_clrsetbits() helper

[Matthias Schiffer]

On Tue, 2024-12-17 at 15:16 +0100, Linus Walleij wrote:
> 
> On Mon, Dec 9, 2024 at 11:36 AM Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com> wrote:
> 
> > +static void tqmx86_gpio_clrsetbits(struct tqmx86_gpio_data *gpio,
> > +                                   u8 clr, u8 set, unsigned int reg)
> > +       __must_hold(&gpio->spinlock)
> > +{
> > +       u8 val = tqmx86_gpio_read(gpio, reg);
> > +
> > +       val &= ~clr;
> > +       val |= set;
> > +
> > +       tqmx86_gpio_write(gpio, val, reg);
> > +}
> 
> Maybe a question that has been asked before but why are we rolling
> a set of tqmx86_* wrappers that start to look like regmap-mmio
> instead of just using regmap-mmio?
> 
> tqmx86_gpio_[read|write|get|set] and now clrsetbits can all
> be handled by corresponding regmap_* calls (in this case
> regmap_update_bits().
> 
> Sure, this driver is using a raq spinlock but regmap-mmio supports
> raw spinlocks too.

A while ago I did have a WIP version of my patches that used a regmap, but it
only added another layer of abstraction without simplifying anything:

- I introduced a tqmx86_gpio_read() wrapper around regmap_read() to avoid
  dealing with the indirect value argument all the time for an operation that
  can't actually fail
- I also kept the tqmx86_gpio_write() for symmetry (just wrapping regmap_write)
- I introduced a tqmx86_gpio_clrsetbits() wrapper around regmap_update_bits()
  (having arguments for set and clear was more convenient than mask and value
   in a few places)
- I was still handling locking outside of regmap because we sometimes want to
  protect a whole sequence of accesses or other driver state
- The TQMx86 GPIO controller has a write-only and a read-only register at the 
  same address, which I understand not to be supported well by regmap (at least
  if you also want to use a regcache)

So I abandoned the regmap approach. If you think it's still a good idea, I can
of course work it into the next set of patches again.

Best regards,
Matthias


> 
> Yours,
> Linus Walleij

-- 
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 3/4] gpio: tqmx86: introduce tqmx86_gpio_clrsetbits() helper

[Linus Walleij]

On Tue, Dec 17, 2024 at 4:11 PM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:

> - I introduced a tqmx86_gpio_read() wrapper around regmap_read() to avoid
>   dealing with the indirect value argument all the time for an operation that
>   can't actually fail
> - I also kept the tqmx86_gpio_write() for symmetry (just wrapping regmap_write)

I don't see why we can't add
unsigned in regmap_read_cantfail()
that always just return the value if this is a common problem for people using
regmap MMIO specifically? Could perhaps be restricted to mmio.

Maybe Mark has objections.

> - I introduced a tqmx86_gpio_clrsetbits() wrapper around regmap_update_bits()
>   (having arguments for set and clear was more convenient than mask and value
>    in a few places)

Isn't that what regmap fields are for?
regmap_field_set_bits()
regmap_field_clear_bits()
...
but I can see why that would feel overdesigned, it's not like I don't get
the point.

> - I was still handling locking outside of regmap because we sometimes want to
>   protect a whole sequence of accesses or other driver state

So reg_sequence cannot be used in this case? (Other driver state seems
to imply that.)

> - The TQMx86 GPIO controller has a write-only and a read-only register at the
>   same address, which I understand not to be supported well by regmap (at least
>   if you also want to use a regcache)

Hehe yeah that is a first! I never saw that before.

Thanks for considering anyway, I can live without regmap.

Yours,
Linus Walleij

Re: [PATCH 3/4] gpio: tqmx86: introduce tqmx86_gpio_clrsetbits() helper

[Mark Brown]

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

On Fri, Dec 20, 2024 at 02:43:15PM +0100, Linus Walleij wrote:

> I don't see why we can't add
> unsigned in regmap_read_cantfail()
> that always just return the value if this is a common problem for people using
> regmap MMIO specifically? Could perhaps be restricted to mmio.

I can't recall people ever having much problem with just ignoring the
return value if they don't care about it.

> > - I introduced a tqmx86_gpio_clrsetbits() wrapper around regmap_update_bits()
> >   (having arguments for set and clear was more convenient than mask and value
> >    in a few places)

> Isn't that what regmap fields are for?
> regmap_field_set_bits()
> regmap_field_clear_bits()

The field API is more for and indirection where bitfields move about,
but there are top level set and clear operations too.

> > - I was still handling locking outside of regmap because we sometimes want to
> >   protect a whole sequence of accesses or other driver state

> So reg_sequence cannot be used in this case? (Other driver state seems
> to imply that.)

Yes, regmap_multi_reg_write() does what it says on the tin.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 4/4] gpio: tqmx86: add support for changing GPIO directions

[Matthias Schiffer]

Only GPIOs 4..7 have IRQ support on the TQMx86 variants currently handled
by the driver, but apart from that, changing directions works fine. The
default directions are left unchanged (0..3 output, 4..7 input) to match
the COM Express specification.

A tqmx86_gpio_set() variant without locking is introduced as a new
helper.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/gpio/gpio-tqmx86.c | 46 ++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 54e7e193bb209..4c7e7b5950426 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -84,6 +84,14 @@ static int tqmx86_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	return !!(tqmx86_gpio_read(gpio, TQMX86_GPIOD) & BIT(offset));
 }
 
+static void _tqmx86_gpio_set(struct tqmx86_gpio_data *gpio, unsigned int offset,
+			     int value)
+	__must_hold(&gpio->spinlock)
+{
+	__assign_bit(offset, gpio->output, value);
+	tqmx86_gpio_write(gpio, bitmap_get_value8(gpio->output, 0), TQMX86_GPIOD);
+}
+
 static void tqmx86_gpio_set(struct gpio_chip *chip, unsigned int offset,
 			    int value)
 {
@@ -91,40 +99,50 @@ static void tqmx86_gpio_set(struct gpio_chip *chip, unsigned int offset,
 	unsigned long flags;
 
 	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_set(gpio, offset, value);
 	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 }
 
 static int tqmx86_gpio_direction_input(struct gpio_chip *chip,
 				       unsigned int offset)
 {
-	/* Direction cannot be changed. Validate is an input. */
-	if (BIT(offset) & TQMX86_DIR_INPUT_MASK)
-		return 0;
-	else
-		return -EINVAL;
+	struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&gpio->spinlock, flags);
+	tqmx86_gpio_clrsetbits(gpio, BIT(offset), 0, TQMX86_GPIODD);
+	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
+
+	return 0;
 }
 
 static int tqmx86_gpio_direction_output(struct gpio_chip *chip,
 					unsigned int offset,
 					int value)
 {
-	/* Direction cannot be changed, validate is an output */
-	if (BIT(offset) & TQMX86_DIR_INPUT_MASK)
-		return -EINVAL;
+	struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&gpio->spinlock, flags);
+	_tqmx86_gpio_set(gpio, offset, value);
+	tqmx86_gpio_clrsetbits(gpio, 0, BIT(offset), TQMX86_GPIODD);
+	raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
 
-	tqmx86_gpio_set(chip, offset, value);
 	return 0;
 }
 
 static int tqmx86_gpio_get_direction(struct gpio_chip *chip,
 				     unsigned int offset)
 {
-	if (TQMX86_DIR_INPUT_MASK & BIT(offset))
-		return GPIO_LINE_DIRECTION_IN;
+	struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
+	u8 val;
+
+	val = tqmx86_gpio_read(gpio, TQMX86_GPIODD);
+
+	if (val & BIT(offset))
+		return GPIO_LINE_DIRECTION_OUT;
 
-	return GPIO_LINE_DIRECTION_OUT;
+	return GPIO_LINE_DIRECTION_IN;
 }
 
 static void tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
-- 
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/4] gpio: tqmx86: add support for changing GPIO directions

[Bartosz Golaszewski]

On Mon, Dec 9, 2024 at 11:36 AM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:
>
> Only GPIOs 4..7 have IRQ support on the TQMx86 variants currently handled
> by the driver, but apart from that, changing directions works fine. The
> default directions are left unchanged (0..3 output, 4..7 input) to match
> the COM Express specification.
>
> A tqmx86_gpio_set() variant without locking is introduced as a new
> helper.
>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/gpio/gpio-tqmx86.c | 46 ++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> index 54e7e193bb209..4c7e7b5950426 100644
> --- a/drivers/gpio/gpio-tqmx86.c
> +++ b/drivers/gpio/gpio-tqmx86.c
> @@ -84,6 +84,14 @@ static int tqmx86_gpio_get(struct gpio_chip *chip, unsigned int offset)
>         return !!(tqmx86_gpio_read(gpio, TQMX86_GPIOD) & BIT(offset));
>  }
>
> +static void _tqmx86_gpio_set(struct tqmx86_gpio_data *gpio, unsigned int offset,
> +                            int value)
> +       __must_hold(&gpio->spinlock)
> +{
> +       __assign_bit(offset, gpio->output, value);
> +       tqmx86_gpio_write(gpio, bitmap_get_value8(gpio->output, 0), TQMX86_GPIOD);
> +}
> +
>  static void tqmx86_gpio_set(struct gpio_chip *chip, unsigned int offset,
>                             int value)
>  {
> @@ -91,40 +99,50 @@ static void tqmx86_gpio_set(struct gpio_chip *chip, unsigned int offset,
>         unsigned long flags;
>
>         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_set(gpio, offset, value);
>         raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
>  }
>
>  static int tqmx86_gpio_direction_input(struct gpio_chip *chip,
>                                        unsigned int offset)
>  {
> -       /* Direction cannot be changed. Validate is an input. */
> -       if (BIT(offset) & TQMX86_DIR_INPUT_MASK)
> -               return 0;
> -       else
> -               return -EINVAL;
> +       struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&gpio->spinlock, flags);
> +       tqmx86_gpio_clrsetbits(gpio, BIT(offset), 0, TQMX86_GPIODD);
> +       raw_spin_unlock_irqrestore(&gpio->spinlock, flags);

Maybe switch to using lock guards if you're changing the code anyway?

Bart

> +
> +       return 0;
>  }
>
>  static int tqmx86_gpio_direction_output(struct gpio_chip *chip,
>                                         unsigned int offset,
>                                         int value)
>  {
> -       /* Direction cannot be changed, validate is an output */
> -       if (BIT(offset) & TQMX86_DIR_INPUT_MASK)
> -               return -EINVAL;
> +       struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&gpio->spinlock, flags);
> +       _tqmx86_gpio_set(gpio, offset, value);
> +       tqmx86_gpio_clrsetbits(gpio, 0, BIT(offset), TQMX86_GPIODD);
> +       raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
>
> -       tqmx86_gpio_set(chip, offset, value);
>         return 0;
>  }
>
>  static int tqmx86_gpio_get_direction(struct gpio_chip *chip,
>                                      unsigned int offset)
>  {
> -       if (TQMX86_DIR_INPUT_MASK & BIT(offset))
> -               return GPIO_LINE_DIRECTION_IN;
> +       struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip);
> +       u8 val;
> +
> +       val = tqmx86_gpio_read(gpio, TQMX86_GPIODD);
> +
> +       if (val & BIT(offset))
> +               return GPIO_LINE_DIRECTION_OUT;
>
> -       return GPIO_LINE_DIRECTION_OUT;
> +       return GPIO_LINE_DIRECTION_IN;
>  }
>
>  static void tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
> --
> 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 0/4] gpio-tqmx86: cleanup + changing directions

[Bartosz Golaszewski]

On Mon, Dec 9, 2024 at 11:36 AM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:
>
> This is the first of two series adding new features to the gpio-tqmx86
> driver. The first 3 patches are cleanup/preparation and the last patch
> adds support for changing the directions of GPIOs.
>
> Once this is merged, the final series will add support for new TQMx86
> variants (SMARC and COM-HPC) that feature up to 14 GPIOs and full IRQ
> support on all lines.
>

It's not like this series is very big, what stops you from posting the
entire thing right away? It would probably add more context to this
series.

Bart

>
> Matthias Schiffer (4):
>   gpio: tqmx86: add macros for interrupt configuration
>   gpio: tqmx86: consistently refer to IRQs by hwirq numbers
>   gpio: tqmx86: introduce tqmx86_gpio_clrsetbits() helper
>   gpio: tqmx86: add support for changing GPIO directions
>
>  drivers/gpio/gpio-tqmx86.c | 135 +++++++++++++++++++++++--------------
>  1 file changed, 84 insertions(+), 51 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/
>

Re: [PATCH 0/4] gpio-tqmx86: cleanup + changing directions

[Matthias Schiffer]

On Wed, 2024-12-11 at 16:12 +0100, Bartosz Golaszewski wrote:
> 
> On Mon, Dec 9, 2024 at 11:36 AM Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com> wrote:
> > 
> > This is the first of two series adding new features to the gpio-tqmx86
> > driver. The first 3 patches are cleanup/preparation and the last patch
> > adds support for changing the directions of GPIOs.
> > 
> > Once this is merged, the final series will add support for new TQMx86
> > variants (SMARC and COM-HPC) that feature up to 14 GPIOs and full IRQ
> > support on all lines.
> > 
> 
> It's not like this series is very big, what stops you from posting the
> entire thing right away? It would probably add more context to this
> series.
> 
> Bart

The second series is bigger and involves both the GPIO and MFD drivers. I kinda
expect a few rounds of reviews to be needed before it gets accepted, so my
intention was to get these smaller, more obvious patches out of the way first.

Best regards,
Matthias



> 
> > 
> > Matthias Schiffer (4):
> >   gpio: tqmx86: add macros for interrupt configuration
> >   gpio: tqmx86: consistently refer to IRQs by hwirq numbers
> >   gpio: tqmx86: introduce tqmx86_gpio_clrsetbits() helper
> >   gpio: tqmx86: add support for changing GPIO directions
> > 
> >  drivers/gpio/gpio-tqmx86.c | 135 +++++++++++++++++++++++--------------
> >  1 file changed, 84 insertions(+), 51 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/
> > 

-- 
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 0/4] gpio-tqmx86: cleanup + changing directions

[Bartosz Golaszewski]

On Wed, Dec 11, 2024 at 4:35 PM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:
>
> On Wed, 2024-12-11 at 16:12 +0100, Bartosz Golaszewski wrote:
> >
> > On Mon, Dec 9, 2024 at 11:36 AM Matthias Schiffer
> > <matthias.schiffer@ew.tq-group.com> wrote:
> > >
> > > This is the first of two series adding new features to the gpio-tqmx86
> > > driver. The first 3 patches are cleanup/preparation and the last patch
> > > adds support for changing the directions of GPIOs.
> > >
> > > Once this is merged, the final series will add support for new TQMx86
> > > variants (SMARC and COM-HPC) that feature up to 14 GPIOs and full IRQ
> > > support on all lines.
> > >
> >
> > It's not like this series is very big, what stops you from posting the
> > entire thing right away? It would probably add more context to this
> > series.
> >
> > Bart
>
> The second series is bigger and involves both the GPIO and MFD drivers. I kinda
> expect a few rounds of reviews to be needed before it gets accepted, so my
> intention was to get these smaller, more obvious patches out of the way first.
>
> Best regards,
> Matthias

Fair enough. Could you address the nits I pointed out? I'll queue these then.

Bart