[PATCH v5 1/3] regulator: implement basic reference counter

[Simon Glass]

Hi Eugen,

On Fri, 28 Apr 2023 at 01:39, Eugen Hristev <eugen.hristev at collabora.com> wrote:
>
> On 4/28/23 02:39, Tim Harvey wrote:
> > On Wed, Apr 19, 2023 at 6:45 AM 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>
> >> Reviewed-by: Simon Glass <sjg at chromium.org>
> >> ---
> >> Changes in v5:
> >>   - none
> >> 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;
> >> +               }
> >> +       }
> >> +
> >
> > Eugen,
> >
> > Thank you for working on this series!
>
> Hi Tim,
>
> Thank you for testing and having a look on my patches.
> >
> > I wonder if instead of returning a failure you should print an error
> > here but return 0 in order to not break unbalanced regulator calls
>
> Initially I did that, but Simon forced me to return error as you see now.

If you want to have a special function that consumes errors, you can
add one, e.g. that wraps the one that does report errors. My objection
is when something goes wrong and it is silently ignored, since it
tends to make problems build up, people add work-arounds, etc.

>
> > (like what is done in clk_disable()). I see that you have another
> > patch in this series which handles htis for
> > regulator_set_enable_if_allowed() but that doesn't cover drivers that
> > call regulator_common_set_enable() directly such as
> > drivers/power/regulator/fixed.c and
> > drivers/power/regulator/gpio-regulator.c.
>
> I believe Jonas (in CC) fixed those with a patch, but at the moment I am
> lost in providing you a link to it
> >
> > I know there is an unbalanced call currently on imx8mm that this patch
> > causes a failure where it previously did not:
> > u-boot=> usb start && usb tree
> > starting USB...
> > Bus usb at 32e40000: Bus usb at 32e50000: Error enabling VBUS supply (ret=-114)
> > probe failed, error -114
> >
>
> That's a good catch.
> I expected such things would happen if I would return error in those
> cases, so it might be that this is not the only case.
> If you are able to test that board, do you wish me to send you a patch
> that changes the code to using regulator_set_enable_if_allowed() ?


Regards,
Simon