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

[Vaittinen, Matti]

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.

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

Rest of the comments seemed all like trivial fixes and I will apply
them =)

> 
> Regards,
> Simon