| Message ID | a7b98f12da735f735b33200f6324360fc380e6d0.1733739697.git.matthias.schiffer@ew.tq-group.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio-tqmx86: cleanup + changing directions | expand |
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
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)
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(-)