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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] gpio_regmap_get() incorrectly uses the physical GPIO data level to determine its direction.
- [High] The new direction check in gpio_regmap_get() spoils the regmap cache with volatile input values.
- [High] The write-enable (WREN) implementation overwrites the register pointer and corrupts hardware registers if WREN and data registers differ.
--

commit aa0f1e735514010e24fe4769039de6cdc0302e5d
Author: Yu-Chun Lin <eleanor.lin@realtek.com>

gpio: regmap: Add gpio_regmap_operation and write-enable support

This commit extends the reg_mask_xlate callback with an operation type
parameter to allow different register and mask combinations per GPIO
operation. It also introduces a write-enable mechanism for hardware
requiring a protection bit before modifying control registers.

> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
[ ... ]
> @@ -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);

Could this read operation spoil the regmap cache with volatile input values?

The comment further down in this function mentions bypassing the cache to
avoid spoiling it with pin input values, but calling regmap_read() here
appears to cache the state.

> +	if (ret)
> +		return ret;
> +
> +	if (val & dir_mask) {

Will this incorrectly evaluate the physical GPIO data level as the direction?

Since base is set to the data register base earlier in the function, it
looks like this checks if the data bit is high or low rather than querying
a separate direction register. Would this cause an input pin driven high
to be treated as an output?

> +		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;

Is it possible for the second reg_mask_xlate() call to overwrite the
register pointer if the write-enable register is different from the data
register? Both calls pass the same reg pointer, which might result in the
first queried address being lost.

>  
> @@ -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;
>  }

Does applying both masks at the same time contradict the mechanism described
in the commit message?

The message states the hardware requires setting the write-enable bit before
modifying GPIO control registers, but this writes both bits simultaneously.
Additionally, if the write-enable mask was intended for a different register,
could this bitwise OR apply the combined masks to the wrong register?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512033317.1602537-1-eleanor.lin@realtek.com?part=3