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

Hi Eugen,

On Wed, 29 Mar 2023 at 02:01, 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

Changes in v2:

• add info in header regarding the function

drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.h | 13 +++++++++++++ 2 files changed, 35 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;
  

Perhaps -EBUSY for this?

You said:

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 ?

No, that doesn't seem like a good argument. Suppressing an error for security reasons?

Any driver can access any other in the system. This is a bootloader, not an OS. Knowing the state of the system is important. Supressing errors creates a lot of confusion and makes bugs hard to track.

•           } else if (!dev_pdata->enable_count) {
  

•                   /* If already disabled, do nothing */
  

•                   return 0;
  

This is a bug, so we should return -EALREADY or something else here. If this error code is reported, we need to fix the bug.

•           }
  

•   }
  

•   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..7c701b69245d 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,18 @@ 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

Returns

• */

int regulator_common_set_enable(const struct udevice *dev, struct regulator_common_plat *dev_pdata, bool enable);

-- 2.34.1

Regards, Simon