[U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs

[Vaittinen, Matti]

Hello Simon and thanks again for taking the time to check this =)

On Wed, 2019-04-24 at 17:58 -0600, Simon Glass wrote:
> HI Matti,
> 
> On Wed, 24 Apr 2019 at 06:37, Matti Vaittinen
> <matti.vaittinen at fi.rohmeurope.com> wrote:
> > 
> > BD71837 and BD71847 is PMIC intended for powering single-core,
> > dual-core, and quad-core SoC’s such as NXP-i.MX 8M. BD71847
> > is used for example on NXP imx8mm EVK.
> > 
> > Add regulator driver for ROHM BD71837 and BD71847 PMICs.
> > BD71837 contains 8 bucks and 7 LDOS. BD71847 is reduced
> > version containing 6 bucks and 6 LDOs. Voltages for DVS
> 
> This is great info and I think it should be in your Kconfig help -
> i.e.a bit more detail in your description of the chip.

Good idea. I'll do so in the next version.

> > +static int bd718x7_probe(struct udevice *dev)
> > +{
> > +       int ret;
> > +       u8 unlock;
> > +
> > +       /* Unlock the PMIC regulator control before probing the
> > children */
> > +       ret = pmic_reg_read(dev, BD718XX_REGLOCK);
> > +       if (ret < 0) {
> > +               debug("%s: %s Failed to read lock register, error
> > %d\n",
> > +                     __func__, dev->name, ret);
> > +               return ret;
> > +       }
> > +
> > +       unlock = ret;
> > +       unlock &= ~(BD718XX_REGLOCK_PWRSEQ | BD718XX_REGLOCK_VREG);
> > +
> > +       ret = pmic_reg_write(dev, BD718XX_REGLOCK, unlock);
> 
> Can you use pmic_clrsetbits() ?

Sure. I'll fix this too. Makes this much nicer.

> > index 0000000000..060e6efe74
> > --- /dev/null
> > +++ b/drivers/power/regulator/bd71837.c
> > @@ -0,0 +1,469 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2019 ROHM Semiconductors
> > + *
> > + * ROHM BD71837 regulator driver
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <errno.h>
> 
> Drop this?

errno.h? I return -EINVAL from few of the functions. Or do you mean
i2c.h? I think that can be dropped, thanks.

> 
> > +#include <i2c.h>
> > +#include <power/pmic.h>
> > +#include <power/regulator.h>
> > +#include <power/bd71837.h>
> 
> Put above power/pmic to keep ordering

I'll do that.

> > 
> > +static int vrange_find_value(struct bd71837_vrange *r, u8 sel,
> 
> Can you use uint instea of u8?

I'll replace u8 with uint8_t for all occurrences in this file. I
personally prefer uint8_t. I've got this u8 infection from the linux
kernel code where u8 seems to be preferred =)

> > +
> > +static int bd71837_set_value(struct udevice *dev, int uvolt)
> > +{
> > +       u8 sel;
> > +       u8 range;
> 
> and here
> 
> > +       int i;
> > +       int not_found = 1;
> 
> I think the logic would be easier if you used 'found'

I see your point =) not_found became not_found just because return
value 0 from vrange_find_selector() (or pretty much any other function
I write) means success. So direct assignment to variable made it
'not_found' :] But "double negation" (!not_....) is indeed a bit
difficult. I'll change this too.

> 
> > +       struct bd71837_platdata *plat = dev_get_platdata(dev);
> > +
> > +       /*
> > +        * An under/overshooting may occur if voltage is changed
> > for other
> > +        * regulators but buck 1,2,3 or 4 when regulator is
> > enabled. Prevent
> > +        * change to protect the HW
> > +        */
> > +       if (!plat->dvs)
> > +               if (bd71837_get_enable(dev)) {
> > +                       pr_err("Only DVS bucks can be changed when
> > enabled\n");
> > +                       return -EINVAL;
> > +               }
> > +
> > +       for (i = 0; i < plat->numranges; i++) {
> > +               struct bd71837_vrange *r = &plat->ranges[i];
> > +
> > +               not_found = vrange_find_selector(r, uvolt, &sel);
> > +               if (!not_found) {
> > +                       unsigned int tmp;
> > +
> > +                       /*
> > +                        * We require exactly the requested value
> > to be
> > +                        * supported - this can be changed later if
> > needed
> > +                        */
> > +                       range = r->rangeval;
> > +                       not_found = vrange_find_value(r, sel,
> > &tmp);
> > +                       if (!not_found && tmp == uvolt)
> > +                               break;
> > +                       not_found = 1;
> > +               }
> > +       }
> > +
> > +       if (not_found)
> > +               return -EINVAL;
> > +
> > +       sel <<= ffs(plat->volt_mask) - 1;
> > +
> > +       if (plat->rangemask)
> > +               sel |= range;
> > +
> > +       return pmic_clrsetbits(dev->parent, plat->volt_reg, plat-
> > >volt_mask |
> > +                              plat->rangemask, sel);
> > +}

Best Regards
	Matti Vaittinen

--
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 ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)