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

[Simon Glass]

Hi Matti,

On Wed, 24 Apr 2019 at 23:58, Vaittinen, Matti
<Matti.Vaittinen at fi.rohmeurope.com> wrote:
>
> 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.

I mean that errno.h should be included already?

>
> >
> > > +#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 =)

No, u8 is preferred over uint8_t.

I mean that you shouldn't be using u8 for arguments. You should use
uint (unsigned int).

>
> > > +
> > > +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.

[..]

> Simon says - in Latin please.
> "non cogito me" dixit Rene Descarte, deinde evanescavit
>
> (Thanks for the translation Simon)

I hope it is close :-)

- Simon