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

[Yu-Chun Lin [林祐君]]

Hi Andy and Michael,

> On Thu, Jul 16, 2026 at 11:08:55AM +0200, Michael Walle wrote:
> > 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:
> 
> ...
> 
> > > 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.
> 
> If I rank the proposals, the worst is #1, the best is #2.
> 
> > 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().
> 
> Yes, we should KISS.
> 
> > For a more generic solution, we should be able to control the written
> > value. We could add another .value_xlate().
> 
> Maybe not now? As per IPs, Synopsys IPs (not exactly GPIO) likes to have that
> kind of "protection". So, from HW perspective it's kinda pattern, and it might
> be possible to see more IPs (including GPIO) that follow it in some cases.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

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.

I have a new idea that might be similar to Michael's suggestion. 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);
		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