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. > > > +#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) _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot