[U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC

[Simon Glass]

Hi Matti,

On Tue, 23 Apr 2019 at 23:59, Vaittinen, Matti
<Matti.Vaittinen at fi.rohmeurope.com> wrote:
>
> Hello Simon,
>
> Thanks a bunch for taking the time and reviewing this! I do appreciate!
>
> On Tue, 2019-04-23 at 21:54 -0600, Simon Glass wrote:
> > Hi Matti,
> >
> > On Wed, 27 Mar 2019 at 06:40, Matti Vaittinen
> > <matti.vaittinen at fi.rohmeurope.com> wrote:
> > >
> > > Add regulator driver for ROHM BD71837 PMIC. BD71837 contains
> > > 8 bucks and 7 LDOS. Voltages for bucks 1-4 can be adjusted
> > > when regulators are enabled. For other bucks and LDOs we may
> > > have over- or undershooting if voltage is adjusted when
> > > regulator is enabled. Thus this is prevented by default.
> > >
> > > BD71837 has a quirk which may leave power output disabled
> > > after reset if enable/disable state was controlled by SW.
> > > Thus the SW control is only allowed for bucks3 and 4 by
> > > default.
> > >
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen at fi.rohmeurope.com>
> > > ---
> > >  drivers/power/regulator/Kconfig   |  15 ++
> > >  drivers/power/regulator/Makefile  |   1 +
> > >  drivers/power/regulator/bd71837.c | 373
> > > ++++++++++++++++++++++++++++++
> > >  include/power/bd71837.h           |  20 ++
> > >  4 files changed, 409 insertions(+)
> > >
> >
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> > But please see nits below.
>
> I see you reviewed the RFC version. I would like to ask you to see also
> the non RFC version https://patchwork.ozlabs.org/patch/1080860/ which
> supports also BD71847. I see that most (maybe all) of these comments
> apply to that patch too - but you might have some additional ideas
> too.
>
> I will create v2 of this non RFC patch based on these comments (and fix
> your comments to patch 1/2 too) but I won't add your reviewed-by just
> yet - I hope you can also check the pieces adding BD71847 support and
> give me a nudge if you see there something to improve. =)
>
> > > --- /dev/null
> > > +++ b/drivers/power/regulator/bd71837.c
> > > @@ -0,0 +1,373 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +//
> > > +// Copyright (C) 2019 ROHM Semiconductors
> > > +//
> > > +// ROHM BD71837 regulator driver
> >
> > The SPDX line has a // comment but everything else should use C
> > comments:
> >
> > /*
> >  * Copyright ...
> >  *
> >  * ROHM ...
> >  */
>
> This is fine for me. But just to note that this differs from Linux
> notation which accepts also the copyright block being // - comments. I
> am not sure how closely u-Boot follows (or wants to follow) kernel
> coding guidelines. But as I said, I don't mind changing this.

U-Boot mostly follows Linux, but you can see conventions by looking at
the code, generally.

>
> > > +
> > > +static int bd71837_regulator_probe(struct udevice *dev)
> > > +{
> > > +       struct bd71837_data *d = dev_get_platdata(dev);
> > > +       int i, ret;
> > > +       struct dm_regulator_uclass_platdata *uc_pdata;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(bd71837_reg_data); i++) {
> > > +               if (!strcmp(dev->name, bd71837_reg_data[i].name)) {
> > > +                       *d = bd71837_reg_data[i];
> > > +                       if (d->enablemask != HW_STATE_CONTROL) {
> > > +                               u8 ctrl;
> > > +
> > > +                               /* Take the regulator under SW
> > > control. Ensure
> > > +                                * the initial state matches dt
> > > flags and then
> > > +                                * write the SEL bit
> > > +                                */
> > > +                               uc_pdata =
> > > dev_get_uclass_platdata(dev);
> > > +                               ret = bd71837_set_enable(dev,
> > > +                                                        !!(uc_pdat
> > > a->boot_on ||
> > > +                                                        uc_pdata-
> > > >always_on));
> > > +                               if (ret)
> > > +                                       return ret;
> > > +
> > > +                               ctrl = pmic_reg_read(dev->parent,
> > > +                                                    d-
> > > >enable_reg);
> > > +                               if (ctrl < 0)
> > > +                                       return ctrl;
> > > +
> > > +                               ctrl |= d->sel_mask;
> > > +                               return pmic_reg_write(dev->parent,
> > > +                                                     d-
> > > >enable_reg, ctrl);
> > > +                       }
> > > +                       return 0;
> > > +               }
> > > +       }
> > > +
> > > +       pr_err("Unknown regulator '%s'\n", dev->name);
> > > +
> > > +       return -EINVAL;
> >
> > -ENOENT ?
>
> At first the -ENOENT sounded to me like a regulator/device is missing.
> I thought that here we have extra (invalid/unknown) regulator in the
> device-tree. Thus I used the -EINVAL. But I think you are right, we can
> think that DT is correct and the driver lacks of correct regulator
> entity. So -ENOENT can be appropriate. => I'll change this too.
>
> > > --
> > > Matti Vaittinen, Linux device drivers
> > > ROHM Semiconductors, Finland SWDC
> > > Kiviharjunlenkki 1E
> > > 90220 OULU
> > > FINLAND
> > >
> > > ~~~ "I don't think so," said Rene Descartes. Just then he vanished
> > > ~~~
> >
> > This would be better in Latin.
>
> Unfortunately that's far beyond my skills =) I can do Finnish though ;)

Me also, but maybe something like this is close?

"non cogito me" dixit Rene Descarte, deinde evanescavit

Regards,
Simon