[PATCH 0/5] regulator: Enhance AXP209 DT support

[PATCH 0/5] regulator: Enhance AXP209 DT support

[Maxime Ripard]

Hi everyone,

This patchset modifies the regulator core and axp209 regulator driver
to be able to set in each regulators sub-node the supply, that should
be possible, given that it's documented as such in the bindings, but
is not at the moment, since whenever looking up the supply in the DT,
of_get_regulator will always look into the parent's device of_node
pointer.

This leads to a common pattern accross the regulators to have multiple
supply in the main device node, while it would be more intuitive yet
follow the documented bindings to look into the regulator sub-nodes
first.

Any comments are, of course, welcome.
Maxime

Maxime Ripard (5):
  regulator: Allow to pass the device node to regulator_dev_lookup
  regulator: Pass the config device node to regulator_dev_lookup
  regulator: axp20x: Update the bindings to use a local parent regulator
  mfd: axp209x: Drop the parent supplies field
  ARM: sun7i: cubieboard2: Enable the AXP209

 Documentation/devicetree/bindings/mfd/axp20x.txt | 22 +++++--------
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts      | 41 +++++++++++++++++++++++
 drivers/mfd/axp20x.c                             | 11 -------
 drivers/regulator/axp20x-regulator.c             | 42 ++++++++++++++----------
 drivers/regulator/core.c                         | 16 ++++++---
 5 files changed, 87 insertions(+), 45 deletions(-)

-- 
1.9.3

[PATCH 1/5] regulator: Allow to pass the device node to regulator_dev_lookup

[Maxime Ripard]

For now, regulator_dev_lookup only cares about the of_node field of the struct
device that is passed as as argument.

This is actually an issue whenever we have several regulators subnodes in the
DT, that all belong to the same device, which is usually the case.

Add an extra of_node field that is used first in regulator_dev_lookup that
allows to first lookup in the regulator node itself, and then fallback to the
old behaviour.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/regulator/core.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 8b6b28b5cfda..3e3954252a23 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -154,7 +154,7 @@ static struct regulator_dev *of_get_regulator(struct device *dev,
 	}
 
 	list_for_each_entry(r, &regulator_list, list)
-		if (r->dev.parent && node == r->dev.of_node)
+		if (r->dev.parent && regnode == r->dev.of_node)
 			return r;
 
 	return ERR_PTR(-EPROBE_DEFER);
