[PATCH v2 0/2] Add device tree support for MAX8997

[PATCH v2 0/2] Add device tree support for MAX8997

[Thomas Abraham]

MAX8997 is a multi-function device which includes support for regulators, rtc,
battery charger and other sub-blocks. This patchset adds device tree support
for the pmic (regulators) sub-block.

The first patch adds irq domain support for the interrupts supported by max8997
mainly for removing the need to pass a irq_base from the platform code. The
irq_base could not anyway be passed in case of device tree based instantiation.

The second patch adds device tree support for max8997. This patch modifies both
mfd and regulator portions of the max8997 code.

This patchset is based on the following tree.
git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git  for-next

and depends on the following patchs.
[1] All the regulator device tree related patches from
    git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git topic/dt
    (from commit 8f446e6fa1d506be2cb80f91c214f1705327c7f9).
[2] Device tree support patches for wakeup interrupt sources on Exynos4.
[3] [PATCH] irqdomain: export irq_domain._simple_op.s for !CONFIG_OF

This patchset has been tested on Origen board.

Thomas Abraham (2):
  mfd: add irq domain support for max8997 interrupts
  regulator: add device tree support for max8997

 .../devicetree/bindings/regulator/max8997-pmic.txt |  120 ++++++++++++++++
 arch/arm/mach-exynos/mach-nuri.c                   |    4 -
 arch/arm/mach-exynos/mach-origen.c                 |    1 -
 drivers/mfd/max8997-irq.c                          |   33 +++--
 drivers/mfd/max8997.c                              |   73 ++++++++++-
 drivers/regulator/max8997.c                        |  143 +++++++++++++++++++-
 include/linux/mfd/max8997-private.h                |    4 +-
 include/linux/mfd/max8997.h                        |    2 +-
 8 files changed, 356 insertions(+), 24 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/max8997-pmic.txt

[PATCH v2 1/2] mfd: add irq domain support for max8997 interrupts

[Thomas Abraham]

Add irq domain support for max8997 interrupts. All uses of irq_base in platform
data and max8997 driver private data are removed.

Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/mach-exynos/mach-nuri.c    |    4 ----
 arch/arm/mach-exynos/mach-origen.c  |    1 -
 drivers/mfd/max8997-irq.c           |   33 +++++++++++++++++++--------------
 drivers/mfd/max8997.c               |    1 -
 include/linux/mfd/max8997-private.h |    4 +++-
 include/linux/mfd/max8997.h         |    1 -
 6 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-exynos/mach-nuri.c b/arch/arm/mach-exynos/mach-nuri.c
