Hi Matti,
On Wed, 24 Apr 2019 at 23:58, Vaittinen, Matti Matti.Vaittinen@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@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.
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