[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.hristev at collabora.com>
>> Reviewed-by: Simon Glass <sjg at 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);
>>   
>