Hi Eugen, On 2023-03-31 11:15, Eugen Hristev wrote: > Some devices share a regulator supply, when the first one will request > regulator disable, the second device will have it's supply cut off before > graciously shutting down. Hence there will be timeouts and other failed > operations. > Implement a reference counter mechanism similar with what is done in > Linux, to keep track of enable and disable requests, and only disable the > regulator when the last of the consumers has requested shutdown. > > Signed-off-by: Eugen Hristev <eugen.hris...@collabora.com> > Reviewed-by: Simon Glass <s...@chromium.org> > --- > Changes in v4: > - add documentation for error codes > Changes in v3: > - add error return codes > Changes in v2: > - add info in header regarding the function > > drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ > drivers/power/regulator/regulator_common.h | 21 +++++++++++++++++++++ > 2 files changed, 43 insertions(+) > > diff --git a/drivers/power/regulator/regulator_common.c > b/drivers/power/regulator/regulator_common.c > index 93d8196b381e..484a4fc31ef7 100644 > --- a/drivers/power/regulator/regulator_common.c > +++ b/drivers/power/regulator/regulator_common.c > @@ -73,6 +73,23 @@ int regulator_common_set_enable(const struct udevice *dev, > return 0; > } > > + /* If previously enabled, increase count */ > + if (enable && dev_pdata->enable_count > 0) { > + dev_pdata->enable_count++; > + return -EALREADY; > + } > + > + if (!enable) { > + if (dev_pdata->enable_count > 1) { > + /* If enabled multiple times, decrease count */ > + dev_pdata->enable_count--; > + return -EBUSY; > + } else if (!dev_pdata->enable_count) { > + /* If already disabled, do nothing */ > + return -EALREADY; > + } > + } > + > ret = dm_gpio_set_value(&dev_pdata->gpio, enable); > if (ret) { > pr_err("Can't set regulator : %s gpio to: %d\n", dev->name, > @@ -87,5 +104,10 @@ int regulator_common_set_enable(const struct udevice *dev, > if (!enable && dev_pdata->off_on_delay_us) > udelay(dev_pdata->off_on_delay_us); > > + if (enable) > + dev_pdata->enable_count++; > + else > + dev_pdata->enable_count--; > + > return 0; > } > diff --git a/drivers/power/regulator/regulator_common.h > b/drivers/power/regulator/regulator_common.h > index c10492f01675..0faab447d099 100644 > --- a/drivers/power/regulator/regulator_common.h > +++ b/drivers/power/regulator/regulator_common.h > @@ -13,6 +13,7 @@ struct regulator_common_plat { > struct gpio_desc gpio; /* GPIO for regulator enable control */ > unsigned int startup_delay_us; > unsigned int off_on_delay_us; > + unsigned int enable_count; > }; > > int regulator_common_of_to_plat(struct udevice *dev, > @@ -20,6 +21,26 @@ int regulator_common_of_to_plat(struct udevice *dev, > char *enable_gpio_name); > int regulator_common_get_enable(const struct udevice *dev, > struct regulator_common_plat *dev_pdata); > +/* > + * Enable or Disable a regulator > + * > + * This is a reentrant function and subsequent calls that enable will > + * increase an internal counter, and disable calls will decrease the counter. > + * The actual resource will be enabled when the counter gets to 1 coming > from 0, > + * and disabled when it reaches 0 coming from 1. > + * > + * @dev: regulator device > + * @dev_pdata: Platform data > + * @enable: bool indicating whether to enable or disable the regulator > + * @return: > + * 0 on Success > + * -EBUSY if the regulator cannot be disabled because it's requested by > + * another device > + * -EALREADY if the regulator has already been enabled or has already been > + * disabled
With this new return value added this has potential to break IO voltage switching for mmc UHS support. Main pattern for IO voltage switch seem to be: disable regulator, set voltage, enable regulator. regulator_set_enable_if_allowed should probably be updated to return success on -EALREADY, but that also has the potential to hide regulators enabled at boot not being disabled unless initial enable_count reflects the initial regulator state. Guessing initial enable_count will match for regulator-boot-on regulators in Rockchip and few other platforms/boards because they the call regulators_enable_boot_on, does not look like all platform will have initial enable_count match regulator initial state. Regards, Jonas > + * -EACCES if there is no possibility to enable/disable the regulator > + * -ve on different error situation > + */ > int regulator_common_set_enable(const struct udevice *dev, > struct regulator_common_plat *dev_pdata, bool enable); >