linux-kernel - Re: [PATCH 3/4] gpio: tqmx86: introduce tqmx86_gpio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <dc83bbd1cd960f8a5daa7ad687f419609f5e14b9.camel@ew.tq-group.com>
Date: Tue, 17 Dec 2024 16:11:27 +0100
From: Matthias Schiffer <matthias.schiffer@...tq-group.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, linux@...tq-group.com, 
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] gpio: tqmx86: introduce tqmx86_gpio_clrsetbits()
 helper

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@...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/