Re: [PATCH v7 5/9] gpio: regmap: Add gpio_regmap_operation to extend reg_mask_xlate callback

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

[-- Attachment #1: Type: text/plain, Size: 4541 bytes --]

On Thu Jul 23, 2026 at 5:42 AM CEST, Yu-Chun Lin wrote:
> Extend the reg_mask_xlate callback with an operation type parameter
> (enum gpio_regmap_operation) to allow drivers to return different
> register/mask combinations depending on the specific GPIO operation.
>
> 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.
>
> Suggested-by: Linus Walleij <linusw@kernel.org>
> Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> ---

...

> @@ -71,7 +73,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 */
> @@ -80,7 +82,18 @@ 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);
> +	else
> +		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_IN, base, offset, &reg, &mask);

What's going on here? Looks like I've missed this in the previous
patches. All the other drivers we are now reading the value twice.
In the regmap_read() above and the one that follows just after this
hunk.

Has the gpio controller two different bits, one for output and one
for input? Are you sure, the input bit doesn't reflect the output
bit if it's configured as an output?

IMHO this shouldn't be part of the core. rtd1625_reg_mask_xlate()
should either return RTD1625_GPIO_IN or RTD1625_GPIO_OUT depending
on the mode, if that's needed at all. I'd guess RTD1625_GPIO_IN
will just work fine and will actually fetch the actual line state.

-michael

>  	if (ret)
>  		return ret;
>  
...

>  
> +/**
> + * enum gpio_regmap_operation - Operation type for reg_mask_xlate callback
> + *
> + * Traditionally, the operation type was inferred from the base register.
> + * However, that approach does not always work — for example, when all control
> + * bits of a single GPIO reside in the same register. This enum allows the
> + * reg_mask_xlate callback to explicitly distinguish between operation types.
> + * The user is free to choose which method to use.
> + *
> + * Value operations:
> + * @GPIO_REGMAP_GET_OP: Mask for reading direction to detect if GPIO is input or
> + *			output. Used in gpio_regmap_get() to determine the GPIO
> + *			direction.

This is also not very intuitive. I'd expect there is only one
operation for the gpio_regmap_get and that is exactly this one.

> + * @GPIO_REGMAP_IN: Mask for reading input value. Used when GPIO is configured as
> + *			input.
> + * @GPIO_REGMAP_OUT: Mask for reading output value. Used when GPIO is configured as
> + *			output.
> + *
> + * Output operations:
> + * @GPIO_REGMAP_SET_OP: Mask for setting GPIO output value.
> + *
> + * Direction operations:
> + * @GPIO_REGMAP_GET_DIR_OP: Mask for reading GPIO direction (input/output).
> + * @GPIO_REGMAP_SET_DIR_OP: Mask for setting GPIO direction (input/output).
> + */
> +enum gpio_regmap_operation {
> +	GPIO_REGMAP_GET_OP,
> +	GPIO_REGMAP_SET_OP,
> +	GPIO_REGMAP_GET_DIR_OP,
> +	GPIO_REGMAP_SET_DIR_OP,
> +	GPIO_REGMAP_IN,
> +	GPIO_REGMAP_OUT,
> +};
> +
>  /**
>   * struct gpio_regmap_config - Description of a generic regmap gpio_chip.
>   * @parent:		The parent device
> @@ -104,9 +138,9 @@ struct gpio_regmap_config {
>  	unsigned long regmap_irq_flags;
>  #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,
> +			      unsigned int base, unsigned int offset,
> +			      unsigned int *reg, unsigned int *mask);
>  
>  	int (*init_valid_mask)(struct gpio_chip *gc,
>  			       unsigned long *valid_mask,


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]