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

["Michael Walle" <mwalle@kernel.org>]

On Thu Jul 16, 2026 at 10:27 AM CEST, Andy Shevchenko wrote:
> On Thu, Jul 16, 2026 at 02:26:14PM +0800, Yu-Chun Lin wrote:
>
> ...
>
>> >> > -     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;
>
> ...
>
>> >> > -     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.
>> 
>> If there are no further concerns, I will proceed with the third approach and
>> send out v6.
>
> From the above list I tend to the approach 2, but this might require to have
> GPIO regmap level of locking. I'm a bit lost in the context, though. I assume
> we need a fresh start, id est issue a v6 with approach 2 or 3 in place and
> summarize the choices in the cover letter, so we can understand what has been
> considered.

I don't really like approach 3. You'd need to check if the regs of
both xlate calls are the same. With the sample code above, you
silently drop the first xlate'd reg.

And honestly, it really seems like a one-off. What controllers, are
there that need a write enable bit. The real problem seems to be
the assumption that we operate on just one bit. IOW we either set
mask or don't set mask in gpio_regmap_set().

For a more generic solution, we should be able to control the
written value. We could add another .value_xlate().

-michael