Hi Mehmet,
On 2026-08-28T10:52:07, Mehmet Fide <[email protected]> wrote:
> regulator: claim the enable GPIO at probe time, not in of_to_plat()
>
> regulator_common_of_to_plat() requests the enable GPIO, which probes
> the GPIO controller and claims the pin while the consumer's platform
> data is still being read. of_to_plat() must not do either: it runs
> before the device's pinctrl state is applied, so on SoCs where the
> direction lives in the pad register - the output enable on Vybrid is
> one - the pinctrl undoes the direction the early claim just set, and a
> fixed regulator configured as always-on powers nothing.
>
> Parse the GPIO into the platform data in of_to_plat() and request it in
> the new regulator_common_probe(), called from the fixed and fixed-clock
> regulator probe methods. The gpio regulator's voltage-control GPIO has
> the same problem and moves the same way.
>
> Signed-off-by: Mehmet Fide <[email protected]>
>
> drivers/power/regulator/fixed.c | 11 +++++++++++
> drivers/power/regulator/gpio-regulator.c | 21 ++++++++++++++++++---
> drivers/power/regulator/regulator_common.c | 25 +++++++++++++++++++++----
> drivers/power/regulator/regulator_common.h | 3 +++
> 4 files changed, 53 insertions(+), 7 deletions(-)
> diff --git a/drivers/power/regulator/gpio-regulator.c
> b/drivers/power/regulator/gpio-regulator.c
> @@ -28,7 +29,6 @@ static int gpio_regulator_of_to_plat(struct udevice *dev)
> - gpio = &plat->gpio;
> - ret = gpio_request_by_name(dev, "gpios", 0, gpio, GPIOD_IS_OUT);
> + ret = gpio_parse_by_name(dev, "gpios", 0, GPIOD_IS_OUT,
> + &plat->gpio_dt);
> if (ret)
> debug("regulator gpio - not found! Error: %d", ret);
The debug message is stale once the request moves to probe() - a parse
failure no longer means the GPIO controller could not give us the pin,
only that the phandle could not be read. Please reword (something like
'gpio-regulator: cannot parse voltage GPIO') and add the missing '\n'.
> diff --git a/drivers/power/regulator/gpio-regulator.c
> b/drivers/power/regulator/gpio-regulator.c
> @@ -76,6 +76,20 @@ static int gpio_regulator_of_to_plat(struct udevice *dev)
> +static int gpio_regulator_probe(struct udevice *dev)
> +{
> + struct gpio_regulator_plat *plat = dev_get_plat(dev);
> + int ret;
> +
> + if (plat->gpio_dt.present) {
> + ret = gpio_request_parsed(dev, &plat->gpio_dt, &plat->gpio);
> + if (ret)
> + return ret;
> + }
> +
> + return regulator_common_probe(dev, &plat->common);
> +}
This quietly changes behaviour: the old of_to_plat() only
debug()-logged a failure from gpio_request_by_name() and carried on,
whereas probe() now aborts the device for any non-ENOENT error. That
may be right, but please call it out in the commit message so anyone
bisecting a regulator that used to 'work' knows where to look.
Also the 'if (plat->gpio_dt.present)' guard duplicates the check that
gpio_request_parsed() already does - the whole point of the -ENOENT
return in patch 1 is that the caller does not need to guard.
regulator_common_probe() below takes the other approach and reads
better; please pick one and use it in both places.
> diff --git a/drivers/power/regulator/fixed.c b/drivers/power/regulator/fixed.c
> @@ -115,6 +120,11 @@ static int fixed_clock_regulator_set_enable(struct
> udevice *dev, bool enable)
> static int fixed_clock_regulator_probe(struct udevice *dev)
> {
> struct fixed_clock_regulator_priv *priv = dev_get_priv(dev);
> + int ret;
> +
> + ret = regulator_common_probe(dev, dev_get_plat(dev));
> + if (ret)
> + return ret;
Does fixed-clock ever actually carry an enable GPIO in the wild? It
shares of_to_plat() with the plain fixed regulator so the parse
happens, but set_enable() only pokes the clock, so the GPIO would be
claimed and never used. If the answer is 'no', I would rather not add
the call here at all; if 'sometimes', a line in the commit message
explaining why fixed-clock needs it too would help.
> diff --git a/drivers/power/regulator/regulator_common.h
> b/drivers/power/regulator/regulator_common.h
> @@ -10,6 +10,7 @@
> struct regulator_common_plat {
> + struct gpio_dt_desc gpio_dt; /* parsed enable GPIO, requested in probe
> */
> struct gpio_desc gpio; /* GPIO for regulator enable control */
Once the device is probed, gpio_dt is dead weight in plat - the
phandle args, list_name pointer and flags are all baked into gpio. Not
critical, but on boards with a lot of regulators this doubles the
per-device plat footprint for no runtime benefit. Worth a note in the
commit message that this is a deliberate trade for keeping the
two-phase split simple.
Regards,
Simon
