Re: [PATCH v4 12/14] gpio: regmap: Add optional runtime PM support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Aug 21, 2026 at 04:07:05PM +0200, Janani Sunil wrote:
> Some gpio-regmap consumers share their regmap with a parent device that
> may be runtime suspended. GPIO register accesses must resume that device
> first.
> 
> Add an optional pm_dev field and acquire it before register translation
> or access. Release it using runtime autosuspend after each operation.
> Keep the device active across the complete direction-output sequence and
> propagate failure when setting the initial output value.

> struct gpio_regmap {

>  	unsigned int reg_clr_base;
>  	unsigned int reg_dir_in_base;
>  	unsigned int reg_dir_out_base;

> +	struct device *pm_dev;

Please, move it out of these fields, the below is coupled with reg_dir_*
in some way.

Having this somewhere near to

	struct gpio_chip gpio_chip;

above makes more sense.

>  	unsigned long *fixed_direction_mask;
>  	unsigned long *fixed_direction_output;

>  };

...

> +static int gpio_regmap_runtime_get(struct gpio_regmap *gpio)
> +{
> +	if (!gpio->pm_dev)
> +		return 0;
> +
> +	return pm_runtime_get_active(gpio->pm_dev, RPM_TRANSPARENT);
> +}
> +
> +static void gpio_regmap_runtime_put(struct gpio_regmap *gpio)
> +{
> +	if (!gpio->pm_dev)
> +		return;
> +
> +	pm_runtime_put_autosuspend(gpio->pm_dev);
> +}

> +DEFINE_GUARD(gpio_regmap_runtime, struct gpio_regmap *,
> +	     gpio_regmap_runtime_get(_T), gpio_regmap_runtime_put(_T))

This is not used, it's for guard()() case.

> +DEFINE_GUARD_COND(gpio_regmap_runtime, _try,
> +		  gpio_regmap_runtime_get(_T), _RET == 0)

Yep, but also makes sense to add

#define GPIO_REGMAP_RUNTIME_ACQUIRE(_dev, _var)			\
	ACQUIRE(gpio_regmap_runtime_try, _var)(_dev)

#define GPIO_REGMAP_RUNTIME_ACQUIRE_ERR(_var_ptr)		\
	ACQUIRE_ERR(gpio_regmap_runtime, _var_ptr)

...

> +	chip->can_sleep = config->pm_dev ||
> +			  regmap_might_sleep(config->regmap);

Fits a single line.

...

>   * @ngpio_per_reg:	(Optional) Number of GPIOs per register
>   * @irq_domain:		(Optional) IRQ domain if the controller is
>   *			interrupt-capable
> + * @pm_dev:		(Optional) Device to use for runtime power management.
>   * @reg_mask_xlate:     (Optional) Translates base address and GPIO
>   *			offset to a register/bitmask pair. If not
>   *			given the default gpio_regmap_simple_xlate()

> struct gpio_regmap_config {

>  	int reg_stride;
>  	int ngpio_per_reg;
>  	struct irq_domain *irq_domain;
> +	struct device *pm_dev;
>  	unsigned long *fixed_direction_mask;
>  	unsigned long *fixed_direction_output;

Oh, this needs synchronisation of the ordering of members and kernel-doc
descriptions. Not a problem for you or in your patch series, just let's
try to find the best fit for a new member from day 1.

Like in the above local structure I suggest to put this just after

	struct regmap *regmap;

-- 
With Best Regards,
Andy Shevchenko