Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
Hello Mark, On 06/23/2014 11:47 AM, Mark Brown wrote: On Mon, Jun 23, 2014 at 11:28:25AM +0200, Javier Martinez Canillas wrote: On 06/21/2014 10:40 PM, Mark Brown wrote: That's not really relevant here - I'm asking if the regulators get their own supplies rather than if anything uses them. Sorry if I keep misunderstanding your question but the regulators in this PMIC don't have a parent supply/regulator node. They should, I'm pretty sure the device does actually regulate one supply into another. Thanks a lot for the clarification. This was not evident to me when I read the PMIC datasheet and because both the max77xxx Chrome OS 3.8 and mainline max77686 drivers used a simplistic model of the power scheme. But Doug confirmed to me that some regulators on this PMIC do indeed use others regulators as a power supply so I'll change this in the next version of the patch-set. Best regards, Javier -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
Hello Mark, On 06/21/2014 10:40 PM, Mark Brown wrote: On Tue, Jun 17, 2014 at 06:05:29PM +0200, Javier Martinez Canillas wrote: On 06/17/2014 04:12 PM, Mark Brown wrote: I just looked at regulator_register() and saw that it does rdev-dev.parent = dev, so yes this has to be the MFD. I noticed that many drivers set config.dev = pdev-dev. The original Chrome OS max77xxx driver and max77686 are two examples but others drivers do the same: Not all drivers are DT drivers that bother specifying supplies. And also I see that mfd_add_device() calls devm_regulator_bulk_register_supply_alias(pdev-dev,...) so I'm confused now about what the correct device should be... Right, but to do that you need to set those aliases up - have you done so? Do the regulators manage to get their supplies? There are no current support in mainline for the devices that use the regulators in this PMIC so I can't tell you if consumers manage to get their supplies correctly (e.g: if regulator_dev_lookup succeeds). That's not really relevant here - I'm asking if the regulators get their own supplies rather than if anything uses them. Sorry if I keep misunderstanding your question but the regulators in this PMIC don't have a parent supply/regulator node. If by own supplies you mean the regulators power outputs (voltage/current constraints), then yes, the regulators manage to get their own voltage output correctly regardless of the value set in config.dev (pdev-dev or pdev-dev.parent). I see in regulator_register() that config.dev is used to set the value of struct regulator_dev .dev.parent and that is used in two places in regulator core: 1) In regulator_register() to get the regmap if config-regmap is not set. 2) In regulator_dev_lookup() checks if r-dev.parent is set. For 1) config.regmap is explicitly set to the MFD regmap in max77802 driver so config.dev is not used in this case and for 2) the value does not matter since it only checks that it's not NULL. Having said that, when I was preparing v3 of the patch-set I noticed that regulator_register() does: dev = config-dev; ... rdev-dev.parent = dev; So I changed to use MFD device instead of pdev-dev in the version I posted last week since the MFD device is the regulator parent. Best regards, Javier -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
On Mon, Jun 23, 2014 at 11:28:25AM +0200, Javier Martinez Canillas wrote: On 06/21/2014 10:40 PM, Mark Brown wrote: That's not really relevant here - I'm asking if the regulators get their own supplies rather than if anything uses them. Sorry if I keep misunderstanding your question but the regulators in this PMIC don't have a parent supply/regulator node. They should, I'm pretty sure the device does actually regulate one supply into another. signature.asc Description: Digital signature
Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
On Tue, Jun 17, 2014 at 06:05:29PM +0200, Javier Martinez Canillas wrote: On 06/17/2014 04:12 PM, Mark Brown wrote: I just looked at regulator_register() and saw that it does rdev-dev.parent = dev, so yes this has to be the MFD. I noticed that many drivers set config.dev = pdev-dev. The original Chrome OS max77xxx driver and max77686 are two examples but others drivers do the same: Not all drivers are DT drivers that bother specifying supplies. And also I see that mfd_add_device() calls devm_regulator_bulk_register_supply_alias(pdev-dev,...) so I'm confused now about what the correct device should be... Right, but to do that you need to set those aliases up - have you done so? Do the regulators manage to get their supplies? There are no current support in mainline for the devices that use the regulators in this PMIC so I can't tell you if consumers manage to get their supplies correctly (e.g: if regulator_dev_lookup succeeds). That's not really relevant here - I'm asking if the regulators get their own supplies rather than if anything uses them. signature.asc Description: Digital signature
Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
On Wed, 18 Jun 2014 15:10:48 +0100 Lee Jones lee.jo...@linaro.org wrote: So I guess you should either a) take the whole patch-set through your mfd tree or b) merge the mfd patches and create an immutable branch that can be pulled by Mark, Mike and Alessandro. I don't know what's the preferred workflow in these cases... That's fine. I'm happy to create shared branches when required. mfd tree will be ok. -- Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
On Wed, 18 Jun 2014, Javier Martinez Canillas wrote: Hello Lee, Thanks a lot for your feedback. On 06/17/2014 11:17 PM, Lee Jones wrote: The MAX77802 PMIC has 10 high-efficiency Buck and 32 Low-dropout (LDO) regulators. This patch adds support for all these regulators found on the MAX77802 PMIC and is based on a driver added by Simon Glass to the Chrome OS kernel 3.8 tree. Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk --- Changes since v1: - Remove unneeded check if num_regulators != MAX77802_MAX_REGULATORS. - Fix .set_suspend_mode handler comment and split regulators ops for regulators that behave differently. Suggested by Mark Brown. - Use module_platform_driver() instead of having init/exit functions. Suggested by Mark Brown. - Use the new descriptor-based GPIO interface instead of the deprecated integer based GPIO one. Suggested by Mark Brown. - Look for regulators child node instead of voltage-regulators to be consistent with other PMIC drivers. Suggested by Mark Brown. drivers/mfd/max77802.c | 1 + Can you remove all of the MFD changes from patches 7, 8 and 9 and create new one. That way there's no requirement for any cross subsystem messiness. Sure, Mark already suggested the same and I'll do it on the next version. But there still be some cross-subsystem dependency/messiness since the regulator, clk and rtc drivers include the mfd max77802 headers that are added in Patch 6. What I should have said was _avoidable_ or _unnecessary_ messiness. So I guess you should either a) take the whole patch-set through your mfd tree or b) merge the mfd patches and create an immutable branch that can be pulled by Mark, Mike and Alessandro. I don't know what's the preferred workflow in these cases... That's fine. I'm happy to create shared branches when required. drivers/regulator/Kconfig| 9 + drivers/regulator/Makefile | 1 + drivers/regulator/max77802.c | 701 +++ 4 files changed, 712 insertions(+) create mode 100644 drivers/regulator/max77802.c Best regards, Javier -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
Hello Mark,
Thanks a lot for your feedback.
On 06/16/2014 09:25 PM, Mark Brown wrote:
On Mon, Jun 16, 2014 at 08:02:35PM +0200, Javier Martinez Canillas wrote:
--- a/drivers/mfd/max77802.c
+++ b/drivers/mfd/max77802.c
@@ -37,6 +37,7 @@
#include linux/err.h
static const struct mfd_cell max77802_devs[] = {
+{ .name = max77802-pmic, },
};
static bool max77802_pmic_is_accessible_reg(struct device *dev,
Please don't do things like this, it makes it harder to apply your
series. Just register all the devices in the MFD when you add the MFD
driver.
Ok, I'll do that. After all mfd core just omits the devices that don't match.
+default:
+pr_warn(%s: regulator_suspend_mode : 0x%x not supported\n,
+rdev-desc-name, mode);
+return -EINVAL;
dev_warn().
Ok.
+static void max77802_copy_reg(struct device *dev, struct regmap *regmap,
+ int from_reg, int to_reg)
+{
+int val;
+int ret;
+
+if (from_reg == to_reg)
+return;
+
+ret = regmap_read(regmap, from_reg, val);
+if (!ret)
+ret = regmap_write(regmap, to_reg, val);
+
+if (ret)
+dev_warn(dev, Copy err %d = %d (%d)\n,
+ from_reg, to_reg, ret);
+}
Again, this looks like it should be generic.
Yes, I missed this from your previous feedback, sorry about that.
I'll add a regmap_copy_reg() function to drivers/base/regmap/regmap.c instead.
+static int max77802_pmic_probe(struct platform_device *pdev)
+{
+dev_dbg(pdev-dev, %s\n, __func__);
This isn't adding anything, just remove it - the core already logs
probes if you want.
Ok.
+config.dev = pdev-dev;
Are you sure this shouldn't be the MFD?
I just looked at regulator_register() and saw that it does rdev-dev.parent =
dev, so yes this has to be the MFD.
+for (i = 0; i MAX77802_MAX_REGULATORS; i++) {
+struct regulator_dev *rdev;
+int id = pdata-regulators[i].id;
+
+config.init_data = pdata-regulators[i].initdata;
+config.of_node = pdata-regulators[i].of_node;
+
+max77802-opmode[id] = MAX77802_OPMODE_NORMAL;
Why isn't this being read from the hardware, this may lead to a
configuration change the first time we pay attention?
The original Chrome OS driver [0] had a regulator-op-mode property similar to
op_mode in Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
to specify the operating mode using DT.
But I removed that since I didn't want to have a specific property for what
appears to be a generic need. I wanted to re-post something along the lines of
what was discussed in [1] and add operating mode support to the generic
regulator code.
So, for now I thought it made sense to set the operating mode to normal on
probe() but I'll change it to read from the hardware if that is better.
I guess I should check in the datasheet if a sane default operating mode for
LDOs is expected when the chip is reseted or if this is left undefined and also
if the bootloader already set this.
Best regards,
Javier
[0]:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/Documentation/devicetree/bindings/mfd/max77xxx.txt
[1]: https://patchwork.kernel.org/patch/1855331/
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
On Tue, Jun 17, 2014 at 12:49:56PM +0200, Javier Martinez Canillas wrote: On 06/16/2014 09:25 PM, Mark Brown wrote: + config.dev = pdev-dev; Are you sure this shouldn't be the MFD? I just looked at regulator_register() and saw that it does rdev-dev.parent = dev, so yes this has to be the MFD. Do the regulators manage to get their supplies? So, for now I thought it made sense to set the operating mode to normal on probe() but I'll change it to read from the hardware if that is better. Yes, otherwise if the device is configured otherwise then when we change the configuration we may break something. I guess I should check in the datasheet if a sane default operating mode for LDOs is expected when the chip is reseted or if this is left undefined and also if the bootloader already set this. You can't do anything based on the particular bootloader you're using in your current system, this has to work in other systems. signature.asc Description: Digital signature
Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
Hello Mark, On 06/17/2014 04:12 PM, Mark Brown wrote: On Tue, Jun 17, 2014 at 12:49:56PM +0200, Javier Martinez Canillas wrote: On 06/16/2014 09:25 PM, Mark Brown wrote: + config.dev = pdev-dev; Are you sure this shouldn't be the MFD? I just looked at regulator_register() and saw that it does rdev-dev.parent = dev, so yes this has to be the MFD. I noticed that many drivers set config.dev = pdev-dev. The original Chrome OS max77xxx driver and max77686 are two examples but others drivers do the same: $ git grep config.dev = pdev-dev drivers/regulator/ | wc -l 35 $ git grep config.dev = pdev-dev.parent drivers/regulator/ | wc -l 11 And also I see that mfd_add_device() calls devm_regulator_bulk_register_supply_alias(pdev-dev,...) so I'm confused now about what the correct device should be... Do the regulators manage to get their supplies? There are no current support in mainline for the devices that use the regulators in this PMIC so I can't tell you if consumers manage to get their supplies correctly (e.g: if regulator_dev_lookup succeeds). But I see in the kernel log that the regulators are registered and configured as expected [0] and also the driver in the Chrome OS 3.8 kernel is working for sure and sets config.dev to pdev-dev instead of the MFD. So, for now I thought it made sense to set the operating mode to normal on probe() but I'll change it to read from the hardware if that is better. Yes, otherwise if the device is configured otherwise then when we change the configuration we may break something. I guess I should check in the datasheet if a sane default operating mode for LDOs is expected when the chip is reseted or if this is left undefined and also if the bootloader already set this. You can't do anything based on the particular bootloader you're using in your current system, this has to work in other systems. Yes, that's why I thought it was a good idea to set to a default operational mode but I'll change it to read from the hardware instead. Thanks a lot and best regards, Javier [0]: http://pastebin.com/raw.php?i=8yyMXcGD -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
The MAX77802 PMIC has 10 high-efficiency Buck and 32 Low-dropout (LDO) regulators. This patch adds support for all these regulators found on the MAX77802 PMIC and is based on a driver added by Simon Glass to the Chrome OS kernel 3.8 tree. Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk --- Changes since v1: - Remove unneeded check if num_regulators != MAX77802_MAX_REGULATORS. - Fix .set_suspend_mode handler comment and split regulators ops for regulators that behave differently. Suggested by Mark Brown. - Use module_platform_driver() instead of having init/exit functions. Suggested by Mark Brown. - Use the new descriptor-based GPIO interface instead of the deprecated integer based GPIO one. Suggested by Mark Brown. - Look for regulators child node instead of voltage-regulators to be consistent with other PMIC drivers. Suggested by Mark Brown. drivers/mfd/max77802.c | 1 + Can you remove all of the MFD changes from patches 7, 8 and 9 and create new one. That way there's no requirement for any cross subsystem messiness. drivers/regulator/Kconfig| 9 + drivers/regulator/Makefile | 1 + drivers/regulator/max77802.c | 701 +++ 4 files changed, 712 insertions(+) create mode 100644 drivers/regulator/max77802.c -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
The MAX77802 PMIC has 10 high-efficiency Buck and 32 Low-dropout
(LDO) regulators. This patch adds support for all these regulators
found on the MAX77802 PMIC and is based on a driver added by Simon
Glass to the Chrome OS kernel 3.8 tree.
Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
---
Changes since v1:
- Remove unneeded check if num_regulators != MAX77802_MAX_REGULATORS.
- Fix .set_suspend_mode handler comment and split regulators ops for
regulators that behave differently. Suggested by Mark Brown.
- Use module_platform_driver() instead of having init/exit functions.
Suggested by Mark Brown.
- Use the new descriptor-based GPIO interface instead of the deprecated
integer based GPIO one. Suggested by Mark Brown.
- Look for regulators child node instead of voltage-regulators to be
consistent with other PMIC drivers. Suggested by Mark Brown.
drivers/mfd/max77802.c | 1 +
drivers/regulator/Kconfig| 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/max77802.c | 701 +++
4 files changed, 712 insertions(+)
create mode 100644 drivers/regulator/max77802.c
diff --git a/drivers/mfd/max77802.c b/drivers/mfd/max77802.c
index c29fcdd..c9dcbab 100644
--- a/drivers/mfd/max77802.c
+++ b/drivers/mfd/max77802.c
@@ -37,6 +37,7 @@
#include linux/err.h
static const struct mfd_cell max77802_devs[] = {
+ { .name = max77802-pmic, },
};
static bool max77802_pmic_is_accessible_reg(struct device *dev,
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 789eb46..17873d3 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -377,6 +377,15 @@ config REGULATOR_MAX77693
and one current regulator 'CHARGER'. This is suitable for
Exynos-4x12 chips.
+config REGULATOR_MAX77802
+ tristate Maxim 77802 regulator
+ depends on MFD_MAX77802
+ help
+ This driver controls a Maxim 77802 regulator
+ via I2C bus. The provided regulator is suitable for
+ Exynos-5 chips to control various voltages. It includes
+ support for control of voltage and ramp speed.
+
config REGULATOR_MC13XXX_CORE
tristate
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index d461110..2aea4b6 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_REGULATOR_MAX8997) += max8997.o
obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o
obj-$(CONFIG_REGULATOR_MAX77686) += max77686.o
obj-$(CONFIG_REGULATOR_MAX77693) += max77693.o
+obj-$(CONFIG_REGULATOR_MAX77802) += max77802.o
obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
obj-$(CONFIG_REGULATOR_MC13XXX_CORE) += mc13xxx-regulator-core.o
diff --git a/drivers/regulator/max77802.c b/drivers/regulator/max77802.c
new file mode 100644
index 000..e1609b5
--- /dev/null
+++ b/drivers/regulator/max77802.c
@@ -0,0 +1,701 @@
+/*
+ * max77802.c - Regulator driver for the Maxim 77802
+ *
+ * Copyright (C) 2013-2014 Google, Inc
+ * Simon Glass s...@chromium.org
+ *
+ * Copyright (C) 2012 Samsung Electronics
+ * Chiwoong Byun woong.b...@smasung.com
+ * Jonghwa Lee jonghwa3@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * This driver is based on max8997.c
+ */
+
+#include linux/kernel.h
+#include linux/bug.h
+#include linux/err.h
+#include linux/gpio.h
+#include linux/slab.h
+#include linux/gpio/consumer.h
+#include linux/platform_device.h
+#include linux/regulator/driver.h
+#include linux/regulator/machine.h
+#include linux/regulator/of_regulator.h
+#include linux/mfd/max77802.h
+#include linux/mfd/max77802-private.h
+
+/* Default ramp delay in case it is not manually set */
+#define MAX77802_RAMP_DELAY10 /* uV/us */
+
+#define MAX77802_OPMODE_SHIFT_LDO 6
+#define MAX77802_OPMODE_BUCK234_SHIFT 4
+#define MAX77802_OPMODE_MASK 0x3
+
+#define MAX77802_VSEL_MASK 0x3F
+#define MAX77802_DVS_VSEL_MASK 0xFF
+
+#define MAX77802_RAMP_RATE_MASK_2BIT 0xC0
+#define MAX77802_RAMP_RATE_SHIFT_2BIT 6
+#define MAX77802_RAMP_RATE_MASK_4BIT 0xF0
+#define MAX77802_RAMP_RATE_SHIFT_4BIT 4
+
+/* LDO16, LDO22 and LDO31 are not available on MAX77802 */
+#define MAX77802_MAX_REGULATORS(MAX77802_REG_MAX - 3)
+
+/* MAX77802 has two register formats: 2-bit and 4-bit */
+static const unsigned int ramp_table_77802_2bit[] = {
+ 12500,
+
Re: [PATCH v2 07/10] regulator: Add driver for Maxim 77802 PMIC regulators
On Mon, Jun 16, 2014 at 08:02:35PM +0200, Javier Martinez Canillas wrote:
--- a/drivers/mfd/max77802.c
+++ b/drivers/mfd/max77802.c
@@ -37,6 +37,7 @@
#include linux/err.h
static const struct mfd_cell max77802_devs[] = {
+ { .name = max77802-pmic, },
};
static bool max77802_pmic_is_accessible_reg(struct device *dev,
Please don't do things like this, it makes it harder to apply your
series. Just register all the devices in the MFD when you add the MFD
driver.
+ default:
+ pr_warn(%s: regulator_suspend_mode : 0x%x not supported\n,
+ rdev-desc-name, mode);
+ return -EINVAL;
dev_warn().
+static void max77802_copy_reg(struct device *dev, struct regmap *regmap,
+ int from_reg, int to_reg)
+{
+ int val;
+ int ret;
+
+ if (from_reg == to_reg)
+ return;
+
+ ret = regmap_read(regmap, from_reg, val);
+ if (!ret)
+ ret = regmap_write(regmap, to_reg, val);
+
+ if (ret)
+ dev_warn(dev, Copy err %d = %d (%d)\n,
+ from_reg, to_reg, ret);
+}
Again, this looks like it should be generic.
+static int max77802_pmic_probe(struct platform_device *pdev)
+{
+ dev_dbg(pdev-dev, %s\n, __func__);
This isn't adding anything, just remove it - the core already logs
probes if you want.
+ config.dev = pdev-dev;
Are you sure this shouldn't be the MFD?
+ for (i = 0; i MAX77802_MAX_REGULATORS; i++) {
+ struct regulator_dev *rdev;
+ int id = pdata-regulators[i].id;
+
+ config.init_data = pdata-regulators[i].initdata;
+ config.of_node = pdata-regulators[i].of_node;
+
+ max77802-opmode[id] = MAX77802_OPMODE_NORMAL;
Why isn't this being read from the hardware, this may lead to a
configuration change the first time we pay attention?
signature.asc
Description: Digital signature
