Re: [PATCH v3 4/7] gpio: regmap: Add set_config callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The `set_config` callback leaks the underlying `struct gpio_chip` to the driver, breaking the abstraction boundary provided by `gpio_regmap`.
--

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

gpio: regmap: Add set_config callback

This commit adds a new set_config callback to struct gpio_regmap_config
to allow drivers to implement hardware-specific configurations like
debounce settings or other platform-specific GPIO properties.

> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> --- a/include/linux/gpio/regmap.h
> +++ b/include/linux/gpio/regmap.h
[ ... ]
> @@ -142,6 +145,10 @@ struct gpio_regmap_config {
>  			       unsigned long *valid_mask,
>  			       unsigned int ngpios);
>  
> +	int (*set_config)(struct gpio_chip *gc,
> +			  unsigned int offset,
> +			  unsigned long config);
> +
>  	void *drvdata;
>  };

Does exposing struct gpio_chip directly to the driver break the
abstraction boundary provided by gpio_regmap?

The gpio_regmap library appears designed to abstract away struct gpio_chip
from generic regmap-based drivers. For instance, the reg_mask_xlate
callback takes a struct gpio_regmap pointer rather than the raw chip.

Because gpio_regmap sets its own internal structure as the private data
of the gpio_chip during registration via gpiochip_add_data(), drivers cannot
simply access their private data using gpiochip_get_data(). They would
instead be forced to use a lookup chain like
gpio_regmap_get_drvdata(gpiochip_get_data(gc)).

Would it be better to have the callback in struct gpio_regmap_config take
a struct gpio_regmap pointer instead?

> 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
[ ... ]
> @@ -371,6 +371,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
>  	if (!gpio->reg_mask_xlate)
>  		gpio->reg_mask_xlate = gpio_regmap_simple_xlate;
>  
> +	chip->set_config = config->set_config;
> +
>  	ret = gpiochip_add_data(chip, gpio);
>  	if (ret < 0)

If the struct gpio_regmap_config callback is updated to use struct
gpio_regmap, could an internal wrapper function be added here in
gpio-regmap.c?

The wrapper could implement the struct gpio_chip set_config signature,
handle the translation from struct gpio_chip to struct gpio_regmap, and then
invoke the driver's config->set_config callback.

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