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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 12 May 2026 11:33:13 +0800
Yu-Chun Lin <eleanor.lin@realtek.com> 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@linaro.org>
> Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>

> 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
> @@ -38,9 +38,10 @@ struct gpio_regmap {
>  	struct regmap_irq_chip_data *irq_chip_data;
>  #endif
>  
> -	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
> -			      unsigned int offset, unsigned int *reg,
> -			      unsigned int *mask);
> +	int (*reg_mask_xlate)(struct gpio_regmap *gpio,
> +			      enum gpio_regmap_operation op,
> +			      unsigned int base, unsigned int offset,
> +			      unsigned int *reg, unsigned int *mask);
>  
>  	void *driver_data;
>  };
> @@ -54,6 +55,7 @@ static unsigned int gpio_regmap_addr(unsigned int addr)
>  }
>  
>  static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
> +				    enum gpio_regmap_operation op,
>  				    unsigned int base, unsigned int offset,
>  				    unsigned int *reg, unsigned int *mask)
>  {
> @@ -61,7 +63,16 @@ static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
>  	unsigned int stride = offset / gpio->ngpio_per_reg;
>  
>  	*reg = base + stride * gpio->reg_stride;
> -	*mask = BIT(line);
> +
> +	switch (op) {
> +	case GPIO_REGMAP_SET_WREN_OP:
> +	case GPIO_REGMAP_SET_DIR_WREN_OP:
> +		*mask = 0;
> +		break;
> +	default:
> +		*mask = BIT(line);
> +		break;
> +	}
>  
>  	return 0;
>  }
> @@ -69,7 +80,7 @@ static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
>  static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct gpio_regmap *gpio = gpiochip_get_data(chip);
> -	unsigned int base, val, reg, mask;
> +	unsigned int base, val, reg, mask, dir_mask;
>  	int ret;
>  
>  	/* we might not have an output register if we are input only */
> @@ -78,10 +89,24 @@ static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
>  	else
>  		base = gpio_regmap_addr(gpio->reg_set_base);
>  
> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_GET_OP, base, offset, &reg, &dir_mask);
>  	if (ret)
>  		return ret;
>  
> +	ret = regmap_read(gpio->regmap, reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val & dir_mask) {
> +		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_OUT, base, offset, &reg, &mask);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_IN, base, offset, &reg, &mask);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* ensure we don't spoil any register cache with pin input values */
>  	if (gpio->reg_dat_base == gpio->reg_set_base)
>  		ret = regmap_read_bypassed(gpio->regmap, reg, &val);
> @@ -98,10 +123,14 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
>  {
>  	struct gpio_regmap *gpio = gpiochip_get_data(chip);
>  	unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
> -	unsigned int reg, mask, mask_val;
> +	unsigned int reg, mask, mask_val, wren_mask;
>  	int ret;
>  
> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WREN_OP, base, offset, &reg, &wren_mask);
> +	if (ret)
> +		return ret;
> +
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_OP, base, offset, &reg, &mask);
>  	if (ret)
>  		return ret;
>  
> @@ -112,9 +141,9 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
>  
>  	/* 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);
> +		ret = regmap_write_bits(gpio->regmap, reg, mask | wren_mask, mask_val | wren_mask);
>  	else
> -		ret = regmap_update_bits(gpio->regmap, reg, mask, mask_val);
> +		ret = regmap_update_bits(gpio->regmap, reg, mask | wren_mask, mask_val | wren_mask);
>  
>  	return ret;
>  }
> @@ -123,7 +152,7 @@ static int gpio_regmap_set_with_clear(struct gpio_chip *chip,
>  				      unsigned int offset, int val)
>  {
>  	struct gpio_regmap *gpio = gpiochip_get_data(chip);
> -	unsigned int base, reg, mask;
> +	unsigned int base, reg, mask, wren_mask;
>  	int ret;
>  
>  	if (val)
> @@ -131,11 +160,15 @@ static int gpio_regmap_set_with_clear(struct gpio_chip *chip,
>  	else
>  		base = gpio_regmap_addr(gpio->reg_clr_base);
>  
> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WREN_OP, base, offset, &reg, &wren_mask);
>  	if (ret)
>  		return ret;
>  
> -	return regmap_write(gpio->regmap, reg, mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WITH_CLEAR_OP, base, offset, &reg, &mask);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(gpio->regmap, reg, mask | wren_mask);
>  }
>  
>  static int gpio_regmap_get_direction(struct gpio_chip *chip,
> @@ -167,7 +200,7 @@ static int gpio_regmap_get_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_GET_DIR_OP, base, offset, &reg, &mask);
>  	if (ret)
>  		return ret;
>  
> @@ -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,