Re: [PATCH v2 5/7] gpio: regmap: Add runtime PM and read_output_reg_set support
Andy Shevchenko <[email protected]>
| Newsgroups | gmane.linux.documentation,gmane.linux.kernel.iio,gmane.linux.drivers.devicetree,gmane.linux.kernel,gmane.linux.kernel.gpio |
|---|---|
| Organization | Intel Finland Oy - BIC 0357606-4 - c/o Alberga Business Park, 6 krs, Bertel Jungin Aukio 5, 02600 Espoo |
| Message-ID | <[email protected]> |
On Thu, Aug 06, 2026 at 05:41:24PM +0200, Janani Sunil wrote:
> The new pm_dev field in gpio_regmap_config allows a driver to supply a
> device for runtime PM. All operations call pm_runtime_resume_and_get()
> before accessing the regmap and pm_runtime_put_autosuspend() on return.
>
> The new read_output_reg_set flag when set, gpio_regmap_get() checks the
> pin direction first and reads from reg_set_base instead of reg_dat_base
> for output pins. Requires both reg_dat_base and reg_set_base to be
> configured.
...
> struct gpio_regmap {
> unsigned int reg_clr_base;
> unsigned int reg_dir_in_base;
> unsigned int reg_dir_out_base;
> + struct device *pm_dev;
> + bool read_output_reg_set;
> unsigned long *fixed_direction_mask;
> unsigned long *fixed_direction_output;
> }
Have you run `pahole`? Is it okay to put bool there and not after one of
unsigned int:s (just from the given context, it might be even better location)?
...
> +static int gpio_regmap_runtime_get(struct gpio_regmap *gpio)
> +{
> + if (!gpio->pm_dev)
> + return 0;
> +
> + return pm_runtime_resume_and_get(gpio->pm_dev);
> +}
> +
> +static void gpio_regmap_runtime_put(struct gpio_regmap *gpio)
> +{
> + if (gpio->pm_dev)
> + pm_runtime_put_autosuspend(gpio->pm_dev);
Same pattern as per above.
> +}
...
> +static int gpio_regmap_get_direction(struct gpio_chip *chip,
> + unsigned int offset);
Can this be avoidable?
...
> /* 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);
> else
> ret = regmap_read(gpio->regmap, reg, &val);
> - if (ret)
> - return ret;
> + if (!ret)
> + ret = !!(val & mask);
>
> - return !!(val & mask);
You want regmap_test_bits() in one case, and the regular pattern in the other.
> +out_pm:
labels should be marked better to show what will be when goto them.
out_pm_put:
> + gpio_regmap_runtime_put(gpio);
> + return ret;
...
> +out_pm:
> + gpio_regmap_runtime_put(gpio);
> return ret;
Ditto.
...
> ret = regmap_read(gpio->regmap, reg, &val);
> if (ret)
> - return ret;
> + goto out_pm;
>
> if (!!(val & mask) ^ invert)
> - return GPIO_LINE_DIRECTION_OUT;
> + ret = GPIO_LINE_DIRECTION_OUT;
> else
> - return GPIO_LINE_DIRECTION_IN;
> + ret = GPIO_LINE_DIRECTION_IN;
Also can be transformed to use regmap_test_bits() (but in a separate change).
...
> struct gpio_regmap_config {
> unsigned int reg_clr_base;
> unsigned int reg_dir_in_base;
> unsigned int reg_dir_out_base;
> + struct device *pm_dev;
> + bool read_output_reg_set;
> int reg_stride;
> int ngpio_per_reg;
> struct irq_domain *irq_domain;
`pahole`?
--
With Best Regards,
Andy Shevchenko