Re: [PATCH v4 1/2] regulator: implement basic reference counter

[Eugen Hristev]

On 4/2/23 13:45, Jonas Karlman wrote:
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.

How can the breakage happen in this case ?


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.

If the regulators enabled at boot are fixed/gpio regulators (which have 
the enable count), then at boot , the autoset should call the 
enable_common, thus taking into account the enable_count.

So I think that if I updated regulator_set_enable_if_allowed to return 
success on -EALREADY it should be fine. Do you have any other scenario 
in mind ?

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.

If the other platforms do not call regulator enable, then they will have 
the enable_count==0, which is expected right ?
You refer to specific platforms where the regulator is already enabled 
from hardware but no code is called to enable, and by looking at 
enable_count it would appear disabled ? There are such cases ? And those 
regulators are not initialized at all ?


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);