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

["Yu-Chun Lin [林祐君]" <eleanor.lin@realtek.com>]

> Hi,
> 
> On Thu Jul 16, 2026 at 12:47 PM CEST, Yu-Chun Lin [林祐君] wrote:
> > I checked with our internal hardware engineers. The WREN bit was
> > designed to avoid race conditions, which requires both the data bit
> > and the WREN bit to be updated simultaneously within the same register.
> 
> Your mail just arrived as I was writing my reply. So yeah, that's basically what I
> was saying.
> 
> > I have a new idea that might be similar to Michael's suggestion.
> 
> If not almost exactly what I had in mind. :)
> 
> > We can
> > introduce a new callback to allow the driver to intercept and modify
> > the mask and the value. By doing so, the driver can determine the
> > corresponding WREN bit based on the original mask, and assemble the
> > WREN bit into this new callback function right before the final write API is
> executed.
> >
> > The idea looks like:
> >
> > static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
> >                          int val)
> > {
> >       struct gpio_regmap *gpio = gpiochip_get_data(chip);
> >       unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
> >       unsigned int reg, mask, mask_val, wren_reg, wren_mask;
> >       int ret;
> >
> >       ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_OP, base,
> offset, &reg, &mask);
> >       if (ret)
> >               return;
> >
> >       if (val)
> >               mask_val = mask;
> >       else
> >               mask_val = 0;
> >
> >     /* Let the driver modify the mask and mask_val to include WREN */
> >       if (gpio->value_xlate) {
> >               ret = gpio->value_xlate(&mask, &mask_val);
> 
> Yeah, but you probably also have to pass gpio :) And at this point I'd call it
> value not mask_val anymore. And the signature should probably looks
> somthing like:
> 
> int (*value_xlate)(struct gpio_chip *chip, enum gpio_regmap_op op, unsigned
> int offset, unsigned int reg, unsigned int mask, unsigned int *value)
> 
> mask shouldn't need to be a pointer as you don't have to modify it, do you?
> reg_mask_xlate, already does that.
> 

Actually, I do need to modify the 'mask', so it still needs to be passed
as a pointer to this callback.

Since the WREN enable bit and the data bit are located at different bit
positions, the final 'mask' used in write API must be expanded to include
both the WREN mask and the data mask.

Aside from needing 'mask' as a pointer, I completely agree with the rest of
your suggested signature.

Best Regards,
Yu-Chun

> -michael
> 
> >               if (ret)
> >                       return ret;
> >       }
> >
> >       /* 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;
> > }
> >
> > Best Regards,
> > Yu-Chun