Hi Matti, On Wed, 24 Apr 2019 at 23:58, Vaittinen, Matti <matti.vaitti...@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.vaitti...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot