Re: [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC
Hi Matti,
On Tue, 23 Apr 2019 at 23:59, Vaittinen, Matti
wrote:
>
> Hello Simon,
>
> Thanks a bunch for taking the time and reviewing this! I do appreciate!
>
> On Tue, 2019-04-23 at 21:54 -0600, Simon Glass wrote:
> > Hi Matti,
> >
> > On Wed, 27 Mar 2019 at 06:40, Matti Vaittinen
> > wrote:
> > >
> > > Add regulator driver for ROHM BD71837 PMIC. BD71837 contains
> > > 8 bucks and 7 LDOS. Voltages for bucks 1-4 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.
> > >
> > > BD71837 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 bucks3 and 4 by
> > > default.
> > >
> > > Signed-off-by: Matti Vaittinen
> > > ---
> > > drivers/power/regulator/Kconfig | 15 ++
> > > drivers/power/regulator/Makefile | 1 +
> > > drivers/power/regulator/bd71837.c | 373
> > > ++
> > > include/power/bd71837.h | 20 ++
> > > 4 files changed, 409 insertions(+)
> > >
> >
> > Reviewed-by: Simon Glass
> >
> > But please see nits below.
>
> I see you reviewed the RFC version. I would like to ask you to see also
> the non RFC version https://patchwork.ozlabs.org/patch/1080860/ which
> supports also BD71847. I see that most (maybe all) of these comments
> apply to that patch too - but you might have some additional ideas
> too.
>
> I will create v2 of this non RFC patch based on these comments (and fix
> your comments to patch 1/2 too) but I won't add your reviewed-by just
> yet - I hope you can also check the pieces adding BD71847 support and
> give me a nudge if you see there something to improve. =)
>
> > > --- /dev/null
> > > +++ b/drivers/power/regulator/bd71837.c
> > > @@ -0,0 +1,373 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +//
> > > +// Copyright (C) 2019 ROHM Semiconductors
> > > +//
> > > +// ROHM BD71837 regulator driver
> >
> > The SPDX line has a // comment but everything else should use C
> > comments:
> >
> > /*
> > * Copyright ...
> > *
> > * ROHM ...
> > */
>
> This is fine for me. But just to note that this differs from Linux
> notation which accepts also the copyright block being // - comments. I
> am not sure how closely u-Boot follows (or wants to follow) kernel
> coding guidelines. But as I said, I don't mind changing this.
U-Boot mostly follows Linux, but you can see conventions by looking at
the code, generally.
>
> > > +
> > > +static int bd71837_regulator_probe(struct udevice *dev)
> > > +{
> > > + struct bd71837_data *d = dev_get_platdata(dev);
> > > + int i, ret;
> > > + struct dm_regulator_uclass_platdata *uc_pdata;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(bd71837_reg_data); i++) {
> > > + if (!strcmp(dev->name, bd71837_reg_data[i].name)) {
> > > + *d = bd71837_reg_data[i];
> > > + if (d->enablemask != HW_STATE_CONTROL) {
> > > + u8 ctrl;
> > > +
> > > + /* Take the regulator under SW
> > > control. Ensure
> > > +* the initial state matches dt
> > > flags and then
> > > +* write the SEL bit
> > > +*/
> > > + uc_pdata =
> > > dev_get_uclass_platdata(dev);
> > > + ret = bd71837_set_enable(dev,
> > > +!!(uc_pdat
> > > a->boot_on ||
> > > +uc_pdata-
> > > >always_on));
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ctrl = pmic_reg_read(dev->parent,
> > > +d-
> > > >enable_reg);
> > > + if (ctrl < 0)
> > > + return ctrl;
> > > +
> > > + ctrl |= d->sel_mask;
> > > + return pmic_reg_write(dev->parent,
> > > + d-
> > > >enable_reg, ctrl);
> > > + }
> > > + return 0;
> > > + }
> > > + }
> > > +
> > > + pr_err("Unknown regulator '%s'\n", dev->name);
> > > +
> > > + return -EINVAL;
> >
> > -ENOENT ?
>
> At first the -ENOENT sounded to me like a regulator/device is missing.
> I thought that here we have extra (invalid/unknown) regulator in the
> device-tree. Thus I used the -EINVAL. But I think you are right, we can
> think that DT is correct and the driver lacks of correct regulator
> entity. So -ENOENT can be appropriate. => I'll change this too.
>
> > > --
> > > Matti
Re: [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC
Hello Simon,
Thanks a bunch for taking the time and reviewing this! I do appreciate!
On Tue, 2019-04-23 at 21:54 -0600, Simon Glass wrote:
> Hi Matti,
>
> On Wed, 27 Mar 2019 at 06:40, Matti Vaittinen
> wrote:
> >
> > Add regulator driver for ROHM BD71837 PMIC. BD71837 contains
> > 8 bucks and 7 LDOS. Voltages for bucks 1-4 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.
> >
> > BD71837 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 bucks3 and 4 by
> > default.
> >
> > Signed-off-by: Matti Vaittinen
> > ---
> > drivers/power/regulator/Kconfig | 15 ++
> > drivers/power/regulator/Makefile | 1 +
> > drivers/power/regulator/bd71837.c | 373
> > ++
> > include/power/bd71837.h | 20 ++
> > 4 files changed, 409 insertions(+)
> >
>
> Reviewed-by: Simon Glass
>
> But please see nits below.
I see you reviewed the RFC version. I would like to ask you to see also
the non RFC version https://patchwork.ozlabs.org/patch/1080860/ which
supports also BD71847. I see that most (maybe all) of these comments
apply to that patch too - but you might have some additional ideas
too.
I will create v2 of this non RFC patch based on these comments (and fix
your comments to patch 1/2 too) but I won't add your reviewed-by just
yet - I hope you can also check the pieces adding BD71847 support and
give me a nudge if you see there something to improve. =)
> > --- /dev/null
> > +++ b/drivers/power/regulator/bd71837.c
> > @@ -0,0 +1,373 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +//
> > +// Copyright (C) 2019 ROHM Semiconductors
> > +//
> > +// ROHM BD71837 regulator driver
>
> The SPDX line has a // comment but everything else should use C
> comments:
>
> /*
> * Copyright ...
> *
> * ROHM ...
> */
This is fine for me. But just to note that this differs from Linux
notation which accepts also the copyright block being // - comments. I
am not sure how closely u-Boot follows (or wants to follow) kernel
coding guidelines. But as I said, I don't mind changing this.
> > +
> > +static int bd71837_regulator_probe(struct udevice *dev)
> > +{
> > + struct bd71837_data *d = dev_get_platdata(dev);
> > + int i, ret;
> > + struct dm_regulator_uclass_platdata *uc_pdata;
> > +
> > + for (i = 0; i < ARRAY_SIZE(bd71837_reg_data); i++) {
> > + if (!strcmp(dev->name, bd71837_reg_data[i].name)) {
> > + *d = bd71837_reg_data[i];
> > + if (d->enablemask != HW_STATE_CONTROL) {
> > + u8 ctrl;
> > +
> > + /* Take the regulator under SW
> > control. Ensure
> > +* the initial state matches dt
> > flags and then
> > +* write the SEL bit
> > +*/
> > + uc_pdata =
> > dev_get_uclass_platdata(dev);
> > + ret = bd71837_set_enable(dev,
> > +!!(uc_pdat
> > a->boot_on ||
> > +uc_pdata-
> > >always_on));
> > + if (ret)
> > + return ret;
> > +
> > + ctrl = pmic_reg_read(dev->parent,
> > +d-
> > >enable_reg);
> > + if (ctrl < 0)
> > + return ctrl;
> > +
> > + ctrl |= d->sel_mask;
> > + return pmic_reg_write(dev->parent,
> > + d-
> > >enable_reg, ctrl);
> > + }
> > + return 0;
> > + }
> > + }
> > +
> > + pr_err("Unknown regulator '%s'\n", dev->name);
> > +
> > + return -EINVAL;
>
> -ENOENT ?
At first the -ENOENT sounded to me like a regulator/device is missing.
I thought that here we have extra (invalid/unknown) regulator in the
device-tree. Thus I used the -EINVAL. But I think you are right, we can
think that DT is correct and the driver lacks of correct regulator
entity. So -ENOENT can be appropriate. => I'll change this too.
> > --
> > 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
> > ~~~
>
> This would be better in Latin.
Unfortunately that's far beyond my skills =) I can do Finnish though ;)
Rest of the comments seemed all like trivial fixes and I will apply
them =)
>
>
Re: [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC
Hi Matti,
On Wed, 27 Mar 2019 at 06:40, Matti Vaittinen
wrote:
>
> Add regulator driver for ROHM BD71837 PMIC. BD71837 contains
> 8 bucks and 7 LDOS. Voltages for bucks 1-4 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.
>
> BD71837 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 bucks3 and 4 by
> default.
>
> Signed-off-by: Matti Vaittinen
> ---
> drivers/power/regulator/Kconfig | 15 ++
> drivers/power/regulator/Makefile | 1 +
> drivers/power/regulator/bd71837.c | 373 ++
> include/power/bd71837.h | 20 ++
> 4 files changed, 409 insertions(+)
>
Reviewed-by: Simon Glass
But please see nits below.
> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
> index 3ed0dd2264..323516587c 100644
> --- a/drivers/power/regulator/Kconfig
> +++ b/drivers/power/regulator/Kconfig
> @@ -43,6 +43,21 @@ config REGULATOR_AS3722
> but does not yet support change voltages. Currently this must be
> done using direct register writes to the PMIC.
>
> +config DM_REGULATOR_BD71837
> + bool "Enable Driver Model for REGULATOR BD71837"
> + depends on DM_REGULATOR && DM_PMIC_BD71837
> + help
> + This config enables implementation of driver-model regulator uclass
> + features for REGULATOR BD71837. The driver implements get/set api for:
> + value and enable.
> +
> +config SPL_DM_REGULATOR_BD71837
> + bool "Enable Driver Model for REGULATOR BD71837 in SPL"
> + depends on DM_REGULATOR_BD71837
> + help
> + This config enables implementation of driver-model regulator uclass
> + features for REGULATOR BD71837 in SPL.
> +
> config DM_REGULATOR_PFUZE100
> bool "Enable Driver Model for REGULATOR PFUZE100"
> depends on DM_REGULATOR && DM_PMIC_PFUZE100
> diff --git a/drivers/power/regulator/Makefile
> b/drivers/power/regulator/Makefile
> index f617ce723a..898ed5f084 100644
> --- a/drivers/power/regulator/Makefile
> +++ b/drivers/power/regulator/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o
> obj-$(CONFIG_REGULATOR_AS3722) += as3722_regulator.o
> obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
> obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o
> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_BD71837) += bd71837.o
> obj-$(CONFIG_$(SPL_)REGULATOR_PWM) += pwm_regulator.o
> obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.o
> obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o
> diff --git a/drivers/power/regulator/bd71837.c
> b/drivers/power/regulator/bd71837.c
> new file mode 100644
> index 00..5b32425ba9
> --- /dev/null
> +++ b/drivers/power/regulator/bd71837.c
> @@ -0,0 +1,373 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +//
> +// Copyright (C) 2019 ROHM Semiconductors
> +//
> +// ROHM BD71837 regulator driver
The SPDX line has a // comment but everything else should use C comments:
/*
* Copyright ...
*
* ROHM ...
*/
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define HW_STATE_CONTROL 0
> +
Need struct comment.
/**
* struct bd71837_vrange - description here
*
* @min_volt: Description here
* ...
*/
> +struct bd71837_vrange {
> + unsigned intmin_volt;
> + unsigned intstep;
> + u8 min_sel;
> + u8 max_sel;
> + u8 rangeval;
> +};
> +
Need struct comment
> +struct bd71837_data {
How about bd71837_platdata?
> + const char *name;
> + u8 enable_reg;
> + u8 enablemask;
> + u8 volt_reg;
> + u8 volt_mask;
> + struct bd71837_vrange *ranges;
> + unsigned intnumranges;
> + u8 rangemask;
> + u8 sel_mask;
> + booldvs;
> +};
> +
> +#define BD_RANGE(_min, _vstep, _sel_low, _sel_hi, _range_sel) \
> +{ \
> + .min_volt = (_min), .step = (_vstep), .min_sel = (_sel_low), \
> + .max_sel = (_sel_hi), .rangeval = (_range_sel) \
> +}
> +
> +#define BD_DATA(_name, enreg, enmask, vreg, vmask, _range, rmask, _dvs, sel)
> \
> +{ \
> + .name = (_name), .enable_reg = (enreg), .enablemask = (enmask), \
> + .volt_reg = (vreg), .volt_mask = (vmask), .ranges = (_range), \
> + .numranges = ARRAY_SIZE(_range), .rangemask = (rmask), .dvs = (_dvs),
> \
> + .sel_mask = (sel) \
> +}
> +
> +static struct bd71837_vrange buck1to4_vranges[] = {
> + BD_RANGE(70, 1, 0, 0x3C, 0),
Please use lower-case hex throughout.
> + BD_RANGE(130, 0, 0x3D, 0x3F, 0),
> +};
> +
> +static
Re: [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC
Hi de Ho Peeps, On Wed, Mar 27, 2019 at 02:40:47PM +0200, Matti Vaittinen wrote: > Add regulator driver for ROHM BD71837 PMIC. BD71837 contains > 8 bucks and 7 LDOS. Voltages for bucks 1-4 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. > > BD71837 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 bucks3 and 4 by > default. > > Signed-off-by: Matti Vaittinen > --- > drivers/power/regulator/Kconfig | 15 ++ > drivers/power/regulator/Makefile | 1 + > drivers/power/regulator/bd71837.c | 373 ++ > include/power/bd71837.h | 20 ++ > 4 files changed, 409 insertions(+) > This Patch: This was my first patch to U-boot so I wonder if there is something to improve. Is this Ok like this or should I have formatted it somehow differently? Also, is this type of submissions welcome? Any place to check if submissions are rejected, being reviewed or forgotten? Next Steps: Finally, the BD71837 is mainly targeted for powering the i.MX8M. There's another ROHM PMIC BD71847 - which is mainly used for powering the i.MX8MM. The Linux driver I wrote does support both of these PMICs and I was thinking that maybe I should add support for BD71847 in this u-Boot driver too. But before investing on that work I would like to get some feedback regarding the BD71837 u-boot driver. At least a sign that this kind of submissions are welcome or information if I am doing something completely wrong =) About RFC tag: I used RFC tag here mainly because I am unsure if the driver design fits what the u-boot is heading on or if this is kind of driver u-Boot should include. Is this correct use of RFC, and what are the consequences of using RFC-tag? My assumption was that the patch with RFC is reviewed as ither patches, but it is also a sign that the patch might have something that makes it unsuitable for applying to u-Boot. Is this correct? Finally, I guess I need to (re)submit the driver(s) without the RFC tag at some point, any suggestions when? Best Regards Matti Vaittinen ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
