[PATCH] regulator: implement basic reference counter

[Simon Glass]

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.

>                 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++;
> +       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.

Regards,
Simon