@@ -1264,6 +1264,7 @@ static void regulator_supply_alias(struct device **dev, const char **supply)
 }
 
 static struct regulator_dev *regulator_dev_lookup(struct device *dev,
+						  struct device_node *of_node,
 						  const char *supply,
 						  int *ret)
 {
@@ -1273,7 +1274,14 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 
 	regulator_supply_alias(&dev, &supply);
 
-	/* first do a dt based lookup */
+	/* First, do a lookup against the provided of_node */
+	if (of_node) {
+		r = of_get_regulator(dev, of_node, supply);
+		if (!IS_ERR(r))
+			return r;
+	}
+
+	/* Then, do a lookup based on the device of_node */
 	if (dev && dev->of_node) {
 		r = of_get_regulator(dev, dev->of_node, supply);
 		if (IS_ERR(r)) {
@@ -1332,7 +1340,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 
 	mutex_lock(&regulator_list_mutex);
 
-	rdev = regulator_dev_lookup(dev, id, &ret);
+	rdev = regulator_dev_lookup(dev, NULL, id, &ret);
 	if (rdev)
 		goto found;
 
@@ -3495,7 +3503,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (supply) {
 		struct regulator_dev *r;
 
-		r = regulator_dev_lookup(dev, supply, &ret);
+		r = regulator_dev_lookup(dev, NULL, supply, &ret);
 
 		if (ret == -ENODEV) {
 			/*
-- 
1.9.3

[PATCH 2/5] regulator: Pass the config device node to regulator_dev_lookup

[Maxime Ripard]

For now, regulator registers only looks for parent supplies in the struct
device of_node.

That means that single devices that expose several regulators in the DT have to
tie the parent supplies to the parent device, instead of the regulator itself
like described in the regulator bindings.

The regulator device node is already present in the regulator_config structure
that is passed to regulator_register. Use it to first match in the regulator
node, and then to the device node itself.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/regulator/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3e3954252a23..c8e26be5ea01 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3503,7 +3503,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (supply) {
 		struct regulator_dev *r;
 
-		r = regulator_dev_lookup(dev, NULL, supply, &ret);
+		r = regulator_dev_lookup(dev, config->of_node, supply, &ret);
 
 		if (ret == -ENODEV) {
 			/*
-- 
1.9.3

[PATCH 3/5] regulator: axp20x: Update the bindings to use a local parent regulator

[Maxime Ripard]

Since the regulator core is now able to handle parent regulators that are tied
to the regulator nodes themselves instead of the parent device in the DT,
update the bindings and the regulator code to use a vin-supply property for
each regulator.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 Documentation/devicetree/bindings/mfd/axp20x.txt | 22 +++++--------
 drivers/regulator/axp20x-regulator.c             | 42 ++++++++++++++----------
 2 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/axp20x.txt b/Documentation/devicetree/bindings/mfd/axp20x.txt
index cc9e01b32715..b21164f882ef 100644
--- a/Documentation/devicetree/bindings/mfd/axp20x.txt
+++ b/Documentation/devicetree/bindings/mfd/axp20x.txt
@@ -11,12 +11,6 @@ Required properties:
 - interrupts: Interrupt specifiers for interrupt sources
 - interrupt-controller: axp20x has its own internal IRQs
 - #interrupt-cells: Should be set to 1
-- acin-supply: The input supply for LDO1
-- vin2-supply: The input supply for DCDC2
-- vin3-supply: The input supply for DCDC3
-- ldo24in-supply: The input supply for LDO2, LDO4
-- ldo3in-supply: The input supply for LDO3
-- ldo5in-supply: The input supply for LDO5
 
 - regulators: A node that houses a sub-node for each regulator. The regulators are
 	      bound using their name as listed here: dcdc2, dcdc3, ldo1, ldo2,
@@ -24,6 +18,8 @@ Required properties:
 	      device can be found in:
 	      Documentation/devicetree/bindings/regulator/regulator.txt with
 	      the exception of x-powers,dcdc-freq
+	      Parent regulators must be set in the vin-supply property
+
 - x-powers,dcdc-freq: defines the work frequency of DC-DC in KHz
 		      (range: 750-1875). Default: 1.5MHz
 
@@ -42,49 +38,49 @@ axp209: pmic at 34 {
 	interrupt-controller;
 	#interrupt-cells = <1>;
 
-	acin-supply = <&axp_ipsout_reg>;
-	vin2-supply = <&axp_ipsout_reg>;
-	vin3-supply = <&axp_ipsout_reg>;
-	ldo24in-supply = <&axp_ipsout_reg>;
-	ldo3in-supply = <&axp_ipsout_reg>;
-	ldo5in-supply = <&axp_ipsout_reg>;
-
 	regulators {
 		x-powers,dcdc-freq = <1500>;
 
 		axp_vcore_reg: dcdc2 {
+			vin-supply = <&axp_ipsout_reg>;
 			regulator-min-microvolt = <700000>;
 			regulator-max-microvolt = <2275000>;
 			regulator-always-on;
 		};
 
 		axp_ddr_reg: dcdc3 {
+			vin-supply = <&axp_ipsout_reg>;
 			regulator-min-microvolt = <700000>;
 			regulator-max-microvolt = <3500000>;
 			regulator-always-on;
 		};
 
 		axp_rtc_reg: ldo1 {
+			vin-supply = <&axp_ipsout_reg>;
 			regulator-always-on;
 		};
 
 		axp_analog_reg: ldo2 {
+			vin-supply = <&axp_ipsout_reg>;
 			regulator-min-microvolt = <1800000>;
 			regulator-max-microvolt = <3300000>;
 			regulator-always-on;
 		};
 
 		axp_pll_reg: ldo3 {
+			vin-supply = <&axp_ipsout_reg>;
 			regulator-min-microvolt = <700000>;
 			regulator-max-microvolt = <3500000>;
 		};
 
 		axp_hdmi_reg: ldo4 {
+			vin-supply = <&axp_ipsout_reg>;
 			regulator-min-microvolt = <1250000>;
 			regulator-max-microvolt = <3300000>;
 		};
 
 		axp_mic_reg: ldo5 {
+			vin-supply = <&axp_ipsout_reg>;
 			regulator-min-microvolt = <1800000>;
 			regulator-max-microvolt = <3300000>;
 		};
diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 78a29e60f53a..0a260df50314 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -22,6 +22,7 @@
 #include <linux/regmap.h>
 #include <linux/mfd/axp20x.h>
 #include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include <linux/regulator/of_regulator.h>
 
 #define AXP20X_IO_ENABLED		0x03
@@ -32,11 +33,10 @@
 
 #define AXP20X_FREQ_DCDC_MASK		0x0f
 
-#define AXP20X_DESC_IO(_id, _supply, _min, _max, _step, _vreg, _vmask, _ereg,   \
+#define AXP20X_DESC_IO(_id, _min, _max, _step, _vreg, _vmask, _ereg,		\
 		       _emask, _enable_val, _disable_val)			\
 	[AXP20X_##_id] = {							\
 		.name		= #_id,						\
-		.supply_name	= (_supply),					\
 		.type		= REGULATOR_VOLTAGE,				\
 		.id		= AXP20X_##_id,					\
 		.n_voltages	= (((_max) - (_min)) / (_step) + 1),		\
@@ -52,11 +52,9 @@
 		.ops		= &axp20x_ops,					\
 	}
 
-#define AXP20X_DESC(_id, _supply, _min, _max, _step, _vreg, _vmask, _ereg,	\
-		    _emask) 							\
+#define AXP20X_DESC(_id, _min, _max, _step, _vreg, _vmask, _ereg, _emask)	\
 	[AXP20X_##_id] = {							\
 		.name		= #_id,						\
-		.supply_name	= (_supply),					\
 		.type		= REGULATOR_VOLTAGE,				\
 		.id		= AXP20X_##_id,					\
 		.n_voltages	= (((_max) - (_min)) / (_step) + 1),		\
@@ -70,10 +68,9 @@
 		.ops		= &axp20x_ops,					\
 	}
 
-#define AXP20X_DESC_FIXED(_id, _supply, _volt)					\
+#define AXP20X_DESC_FIXED(_id, _volt)						\
 	[AXP20X_##_id] = {							\
 		.name		= #_id,						\
-		.supply_name	= (_supply),					\
 		.type		= REGULATOR_VOLTAGE,				\
 		.id		= AXP20X_##_id,					\
 		.n_voltages	= 1,						\
@@ -82,10 +79,9 @@
 		.ops		= &axp20x_ops_fixed				\
 	}
 
-#define AXP20X_DESC_TABLE(_id, _supply, _table, _vreg, _vmask, _ereg, _emask)	\
+#define AXP20X_DESC_TABLE(_id, _table, _vreg, _vmask, _ereg, _emask)		\
 	[AXP20X_##_id] = {							\
 		.name		= #_id,						\
-		.supply_name	= (_supply),					\
 		.type		= REGULATOR_VOLTAGE,				\
 		.id		= AXP20X_##_id,					\
 		.n_voltages	= ARRAY_SIZE(_table),				\
@@ -126,18 +122,18 @@ static struct regulator_ops axp20x_ops = {
 };
 
 static const struct regulator_desc axp20x_regulators[] = {
-	AXP20X_DESC(DCDC2, "vin2", 700, 2275, 25, AXP20X_DCDC2_V_OUT, 0x3f,
+	AXP20X_DESC(DCDC2, 700, 2275, 25, AXP20X_DCDC2_V_OUT, 0x3f,
 		    AXP20X_PWR_OUT_CTRL, 0x10),
-	AXP20X_DESC(DCDC3, "vin3", 700, 3500, 25, AXP20X_DCDC3_V_OUT, 0x7f,
+	AXP20X_DESC(DCDC3, 700, 3500, 25, AXP20X_DCDC3_V_OUT, 0x7f,
 		    AXP20X_PWR_OUT_CTRL, 0x02),
-	AXP20X_DESC_FIXED(LDO1, "acin", 1300),
-	AXP20X_DESC(LDO2, "ldo24in", 1800, 3300, 100, AXP20X_LDO24_V_OUT, 0xf0,
+	AXP20X_DESC_FIXED(LDO1, 1300),
+	AXP20X_DESC(LDO2, 1800, 3300, 100, AXP20X_LDO24_V_OUT, 0xf0,
 		    AXP20X_PWR_OUT_CTRL, 0x04),
-	AXP20X_DESC(LDO3, "ldo3in", 700, 3500, 25, AXP20X_LDO3_V_OUT, 0x7f,
+	AXP20X_DESC(LDO3, 700, 3500, 25, AXP20X_LDO3_V_OUT, 0x7f,
 		    AXP20X_PWR_OUT_CTRL, 0x40),
-	AXP20X_DESC_TABLE(LDO4, "ldo24in", axp20x_ldo4_data, AXP20X_LDO24_V_OUT, 0x0f,
+	AXP20X_DESC_TABLE(LDO4, axp20x_ldo4_data, AXP20X_LDO24_V_OUT, 0x0f,
 			  AXP20X_PWR_OUT_CTRL, 0x08),
-	AXP20X_DESC_IO(LDO5, "ldo5in", 1800, 3300, 100, AXP20X_LDO5_V_OUT, 0xf0,
+	AXP20X_DESC_IO(LDO5, 1800, 3300, 100, AXP20X_LDO5_V_OUT, 0xf0,
 		       AXP20X_GPIO0_CTRL, 0x07, AXP20X_IO_ENABLED,
 		       AXP20X_IO_DISABLED),
 };
@@ -181,7 +177,8 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
 static int axp20x_regulator_parse_dt(struct platform_device *pdev)
 {
 	struct device_node *np, *regulators;
-	int ret;
+	struct device_node *child;
+	int ret, i;
 	u32 dcdcfreq;
 
 	np = of_node_get(pdev->dev.parent->of_node);
@@ -207,6 +204,17 @@ static int axp20x_regulator_parse_dt(struct platform_device *pdev)
 			return ret;
 		}
 
+		for_each_child_of_node(regulators, child) {
+			for (i = 0; i < ARRAY_SIZE(axp20x_matches); i++) {
+				struct of_regulator_match *match = &axp20x_matches[i];
+				if (!match->of_node)
+					continue;
+
+				if (of_get_property(child, "vin-supply", NULL))
+					match->init_data->supply_regulator = "vin";
+			}
+		}
+
 		of_node_put(regulators);
 	}
 
-- 
1.9.3

[PATCH 4/5] mfd: axp209x: Drop the parent supplies field

[Maxime Ripard]

Now that the regulator code get its parent supplies purely from the DT, we can
drop the parent supplies resources in the MFD driver.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/mfd/axp20x.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 5734f8c82abf..6231adbb295d 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -140,15 +140,6 @@ static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
 	.init_ack_masked	= true,
 };
 
-static const char * axp20x_supplies[] = {
-	"acin",
-	"vin2",
-	"vin3",
-	"ldo24in",
-	"ldo3in",
-	"ldo5in",
-};
-
 static struct mfd_cell axp20x_cells[] = {
 	{
 		.name			= "axp20x-pek",
@@ -156,8 +147,6 @@ static struct mfd_cell axp20x_cells[] = {
 		.resources		= axp20x_pek_resources,
 	}, {
 		.name			= "axp20x-regulator",
-		.parent_supplies	= axp20x_supplies,
-		.num_parent_supplies	= ARRAY_SIZE(axp20x_supplies),
 	},
 };
 
-- 
1.9.3

[PATCH 5/5] ARM: sun7i: cubieboard2: Enable the AXP209

[Maxime Ripard]

Add the AXP209 PMIC with the regulators in use on the cubie2.

The RTC regulator can be disabled, but since the RTC driver doesn't grab the
regulator for now, the driver wouldn't work anymore.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 41 +++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index a5ad945197e8..1b9983b3f45e 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -76,6 +76,39 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&i2c0_pins_a>;
 			status = "okay";
+
+			axp209: pmic at 34 {
+				compatible = "x-powers,axp209";
+				reg = <0x34>;
+				interrupt-parent = <&nmi_intc>;
+				interrupts = <0 8>;
+
+				interrupt-controller;
+				#interrupt-cells = <1>;
+
+				regulators {
+					x-powers,dcdc-freq = <1500>;
+
+					vdd_cpu: dcdc2 {
+						vin-supply = <&reg_axp_ipsout>;
+						regulator-always-on;
+					};
+
+					vdd_dll: dcdc3 {
+						vin-supply = <&reg_axp_ipsout>;
+						regulator-always-on;
+					};
+
+					vdd_rtc: ldo1 {
+						vin-supply = <&reg_axp_ipsout>;
+					};
+
+					avcc: ldo2 {
+						vin-supply = <&reg_axp_ipsout>;
+						regulator-always-on;
+					};
+				};
+			};
 		};
 
 		i2c1: i2c at 01c2b000 {
@@ -124,4 +157,12 @@
 	reg_usb2_vbus: usb2-vbus {
 		status = "okay";
 	};
+
+	reg_axp_ipsout: axp_ipsout {
+		compatible = "regulator-fixed";
+		regulator-name = "axp-ipsout";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+	};
 };
-- 
1.9.3

[PATCH 0/5] regulator: Enhance AXP209 DT support

[Mark Brown]

On Wed, May 28, 2014 at 07:11:04PM +0200, Maxime Ripard wrote:

> This patchset modifies the regulator core and axp209 regulator driver
> to be able to set in each regulators sub-node the supply, that should
> be possible, given that it's documented as such in the bindings, but

It is?  We should fix that.

> is not at the moment, since whenever looking up the supply in the DT,
> of_get_regulator will always look into the parent's device of_node
> pointer.

> This leads to a common pattern accross the regulators to have multiple
> supply in the main device node, while it would be more intuitive yet
> follow the documented bindings to look into the regulator sub-nodes
> first.

No, we've been round this loop several times before.  This reduces
consistency in how we map supplies since the user has to work out which
subnode the supply is associated with and what it's called there instead
of being able to just look at the schematic and translate the supply
name into a property name.  It also means you have to map supplies into
multiple child nodes if the same supply is used in multiple places.

The idea is that supplies that happen to be used to supply a regulator
don't get treated any differently to any other supply and that we do
that at the physical package level.
-------------- 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/20140528/3f997023/attachment.sig>

[PATCH 3/5] regulator: axp20x: Update the bindings to use a local parent regulator

[Mark Brown]

On Wed, May 28, 2014 at 07:11:07PM +0200, Maxime Ripard wrote:

>  		axp_vcore_reg: dcdc2 {
> +			vin-supply = <&axp_ipsout_reg>;

>  		axp_ddr_reg: dcdc3 {
> +			vin-supply = <&axp_ipsout_reg>;

>  		axp_rtc_reg: ldo1 {
> +			vin-supply = <&axp_ipsout_reg>;

>  		axp_analog_reg: ldo2 {
> +			vin-supply = <&axp_ipsout_reg>;

>  		axp_pll_reg: ldo3 {
> +			vin-supply = <&axp_ipsout_reg>;

>  		axp_hdmi_reg: ldo4 {
> +			vin-supply = <&axp_ipsout_reg>;

>  		axp_mic_reg: ldo5 {
> +			vin-supply = <&axp_ipsout_reg>;

This is saying that every single regulator on the device is powered by a
(presumably) single input called "vin".  This would certainly be an
unusual package design...  As ever the name of the supply should follow
the name of the pin on the device.
-------------- 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/20140528/742d1fba/attachment.sig>

[PATCH 4/5] mfd: axp209x: Drop the parent supplies field

[Lee Jones]

> Now that the regulator code get its parent supplies purely from the DT, we can
> drop the parent supplies resources in the MFD driver.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/mfd/axp20x.c | 11 -----------
>  1 file changed, 11 deletions(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 5734f8c82abf..6231adbb295d 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -140,15 +140,6 @@ static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
>  	.init_ack_masked	= true,
>  };
>  
> -static const char * axp20x_supplies[] = {
> -	"acin",
> -	"vin2",
> -	"vin3",
> -	"ldo24in",
> -	"ldo3in",
> -	"ldo5in",
> -};
> -
>  static struct mfd_cell axp20x_cells[] = {
>  	{
>  		.name			= "axp20x-pek",
> @@ -156,8 +147,6 @@ static struct mfd_cell axp20x_cells[] = {
>  		.resources		= axp20x_pek_resources,
>  	}, {
>  		.name			= "axp20x-regulator",
> -		.parent_supplies	= axp20x_supplies,
> -		.num_parent_supplies	= ARRAY_SIZE(axp20x_supplies),
>  	},
>  };
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 0/5] regulator: Enhance AXP209 DT support

[Maxime Ripard]

On Wed, May 28, 2014 at 07:47:52PM +0100, Mark Brown wrote:
> On Wed, May 28, 2014 at 07:11:04PM +0200, Maxime Ripard wrote:
> 
> > This patchset modifies the regulator core and axp209 regulator driver
> > to be able to set in each regulators sub-node the supply, that should
> > be possible, given that it's documented as such in the bindings, but
> 
> It is?  We should fix that.

>From Documentation/devicetree/bindings/regulator/regulator.txt:

  - <name>-supply: phandle to the parent supply/regulator node

With the example:

    xyzreg: regulator at 0 {
        regulator-min-microvolt = <1000000>;
        regulator-max-microvolt = <2500000>;
        regulator-always-on;
        vin-supply = <&vin>;
    };

If not right, then it's strongly misleading.

> 
> > is not at the moment, since whenever looking up the supply in the DT,
> > of_get_regulator will always look into the parent's device of_node
> > pointer.
> 
> > This leads to a common pattern accross the regulators to have multiple
> > supply in the main device node, while it would be more intuitive yet
> > follow the documented bindings to look into the regulator sub-nodes
> > first.
> 
> No, we've been round this loop several times before.  This reduces
> consistency in how we map supplies since the user has to work out which
> subnode the supply is associated with and what it's called there instead
> of being able to just look at the schematic and translate the supply
> name into a property name.  It also means you have to map supplies into
> multiple child nodes if the same supply is used in multiple places.

Which might be what your schematics actually show. If you have a
single input pin for each regulator, even if the name changes from one
pin to another, you're still pretty much in this kind of construct.

> The idea is that supplies that happen to be used to supply a regulator
> don't get treated any differently to any other supply and that we do
> that at the physical package level.

In our case, each regulator found in the PMIC has an input of its own,
which is a very similar setup than the one found in a gpio-controlled
regulator for example.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140603/73276233/attachment.sig>

[PATCH 3/5] regulator: axp20x: Update the bindings to use a local parent regulator

[Maxime Ripard]

On Wed, May 28, 2014 at 07:50:51PM +0100, Mark Brown wrote:
> On Wed, May 28, 2014 at 07:11:07PM +0200, Maxime Ripard wrote:
> 
> >  		axp_vcore_reg: dcdc2 {
> > +			vin-supply = <&axp_ipsout_reg>;
> 
> >  		axp_ddr_reg: dcdc3 {
> > +			vin-supply = <&axp_ipsout_reg>;
> 
> >  		axp_rtc_reg: ldo1 {
> > +			vin-supply = <&axp_ipsout_reg>;
> 
> >  		axp_analog_reg: ldo2 {
> > +			vin-supply = <&axp_ipsout_reg>;
> 
> >  		axp_pll_reg: ldo3 {
> > +			vin-supply = <&axp_ipsout_reg>;
> 
> >  		axp_hdmi_reg: ldo4 {
> > +			vin-supply = <&axp_ipsout_reg>;
> 
> >  		axp_mic_reg: ldo5 {
> > +			vin-supply = <&axp_ipsout_reg>;
> 
> This is saying that every single regulator on the device is powered by a
> (presumably) single input called "vin".  This would certainly be an
> unusual package design...  As ever the name of the supply should follow
> the name of the pin on the device.

Not exactly. Each regulator has its own input pin, named
<regulator_name>in in the schematics, with the exception of LDO2 and
LDO4 that share the same pin.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140603/9a0c1b06/attachment-0001.sig>

[PATCH 0/5] regulator: Enhance AXP209 DT support

[Mark Brown]

On Tue, Jun 03, 2014 at 03:09:38PM +0200, Maxime Ripard wrote:
> On Wed, May 28, 2014 at 07:47:52PM +0100, Mark Brown wrote:
> > On Wed, May 28, 2014 at 07:11:04PM +0200, Maxime Ripard wrote:

> > > This patchset modifies the regulator core and axp209 regulator driver
> > > to be able to set in each regulators sub-node the supply, that should
> > > be possible, given that it's documented as such in the bindings, but

> > It is?  We should fix that.

> From Documentation/devicetree/bindings/regulator/regulator.txt:

>   - <name>-supply: phandle to the parent supply/regulator node

> With the example:
> 
>     xyzreg: regulator at 0 {
>         regulator-min-microvolt = <1000000>;
>         regulator-max-microvolt = <2500000>;
>         regulator-always-on;
>         vin-supply = <&vin>;
>     };

> If not right, then it's strongly misleading.

That's misleading, the supplies are for the bit of silicon not some
subfunction on it.

> > No, we've been round this loop several times before.  This reduces
> > consistency in how we map supplies since the user has to work out which
> > subnode the supply is associated with and what it's called there instead
> > of being able to just look at the schematic and translate the supply
> > name into a property name.  It also means you have to map supplies into
> > multiple child nodes if the same supply is used in multiple places.

> Which might be what your schematics actually show. If you have a
> single input pin for each regulator, even if the name changes from one
> pin to another, you're still pretty much in this kind of construct.

That's very common, but I do expect that the supplies are all uniquely
named on the silicon so not a big deal and bear in mind that this is
not just about consistency between PMICs but also about consistency
between PMICs and other devices.
-------------- 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/20140603/6dbe7192/attachment-0001.sig>

[PATCH 3/5] regulator: axp20x: Update the bindings to use a local parent regulator

[Mark Brown]

On Tue, Jun 03, 2014 at 03:12:04PM +0200, Maxime Ripard wrote:
> On Wed, May 28, 2014 at 07:50:51PM +0100, Mark Brown wrote:
> > On Wed, May 28, 2014 at 07:11:07PM +0200, Maxime Ripard wrote:

> > >  		axp_mic_reg: ldo5 {
> > > +			vin-supply = <&axp_ipsout_reg>;

> > This is saying that every single regulator on the device is powered by a
> > (presumably) single input called "vin".  This would certainly be an
> > unusual package design...  As ever the name of the supply should follow
> > the name of the pin on the device.

> Not exactly. Each regulator has its own input pin, named
> <regulator_name>in in the schematics, with the exception of LDO2 and
> LDO4 that share the same pin.

OK, so the supplies should be called <regulator_name>in then not vin.
-------------- 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/20140603/311e4ddb/attachment.sig>

[PATCH 3/5] regulator: axp20x: Update the bindings to use a local parent regulator

[Maxime Ripard]

On Tue, Jun 03, 2014 at 03:43:19PM +0100, Mark Brown wrote:
> On Tue, Jun 03, 2014 at 03:12:04PM +0200, Maxime Ripard wrote:
> > On Wed, May 28, 2014 at 07:50:51PM +0100, Mark Brown wrote:
> > > On Wed, May 28, 2014 at 07:11:07PM +0200, Maxime Ripard wrote:
> 
> > > >  		axp_mic_reg: ldo5 {
> > > > +			vin-supply = <&axp_ipsout_reg>;
> 
> > > This is saying that every single regulator on the device is powered by a
> > > (presumably) single input called "vin".  This would certainly be an
> > > unusual package design...  As ever the name of the supply should follow
> > > the name of the pin on the device.
> 
> > Not exactly. Each regulator has its own input pin, named
> > <regulator_name>in in the schematics, with the exception of LDO2 and
> > LDO4 that share the same pin.
> 
> OK, so the supplies should be called <regulator_name>in then not vin.

Which they already do. So I guess we don't have to change the bindings
after all. One last thing I fail to understand though, is why do we
need to have that duplication of the informations between the mfd
driver and the regulator one.

You already list the regulators available and their supply in the
regulator driver, why do you need to set the regulator parents in the
mfd driver as well?

My guess is that it's to work around the fact that
regulator_dev_lookup only looks for the regulator's device of_node (so
not the PMIC one, but one of its child), which doesn't have the supply
properties, and then just falls back on the regulator alias
list. Would it make some sense to add a lookup in the parent device
of_node (which would be the "main" PMIC node in our case)?

Also, there's also the fact that all the supply properties seems to
also be mandatory in the DT, even though the regulator itself might
not be used at all on the board, and the input voltage not wired to
anything.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140605/d2ca14ff/attachment.sig>

[PATCH 3/5] regulator: axp20x: Update the bindings to use a local parent regulator

[Mark Brown]

On Thu, Jun 05, 2014 at 04:27:29PM +0200, Maxime Ripard wrote:

> You already list the regulators available and their supply in the
> regulator driver, why do you need to set the regulator parents in the
> mfd driver as well?

Unless they're being used by the MFD directly there should be no need
for the MFD to know anything about the supplies.

> My guess is that it's to work around the fact that
> regulator_dev_lookup only looks for the regulator's device of_node (so
> not the PMIC one, but one of its child), which doesn't have the supply
> properties, and then just falls back on the regulator alias
> list. Would it make some sense to add a lookup in the parent device
> of_node (which would be the "main" PMIC node in our case)?

This sounds like you are passing the MFD child device into the regulator
API when you should be passing the parent device in.

> Also, there's also the fact that all the supply properties seems to
> also be mandatory in the DT, even though the regulator itself might
> not be used at all on the board, and the input voltage not wired to
> anything.

For electrical engineering reasons it's unlikely that the supplies are
actually floating but yes, they are mandatory.  This is an issue with
registering one device for the entire regulator subsystem on the PMIC,
it interacts somewhat poorly with deferred probe.  However for systems
with full constraints like DT and ACPI ones it should be mostly
sidestepped since the if there is no supply mapped a dummy supply will
be substituted.
-------------- 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/20140605/84478cc3/attachment.sig>

[PATCH 3/5] regulator: axp20x: Update the bindings to use a local parent regulator

[Maxime Ripard]

On Thu, Jun 05, 2014 at 04:49:31PM +0100, Mark Brown wrote:
> On Thu, Jun 05, 2014 at 04:27:29PM +0200, Maxime Ripard wrote:
> 
> > You already list the regulators available and their supply in the
> > regulator driver, why do you need to set the regulator parents in the
> > mfd driver as well?
> 
> Unless they're being used by the MFD directly there should be no need
> for the MFD to know anything about the supplies.

Ok.

> > My guess is that it's to work around the fact that
> > regulator_dev_lookup only looks for the regulator's device of_node (so
> > not the PMIC one, but one of its child), which doesn't have the supply
> > properties, and then just falls back on the regulator alias
> > list. Would it make some sense to add a lookup in the parent device
> > of_node (which would be the "main" PMIC node in our case)?
> 
> This sounds like you are passing the MFD child device into the regulator
> API when you should be passing the parent device in.

We're passing the device coming from the platform_device that is
passed in probe, that has been created by mfd_add_device, which is
indeed the child device from the MFD device. So we should always use
the platform device parent's instead?

> > Also, there's also the fact that all the supply properties seems to
> > also be mandatory in the DT, even though the regulator itself might
> > not be used at all on the board, and the input voltage not wired to
> > anything.
> 
> For electrical engineering reasons it's unlikely that the supplies are
> actually floating but yes, they are mandatory.  This is an issue with
> registering one device for the entire regulator subsystem on the PMIC,
> it interacts somewhat poorly with deferred probe.  However for systems
> with full constraints like DT and ACPI ones it should be mostly
> sidestepped since the if there is no supply mapped a dummy supply will
> be substituted.

Yes, they are actually tied to the ground, but it's still something
meaningless, that I guess shouldn't be expressed in the DT?

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140606/cebe4eac/attachment.sig>

[PATCH 3/5] regulator: axp20x: Update the bindings to use a local parent regulator

[Mark Brown]

On Fri, Jun 06, 2014 at 06:05:52PM +0200, Maxime Ripard wrote:
> On Thu, Jun 05, 2014 at 04:49:31PM +0100, Mark Brown wrote:

> > This sounds like you are passing the MFD child device into the regulator
> > API when you should be passing the parent device in.

> We're passing the device coming from the platform_device that is
> passed in probe, that has been created by mfd_add_device, which is
> indeed the child device from the MFD device. So we should always use
> the platform device parent's instead?

Yes.

> > For electrical engineering reasons it's unlikely that the supplies are
> > actually floating but yes, they are mandatory.  This is an issue with
> > registering one device for the entire regulator subsystem on the PMIC,
> > it interacts somewhat poorly with deferred probe.  However for systems
> > with full constraints like DT and ACPI ones it should be mostly
> > sidestepped since the if there is no supply mapped a dummy supply will
> > be substituted.

> Yes, they are actually tied to the ground, but it's still something
> meaningless, that I guess shouldn't be expressed in the DT?

Probably, that does seem like a valid case for omitting them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140816/54db6531/attachment.sig>