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

чт, 3 жовт. 2024 р. о 18:03 Simon Glass <[email protected]> пише:
>
> Hi Tom,
>
> On Wed, 2 Oct 2024 at 19:40, Tom Rini <[email protected]> wrote:
> >
> > On Wed, Oct 02, 2024 at 04:55:39PM -0600, Simon Glass wrote:
> > > Hi,
> > >
> > > On Thu, 26 Sept 2024 at 17:15, Marek Vasut <[email protected]> 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 <[email protected]>
> > > > ---
> > > > Cc: Ben Wolsieffer <[email protected]>
> > > > Cc: Caleb Connolly <[email protected]>
> > > > Cc: Chris Morgan <[email protected]>
> > > > Cc: Dragan Simic <[email protected]>
> > > > Cc: Eugen Hristev <[email protected]>
> > > > Cc: Francesco Dolcini <[email protected]>
> > > > Cc: Heinrich Schuchardt <[email protected]>
> > > > Cc: Jaehoon Chung <[email protected]>
> > > > Cc: Jagan Teki <[email protected]>
> > > > Cc: Jonas Karlman <[email protected]>
> > > > Cc: Kever Yang <[email protected]>
> > > > Cc: Matteo Lisi <[email protected]>
> > > > Cc: Mattijs Korpershoek <[email protected]>
> > > > Cc: Max Krummenacher <[email protected]>
> > > > Cc: Neil Armstrong <[email protected]>
> > > > Cc: Patrice Chotard <[email protected]>
> > > > Cc: Patrick Delaunay <[email protected]>
> > > > Cc: Philipp Tomsich <[email protected]>
> > > > Cc: Quentin Schulz <[email protected]>
> > > > Cc: Sam Day <[email protected]>
> > > > Cc: Simon Glass <[email protected]>
> > > > Cc: Sumit Garg <[email protected]>
> > > > Cc: Svyatoslav Ryhel <[email protected]>
> > > > Cc: Thierry Reding <[email protected]>
> > > > Cc: Tom Rini <[email protected]>
> > > > Cc: Volodymyr Babchuk <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > ---
> > > > 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.
>
> OK ta. So long as we can revert/change that is fine.
>
> >
> > > 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.
>
> OK, I actually never fully understood the problem, but yes, sandbox is
> supposed to work similarly.
>
> Regards,
> Simon

Simon,

Please inform me when you come up with a solution, I would like to
test it. Thanks.