index 3df8bf4..78f64be 100644
--- a/arch/arm/mach-exynos/mach-nuri.c
+++ b/arch/arm/mach-exynos/mach-nuri.c
@@ -1071,12 +1071,8 @@ static struct platform_device nuri_max8903_device = {
 static void __init nuri_power_init(void)
 {
 	int gpio;
-	int irq_base = IRQ_GPIO_END + 1;
 	int ta_en = 0;
 
-	nuri_max8997_pdata.irq_base = irq_base;
-	irq_base += MAX8997_IRQ_NR;
-
 	gpio = EXYNOS4_GPX0(7);
 	gpio_request(gpio, "AP_PMIC_IRQ");
 	s3c_gpio_cfgpin(gpio, S3C_GPIO_SFN(0xf));
diff --git a/arch/arm/mach-exynos/mach-origen.c b/arch/arm/mach-exynos/mach-origen.c
index a011603..d3f3cef 100644
--- a/arch/arm/mach-exynos/mach-origen.c
+++ b/arch/arm/mach-exynos/mach-origen.c
@@ -422,7 +422,6 @@ struct max8997_platform_data __initdata origen_max8997_pdata = {
 	.buck1_gpiodvs	= false,
 	.buck2_gpiodvs	= false,
 	.buck5_gpiodvs	= false,
-	.irq_base	= IRQ_GPIO_END + 1,
 
 	.ignore_gpiodvs_side_effect = true,
 	.buck125_default_idx = 0x0,
diff --git a/drivers/mfd/max8997-irq.c b/drivers/mfd/max8997-irq.c
index 09274cf..eb9cad5 100644
--- a/drivers/mfd/max8997-irq.c
+++ b/drivers/mfd/max8997-irq.c
@@ -142,7 +142,8 @@ static void max8997_irq_sync_unlock(struct irq_data *data)
 static const inline struct max8997_irq_data *
 irq_to_max8997_irq(struct max8997_dev *max8997, int irq)
 {
-	return &max8997_irqs[irq - max8997->irq_base];
+	struct irq_data *data = irq_get_irq_data(irq);
+	return &max8997_irqs[data->hwirq];
 }
 
 static void max8997_irq_mask(struct irq_data *data)
@@ -179,10 +180,11 @@ static struct irq_chip max8997_irq_chip = {
 static irqreturn_t max8997_irq_thread(int irq, void *data)
 {
 	struct max8997_dev *max8997 = data;
+	struct irq_domain *domain = &max8997->irq_domain;
 	u8 irq_reg[MAX8997_IRQ_GROUP_NR] = {};
 	u8 irq_src;
 	int ret;
-	int i;
+	int i, cur_irq;
 
 	ret = max8997_read_reg(max8997->i2c, MAX8997_REG_INTSRC, &irq_src);
 	if (ret < 0) {
@@ -268,9 +270,9 @@ static irqreturn_t max8997_irq_thread(int irq, void *data)
 		irq_reg[i] &= ~max8997->irq_masks_cur[i];
 
 	/* Report */
-	for (i = 0; i < MAX8997_IRQ_NR; i++) {
+	irq_domain_for_each_irq(domain, i, cur_irq) {
 		if (irq_reg[max8997_irqs[i].group] & max8997_irqs[i].mask)
-			handle_nested_irq(max8997->irq_base + i);
+			handle_nested_irq(cur_irq);
 	}
 
 	return IRQ_HANDLED;
@@ -278,13 +280,14 @@ static irqreturn_t max8997_irq_thread(int irq, void *data)
 
 int max8997_irq_resume(struct max8997_dev *max8997)
 {
-	if (max8997->irq && max8997->irq_base)
-		max8997_irq_thread(max8997->irq_base, max8997);
+	if (max8997->irq && max8997->irq_domain.irq_base)
+		max8997_irq_thread(max8997->irq_domain.irq_base, max8997);
 	return 0;
 }
 
 int max8997_irq_init(struct max8997_dev *max8997)
 {
+	struct irq_domain *domain = &max8997->irq_domain;
 	int i;
 	int cur_irq;
 	int ret;
@@ -292,12 +295,6 @@ int max8997_irq_init(struct max8997_dev *max8997)
 
 	if (!max8997->irq) {
 		dev_warn(max8997->dev, "No interrupt specified.\n");
-		max8997->irq_base = 0;
-		return 0;
-	}
-
-	if (!max8997->irq_base) {
-		dev_err(max8997->dev, "No interrupt base specified.\n");
 		return 0;
 	}
 
@@ -327,9 +324,17 @@ int max8997_irq_init(struct max8997_dev *max8997)
 					true : false;
 	}
 
+	domain->irq_base = irq_alloc_descs(-1, 0, MAX8997_IRQ_NR, 0);
+	if (domain->irq_base < 0) {
+		dev_err(max8997->dev, "failed to alloc irq descs\n");
+		return 0;
+	}
+	domain->nr_irq = MAX8997_IRQ_NR;
+	domain->ops = &irq_domain_simple_ops;
+	irq_domain_add(domain);
+
 	/* Register with genirq */
-	for (i = 0; i < MAX8997_IRQ_NR; i++) {
-		cur_irq = i + max8997->irq_base;
+	irq_domain_for_each_irq(domain, i, cur_irq) {
 		irq_set_chip_data(cur_irq, max8997);
 		irq_set_chip_and_handler(cur_irq, &max8997_irq_chip,
 				handle_edge_irq);
diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
index 5be53ae..b996c6e 100644
--- a/drivers/mfd/max8997.c
+++ b/drivers/mfd/max8997.c
@@ -142,7 +142,6 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
 	if (!pdata)
 		goto err;
 
-	max8997->irq_base = pdata->irq_base;
 	max8997->ono = pdata->ono;
 
 	mutex_init(&max8997->iolock);
diff --git a/include/linux/mfd/max8997-private.h b/include/linux/mfd/max8997-private.h
index 3f4deb6..04c5779 100644
--- a/include/linux/mfd/max8997-private.h
+++ b/include/linux/mfd/max8997-private.h
@@ -23,6 +23,8 @@
 #define __LINUX_MFD_MAX8997_PRIV_H
 
 #include <linux/i2c.h>
+#include <linux/export.h>
+#include <linux/irqdomain.h>
 
 #define MAX8997_REG_INVALID	(0xff)
 
@@ -325,7 +327,7 @@ struct max8997_dev {
 
 	int irq;
 	int ono;
-	int irq_base;
+	struct irq_domain irq_domain;
 	struct mutex irqlock;
 	int irq_masks_cur[MAX8997_IRQ_GROUP_NR];
 	int irq_masks_cache[MAX8997_IRQ_GROUP_NR];
diff --git a/include/linux/mfd/max8997.h b/include/linux/mfd/max8997.h
index 0bbd13d..1e0b9cc 100644
--- a/include/linux/mfd/max8997.h
+++ b/include/linux/mfd/max8997.h
@@ -79,7 +79,6 @@ struct max8997_regulator_data {
 
 struct max8997_platform_data {
 	/* IRQ */
-	int irq_base;
 	int ono;
 	int wakeup;
 
-- 
1.6.6.rc2

[PATCH v2 2/2] regulator: add device tree support for max8997

[Thomas Abraham]

Add device tree based discovery support for max8997.

Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 .../devicetree/bindings/regulator/max8997-pmic.txt |  120 ++++++++++++++++
 drivers/mfd/max8997.c                              |   72 ++++++++++-
 drivers/regulator/max8997.c                        |  143 +++++++++++++++++++-
 include/linux/mfd/max8997.h                        |    1 +
 4 files changed, 334 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/max8997-pmic.txt

diff --git a/Documentation/devicetree/bindings/regulator/max8997-pmic.txt b/Documentation/devicetree/bindings/regulator/max8997-pmic.txt
new file mode 100644
index 0000000..0c4559d
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/max8997-pmic.txt
@@ -0,0 +1,120 @@
+* Maxim MAX8997 Voltage and Current Regulator
+
+The Maxim MAX8997 is a multi-function device which includes volatage and
+current regulators, rtc, charger controller and other sub-blocks. It is
+interfaced to the host controller using a i2c interface. Each sub-block is
+addressed by the host system using different i2c slave address. This document
+describes the bindings for 'pmic' sub-block of max8997.
+
+Required properties:
+- compatible: Should be "maxim,max8997-pmic".
+- reg: Specifies the i2c slave address of the pmic block. It should be 0x66.
+
+Optional properties:
+- interrupt-parent: Specifies the phandle of the interrupt controller to which
+  the interrupts from max8997 are delivered to.
+- interrupts: Interrupt specifiers for two interrupt sources.
+  - First interrupt specifier is for 'irq1' interrupt.
+  - Second interrupt specifier is for 'alert' interrupt.
+- max8997,pmic-buck1-uses-gpio-dvs: 'buck1' can be controlled by gpio dvs.
+- max8997,pmic-buck2-uses-gpio-dvs: 'buck2' can be controlled by gpio dvs.
+- max8997,pmic-buck5-uses-gpio-dvs: 'buck5' can be controlled by gpio dvs.
+
+Additional properties required if either of the optional properties are used:
+- max8997,pmic-ignore-gpiodvs-side-effect: When GPIO-DVS mode is used for
+  multiple bucks, changing the voltage value of one of the bucks may affect
+  that of another buck, which is the side effect of the change (set_voltage).
+  Use this property to ignore such side effects and change the voltage.
+
+- max8997,pmic-buck125-default-dvs-idx: Default voltage setting selected from
+  the possible 8 options selectable by the dvs gpios. The value of this
+  property should be between 0 and 7. If not specified or if out of range, the
+  default value of this property is set to 0.
+
+- max8997,pmic-buck125-dvs-gpios: GPIO specifiers for three host gpio's used
+  for dvs. The format of the gpio specifier depends in the gpio controller.
+
+- max8997,pmic-buck1-dvs-voltage: A set of 8 voltage values in micro-volt (uV)
+  units for buck1 when changing voltage using gpio dvs.
+
+- max8997,pmic-buck2-dvs-voltage: A set of 8 voltage values in micro-volt (uV)
+  units for buck2 when changing voltage using gpio dvs.
+
+- max8997,pmic-buck5-dvs-voltage: A set of 8 voltage values in micro-volt (uV)
+  units for buck5 when changing voltage using gpio dvs.
+
+Regulators: The regulators of max8997 that have to be instantiated should be
+included in a sub-node named 'regulators'. Regulator nodes included in this
+sub-node should be of the format as below. Note: The 'n' in LDOn and BUCKn
+represents the LDO or BUCK number as per the datasheet of max8997.
+
+    For LDO's:
+		LDOn {
+			standard regulator bindings here
+		};
+
+    For BUCK's
+		BUCKn {
+			standard regulator bindings here
+		};
+
+The bindings inside the regulator nodes use the standard regulator bindings
+which are documented elsewhere.
+
+Example:
+
+	max8997_pmic at 66 {
+		compatible = "maxim,max8997-pmic";
+		interrupt-parent = <&wakeup_eint>;
+		reg = <0x66>;
+		interrupts = <4 0>, <3 0>;
+
+		max8997,pmic-buck1-uses-gpio-dvs;
+		max8997,pmic-buck2-uses-gpio-dvs;
+		max8997,pmic-buck5-uses-gpio-dvs;
+
+		max8997,pmic-ignore-gpiodvs-side-effect;
+		max8997,pmic-buck125-default-dvs-idx = <0>;
+
+		max8997,pmic-buck125-dvs-gpios = <&gpx0 0 1 0 0>, /* SET1 */
+						 <&gpx0 1 1 0 0>, /* SET2 */
+						 <&gpx0 2 1 0 0>; /* SET3 */
+
+		max8997,pmic-buck1-dvs-voltage = <1350000>, <1300000>,
+						 <1250000>, <1200000>,
+						 <1150000>, <1100000>,
+						 <1000000>, <950000>;
+
+		max8997,pmic-buck2-dvs-voltage = <1100000>, <1100000>,
+				    		 <1100000>, <1100000>,
+						 <1000000>, <1000000>,
+						 <1000000>, <1000000>;
+
+		max8997,pmic-buck5-dvs-voltage = <1200000>, <1200000>,
+						 <1200000>, <1200000>,
+						 <1200000>, <1200000>,
+						 <1200000>, <1200000>;
+
+		regulators {
+			ldo1_reg: LDO1 {
+				regulator-name = "VDD_ABB_3.3V";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo2_reg: LDO2 {
+				regulator-name = "VDD_ALIVE_1.1V";
+				regulator-min-microvolt = <1100000>;
+				regulator-max-microvolt = <1100000>;
+				regulator-always-on;
+			};
+
+			buck1_reg: BUCK1 {
+				regulator-name = "VDD_ARM_1.2V";
+				regulator-min-microvolt = <950000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+		};
+	};
diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
index b996c6e..686dffd 100644
--- a/drivers/mfd/max8997.c
+++ b/drivers/mfd/max8997.c
@@ -23,6 +23,7 @@
 
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/of_irq.h>
 #include <linux/interrupt.h>
 #include <linux/pm_runtime.h>
 #include <linux/module.h>
@@ -122,6 +123,60 @@ int max8997_update_reg(struct i2c_client *i2c, u8 reg, u8 val, u8 mask)
 }
 EXPORT_SYMBOL_GPL(max8997_update_reg);
 
+#ifdef CONFIG_OF
+/*
+ * Only the common platform data elements for max8997 are parsed here from the
+ * device tree. Other sub-modules of max8997 such as pmic, rtc and others have
+ * to parse their own platform data elements from device tree.
+ *
+ * The max8997 platform data structure is instantiated here and the drivers for
+ * the sub-modules need not instantiate another instance while parsing their
+ * platform data.
+ */
+static int max8997_i2c_parse_dt_pdata(struct device *dev,
+					struct max8997_platform_data **pdata)
+{
+	struct max8997_platform_data *pd;
+
+	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd) {
+		dev_err(dev, "could not allocate memory for pdata\n");
+		return -ENOMEM;
+	}
+
+	pd->ono = irq_of_parse_and_map(dev->of_node, 1);
+
+	/*
+	 * ToDo: the 'wakeup' member in the platform data is more of a linux
+	 * specfic information. Hence, there is no binding for that yet and
+	 * not parsed here.
+	 */
+
+	*pdata = pd;
+	return 0;
+}
+#else
+static int max8997_i2c_parse_dt_pdata(struct device *dev,
+					struct max8997_platform_data **pdata)
+{
+	return 0;
+}
+#endif
+
+static struct of_device_id __devinitdata max8997_pmic_dt_match[];
+static inline int max8997_i2c_get_driver_data(struct i2c_client *i2c,
+						const struct i2c_device_id *id)
+{
+#ifdef CONFIG_OF
+	if (i2c->dev.of_node) {
+		const struct of_device_id *match;
+		match = of_match_node(max8997_pmic_dt_match, i2c->dev.of_node);
+		return (int)match->data;
+	}
+#endif
+	return (int)id->driver_data;
+}
+
 static int max8997_i2c_probe(struct i2c_client *i2c,
 			    const struct i2c_device_id *id)
 {
@@ -136,9 +191,16 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
 	i2c_set_clientdata(i2c, max8997);
 	max8997->dev = &i2c->dev;
 	max8997->i2c = i2c;
-	max8997->type = id->driver_data;
+	max8997->type = max8997_i2c_get_driver_data(i2c, id);
 	max8997->irq = i2c->irq;
 
+	if (max8997->dev->of_node) {
+		ret = max8997_i2c_parse_dt_pdata(max8997->dev, &pdata);
+		if (ret)
+			goto err;
+		i2c->dev.platform_data = pdata;
+	}
+
 	if (!pdata)
 		goto err;
 
@@ -428,11 +490,19 @@ const struct dev_pm_ops max8997_pm = {
 	.restore = max8997_restore,
 };
 
+#ifdef CONFIG_OF
+static struct of_device_id __devinitdata max8997_pmic_dt_match[] = {
+	{ .compatible = "maxim,max8997-pmic", .data = TYPE_MAX8997 },
+	{},
+};
+#endif
+
 static struct i2c_driver max8997_i2c_driver = {
 	.driver = {
 		   .name = "max8997",
 		   .owner = THIS_MODULE,
 		   .pm = &max8997_pm,
+		   .of_match_table = of_match_ptr(max8997_pmic_dt_match),
 	},
 	.probe = max8997_i2c_probe,
 	.remove = max8997_i2c_remove,
diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c
index d26e864..053d0b7 100644
--- a/drivers/regulator/max8997.c
+++ b/drivers/regulator/max8997.c
@@ -25,6 +25,7 @@
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -32,6 +33,7 @@
 #include <linux/regulator/machine.h>
 #include <linux/mfd/max8997.h>
 #include <linux/mfd/max8997-private.h>
+#include <linux/regulator/of_regulator.h>
 
 struct max8997_data {
 	struct device *dev;
@@ -958,6 +960,138 @@ static struct regulator_desc regulators[] = {
 	},
 };
 
+#if CONFIG_OF
+static int max8997_pmic_dt_parse_dvs_gpio(struct max8997_dev *iodev,
+			struct max8997_platform_data *pdata,
+			struct device_node *pmic_np)
+{
+	int i, gpio;
+
+	for (i = 0; i < 3; i++) {
+		gpio = of_get_named_gpio(pmic_np,
+					"max8997,pmic-buck125-dvs-gpios", i);
+		if (!gpio_is_valid(gpio)) {
+			dev_err(iodev->dev, "invalid gpio[%d]: %d\n", i, gpio);
+			return -EINVAL;
+		}
+		pdata->buck125_gpios[i] = gpio;
+	}
+	return 0;
+}
+
+static int max8997_pmic_dt_parse_pdata(struct max8997_dev *iodev,
+					struct max8997_platform_data *pdata)
+{
+	struct device_node *pmic_np, *regulators_np, *reg_np;
+	struct max8997_regulator_data *rdata;
+	unsigned int i, ret;
+
+	pmic_np = iodev->dev->of_node;
+	if (!pmic_np) {
+		dev_err(iodev->dev, "could not find pmic sub-node\n");
+		return -ENODEV;
+	}
+
+	regulators_np = of_find_node_by_name(pmic_np, "regulators");
+	if (!regulators_np) {
+		dev_err(iodev->dev, "could not find regulators sub-node\n");
+		return -EINVAL;
+	}
+
+	/* count the number of regulators to be supported in pmic */
+	pdata->num_regulators = 0;
+	for_each_child_of_node(regulators_np, reg_np)
+		pdata->num_regulators++;
+
+	rdata = devm_kzalloc(iodev->dev, sizeof(*rdata) *
+				pdata->num_regulators, GFP_KERNEL);
+	if (!rdata) {
+		dev_err(iodev->dev, "could not allocate memory for "
+						"regulator data\n");
+		return -ENOMEM;
+	}
+
+	pdata->regulators = rdata;
+	for_each_child_of_node(regulators_np, reg_np) {
+		for (i = 0; i < ARRAY_SIZE(regulators); i++)
+			if (!of_node_cmp(reg_np->name, regulators[i].name))
+				break;
+		rdata->id = i;
+		rdata->initdata = of_get_regulator_init_data(
+						iodev->dev, reg_np);
+		rdata->reg_node = reg_np;
+		rdata++;
+	}
+
+	if (of_get_property(pmic_np, "max8997,pmic-buck1-uses-gpio-dvs", NULL))
+		pdata->buck1_gpiodvs = true;
+
+	if (of_get_property(pmic_np, "max8997,pmic-buck2-uses-gpio-dvs", NULL))
+		pdata->buck2_gpiodvs = true;
+
+	if (of_get_property(pmic_np, "max8997,pmic-buck5-uses-gpio-dvs", NULL))
+		pdata->buck5_gpiodvs = true;
+
+	if (pdata->buck1_gpiodvs) {
+		if (of_property_read_u32_array(pmic_np,
+				"max8997,pmic-buck1-dvs-voltage",
+				pdata->buck1_voltage, 8)) {
+			dev_err(iodev->dev, "buck1 voltages not specified\n");
+			return -EINVAL;
+		}
+	}
+
+	if (pdata->buck2_gpiodvs) {
+		if (of_property_read_u32_array(pmic_np,
+				"max8997,pmic-buck2-dvs-voltage",
+				pdata->buck2_voltage, 8)) {
+			dev_err(iodev->dev, "buck2 voltages not specified\n");
+			return -EINVAL;
+		}
+	}
+
+	if (pdata->buck5_gpiodvs) {
+		if (of_property_read_u32_array(pmic_np,
+				"max8997,pmic-buck5-dvs-voltage",
+				pdata->buck5_voltage, 8)) {
+			dev_err(iodev->dev, "buck5 voltages not specified\n");
+			return -EINVAL;
+		}
+	}
+
+	if (pdata->buck1_gpiodvs || pdata->buck2_gpiodvs ||
+						pdata->buck5_gpiodvs) {
+		ret = max8997_pmic_dt_parse_dvs_gpio(iodev, pdata, pmic_np);
+		if (ret)
+			return -EINVAL;
+
+		if (of_property_read_u32(pmic_np,
+				"max8997,pmic-buck125-default-dvs-idx",
+				&pdata->buck125_default_idx)) {
+			pdata->buck125_default_idx = 0;
+		} else {
+			if (pdata->buck125_default_idx >= 8) {
+				pdata->buck125_default_idx = 0;
+				dev_info(iodev->dev, "invalid value for "
+				"default dvs index, using 0 instead\n");
+			}
+		}
+
+		if (of_get_property(pmic_np,
+			"max8997,pmic-ignore-gpiodvs-side-effect", NULL))
+			pdata->ignore_gpiodvs_side_effect = true;
+	}
+
+	return 0;
+}
+#else
+static int max8997_pmic_dt_parse_pdata(struct max8997_dev *iodev,
+					struct max8997_platform_data *pdata)
+{
+	return 0;
+}
+#endif /* CONFIG_OF */
+
 static __devinit int max8997_pmic_probe(struct platform_device *pdev)
 {
 	struct max8997_dev *iodev = dev_get_drvdata(pdev->dev.parent);
@@ -968,6 +1102,12 @@ static __devinit int max8997_pmic_probe(struct platform_device *pdev)
 	int i, ret, size;
 	u8 max_buck1 = 0, max_buck2 = 0, max_buck5 = 0;
 
+	if (iodev->dev->of_node) {
+		ret = max8997_pmic_dt_parse_pdata(iodev, pdata);
+		if (ret)
+			return ret;
+	}
+
 	if (!pdata) {
 		dev_err(pdev->dev.parent, "No platform init data supplied.\n");
 		return -ENODEV;
@@ -1146,7 +1286,8 @@ static __devinit int max8997_pmic_probe(struct platform_device *pdev)
 			regulators[id].n_voltages = 16;
 
 		rdev[i] = regulator_register(&regulators[id], max8997->dev,
-				pdata->regulators[i].initdata, max8997, NULL);
+				pdata->regulators[i].initdata, max8997,
+				pdata->regulators[i].reg_node);
 		if (IS_ERR(rdev[i])) {
 			ret = PTR_ERR(rdev[i]);
 			dev_err(max8997->dev, "regulator init failed for %d\n",
diff --git a/include/linux/mfd/max8997.h b/include/linux/mfd/max8997.h
index 1e0b9cc..0c2e063 100644
--- a/include/linux/mfd/max8997.h
+++ b/include/linux/mfd/max8997.h
@@ -75,6 +75,7 @@ enum max8998_regulators {
 struct max8997_regulator_data {
 	int id;
 	struct regulator_init_data *initdata;
+	struct device_node *reg_node;
 };
 
 struct max8997_platform_data {
-- 
1.6.6.rc2

[PATCH v2 2/2] regulator: add device tree support for max8997

[MyungJoo Ham]

On Thu, Jan 12, 2012 at 4:35 PM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> Add device tree based discovery support for max8997.
>
> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
> ?.../devicetree/bindings/regulator/max8997-pmic.txt | ?120 ++++++++++++++++
> ?drivers/mfd/max8997.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 72 ++++++++++-
> ?drivers/regulator/max8997.c ? ? ? ? ? ? ? ? ? ? ? ?| ?143 +++++++++++++++++++-
> ?include/linux/mfd/max8997.h ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
> ?4 files changed, 334 insertions(+), 2 deletions(-)
> ?create mode 100644 Documentation/devicetree/bindings/regulator/max8997-pmic.txt
>
> diff --git a/Documentation/devicetree/bindings/regulator/max8997-pmic.txt b/Documentation/devicetree/bindings/regulator/max8997-pmic.txt
> new file mode 100644
> index 0000000..0c4559d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/max8997-pmic.txt
> @@ -0,0 +1,120 @@
> +* Maxim MAX8997 Voltage and Current Regulator
> +
> +The Maxim MAX8997 is a multi-function device which includes volatage and
> +current regulators, rtc, charger controller and other sub-blocks. It is
> +interfaced to the host controller using a i2c interface. Each sub-block is
> +addressed by the host system using different i2c slave address. This document
> +describes the bindings for 'pmic' sub-block of max8997.
> +
> +Required properties:
> +- compatible: Should be "maxim,max8997-pmic".
> +- reg: Specifies the i2c slave address of the pmic block. It should be 0x66.
> +
> +Optional properties:
> +- interrupt-parent: Specifies the phandle of the interrupt controller to which
> + ?the interrupts from max8997 are delivered to.
> +- interrupts: Interrupt specifiers for two interrupt sources.
> + ?- First interrupt specifier is for 'irq1' interrupt.
> + ?- Second interrupt specifier is for 'alert' interrupt.
> +- max8997,pmic-buck1-uses-gpio-dvs: 'buck1' can be controlled by gpio dvs.
> +- max8997,pmic-buck2-uses-gpio-dvs: 'buck2' can be controlled by gpio dvs.
> +- max8997,pmic-buck5-uses-gpio-dvs: 'buck5' can be controlled by gpio dvs.
> +
> +Additional properties required if either of the optional properties are used:
> +- max8997,pmic-ignore-gpiodvs-side-effect: When GPIO-DVS mode is used for
> + ?multiple bucks, changing the voltage value of one of the bucks may affect
> + ?that of another buck, which is the side effect of the change (set_voltage).
> + ?Use this property to ignore such side effects and change the voltage.
> +
> +- max8997,pmic-buck125-default-dvs-idx: Default voltage setting selected from
> + ?the possible 8 options selectable by the dvs gpios. The value of this
> + ?property should be between 0 and 7. If not specified or if out of range, the
> + ?default value of this property is set to 0.
> +
> +- max8997,pmic-buck125-dvs-gpios: GPIO specifiers for three host gpio's used
> + ?for dvs. The format of the gpio specifier depends in the gpio controller.
> +
> +- max8997,pmic-buck1-dvs-voltage: A set of 8 voltage values in micro-volt (uV)
> + ?units for buck1 when changing voltage using gpio dvs.
> +
> +- max8997,pmic-buck2-dvs-voltage: A set of 8 voltage values in micro-volt (uV)
> + ?units for buck2 when changing voltage using gpio dvs.
> +
> +- max8997,pmic-buck5-dvs-voltage: A set of 8 voltage values in micro-volt (uV)
> + ?units for buck5 when changing voltage using gpio dvs.
> +

These three values are _not_ optional.

If they are omitted and at least one of buck1/2/5 is supplying
critical power to the system, which they do in Exynos boards, system
will not boot properly.

In order to work properly, at least the first element of each array
should have a proper value (preferrably the maximum safety voltage if
this gpio-dvs feature is not used) if others are filled with zeros.

If GPIO-DVS mode is enabled, all the 8 elements of each array should
be filled with proper values.


[]
> diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c
> index d26e864..053d0b7 100644
> --- a/drivers/regulator/max8997.c
> +++ b/drivers/regulator/max8997.c
[]
> @@ -32,6 +33,7 @@
> ?struct max8997_data {
> ? ? ? ?struct device *dev;
> @@ -958,6 +960,138 @@ static struct regulator_desc regulators[] = {
> ? ? ? ?},
> ?};
>
> +#if CONFIG_OF

#ifdef?

[]
> +static int max8997_pmic_dt_parse_pdata(struct max8997_dev *iodev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct max8997_platform_data *pdata)
> +{
[]
> + ? ? ? if (pdata->buck1_gpiodvs) {
> + ? ? ? ? ? ? ? if (of_property_read_u32_array(pmic_np,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "max8997,pmic-buck1-dvs-voltage",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck1_voltage, 8)) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(iodev->dev, "buck1 voltages not specified\n");
> + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? ? ? ? ? }
> + ? ? ? }
> +
> + ? ? ? if (pdata->buck2_gpiodvs) {
> + ? ? ? ? ? ? ? if (of_property_read_u32_array(pmic_np,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "max8997,pmic-buck2-dvs-voltage",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck2_voltage, 8)) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(iodev->dev, "buck2 voltages not specified\n");
> + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? ? ? ? ? }
> + ? ? ? }
> +
> + ? ? ? if (pdata->buck5_gpiodvs) {
> + ? ? ? ? ? ? ? if (of_property_read_u32_array(pmic_np,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "max8997,pmic-buck5-dvs-voltage",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck5_voltage, 8)) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(iodev->dev, "buck5 voltages not specified\n");
> + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? ? ? ? ? }
> + ? ? ? }

As mentioned above, these array should be loaded to probe function
even if buck?_gpiodvs is false.

[]



Cheers!
MyungJoo


-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics

[PATCH v2 2/2] regulator: add device tree support for max8997

[Thomas Abraham]

Dear Mr. Ham,

On 12 January 2012 15:19, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> On Thu, Jan 12, 2012 at 4:35 PM, Thomas Abraham
> <thomas.abraham@linaro.org> wrote:
>> Add device tree based discovery support for max8997.
>>
>> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Cc: Rajendra Nayak <rnayak@ti.com>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>> ?.../devicetree/bindings/regulator/max8997-pmic.txt | ?120 ++++++++++++++++
>> ?drivers/mfd/max8997.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 72 ++++++++++-
>> ?drivers/regulator/max8997.c ? ? ? ? ? ? ? ? ? ? ? ?| ?143 +++++++++++++++++++-
>> ?include/linux/mfd/max8997.h ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
>> ?4 files changed, 334 insertions(+), 2 deletions(-)
>> ?create mode 100644 Documentation/devicetree/bindings/regulator/max8997-pmic.txt
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/max8997-pmic.txt b/Documentation/devicetree/bindings/regulator/max8997-pmic.txt
>> new file mode 100644
>> index 0000000..0c4559d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/max8997-pmic.txt
>> @@ -0,0 +1,120 @@
>> +* Maxim MAX8997 Voltage and Current Regulator
>> +
>> +The Maxim MAX8997 is a multi-function device which includes volatage and
>> +current regulators, rtc, charger controller and other sub-blocks. It is
>> +interfaced to the host controller using a i2c interface. Each sub-block is
>> +addressed by the host system using different i2c slave address. This document
>> +describes the bindings for 'pmic' sub-block of max8997.
>> +
>> +Required properties:
>> +- compatible: Should be "maxim,max8997-pmic".
>> +- reg: Specifies the i2c slave address of the pmic block. It should be 0x66.
>> +
>> +Optional properties:
>> +- interrupt-parent: Specifies the phandle of the interrupt controller to which
>> + ?the interrupts from max8997 are delivered to.
>> +- interrupts: Interrupt specifiers for two interrupt sources.
>> + ?- First interrupt specifier is for 'irq1' interrupt.
>> + ?- Second interrupt specifier is for 'alert' interrupt.
>> +- max8997,pmic-buck1-uses-gpio-dvs: 'buck1' can be controlled by gpio dvs.
>> +- max8997,pmic-buck2-uses-gpio-dvs: 'buck2' can be controlled by gpio dvs.
>> +- max8997,pmic-buck5-uses-gpio-dvs: 'buck5' can be controlled by gpio dvs.
>> +
>> +Additional properties required if either of the optional properties are used:
>> +- max8997,pmic-ignore-gpiodvs-side-effect: When GPIO-DVS mode is used for
>> + ?multiple bucks, changing the voltage value of one of the bucks may affect
>> + ?that of another buck, which is the side effect of the change (set_voltage).
>> + ?Use this property to ignore such side effects and change the voltage.
>> +
>> +- max8997,pmic-buck125-default-dvs-idx: Default voltage setting selected from
>> + ?the possible 8 options selectable by the dvs gpios. The value of this
>> + ?property should be between 0 and 7. If not specified or if out of range, the
>> + ?default value of this property is set to 0.
>> +
>> +- max8997,pmic-buck125-dvs-gpios: GPIO specifiers for three host gpio's used
>> + ?for dvs. The format of the gpio specifier depends in the gpio controller.
>> +
>> +- max8997,pmic-buck1-dvs-voltage: A set of 8 voltage values in micro-volt (uV)
>> + ?units for buck1 when changing voltage using gpio dvs.
>> +
>> +- max8997,pmic-buck2-dvs-voltage: A set of 8 voltage values in micro-volt (uV)
>> + ?units for buck2 when changing voltage using gpio dvs.
>> +
>> +- max8997,pmic-buck5-dvs-voltage: A set of 8 voltage values in micro-volt (uV)
>> + ?units for buck5 when changing voltage using gpio dvs.
>> +
>
> These three values are _not_ optional.
>
> If they are omitted and at least one of buck1/2/5 is supplying
> critical power to the system, which they do in Exynos boards, system
> will not boot properly.
>
> In order to work properly, at least the first element of each array
> should have a proper value (preferrably the maximum safety voltage if
> this gpio-dvs feature is not used) if others are filled with zeros.
>
> If GPIO-DVS mode is enabled, all the 8 elements of each array should
> be filled with proper values.

Thanks for this information. I was not aware of that. I assumed that
if GPIO DVS is not used for BUCK 1/2/5, then there is no need to
supply the DVS voltage options. I will modify this accordingly.

>
>
> []
>> diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c
>> index d26e864..053d0b7 100644
>> --- a/drivers/regulator/max8997.c
>> +++ b/drivers/regulator/max8997.c
> []
>> @@ -32,6 +33,7 @@
>> ?struct max8997_data {
>> ? ? ? ?struct device *dev;
>> @@ -958,6 +960,138 @@ static struct regulator_desc regulators[] = {
>> ? ? ? ?},
>> ?};
>>
>> +#if CONFIG_OF
>
> #ifdef?

Yes, I will fix this.

>
> []
>> +static int max8997_pmic_dt_parse_pdata(struct max8997_dev *iodev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct max8997_platform_data *pdata)
>> +{
> []
>> + ? ? ? if (pdata->buck1_gpiodvs) {
>> + ? ? ? ? ? ? ? if (of_property_read_u32_array(pmic_np,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "max8997,pmic-buck1-dvs-voltage",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck1_voltage, 8)) {
>> + ? ? ? ? ? ? ? ? ? ? ? dev_err(iodev->dev, "buck1 voltages not specified\n");
>> + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? ? ? ? ? }
>> + ? ? ? }
>> +
>> + ? ? ? if (pdata->buck2_gpiodvs) {
>> + ? ? ? ? ? ? ? if (of_property_read_u32_array(pmic_np,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "max8997,pmic-buck2-dvs-voltage",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck2_voltage, 8)) {
>> + ? ? ? ? ? ? ? ? ? ? ? dev_err(iodev->dev, "buck2 voltages not specified\n");
>> + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? ? ? ? ? }
>> + ? ? ? }
>> +
>> + ? ? ? if (pdata->buck5_gpiodvs) {
>> + ? ? ? ? ? ? ? if (of_property_read_u32_array(pmic_np,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "max8997,pmic-buck5-dvs-voltage",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck5_voltage, 8)) {
>> + ? ? ? ? ? ? ? ? ? ? ? dev_err(iodev->dev, "buck5 voltages not specified\n");
>> + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? ? ? ? ? }
>> + ? ? ? }
>
> As mentioned above, these array should be loaded to probe function
> even if buck?_gpiodvs is false.

Ok.

Thanks for reviewing this patch.

Regards,
Thomas.

>
> []
>
>
>
> Cheers!
> MyungJoo
>
>
> --
> MyungJoo Ham, Ph.D.
> Mobile Software Platform Lab, DMC Business, Samsung Electronics

[PATCH v2 2/2] regulator: add device tree support for max8997

[Karol Lewandowski]

On 12.01.2012 08:35, Thomas Abraham wrote:

Hi!

> Add device tree based discovery support for max8997.

> +static int max8997_pmic_dt_parse_pdata(struct max8997_dev *iodev,
> +					struct max8997_platform_data *pdata)
> +{
...
> +	pdata->regulators = rdata;
> +	for_each_child_of_node(regulators_np, reg_np) {
> +		for (i = 0; i<  ARRAY_SIZE(regulators); i++)
> +			if (!of_node_cmp(reg_np->name, regulators[i].name))
> +				break;

If I read your patch correctly it would impossible to configure 
following outputs when pmic is instantiated from DT:

     enum max8998_regulators {
     ...
  1    MAX8997_EN32KHZ_AP,
  2    MAX8997_EN32KHZ_CP,
  3    MAX8997_CHARGER_CV,
  4    MAX8997_CHARGER_TOPOFF, /* MBCCTRL5 */

This is due to use of embedded spaces in regulator names (which are used 
in comparision).

This can be fixed with trivial patch[1].

What bothers me more, are following regulators:

       MAX8997_EN32KHZ_AP,
       MAX8997_EN32KHZ_CP,
       MAX8997_ENVICHG,
       MAX8997_ESAFEOUT1,
       MAX8997_ESAFEOUT2,

Aren't these fixed? i.e. - is it really needed to configure these either 
by platform data or DT at all?


[1]

 From 5cfba526210bc596c7d14e33fea93648baa0a227 Mon Sep 17 00:00:00 2001
From: Karol Lewandowski <k.lewandowsk@samsung.com>
Date: Mon, 23 Jan 2012 17:20:25 +0100
Subject: [PATCH] max8997: Avoid spaces in regulator names

max8997-pmic instantiated from device tree uses names,
not numerical ids to distinguish between outputs.

Use names without spaces to make it possible to specify
parameters for said regulators in DTS.

Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
  drivers/regulator/max8997.c |    8 ++++----
  1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c
index 053d0b7..4b572eb 100644
--- a/drivers/regulator/max8997.c
+++ b/drivers/regulator/max8997.c
@@ -910,13 +910,13 @@ static struct regulator_desc regulators[] = {
         },
         regulator_desc_buck(7),
         {
-               .name   = "EN32KHz AP",
+               .name   = "EN32KHz_AP",
                 .id     = MAX8997_EN32KHZ_AP,
                 .ops    = &max8997_fixedvolt_ops,
                 .type   = REGULATOR_VOLTAGE,
                 .owner  = THIS_MODULE,
         }, {
-               .name   = "EN32KHz CP",
+               .name   = "EN32KHz_CP",
                 .id     = MAX8997_EN32KHZ_CP,
                 .ops    = &max8997_fixedvolt_ops,
                 .type   = REGULATOR_VOLTAGE,
@@ -940,7 +940,7 @@ static struct regulator_desc regulators[] = {
                 .type   = REGULATOR_VOLTAGE,
                 .owner   = THIS_MODULE,
         }, {
-               .name   = "CHARGER CV",
+               .name   = "CHARGER_CV",
                 .id     = MAX8997_CHARGER_CV,
                 .ops    = &max8997_fixedstate_ops,
                 .type   = REGULATOR_VOLTAGE,
@@ -952,7 +952,7 @@ static struct regulator_desc regulators[] = {
                 .type   = REGULATOR_CURRENT,
                 .owner   = THIS_MODULE,
         }, {
-               .name   = "CHARGER TOPOFF",
+               .name   = "CHARGER_TOPOFF",
                 .id     = MAX8997_CHARGER_TOPOFF,
                 .ops    = &max8997_charger_fixedstate_ops,
                 .type   = REGULATOR_CURRENT,
-- 
1.7.8.3

[PATCH v2 2/2] regulator: add device tree support for max8997

[Mark Brown]

On Mon, Jan 23, 2012 at 06:50:22PM +0100, Karol Lewandowski wrote:

> Aren't these fixed? i.e. - is it really needed to configure these
> either by platform data or DT at all?
> 
> 
> [1]
> 
> From 5cfba526210bc596c7d14e33fea93648baa0a227 Mon Sep 17 00:00:00 2001
> From: Karol Lewandowski <k.lewandowsk@samsung.com>

Documentation/SubmittingPatches please...

[PATCH v2 2/2] regulator: add device tree support for max8997

[Karol Lewandowski]

On 01/23/2012 07:20 PM, Mark Brown wrote:
> On Mon, Jan 23, 2012 at 06:50:22PM +0100, Karol Lewandowski wrote:
>
>> Aren't these fixed? i.e. - is it really needed to configure these
>> either by platform data or DT at all?
>>
>>
>> [1]
>>
>>  From 5cfba526210bc596c7d14e33fea93648baa0a227 Mon Sep 17 00:00:00 2001
>> From: Karol Lewandowski<k.lewandowsk@samsung.com>
>
> Documentation/SubmittingPatches please...

I should have stated explicitly that purpose of this patch (I should 
have called it sniplet) was to show my point only.

IMHO it's still up to debate how above problem should be solved.

I'm not entirely sure that we really need things like e.g. "EN32KHz AP" 
in DT (nor in platform data, for that matter).  I would like to see 
Thomas' opinion first.

Thanks

Regards,
Karol Lewandowski

[PATCH v2 2/2] regulator: add device tree support for max8997

[Mark Brown]

On Mon, Jan 23, 2012 at 08:21:48PM +0100, Karol Lewandowski wrote:
> On 01/23/2012 07:20 PM, Mark Brown wrote:

> >Documentation/SubmittingPatches please...

> I should have stated explicitly that purpose of this patch (I should
> have called it sniplet) was to show my point only.

> IMHO it's still up to debate how above problem should be solved.

It looks like a reasonable solution to me, and it wouldn't conflict with
any better solution that does come along.

> I'm not entirely sure that we really need things like e.g. "EN32KHz
> AP" in DT (nor in platform data, for that matter).  I would like to
> see Thomas' opinion first.

The 32kHz crystals are a bit of an odd case, really those should be
handled by the clock API but obviously that's stalled and has been for
quite some time.

[PATCH v2 2/2] regulator: add device tree support for max8997

[Karol Lewandowski]

On 12.01.2012 08:35, Thomas Abraham wrote:
> Add device tree based discovery support for max8997.

> +static int max8997_pmic_dt_parse_pdata(struct max8997_dev *iodev,
> +					struct max8997_platform_data *pdata)
> +{

> +	pdata->regulators = rdata;
> +	for_each_child_of_node(regulators_np, reg_np) {
> +		for (i = 0; i<  ARRAY_SIZE(regulators); i++)
> +			if (!of_node_cmp(reg_np->name, regulators[i].name))
> +				break;
> +		rdata->id = i;
> +		rdata->initdata = of_get_regulator_init_data(
> +						iodev->dev, reg_np);

One more thing - of_get_regulator_init_data() will set apply_uV to 1 so 
we need to reset it for BUCK6, which doesn't provide set_voltage() ops, 
like:

	if (rdata->initdata && regulators[i].id == MAX8997_BUCK6)
		rdata->initdata->constraints.apply_uV = 0;

Thanks

[PATCH v2 2/2] regulator: add device tree support for max8997

[Mark Brown]

On Wed, Jan 25, 2012 at 10:55:49AM +0100, Karol Lewandowski wrote:
> On 12.01.2012 08:35, Thomas Abraham wrote:
> >Add device tree based discovery support for max8997.
> 
> >+	pdata->regulators = rdata;
> >+	for_each_child_of_node(regulators_np, reg_np) {
> >+		for (i = 0; i<  ARRAY_SIZE(regulators); i++)
> >+			if (!of_node_cmp(reg_np->name, regulators[i].name))
> >+				break;
> >+		rdata->id = i;
> >+		rdata->initdata = of_get_regulator_init_data(
> >+						iodev->dev, reg_np);

> One more thing - of_get_regulator_init_data() will set apply_uV to 1
> so we need to reset it for BUCK6, which doesn't provide
> set_voltage() ops, like:

> 	if (rdata->initdata && regulators[i].id == MAX8997_BUCK6)
> 		rdata->initdata->constraints.apply_uV = 0;

So, over in the other thready you were referring to mailing list posts
you made in the past few moments as examples of past issues.  Please at
least mention that there hasn't been any actual discussion when doing
this...

I don't see a problem here, if the device can't set the voltage then
setting constraints to allow the voltage to be changed is silly and the
user just shouldn't do that.

[PATCH v2 2/2] regulator: add device tree support for max8997

[Karol Lewandowski]

On 25.01.2012 12:26, Mark Brown wrote:
> On Wed, Jan 25, 2012 at 10:55:49AM +0100, Karol Lewandowski wrote:
>> On 12.01.2012 08:35, Thomas Abraham wrote:
>>> Add device tree based discovery support for max8997.
>>
>>> +	pdata->regulators = rdata;
>>> +	for_each_child_of_node(regulators_np, reg_np) {
>>> +		for (i = 0; i<   ARRAY_SIZE(regulators); i++)
>>> +			if (!of_node_cmp(reg_np->name, regulators[i].name))
>>> +				break;
>>> +		rdata->id = i;
>>> +		rdata->initdata = of_get_regulator_init_data(
>>> +						iodev->dev, reg_np);
>
>> One more thing - of_get_regulator_init_data() will set apply_uV to 1
>> so we need to reset it for BUCK6, which doesn't provide
>> set_voltage() ops, like:
>
>> 	if (rdata->initdata&&  regulators[i].id == MAX8997_BUCK6)
>> 		rdata->initdata->constraints.apply_uV = 0;
>
> So, over in the other thready you were referring to mailing list posts
> you made in the past few moments as examples of past issues.  Please at
> least mention that there hasn't been any actual discussion when doing
> this...

Sorry if this caused confusion.  I'll be more cautious next time.

> I don't see a problem here, if the device can't set the voltage then
> setting constraints to allow the voltage to be changed is silly and the
> user just shouldn't do that.

Agreed.  I've assumed that old platform code done right thing when it 
set buck6 voltage constraints [1].

However, I still find it little problematic that dt and non-dt versions 
behave differently when given the same set of parameters (previously 
apply_uV wasn't the default and required explicit flag).


Thomas, would you mind adding small note to pmic's bindings 
documentation stating that Buck6 is basically on/off switch? (Thus, no 
voltage should nor can be specified).

[1] http://lxr.linux.no/linux+v3.2.1/arch/arm/mach-exynos/mach-nuri.c#L802

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

[PATCH v2 2/2] regulator: add device tree support for max8997

[Mark Brown]

On Wed, Jan 25, 2012 at 01:02:29PM +0100, Karol Lewandowski wrote:
> On 25.01.2012 12:26, Mark Brown wrote:

> However, I still find it little problematic that dt and non-dt
> versions behave differently when given the same set of parameters
> (previously apply_uV wasn't the default and required explicit flag).

Well, they're different things.  Device tree isn't Linux specific at
all.

> Thomas, would you mind adding small note to pmic's bindings
> documentation stating that Buck6 is basically on/off switch? (Thus,
> no voltage should nor can be specified).

At some point you end up copying the entire datasheet in there...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120125/78bbafcf/attachment.sig>

[PATCH v2 2/2] regulator: add device tree support for max8997

[Karol Lewandowski]

On 25.01.2012 14:32, Mark Brown wrote:
> On Wed, Jan 25, 2012 at 01:02:29PM +0100, Karol Lewandowski wrote:
>> On 25.01.2012 12:26, Mark Brown wrote:
>
>> However, I still find it little problematic that dt and non-dt
>> versions behave differently when given the same set of parameters
>> (previously apply_uV wasn't the default and required explicit flag).
>
> Well, they're different things.  Device tree isn't Linux specific at
> all.

There is no official platform-agnostic regulator API, nor DT-bindings
document I'm aware of.  Thus, I don't see why, while transitioning to
DT, should we lose ability to describe certain hardware configurations.

On 25.01.2012 12:22, Mark Brown wrote:
 > The big problem there seems like specifying voltages in the first
 > place, if we know what device it is we should already know what's
 > going on.

Driver which handles said regulator might know what's going on, but
that might not be case for its consumers.  Should we limit ability to
query given parameter just because its value is hardcoded in hardware?


My understanding of this whole device tree effort was to provide
ability to describe hardware properties which can't be queried from
hardware itself.

Consequently, if it's property of hardware that it provides fixed
voltage somewhere shouldn't it be possible to describe this fact
in DT?

If device tree isn't good place for it then which one is?

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

[PATCH v2 2/2] regulator: add device tree support for max8997

[Mark Brown]

On Thu, Jan 26, 2012 at 04:28:00PM +0100, Karol Lewandowski wrote:
> On 25.01.2012 14:32, Mark Brown wrote:

> >Well, they're different things.  Device tree isn't Linux specific at
> >all.

> There is no official platform-agnostic regulator API, nor DT-bindings
> document I'm aware of.  Thus, I don't see why, while transitioning to

The binding for the regulator API is supposed to be the one true
platform agnostic binding, none of the device tree stuff is supposed to
be Linux specific.

> DT, should we lose ability to describe certain hardware configurations.

We don't loose anything, a single voltage constraint that didn't set
apply_uV was always meaningless.  I keep meaning to make the core
complain about things like that and people specifying voltage ranges
without SET_VOLTAGE.

> On 25.01.2012 12:22, Mark Brown wrote:
> > The big problem there seems like specifying voltages in the first
> > place, if we know what device it is we should already know what's
> > going on.

> Driver which handles said regulator might know what's going on, but
> that might not be case for its consumers.  Should we limit ability to
> query given parameter just because its value is hardcoded in hardware?

I'm sorry, this makes no sense.  Setting a value in the constraints is
not going to have any impact on the value reported by the driver, it
never has.

> Consequently, if it's property of hardware that it provides fixed
> voltage somewhere shouldn't it be possible to describe this fact
> in DT?

If the device has a fixed voltage output the driver should just know
this without having to read the information from device tree, device
tree is for configuration.  If the device has some hardware fixed
configurability it should define this in a sensible fashion in the
bindings (which may for example be a case of specifying the values of
the passive components for ease of use).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120126/3c99420f/attachment.sig>

[PATCH v2 2/2] regulator: add device tree support for max8997

[Karol Lewandowski]

>> On 25.01.2012 12:22, Mark Brown wrote:
>>> The big problem there seems like specifying voltages in the first
>>> place, if we know what device it is we should already know what's
>>> going on.
>
>> Driver which handles said regulator might know what's going on, but
>> that might not be case for its consumers.  Should we limit ability to
>> query given parameter just because its value is hardcoded in hardware?
>
> I'm sorry, this makes no sense.  Setting a value in the constraints is
> not going to have any impact on the value reported by the driver, it
> never has.

... with the exception of fixed regulator, that is. This is from where I 
got my flawed understanding.

Looking at other drivers I see that's indeed special case not practiced 
elsewhere.

Thanks for explaining this.

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

[PATCH v2 2/2] regulator: add device tree support for max8997

[Mark Brown]

On Fri, Jan 27, 2012 at 10:58:08AM +0100, Karol Lewandowski wrote:
> >>On 25.01.2012 12:22, Mark Brown wrote:

> >I'm sorry, this makes no sense.  Setting a value in the constraints is
> >not going to have any impact on the value reported by the driver, it
> >never has.

> ... with the exception of fixed regulator, that is. This is from
> where I got my flawed understanding.

Note also that this only happens for device tree - with platform data
there's separate platform data for the voltage.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120127/c097b06d/attachment-0001.sig>