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/

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

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/

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

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.