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 > <matti.vaitti...@fi.rohmeurope.com> 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 <matti.vaitti...@fi.rohmeurope.com> > > --- > > 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 <s...@chromium.org> > > 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 =) > > Regards, > Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
