RE: [PATCH v3 3/7] gpio: regmap: Add gpio_regmap_operation and write-enable support

[Yu-Chun Lin [林祐君]]

Hi Jonathan and Linus,

It's been a while since this thread, sorry for the delay.
I abandoned the v4/v5 approaches and decided to go back to the v3 design,
so I am returning here to fix the problems you pointed out back then.

> On Tue, 12 May 2026 11:33:13 +0800
> Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx> wrote:
> 
> > Extend the reg_mask_xlate callback with an operation type parameter
> > (gpio_regmap_operation) to allow drivers to return different
> > register/mask combinations for different GPIO operations.
> >
> > Also add write-enable mechanism for hardware that requires setting a
> > write-enable bit before modifying GPIO control registers.
> >
> > Consequently, update all existing drivers utilizing the gpio-regmap
> > framework (across drivers/gpio, drivers/iio, and drivers/pinctrl) to
> > accommodate the new reg_mask_xlate function signature.
> 
> What is the reasoning behind setting *mask = 0 for unsupported operations?
> If it is a special value why not just return an error code to indicate not
> supported?  This seems to come from the assumption that you will want to |
> that with masks for another operation.
> 
> I'm coming into this late but to me there look to be a bunch of undocumented
> assumptions. Why do the operations have to share a register for example?
> 
> Perhaps an interface where you provide a single operation for write_enable
> and whatever else and hence t here is only one 'reg' would work better?
> 
> >
> > Suggested-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > Signed-off-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> 
> > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> > index deb9eebb58de..c76eef20e412 100644
> > --- a/drivers/gpio/gpio-regmap.c
> > +++ b/drivers/gpio/gpio-regmap.c

(...)

> >
> > @@ -185,7 +218,7 @@ static int gpio_regmap_set_direction(struct
> gpio_chip *chip,
> >                                    unsigned int offset, bool output)
> > {
> >       struct gpio_regmap *gpio = gpiochip_get_data(chip);
> > -     unsigned int base, val, reg, mask;
> > +     unsigned int base, val, reg, mask, wren_mask;
> >       int invert, ret;
> >
> >       if (gpio->reg_dir_out_base) {
> > @@ -198,7 +231,12 @@ static int gpio_regmap_set_direction(struct
> gpio_chip *chip,
> >               return -ENOTSUPP;
> >       }
> >
> > -     ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> > +     ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_DIR_OP, base,
> offset, &reg, &mask);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_DIR_WREN_OP,
> base, offset, &reg,
> > +                                &wren_mask);
> 
> What constrains these two to provide the same value back for reg?
> To me it seems like the write enable might well be in a different register.
> 
> >       if (ret)
> >               return ret;
> >
> > @@ -207,7 +245,7 @@ static int gpio_regmap_set_direction(struct
> gpio_chip *chip,
> >       else
> >               val = output ? mask : 0;
> >
> > -     return regmap_update_bits(gpio->regmap, reg, mask, val);
> > +     return regmap_update_bits(gpio->regmap, reg, mask | wren_mask,
> > + val | wren_mask);
> >  }
> >
> >  static int gpio_regmap_direction_input(struct gpio_chip *chip,

My initial design indeed assumed that the WREN mask and Data mask reside in
the same register.
 
Regarding WREN support, especially if WREN and Data use separate registers, I
came up with three ideas. Which direction do you prefer?
 
Approach 1: Provide Custom Callbacks in config (Let consumer driver handle it)
We can add '.set' and '.set_direction' function pointers in
'struct gpio_regmap_config'. If a driver requires WREN, it can implement these
callbacks itself.
 
static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset, int val)
{
        struct gpio_regmap *gpio = gpiochip_get_data(chip);
 
        /* If the driver provides a custom set (to handle WREN), delegate to it */
        if (gpio->set) {
                gpio->set(chip, offset, val);
                return;
        }
        /* ... existing generic regmap logic ... */
}

Pros: Clean core, no need to touch existing drivers' xlate signature. The consumer
driver handles its own locking for different registers.
Cons: It feels a bit strange and inconsistent to expose only '.set' and
'.set_direction' overrides while keeping other operations entirely abstracted.
 
Approach 2: Handle separate WREN register in the core (with locking concerns)
We keep the 'XX_WREN_OP' in 'xlate'. If someone needs WREN and 'wren_reg != reg',
we write to both.
 
static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
                           int val)
{
        /* skip */
        ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WREN_OP, base, offset, &wren_reg,
                                   &wren_mask);
        if (ret == -ENOTSUPP)
                has_wren = false;
        else if (ret)
                return ret;
 
        ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_OP, base, offset, &reg, &mask);
 
        if (has_wren && reg == wren_reg) {
                mask |= wren_mask;
                mask_val |= wren_mask;
                has_wren = false;
        }
 
        if (has_wren)
                ret = regmap_set_bits(gpio->regmap, wren_reg, wren_mask); 

        /* ignore input values which shadow the old output value */
        if (gpio->reg_dat_base == gpio->reg_set_base)
                ret = regmap_write_bits(gpio->regmap, reg, mask, mask_val);
        else
                ret = regmap_update_bits(gpio->regmap, reg, mask, mask_val);
 
        return ret;
}
 
Pros: Keeps all WREN logic unified inside the core framework.
Cons: Introduces a locking issue. writing to 'wren_reg' and then 'reg' requires an
external lock to be atomic, which seems to defeat the purpose of relying on regmap's
internal lock. 

Approach 3: Assume WREN and Data always share the same register

static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset, int val)
{
        /* ... */
        ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WREN_OP, base, offset, &reg, &wren_mask);
        if (ret == -ENOTSUPP)
                wren_mask = 0;
        else if (ret)
                return ret;
 
        ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_OP, base, offset, &reg, &mask);
 
        ret = regmap_update_bits(gpio->regmap, reg, mask | wren_mask, mask_val | wren_mask);
        return ret;
}
 
Regarding this approach, I would like to ask from your experience: Is it
actually common for hardware designs to place WREN and Data bits in completely
different registers for GPIO operations?

If they practically always share the same register, this simpler approach might
suffice.
 
Best Regards,
Yu Chun Lin