Re: [PATCH v3 2/4] power: regulator: Convert regulators_enable_boot_on/off() to regulator_post_probe

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 6673 bytes --]

On Wed, Oct 02, 2024 at 04:55:39PM -0600, Simon Glass wrote:
> Hi,
> 
> On Thu, 26 Sept 2024 at 17:15, Marek Vasut <marex@denx.de> wrote:
> >
> > Turn regulators_enable_boot_on() and regulators_enable_boot_off() into
> > empty functions. Implement matching functionality in regulator_post_probe()
> > instead. The regulator_post_probe() is called for all regulators after they
> > probe, and regulators that have regulator-always-on or regulator-boot-on DT
> > properties now always probe due to DM_FLAG_PROBE_AFTER_BIND being set on
> > such regulators in regulator_post_bind().
> >
> > Finally, fold regulator_unset() functionality into regulator_autoset().
> >
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > ---
> > Cc: Ben Wolsieffer <benwolsieffer@gmail.com>
> > Cc: Caleb Connolly <caleb.connolly@linaro.org>
> > Cc: Chris Morgan <macromorgan@hotmail.com>
> > Cc: Dragan Simic <dsimic@manjaro.org>
> > Cc: Eugen Hristev <eugen.hristev@collabora.com>
> > Cc: Francesco Dolcini <francesco.dolcini@toradex.com>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Jaehoon Chung <jh80.chung@samsung.com>
> > Cc: Jagan Teki <jagan@amarulasolutions.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Kever Yang <kever.yang@rock-chips.com>
> > Cc: Matteo Lisi <matteo.lisi@engicam.com>
> > Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> > Cc: Max Krummenacher <max.krummenacher@toradex.com>
> > Cc: Neil Armstrong <neil.armstrong@linaro.org>
> > Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > Cc: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > Cc: Quentin Schulz <quentin.schulz@cherry.de>
> > Cc: Sam Day <me@samcday.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Sumit Garg <sumit.garg@linaro.org>
> > Cc: Svyatoslav Ryhel <clamor95@gmail.com>
> > Cc: Thierry Reding <treding@nvidia.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> > Cc: u-boot-amlogic@groups.io
> > Cc: u-boot-qcom@groups.io
> > Cc: u-boot@dh-electronics.com
> > Cc: u-boot@lists.denx.de
> > Cc: uboot-stm32@st-md-mailman.stormreply.com
> > ---
> > V2: Rebase on current u-boot/next
> > V3: No change
> > ---
> >  drivers/power/regulator/regulator-uclass.c | 60 +++++++---------------
> >  1 file changed, 19 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
> > index 1a970004540..9fcc4bd85b9 100644
> > --- a/drivers/power/regulator/regulator-uclass.c
> > +++ b/drivers/power/regulator/regulator-uclass.c
> > @@ -314,6 +314,11 @@ int regulator_autoset(struct udevice *dev)
> >                         return ret;
> >         }
> >
> > +       if (uc_pdata->force_off) {
> > +               ret = regulator_set_enable(dev, false);
> > +               goto out;
> > +       }
> > +
> >         if (!uc_pdata->always_on && !uc_pdata->boot_on) {
> >                 ret = -EMEDIUMTYPE;
> >                 goto out;
> > @@ -518,56 +523,28 @@ static int regulator_pre_probe(struct udevice *dev)
> >         return 0;
> >  }
> >
> > -int regulators_enable_boot_on(bool verbose)
> > +static int regulator_post_probe(struct udevice *dev)
> >  {
> > -       struct udevice *dev;
> > -       struct uclass *uc;
> >         int ret;
> >
> > -       ret = uclass_get(UCLASS_REGULATOR, &uc);
> > -       if (ret)
> > +       ret = regulator_autoset(dev);
> > +       if (ret && ret != -EMEDIUMTYPE && ret != -EALREADY && ret != ENOSYS)
> >                 return ret;
> > -       for (uclass_first_device(UCLASS_REGULATOR, &dev);
> > -            dev;
> > -            uclass_next_device(&dev)) {
> > -               ret = regulator_autoset(dev);
> > -               if (ret == -EMEDIUMTYPE || ret == -EALREADY) {
> > -                       ret = 0;
> > -                       continue;
> > -               }
> > -               if (verbose)
> > -                       regulator_show(dev, ret);
> > -               if (ret == -ENOSYS)
> > -                       ret = 0;
> > -       }
> >
> > -       return ret;
> > +       if (_DEBUG)
> > +               regulator_show(dev, ret);
> > +
> > +       return 0;
> >  }
> >
> > -int regulators_enable_boot_off(bool verbose)
> > +int regulators_enable_boot_on(bool verbose)
> >  {
> > -       struct udevice *dev;
> > -       struct uclass *uc;
> > -       int ret;
> > -
> > -       ret = uclass_get(UCLASS_REGULATOR, &uc);
> > -       if (ret)
> > -               return ret;
> > -       for (uclass_first_device(UCLASS_REGULATOR, &dev);
> > -            dev;
> > -            uclass_next_device(&dev)) {
> > -               ret = regulator_unset(dev);
> > -               if (ret == -EMEDIUMTYPE) {
> > -                       ret = 0;
> > -                       continue;
> > -               }
> > -               if (verbose)
> > -                       regulator_show(dev, ret);
> > -               if (ret == -ENOSYS)
> > -                       ret = 0;
> > -       }
> > +       return 0;
> > +}
> >
> > -       return ret;
> > +int regulators_enable_boot_off(bool verbose)
> > +{
> > +       return 0;
> >  }
> >
> >  UCLASS_DRIVER(regulator) = {
> > @@ -575,5 +552,6 @@ UCLASS_DRIVER(regulator) = {
> >         .name           = "regulator",
> >         .post_bind      = regulator_post_bind,
> >         .pre_probe      = regulator_pre_probe,
> > +       .post_probe     = regulator_post_probe,
> >         .per_device_plat_auto   = sizeof(struct dm_regulator_uclass_plat),
> >  };
> > --
> > 2.45.2
> >
> 
> I thought I objected to this patch, but it seems to be in -next? Does
> anyone know what has happened here?

Yes, you missed me noting that this fixes real problems and has been
needing to be fixed for a while. I said you should provide your
alternative approach and we'll get this sorted out in -next and I took
the work-around for master so the release wouldn't be broken.

> I am seeing these errors now when running sandbox 'u-boot -D':
> 
>        i2c_emul_find() No emulators for device 'sandbox_pmic'
>   sandbox_pmic_write() write error to device: 0000000018c49db0 register: 0x0!
>        out_set_value() PMIC write failed: -5
>        i2c_emul_find() No emulators for device 'sandbox_pmic'
>   sandbox_pmic_write() write error to device: 0000000018c49db0 register: 0x0!
>        out_set_value() PMIC write failed: -5

I suppose the good news is that perhaps this is also related to the
problem that Svyatoslav was reporting.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]