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@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 thechildren */
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 changedfor other
* regulators but buck 1,2,3 or 4 when regulator isenabled. Prevent
* change to protect the HW*/if (!plat->dvs)if (bd71837_get_enable(dev)) {pr_err("Only DVS bucks can be changed whenenabled\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 valueto be
* supported - this can be changed later ifneeded
*/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)