[PATCH] regulator: implement basic reference counter
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.hristev@collabora.com --- drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.h | 1 + 2 files changed, 23 insertions(+)
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 93d8196b381e..edda25336176 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 0; + } + + if (!enable) { + if (dev_pdata->enable_count > 1) { + /* If enabled multiple times, decrease count */ + dev_pdata->enable_count--; + return 0; + } else if (!dev_pdata->enable_count) { + /* If already disabled, do nothing */ + return 0; + } + } + 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..8167bc866261 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,
Hi Eugen,
On Thu, 16 Mar 2023 at 07:53, Eugen Hristev eugen.hristev@collabora.com 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.hristev@collabora.com
drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.h | 1 + 2 files changed, 23 insertions(+)
This seems reasonable to me. I mostly worry about visibility, since it returns 0 in all cases so if you do actually want to shut it down, it silently ignored you in some situations. I've made some suggestions below.
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 93d8196b381e..edda25336176 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,
Please update the docs for this function in the header file.
return 0; }
/* If previously enabled, increase count */if (enable && dev_pdata->enable_count > 0) {dev_pdata->enable_count++;return 0;
Should we return -EALREADY ?
}if (!enable) {if (dev_pdata->enable_count > 1) {/* If enabled multiple times, decrease count */dev_pdata->enable_count--;
Return -EBUSY ?
return 0;} else if (!dev_pdata->enable_count) {/* If already disabled, do nothing */
Is this case an error? Should we return -EPERM ?
return 0;}}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++;elsedev_pdata->enable_count--;return 0;} diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index c10492f01675..8167bc866261 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,
2.34.1
Also I would appreciate a patch to rename dev_pdata to plat if you have time.
Regards, Simon
On 3/18/23 22:20, Simon Glass wrote:
Hi Eugen,
On Thu, 16 Mar 2023 at 07:53, Eugen Hristev eugen.hristev@collabora.com 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.hristev@collabora.com
drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.h | 1 + 2 files changed, 23 insertions(+)
This seems reasonable to me. I mostly worry about visibility, since it returns 0 in all cases so if you do actually want to shut it down, it silently ignored you in some situations. I've made some suggestions below.
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 93d8196b381e..edda25336176 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,
Please update the docs for this function in the header file.
Okay
return 0; }
/* If previously enabled, increase count */if (enable && dev_pdata->enable_count > 0) {dev_pdata->enable_count++;return 0;Should we return -EALREADY ?
I think not. It's not an error, we should just increase count, the caller should be told that there is no error happening.
}if (!enable) {if (dev_pdata->enable_count > 1) {/* If enabled multiple times, decrease count */dev_pdata->enable_count--;Return -EBUSY ?
Same goes here. The disable was successful, hence no error, but the regulator is not turned off , because someone else is still using it.
return 0;} else if (!dev_pdata->enable_count) {/* If already disabled, do nothing */Is this case an error? Should we return -EPERM ?
Maybe here we could return an error, saying that it's already disabled, but since it's a shared resource, this could tell the caller whether someone else is using the resource, and I don't think we should leak this information. It might be used in a way to find out information about the state of the system which might be a security problem ? I think that a shared resource usage should be transparent to the caller , such that only the driver should manage it's power on/off and know who is using it.
Do you agree ?
return 0;}}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++;elsedev_pdata->enable_count--;return 0;diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index c10492f01675..8167bc866261 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,
-- 2.34.1
Also I would appreciate a patch to rename dev_pdata to plat if you have time.
I will have a look
Regards, Simon
participants (2)
-
Eugen Hristev -
Simon Glass