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

[Simon Glass]

Hi Eugen,

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