[PATCH] regulator: implement basic reference counter

[Eugen Hristev]

On 3/18/23 22:20, Simon Glass wrote:
> Hi Eugen,
> 
> On Thu, 16 Mar 2023 at 07:53, Eugen Hristev <eugen.hristev at 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 at 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++;
>> +       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,
>> --
>> 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