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

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

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.

-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