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

[Matthias Schiffer <matthias.schiffer@ew.tq-group.com>]

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/