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.vaittinen@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.vaittinen@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 sjg@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 SWcontrol. Ensure
* the initial state matches dtflags and then
* write the SEL bit*/uc_pdata =dev_get_uclass_platdata(dev);
ret = bd71837_set_enable(dev,!!(uc_pdata->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
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