Re: [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs
Hi Matti,
On Wed, 24 Apr 2019 at 23:58, Vaittinen, Matti
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
> > 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 00..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
> > > +#include
> > > +#include
> >
> > 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
> > > +#include
> > > +#include
> > > +#include
> >
> > 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
Re: [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs
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
> 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 00..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
> > +#include
> > +#include
>
> 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
> > +#include
> > +#include
> > +#include
>
> 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 = >ranges[i];
> > +
> > + not_found = vrange_find_selector(r, uvolt, );
> > + 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,
> > );
> > + 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
Re: [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs
HI Matti,
On Wed, 24 Apr 2019 at 06:37, Matti Vaittinen
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.
> bucks (1-4 on BD71837, 1 and 2 on BD71847) can be adjusted
> when regulators are enabled. For other bucks and LDOs we may
> have over- or undershooting if voltage is adjusted when
> regulator is enabled. Thus this is prevented by default.
>
> BD718x7 has a quirk which may leave power output disabled
> after reset if enable/disable state was controlled by SW.
> Thus the SW control is only allowed for BD71837 bucks
> 3 and 4 by default. The impact of this limitation must be
> evaluated board-by board and restrictions may need to be
> modified. (Linux driver get's these limitations from DT and we
> may want to implement same on u-Boot driver).
>
> Signed-off-by: Matti Vaittinen
> ---
>
> Changelog v1 => v2:
> - document structs containing the platdata
> - use pmic_clrsetbits() instead of using separate reads and writes
> - fix styling issues
>
> drivers/power/pmic/bd71837.c | 42 ++-
> drivers/power/regulator/Kconfig | 15 +
> drivers/power/regulator/Makefile | 1 +
> drivers/power/regulator/bd71837.c | 469 ++
> include/power/bd71837.h | 147 ++
> 5 files changed, 616 insertions(+), 58 deletions(-)
> create mode 100644 drivers/power/regulator/bd71837.c
>
> diff --git a/drivers/power/pmic/bd71837.c b/drivers/power/pmic/bd71837.c
> index b749f9430a..12c2e15a19 100644
> --- a/drivers/power/pmic/bd71837.c
> +++ b/drivers/power/pmic/bd71837.c
> @@ -3,6 +3,8 @@
> * Copyright 2018 NXP
> */
>
> +#define DEBUG
> +
> #include
> #include
> #include
> @@ -16,15 +18,15 @@ DECLARE_GLOBAL_DATA_PTR;
>
> static const struct pmic_child_info pmic_children_info[] = {
> /* buck */
> - { .prefix = "b", .driver = BD71837_REGULATOR_DRIVER},
> + { .prefix = "b", .driver = BD718XX_REGULATOR_DRIVER},
> /* ldo */
> - { .prefix = "l", .driver = BD71837_REGULATOR_DRIVER},
> + { .prefix = "l", .driver = BD718XX_REGULATOR_DRIVER},
> { },
> };
>
> static int bd71837_reg_count(struct udevice *dev)
> {
> - return BD71837_REG_NUM;
> + return BD718XX_MAX_REGISTER - 1;
> }
>
> static int bd71837_write(struct udevice *dev, uint reg, const uint8_t *buff,
> @@ -55,7 +57,7 @@ static int bd71837_bind(struct udevice *dev)
>
> regulators_node = dev_read_subnode(dev, "regulators");
> if (!ofnode_valid(regulators_node)) {
> - debug("%s: %s regulators subnode not found!", __func__,
> + debug("%s: %s regulators subnode not found!\n", __func__,
> dev->name);
> return -ENXIO;
> }
> @@ -70,6 +72,34 @@ static int bd71837_bind(struct udevice *dev)
> return 0;
> }
>
> +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() ?
> + if (ret) {
> + debug("%s: %s Failed to unlock regulator control\n", __func__,
> + dev->name);
> + return ret;
> + }
> + debug("%s: '%s' - BD718x7 PMIC register unlocked\n", __func__,
> + dev->name);
> +
> + return 0;
> +}
> +
> static struct dm_pmic_ops bd71837_ops = {
> .reg_count = bd71837_reg_count,
> .read = bd71837_read,
> @@ -77,7 +107,8 @@ static struct dm_pmic_ops bd71837_ops = {
> };
>
> static const struct udevice_id bd71837_ids[] = {
> - { .compatible = "rohm,bd71837", .data = 0x4b, },
> + { .compatible = "rohm,bd71837", .data = ROHM_CHIP_TYPE_BD71837, },
> + { .compatible = "rohm,bd71847", .data = ROHM_CHIP_TYPE_BD71847, },
> { }
> };
>
> @@ -86,5 +117,6 @@ U_BOOT_DRIVER(pmic_bd71837) = {
> .id = UCLASS_PMIC,
> .of_match = bd71837_ids,
> .bind = bd71837_bind,
> + .probe = bd718x7_probe,
> .ops = _ops,
> };
> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
> index 3ed0dd2264..323516587c 100644
> ---
