[PATCH v3 0/3] Add device tree support for mc13892 regulator driver

[PATCH v3 0/3] Add device tree support for mc13892 regulator driver

[Shawn Guo]

Changes since v2:
 * Drop '[PATCH v2 2/4] regulator: pass device_node to
   of_get_regulator_init_data()' which has been picked up by Mark
 * Add a note on how driver matches regulator device and the node in
   binding document

Changes since v1:
 * Drop '[PATCH 2/5] regulator: fix label names used in device tree
   bindings' which has been picked up
 * Add binding document for mfd/mc13xxx-core device tree support
 * Fix regulator/fixed.c breakage caused by interface change on
   of_get_regulator_init_data()
 * Reword the commit message of patch #2 a little bit to make the
   reason for changing clear
 * Retrieve the irq from gpio irq domain

Shawn Guo (3):
      mfd: mc13xxx: add device tree probe support
      regulator: mc13892: add device tree probe support
      arm/imx: add mc13892 support into imx51-babbage.dts

 Documentation/devicetree/bindings/mfd/mc13xxx.txt |   53 ++++++++++
 arch/arm/boot/dts/imx51-babbage.dts               |  103 ++++++++++++++++++++-
 drivers/mfd/mc13xxx-core.c                        |  106 +++++++++++++++------
 drivers/regulator/mc13892-regulator.c             |   43 ++++++--
 drivers/regulator/mc13xxx-regulator-core.c        |   57 +++++++++++
 drivers/regulator/mc13xxx.h                       |   20 ++++
 include/linux/mfd/mc13xxx.h                       |    1 +
 7 files changed, 339 insertions(+), 44 deletions(-)

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Shawn Guo]

It adds device tree probe support for mc13xxx mfd driver.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
---
 Documentation/devicetree/bindings/mfd/mc13xxx.txt |   53 ++++++++++
 drivers/mfd/mc13xxx-core.c                        |  106 +++++++++++++++------
 2 files changed, 128 insertions(+), 31 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/mc13xxx.txt

diff --git a/Documentation/devicetree/bindings/mfd/mc13xxx.txt b/Documentation/devicetree/bindings/mfd/mc13xxx.txt
new file mode 100644
index 0000000..4ed46a61
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/mc13xxx.txt
@@ -0,0 +1,53 @@
+* Freescale MC13783/MC13892 Power Management Integrated Circuit (PMIC)
+
+Required properties:
+- compatible : Should be "fsl,mc13783" or "fsl,mc13892"
+
+Optional properties:
+- fsl,mc13xxx-uses-adc : Indicate the ADC is being used
+- fsl,mc13xxx-uses-codec : Indicate the Audio Codec is being used
+- fsl,mc13xxx-uses-rtc : Indicate the RTC is being used
+- fsl,mc13xxx-uses-touch : Indicate the touchscreen controller is being used
+
+Sub-nodes:
+- regulators : Contain the regulator nodes.  The name of regulator node
+  is being used by mc13xxx regulator driver to find the correct relator
+  device.
+
+  The bindings details of individual regulator device can be found in:
+  Documentation/devicetree/bindings/regulator/regulator.txt
+
+Examples:
+
+ecspi at 70010000 { /* ECSPI1 */
+	fsl,spi-num-chipselects = <2>;
+	cs-gpios = <&gpio3 24 0>, /* GPIO4_24 */
+		   <&gpio3 25 0>; /* GPIO4_25 */
+	status = "okay";
+
+	pmic: mc13892 at 0 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "fsl,mc13892";
+		spi-max-frequency = <6000000>;
+		reg = <0>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <8>;
+
+		regulators {
+			sw1_reg: mc13892__sw1 {
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <1375000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			sw2_reg: mc13892__sw2 {
+				regulator-min-microvolt = <900000>;
+				regulator-max-microvolt = <1850000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+	};
+};
diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index e9619ac..51b2dcc 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -18,11 +18,15 @@
 #include <linux/spi/spi.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/mc13xxx.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
 
 struct mc13xxx {
 	struct spi_device *spidev;
 	struct mutex lock;
 	int irq;
+	int flags;
 
 	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
 	void *irqdata[MC13XXX_NUM_IRQ];
@@ -550,10 +554,7 @@ static const char *mc13xxx_get_chipname(struct mc13xxx *mc13xxx)
 
 int mc13xxx_get_flags(struct mc13xxx *mc13xxx)
 {
-	struct mc13xxx_platform_data *pdata =
-		dev_get_platdata(&mc13xxx->spidev->dev);
-
-	return pdata->flags;
+	return mc13xxx->flags;
 }
 EXPORT_SYMBOL(mc13xxx_get_flags);
 
@@ -696,17 +697,67 @@ static int mc13xxx_add_subdevice(struct mc13xxx *mc13xxx, const char *format)
 	return mc13xxx_add_subdevice_pdata(mc13xxx, format, NULL, 0);
 }
 
+#ifdef CONFIG_OF
+static int mc13xxx_probe_flags_dt(struct mc13xxx *mc13xxx)
+{
+	struct device_node *np = mc13xxx->spidev->dev.of_node;
+
+	if (!np)
+		return -ENODEV;
+
+	if (of_get_property(np, "fsl,mc13xxx-uses-adc", NULL))
+		mc13xxx->flags |= MC13XXX_USE_ADC;
+
+	if (of_get_property(np, "fsl,mc13xxx-uses-codec", NULL))
+		mc13xxx->flags |= MC13XXX_USE_CODEC;
+
+	if (of_get_property(np, "fsl,mc13xxx-uses-rtc", NULL))
+		mc13xxx->flags |= MC13XXX_USE_RTC;
+
+	if (of_get_property(np, "fsl,mc13xxx-uses-touch", NULL))
+		mc13xxx->flags |= MC13XXX_USE_TOUCHSCREEN;
+
+	return 0;
+}
+#else
+static inline int mc13xxx_probe_flags_dt(struct mc13xxx *mc13xxx)
+{
+	return -ENODEV;
+}
+#endif
+
+static const struct spi_device_id mc13xxx_device_id[] = {
+	{
+		.name = "mc13783",
+		.driver_data = MC13XXX_ID_MC13783,
+	}, {
+		.name = "mc13892",
+		.driver_data = MC13XXX_ID_MC13892,
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(spi, mc13xxx_device_id);
+
+static const struct of_device_id mc13xxx_dt_ids[] = {
+	{ .compatible = "fsl,mc13783", .data = (void *) MC13XXX_ID_MC13783, },
+	{ .compatible = "fsl,mc13892", .data = (void *) MC13XXX_ID_MC13892, },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mc13xxx_dt_ids);
+
 static int mc13xxx_probe(struct spi_device *spi)
 {
+	const struct of_device_id *of_id;
+	struct spi_driver *sdrv = to_spi_driver(spi->dev.driver);
 	struct mc13xxx *mc13xxx;
 	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
 	enum mc13xxx_id id;
 	int ret;
 
-	if (!pdata) {
-		dev_err(&spi->dev, "invalid platform data\n");
-		return -EINVAL;
-	}
+	of_id = of_match_device(mc13xxx_dt_ids, &spi->dev);
+	if (of_id)
+		sdrv->id_table = &mc13xxx_device_id[(enum mc13xxx_id) of_id->data];
 
 	mc13xxx = kzalloc(sizeof(*mc13xxx), GFP_KERNEL);
 	if (!mc13xxx)
@@ -749,28 +800,33 @@ err_revision:
 
 	mc13xxx_unlock(mc13xxx);
 
-	if (pdata->flags & MC13XXX_USE_ADC)
+	if (mc13xxx_probe_flags_dt(mc13xxx) < 0 && pdata)
+		mc13xxx->flags = pdata->flags;
+
+	if (mc13xxx->flags & MC13XXX_USE_ADC)
 		mc13xxx_add_subdevice(mc13xxx, "%s-adc");
 
-	if (pdata->flags & MC13XXX_USE_CODEC)
+	if (mc13xxx->flags & MC13XXX_USE_CODEC)
 		mc13xxx_add_subdevice(mc13xxx, "%s-codec");
 
-	mc13xxx_add_subdevice_pdata(mc13xxx, "%s-regulator",
-		&pdata->regulators, sizeof(pdata->regulators));
-
-	if (pdata->flags & MC13XXX_USE_RTC)
+	if (mc13xxx->flags & MC13XXX_USE_RTC)
 		mc13xxx_add_subdevice(mc13xxx, "%s-rtc");
 
-	if (pdata->flags & MC13XXX_USE_TOUCHSCREEN)
+	if (mc13xxx->flags & MC13XXX_USE_TOUCHSCREEN)
 		mc13xxx_add_subdevice(mc13xxx, "%s-ts");
 
-	if (pdata->leds)
+	if (pdata) {
+		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-regulator",
+			&pdata->regulators, sizeof(pdata->regulators));
 		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led",
 				pdata->leds, sizeof(*pdata->leds));
-
-	if (pdata->buttons)
 		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-pwrbutton",
 				pdata->buttons, sizeof(*pdata->buttons));
+	} else {
+		mc13xxx_add_subdevice(mc13xxx, "%s-regulator");
+		mc13xxx_add_subdevice(mc13xxx, "%s-led");
+		mc13xxx_add_subdevice(mc13xxx, "%s-pwrbutton");
+	}
 
 	return 0;
 }
@@ -788,25 +844,13 @@ static int __devexit mc13xxx_remove(struct spi_device *spi)
 	return 0;
 }
 
-static const struct spi_device_id mc13xxx_device_id[] = {
-	{
-		.name = "mc13783",
-		.driver_data = MC13XXX_ID_MC13783,
-	}, {
-		.name = "mc13892",
-		.driver_data = MC13XXX_ID_MC13892,
-	}, {
-		/* sentinel */
-	}
-};
-MODULE_DEVICE_TABLE(spi, mc13xxx_device_id);
-
 static struct spi_driver mc13xxx_driver = {
 	.id_table = mc13xxx_device_id,
 	.driver = {
 		.name = "mc13xxx",
 		.bus = &spi_bus_type,
 		.owner = THIS_MODULE,
+		.of_match_table = mc13xxx_dt_ids,
 	},
 	.probe = mc13xxx_probe,
 	.remove = __devexit_p(mc13xxx_remove),
-- 
1.7.4.1

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Mark Brown]

On Mon, Dec 12, 2011 at 11:15:56PM +0800, Shawn Guo wrote:

> +Sub-nodes:
> +- regulators : Contain the regulator nodes.  The name of regulator node
> +  is being used by mc13xxx regulator driver to find the correct relator
> +  device.

Reading this I'm not clear what the name of the node is, or what the
valid node names are.

> +			sw1_reg: mc13892__sw1 {

The examples don't really elucidate this either (and the __ is rather
odd).

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Shawn Guo]

On Tue, Dec 20, 2011 at 12:57:09AM +0000, Mark Brown wrote:
> On Mon, Dec 12, 2011 at 11:15:56PM +0800, Shawn Guo wrote:
> 
> > +Sub-nodes:
> > +- regulators : Contain the regulator nodes.  The name of regulator node
> > +  is being used by mc13xxx regulator driver to find the correct relator
> > +  device.
> 
> Reading this I'm not clear what the name of the node is, or what the
> valid node names are.
> 
In the example below, the name would be 'mc13892__sw1'.

> > +			sw1_reg: mc13892__sw1 {
> 
> The examples don't really elucidate this either (and the __ is rather
> odd).

Yes, that's because the name defined by mc13892 regulator driver is odd.

There is a '_' in 'MC13892_' when using MC13xxx_DEFINE below.

#define MC13892_SW_DEFINE(name, reg, vsel_reg, voltages)        \
        MC13xxx_DEFINE(MC13892_, name, reg, vsel_reg, voltages, \
                        mc13892_sw_regulator_ops)

And the second one comes from the definition of .name below.

#define MC13xxx_DEFINE(prefix, _name, _reg, _vsel_reg, _voltages, _ops) \
        [prefix ## _name] = {                           \
                .desc = {                                               \
                        .name = #prefix "_" #_name,                     \

Using the regulator name to find the correct device is the approach I
can see that brings the least churn to the existing driver.  You may
think the regulator name is Linux mc13892 regulator driver specific.
But I think it should be something from hardware manual and should be
used in driver and device tree consistently.  Unfortunately, the driver
created an odd name first, and we have to use that name in device tree
to minimize the diff stat.

-- 
Regards,
Shawn

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Mark Brown]

On Tue, Dec 20, 2011 at 10:01:02AM +0800, Shawn Guo wrote:
> On Tue, Dec 20, 2011 at 12:57:09AM +0000, Mark Brown wrote:
> > On Mon, Dec 12, 2011 at 11:15:56PM +0800, Shawn Guo wrote:

> > > +Sub-nodes:
> > > +- regulators : Contain the regulator nodes.  The name of regulator node
> > > +  is being used by mc13xxx regulator driver to find the correct relator
> > > +  device.

> > Reading this I'm not clear what the name of the node is, or what the
> > valid node names are.

> In the example below, the name would be 'mc13892__sw1'.

You're missing the point - someone reading the documentation needs to be
able to figure out what strings they need to use for all the different
regulators on the chip are without groveling through the driver code.

> > > +			sw1_reg: mc13892__sw1 {

> > The examples don't really elucidate this either (and the __ is rather
> > odd).

> Yes, that's because the name defined by mc13892 regulator driver is odd.

That's *not* something that we should be exposing in the device tree
bindings.  This is an implementation detail of the Linux driver which
might well change in future.

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Shawn Guo]

On Tue, Dec 20, 2011 at 01:59:32AM +0000, Mark Brown wrote:
> On Tue, Dec 20, 2011 at 10:01:02AM +0800, Shawn Guo wrote:
> > On Tue, Dec 20, 2011 at 12:57:09AM +0000, Mark Brown wrote:
> > > On Mon, Dec 12, 2011 at 11:15:56PM +0800, Shawn Guo wrote:
> 
> > > > +Sub-nodes:
> > > > +- regulators : Contain the regulator nodes.  The name of regulator node
> > > > +  is being used by mc13xxx regulator driver to find the correct relator
> > > > +  device.
> 
> > > Reading this I'm not clear what the name of the node is, or what the
> > > valid node names are.
> 
> > In the example below, the name would be 'mc13892__sw1'.
> 
> You're missing the point - someone reading the documentation needs to be
> able to figure out what strings they need to use for all the different
> regulators on the chip are without groveling through the driver code.
> 
> > > > +			sw1_reg: mc13892__sw1 {
> 
> > > The examples don't really elucidate this either (and the __ is rather
> > > odd).
> 
> > Yes, that's because the name defined by mc13892 regulator driver is odd.
> 
> That's *not* something that we should be exposing in the device tree
> bindings.  This is an implementation detail of the Linux driver which
> might well change in future.
> 
Any suggestion on a better binding for mc13892 regulator device?

-- 
Regards,
Shawn

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Mark Brown]

On Tue, Dec 20, 2011 at 11:03:48AM +0800, Shawn Guo wrote:
> On Tue, Dec 20, 2011 at 01:59:32AM +0000, Mark Brown wrote:

> > You're missing the point - someone reading the documentation needs to be
> > able to figure out what strings they need to use for all the different
> > regulators on the chip are without groveling through the driver code.

...

> > That's *not* something that we should be exposing in the device tree
> > bindings.  This is an implementation detail of the Linux driver which
> > might well change in future.

> Any suggestion on a better binding for mc13892 regulator device?

Well, removing the random extra _s would be a big start (though I'd just
drop the chip name entirely from the name of the regulators since by the
time we're looking at the regulator we've already identified the chip)
and as I keep saying you need to document what the names mean - what are
the possible names and how do they map onto the hardware?

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Shawn Guo]

On Tue, Dec 20, 2011 at 11:25:11AM +0000, Mark Brown wrote:
> On Tue, Dec 20, 2011 at 11:03:48AM +0800, Shawn Guo wrote:
> > On Tue, Dec 20, 2011 at 01:59:32AM +0000, Mark Brown wrote:
> 
> > > You're missing the point - someone reading the documentation needs to be
> > > able to figure out what strings they need to use for all the different
> > > regulators on the chip are without groveling through the driver code.
> 
> ...
> 
> > > That's *not* something that we should be exposing in the device tree
> > > bindings.  This is an implementation detail of the Linux driver which
> > > might well change in future.
> 
> > Any suggestion on a better binding for mc13892 regulator device?
> 
> Well, removing the random extra _s would be a big start (though I'd just
> drop the chip name entirely from the name of the regulators since by the
> time we're looking at the regulator we've already identified the chip)
> and as I keep saying you need to document what the names mean - what are
> the possible names and how do they map onto the hardware?
> 
I just came up with an idea which can totally avoid matching name.  It
seems that we can identify a regulator using register plus enable bit,
which is basically 'reg' and 'enable_bit' in 'mc13xxx_regulator'.  As
these data must be coming from hardware manual, they should be stable
enough for binding a regulator.  What do you think?

-- 
Regards,
Shawn

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Mark Brown]

On Tue, Dec 20, 2011 at 09:52:52PM +0800, Shawn Guo wrote:
> On Tue, Dec 20, 2011 at 11:25:11AM +0000, Mark Brown wrote:

> > Well, removing the random extra _s would be a big start (though I'd just
> > drop the chip name entirely from the name of the regulators since by the
> > time we're looking at the regulator we've already identified the chip)
> > and as I keep saying you need to document what the names mean - what are
> > the possible names and how do they map onto the hardware?

> I just came up with an idea which can totally avoid matching name.  It
> seems that we can identify a regulator using register plus enable bit,
> which is basically 'reg' and 'enable_bit' in 'mc13xxx_regulator'.  As
> these data must be coming from hardware manual, they should be stable
> enough for binding a regulator.  What do you think?

I don't know that that helps, register numbers and shifts aren't going
to do anything for legibility.  The problem with the names was that the
names chosen were poorly defined (why call DCDCn chipname__dcdcn?) and
not documented in the binding, not that they were names.

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Shawn Guo]

On Tue, Dec 20, 2011 at 02:35:58PM +0000, Mark Brown wrote:
> On Tue, Dec 20, 2011 at 09:52:52PM +0800, Shawn Guo wrote:
> > On Tue, Dec 20, 2011 at 11:25:11AM +0000, Mark Brown wrote:
> 
> > > Well, removing the random extra _s would be a big start (though I'd just
> > > drop the chip name entirely from the name of the regulators since by the
> > > time we're looking at the regulator we've already identified the chip)
> > > and as I keep saying you need to document what the names mean - what are
> > > the possible names and how do they map onto the hardware?
> 
> > I just came up with an idea which can totally avoid matching name.  It
> > seems that we can identify a regulator using register plus enable bit,
> > which is basically 'reg' and 'enable_bit' in 'mc13xxx_regulator'.  As
> > these data must be coming from hardware manual, they should be stable
> > enough for binding a regulator.  What do you think?
> 
> I don't know that that helps, register numbers and shifts aren't going
> to do anything for legibility.

I think it helps.  It's not about legibility but identification.  For
mc13892 driver to work, it has to match the register number and enable
bit shift given by mc13892 reference manual.  So register number +
enable bit shift is stable and unique to identify a mc13892 regulator.

> The problem with the names was that the
> names chosen were poorly defined (why call DCDCn chipname__dcdcn?) and
> not documented in the binding, not that they were names.
> 
The problem with name is it's name.  The mc13892 driver can still work
with naming regulator differently from mc13892 reference manual.  So
using name to bind a regulator means we are binding with Linux mc13892
driver.  In that case, DT probe works if and only if the name in dts
matches the name used in mc13892 driver, and it does not work as long
as the name dts does not match the name used in mc13892 driver.

Yes, I flipped and now think using name to bind is a bad idea, no
matter how we document it.

-- 
Regards,
Shawn

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Mark Brown]

On Tue, Dec 20, 2011 at 11:31:03PM +0800, Shawn Guo wrote:
> On Tue, Dec 20, 2011 at 02:35:58PM +0000, Mark Brown wrote:

> > I don't know that that helps, register numbers and shifts aren't going
> > to do anything for legibility.

> I think it helps.  It's not about legibility but identification.  For
> mc13892 driver to work, it has to match the register number and enable
> bit shift given by mc13892 reference manual.  So register number +
> enable bit shift is stable and unique to identify a mc13892 regulator.

It's unique but I feel the disadvantages in terms of legibility of the
resulting device trees are substantial - we want humans to be able to
read and write device trees, preferrably without having to dig out the
datasheet for the part.  So long as the names are reasonably sensible
and can be understood in the case of any lack of clarity we should be
OK.

> > The problem with the names was that the
> > names chosen were poorly defined (why call DCDCn chipname__dcdcn?) and
> > not documented in the binding, not that they were names.

> The problem with name is it's name.  The mc13892 driver can still work
> with naming regulator differently from mc13892 reference manual.  So
> using name to bind a regulator means we are binding with Linux mc13892
> driver.  In that case, DT probe works if and only if the name in dts
> matches the name used in mc13892 driver, and it does not work as long
> as the name dts does not match the name used in mc13892 driver.

This is the whole reason why I'm saying that you need to define the
names used in the binding - if the names are a defined part of the
binding then there's nothing driver specific about them.

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Shawn Guo]

On Tue, Dec 20, 2011 at 11:25:27PM +0000, Mark Brown wrote:
> On Tue, Dec 20, 2011 at 11:31:03PM +0800, Shawn Guo wrote:
> > On Tue, Dec 20, 2011 at 02:35:58PM +0000, Mark Brown wrote:
> 
> > > I don't know that that helps, register numbers and shifts aren't going
> > > to do anything for legibility.
> 
> > I think it helps.  It's not about legibility but identification.  For
> > mc13892 driver to work, it has to match the register number and enable
> > bit shift given by mc13892 reference manual.  So register number +
> > enable bit shift is stable and unique to identify a mc13892 regulator.
> 
> It's unique but I feel the disadvantages in terms of legibility of the
> resulting device trees are substantial - we want humans to be able to
> read and write device trees, preferrably without having to dig out the
> datasheet for the part.

Eh, device tree is all about describing hardware.  I do not understand
what is wrong with looking at hardware manual when writing dts.

> So long as the names are reasonably sensible
> and can be understood in the case of any lack of clarity we should be
> OK.
> 
The problem is even if we have the name defined that way, it has to be
matched to the name used in mc13892 driver somehow, if we want to use
name as the key to find the regulator defined in mc13892 driver as
array mc13892_regulators[].

> > > The problem with the names was that the
> > > names chosen were poorly defined (why call DCDCn chipname__dcdcn?) and
> > > not documented in the binding, not that they were names.
> 
> > The problem with name is it's name.  The mc13892 driver can still work
> > with naming regulator differently from mc13892 reference manual.  So
> > using name to bind a regulator means we are binding with Linux mc13892
> > driver.  In that case, DT probe works if and only if the name in dts
> > matches the name used in mc13892 driver, and it does not work as long
> > as the name dts does not match the name used in mc13892 driver.
> 
> This is the whole reason why I'm saying that you need to define the
> names used in the binding - if the names are a defined part of the
> binding then there's nothing driver specific about them.
> 
Can you please help me understand how this can be achieved with an
example?  Taking 'mc13892__sw1' as the example, if I read your comment
before correctly, 'sw1' is the name that you are suggesting.  How this
name can be nothing driver specific?  After all, we need to find
regulator MC13892_SW1 defined in mc13892_regulators[] by matching name
'sw1' defined by binding and name 'MC13892_SW1' defined by driver
somehow, if we want to use name to bind the regulator.

I feel I might miss your point again.  So please help clarify.

-- 
Regards,
Shawn

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Mark Brown]

On Wed, Dec 21, 2011 at 09:25:21AM +0800, Shawn Guo wrote:
> On Tue, Dec 20, 2011 at 11:25:27PM +0000, Mark Brown wrote:

> > It's unique but I feel the disadvantages in terms of legibility of the
> > resulting device trees are substantial - we want humans to be able to
> > read and write device trees, preferrably without having to dig out the
> > datasheet for the part.

> Eh, device tree is all about describing hardware.  I do not understand
> what is wrong with looking at hardware manual when writing dts.

For the same reason we define constants for register names and values
when writing drivers - so humans can make sense of what's in front of
them and to reduce the chance of errors when things are being written.

> > So long as the names are reasonably sensible
> > and can be understood in the case of any lack of clarity we should be
> > OK.

> The problem is even if we have the name defined that way, it has to be
> matched to the name used in mc13892 driver somehow, if we want to use
> name as the key to find the regulator defined in mc13892 driver as
> array mc13892_regulators[].

This doesn't seem like a problem - the most obvious thing would just be
to adjust the strings in the code to correspond to what the device tree
binding says.

> > This is the whole reason why I'm saying that you need to define the
> > names used in the binding - if the names are a defined part of the
> > binding then there's nothing driver specific about them.

> Can you please help me understand how this can be achieved with an
> example?  Taking 'mc13892__sw1' as the example, if I read your comment
> before correctly, 'sw1' is the name that you are suggesting.  How this
> name can be nothing driver specific?  After all, we need to find
> regulator MC13892_SW1 defined in mc13892_regulators[] by matching name
> 'sw1' defined by binding and name 'MC13892_SW1' defined by driver
> somehow, if we want to use name to bind the regulator.

Your binding document would say something like "the regulators are bound
using their names as listed below with their enable bits:

  sw1 - SW1 regulator (register X bit Y)

" so in the binding document it would say what strings map onto which
regulators.  Then any driver implementing that binding would pick the
same strings.

[PATCH v3 1/3] mfd: mc13xxx: add device tree probe support

[Shawn Guo]

On Wed, Dec 21, 2011 at 01:37:03AM +0000, Mark Brown wrote:
> On Wed, Dec 21, 2011 at 09:25:21AM +0800, Shawn Guo wrote:
> > The problem is even if we have the name defined that way, it has to be
> > matched to the name used in mc13892 driver somehow, if we want to use
> > name as the key to find the regulator defined in mc13892 driver as
> > array mc13892_regulators[].
> 
> This doesn't seem like a problem - the most obvious thing would just be
> to adjust the strings in the code to correspond to what the device tree
> binding says.
> 
Okay, that's it.  Something obvious to you is not obvious to me :)
As I'm migrating an existing driver to be DT aware, I'm trying to avoid
changing any non-dt code.  Your suggestion would be obvious to me if I'm
writing a new driver that only supports DT.

Anyway, I got your point, and will follow that since you seems fine
with changing non-dt code to adopt dt support.

-- 
Regards,
Shawn

[PATCH v3 2/3] regulator: mc13892: add device tree probe support

[Shawn Guo]

It adds device tree probe support for mc13892-regulator driver.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
---
 drivers/regulator/mc13892-regulator.c      |   43 +++++++++++++++-----
 drivers/regulator/mc13xxx-regulator-core.c |   57 ++++++++++++++++++++++++++++
 drivers/regulator/mc13xxx.h                |   20 ++++++++++
 include/linux/mfd/mc13xxx.h                |    1 +
 4 files changed, 110 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/mc13892-regulator.c b/drivers/regulator/mc13892-regulator.c
index 2824804..46bfa4a 100644
--- a/drivers/regulator/mc13892-regulator.c
+++ b/drivers/regulator/mc13892-regulator.c
@@ -527,18 +527,27 @@ static int __devinit mc13892_regulator_probe(struct platform_device *pdev)
 	struct mc13xxx *mc13892 = dev_get_drvdata(pdev->dev.parent);
 	struct mc13xxx_regulator_platform_data *pdata =
 		dev_get_platdata(&pdev->dev);
-	struct mc13xxx_regulator_init_data *init_data;
+	struct mc13xxx_regulator_init_data *mc13xxx_data;
 	int i, ret;
+	int num_regulators = 0;
 	u32 val;
 
+	num_regulators = mc13xxx_get_num_regulators_dt(pdev);
+	if (num_regulators <= 0 && pdata)
+		num_regulators = pdata->num_regulators;
+	if (num_regulators <= 0)
+		return -EINVAL;
+
 	priv = kzalloc(sizeof(*priv) +
-		pdata->num_regulators * sizeof(priv->regulators[0]),
+		num_regulators * sizeof(priv->regulators[0]),
 		GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
+	priv->num_regulators = num_regulators;
 	priv->mc13xxx_regulators = mc13892_regulators;
 	priv->mc13xxx = mc13892;
+	platform_set_drvdata(pdev, priv);
 
 	mc13xxx_lock(mc13892);
 	ret = mc13xxx_reg_read(mc13892, MC13892_REVISION, &val);
@@ -569,11 +578,27 @@ static int __devinit mc13892_regulator_probe(struct platform_device *pdev)
 		= mc13892_vcam_set_mode;
 	mc13892_regulators[MC13892_VCAM].desc.ops->get_mode
 		= mc13892_vcam_get_mode;
-	for (i = 0; i < pdata->num_regulators; i++) {
-		init_data = &pdata->regulators[i];
+
+	mc13xxx_data = mc13xxx_parse_regulators_dt(pdev, mc13892_regulators,
+					ARRAY_SIZE(mc13892_regulators));
+	for (i = 0; i < num_regulators; i++) {
+		struct regulator_init_data *init_data;
+		struct regulator_desc *desc;
+		struct device_node *node = NULL;
+		int id;
+
+		if (mc13xxx_data) {
+			id = mc13xxx_data[i].id;
+			init_data = mc13xxx_data[i].init_data;
+			node = mc13xxx_data[i].node;
+		} else {
+			id = pdata->regulators[i].id;
+			init_data = pdata->regulators[i].init_data;
+		}
+		desc = &mc13892_regulators[id].desc;
+
 		priv->regulators[i] = regulator_register(
-			&mc13892_regulators[init_data->id].desc,
-			&pdev->dev, init_data->init_data, priv, NULL);
+			desc, &pdev->dev, init_data, priv, node);
 
 		if (IS_ERR(priv->regulators[i])) {
 			dev_err(&pdev->dev, "failed to register regulator %s\n",
@@ -583,8 +608,6 @@ static int __devinit mc13892_regulator_probe(struct platform_device *pdev)
 		}
 	}
 
-	platform_set_drvdata(pdev, priv);
-
 	return 0;
 err:
 	while (--i >= 0)
@@ -600,13 +623,11 @@ err_free:
 static int __devexit mc13892_regulator_remove(struct platform_device *pdev)
 {
 	struct mc13xxx_regulator_priv *priv = platform_get_drvdata(pdev);
-	struct mc13xxx_regulator_platform_data *pdata =
-		dev_get_platdata(&pdev->dev);
 	int i;
 
 	platform_set_drvdata(pdev, NULL);
 
-	for (i = 0; i < pdata->num_regulators; i++)
+	for (i = 0; i < priv->num_regulators; i++)
 		regulator_unregister(priv->regulators[i]);
 
 	kfree(priv);
diff --git a/drivers/regulator/mc13xxx-regulator-core.c b/drivers/regulator/mc13xxx-regulator-core.c
index 6532853..80ecafe 100644
--- a/drivers/regulator/mc13xxx-regulator-core.c
+++ b/drivers/regulator/mc13xxx-regulator-core.c
@@ -18,12 +18,14 @@
 #include <linux/mfd/mc13xxx.h>
 #include <linux/regulator/machine.h>
 #include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
 #include <linux/platform_device.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/err.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include "mc13xxx.h"
 
 static int mc13xxx_regulator_enable(struct regulator_dev *rdev)
@@ -236,6 +238,61 @@ int mc13xxx_sw_regulator_is_enabled(struct regulator_dev *rdev)
 }
 EXPORT_SYMBOL_GPL(mc13xxx_sw_regulator_is_enabled);
 
+#ifdef CONFIG_OF
+int __devinit mc13xxx_get_num_regulators_dt(struct platform_device *pdev)
+{
+	struct device_node *parent, *child;
+	int num = 0;
+
+	of_node_get(pdev->dev.parent->of_node);
+	parent = of_find_node_by_name(pdev->dev.parent->of_node, "regulators");
+	if (!parent)
+		return -ENODEV;
+
+	for_each_child_of_node(parent, child)
+		num++;
+
+	return num;
+}
+
+struct mc13xxx_regulator_init_data * __devinit mc13xxx_parse_regulators_dt(
+	struct platform_device *pdev, struct mc13xxx_regulator *regulators,
+	int num_regulators)
+{
+	struct mc13xxx_regulator_priv *priv = platform_get_drvdata(pdev);
+	struct mc13xxx_regulator_init_data *data, *p;
+	struct device_node *parent, *child;
+	int i;
+
+	of_node_get(pdev->dev.parent->of_node);
+	parent = of_find_node_by_name(pdev->dev.parent->of_node, "regulators");
+	if (!parent)
+		return NULL;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data) * priv->num_regulators,
+			    GFP_KERNEL);
+	if (!data)
+		return NULL;
+	p = data;
+
+	for_each_child_of_node(parent, child) {
+		for (i = 0; i < num_regulators; i++) {
+			if (!of_node_cmp(child->name,
+					 regulators[i].desc.name)) {
+				p->id = i;
+				p->init_data = of_get_regulator_init_data(
+							&pdev->dev, child);
+				p->node = child;
+				p++;
+				break;
+			}
+		}
+	}
+
+	return data;
+}
+#endif
+
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Yong Shen <yong.shen@linaro.org>");
 MODULE_DESCRIPTION("Regulator Driver for Freescale MC13xxx PMIC");
diff --git a/drivers/regulator/mc13xxx.h b/drivers/regulator/mc13xxx.h
index 2775826..419aee5 100644
--- a/drivers/regulator/mc13xxx.h
+++ b/drivers/regulator/mc13xxx.h
@@ -29,6 +29,7 @@ struct mc13xxx_regulator_priv {
 	struct mc13xxx *mc13xxx;
 	u32 powermisc_pwgt_state;
 	struct mc13xxx_regulator *mc13xxx_regulators;
+	int num_regulators;
 	struct regulator_dev *regulators[];
 };
 
@@ -42,6 +43,25 @@ extern int mc13xxx_fixed_regulator_set_voltage(struct regulator_dev *rdev,
 		int min_uV, int max_uV, unsigned *selector);
 extern int mc13xxx_fixed_regulator_get_voltage(struct regulator_dev *rdev);
 
+#ifdef CONFIG_OF
+extern int mc13xxx_get_num_regulators_dt(struct platform_device *pdev);
+extern struct mc13xxx_regulator_init_data *mc13xxx_parse_regulators_dt(
+	struct platform_device *pdev, struct mc13xxx_regulator *regulators,
+	int num_regulators);
+#else
+static inline int mc13xxx_get_num_regulators_dt(struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+
+static inline struct mc13xxx_regulator_init_data *mc13xxx_parse_regulators_dt(
+	struct platform_device *pdev, struct mc13xxx_regulator *regulators,
+	int num_regulators)
+{
+	return NULL;
+}
+#endif
+
 extern struct regulator_ops mc13xxx_regulator_ops;
 extern struct regulator_ops mc13xxx_fixed_regulator_ops;
 
diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
index 3816c2f..a98e2a3 100644
--- a/include/linux/mfd/mc13xxx.h
+++ b/include/linux/mfd/mc13xxx.h
@@ -69,6 +69,7 @@ struct regulator_init_data;
 struct mc13xxx_regulator_init_data {
 	int id;
 	struct regulator_init_data *init_data;
+	struct device_node *node;
 };
 
 struct mc13xxx_regulator_platform_data {
-- 
1.7.4.1

[PATCH v3 2/3] regulator: mc13892: add device tree probe support

[Shawn Guo]

On Mon, Dec 12, 2011 at 11:15:57PM +0800, Shawn Guo wrote:
> It adds device tree probe support for mc13892-regulator driver.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Liam Girdwood <lrg@ti.com>

Ping?

-- 
Regards,
Shawn

[PATCH v3 2/3] regulator: mc13892: add device tree probe support

[Mark Brown]

On Mon, Dec 19, 2011 at 10:07:31PM +0800, Shawn Guo wrote:
> On Mon, Dec 12, 2011 at 11:15:57PM +0800, Shawn Guo wrote:
> > It adds device tree probe support for mc13892-regulator driver.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > Cc: Liam Girdwood <lrg@ti.com>
> 
> Ping?

I can't really do anything with this without the MFD portion being OK.

[PATCH v3 2/3] regulator: mc13892: add device tree probe support

[Shawn Guo]

On Tue, Dec 20, 2011 at 12:54:15AM +0000, Mark Brown wrote:
> On Mon, Dec 19, 2011 at 10:07:31PM +0800, Shawn Guo wrote:
> > On Mon, Dec 12, 2011 at 11:15:57PM +0800, Shawn Guo wrote:
> > > It adds device tree probe support for mc13892-regulator driver.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > > Cc: Liam Girdwood <lrg@ti.com>
> > 
> > Ping?
> 
> I can't really do anything with this without the MFD portion being OK.
> 
It seems that Samuel has applied it.

http://article.gmane.org/gmane.linux.drivers.devicetree/10441

-- 
Regards,
Shawn

[PATCH v3 2/3] regulator: mc13892: add device tree probe support

[Mark Brown]

On Tue, Dec 20, 2011 at 09:37:12AM +0800, Shawn Guo wrote:

> It seems that Samuel has applied it.

> http://article.gmane.org/gmane.linux.drivers.devicetree/10441

Oh, that's a bit unfortunate as looking at the first patch I'm still not
happy I understand what the regulator part of the binding is supposed to
be...

[PATCH v3 2/3] regulator: mc13892: add device tree probe support

[Shawn Guo]

On Tue, Dec 20, 2011 at 01:34:25AM +0000, Mark Brown wrote:
> On Tue, Dec 20, 2011 at 09:37:12AM +0800, Shawn Guo wrote:
> 
> > It seems that Samuel has applied it.
> 
> > http://article.gmane.org/gmane.linux.drivers.devicetree/10441
> 
> Oh, that's a bit unfortunate as looking at the first patch I'm still not
> happy I understand what the regulator part of the binding is supposed to
> be...
> 
I will be very happy to send any incremental patch for anything that we
agree to improve.

-- 
Regards,
Shawn

[PATCH v3 3/3] arm/imx: add mc13892 support into imx51-babbage.dts

[Shawn Guo]

It adds mc13892 support into imx51-babbage device tree source.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/boot/dts/imx51-babbage.dts |  103 ++++++++++++++++++++++++++++++++++-
 1 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
index f8766af..407fc0b 100644
--- a/arch/arm/boot/dts/imx51-babbage.dts
+++ b/arch/arm/boot/dts/imx51-babbage.dts
@@ -31,12 +31,14 @@
 				esdhc at 70004000 { /* ESDHC1 */
 					fsl,cd-internal;
 					fsl,wp-internal;
+					vmmc-supply = <&vsd_reg>;
 					status = "okay";
 				};
 
 				esdhc at 70008000 { /* ESDHC2 */
 					cd-gpios = <&gpio0 6 0>; /* GPIO1_6 */
 					wp-gpios = <&gpio0 5 0>; /* GPIO1_5 */
+					vmmc-supply = <&vsd_reg>;
 					status = "okay";
 				};
 
@@ -57,8 +59,105 @@
 						compatible = "fsl,mc13892";
 						spi-max-frequency = <6000000>;
 						reg = <0>;
-						mc13xxx-irq-gpios = <&gpio0 8 0>; /* GPIO1_8 */
-						fsl,mc13xxx-uses-regulator;
+						interrupt-parent = <&gpio0>;
+						interrupts = <8>;
+
+						regulators {
+							sw1_reg: mc13892__sw1 {
+								regulator-min-microvolt = <600000>;
+								regulator-max-microvolt = <1375000>;
+								regulator-boot-on;
+								regulator-always-on;
+							};
+
+							sw2_reg: mc13892__sw2 {
+								regulator-min-microvolt = <900000>;
+								regulator-max-microvolt = <1850000>;
+								regulator-boot-on;
+								regulator-always-on;
+							};
+
+							sw3_reg: mc13892__sw3 {
+								regulator-min-microvolt = <1100000>;
+								regulator-max-microvolt = <1850000>;
+								regulator-boot-on;
+								regulator-always-on;
+							};
+
+							sw4_reg: mc13892__sw4 {
+								regulator-min-microvolt = <1100000>;
+								regulator-max-microvolt = <1850000>;
+								regulator-boot-on;
+								regulator-always-on;
+							};
+
+							viohi_reg: mc13892__viohi {
+								regulator-boot-on;
+								regulator-always-on;
+							};
+
+							vpll_reg: mc13892__vpll {
+								regulator-min-microvolt = <1050000>;
+								regulator-max-microvolt = <1800000>;
+								regulator-boot-on;
+								regulator-always-on;
+							};
+
+							vdig_reg: mc13892__vdig {
+								regulator-min-microvolt = <1650000>;
+								regulator-max-microvolt = <1650000>;
+								regulator-boot-on;
+							};
+
+							vsd_reg: mc13892__vsd {
+								regulator-min-microvolt = <1800000>;
+								regulator-max-microvolt = <3150000>;
+							};
+
+							vusb2_reg: mc13892__vusb2 {
+								regulator-min-microvolt = <2400000>;
+								regulator-max-microvolt = <2775000>;
+								regulator-boot-on;
+								regulator-always-on;
+							};
+
+							vvideo_reg: mc13892__vvideo {
+								regulator-min-microvolt = <2775000>;
+								regulator-max-microvolt = <2775000>;
+							};
+
+							vaudio_reg: mc13892__vaudio {
+								regulator-min-microvolt = <2300000>;
+								regulator-max-microvolt = <3000000>;
+							};
+
+							vcam_reg: mc13892__vcam {
+								regulator-min-microvolt = <2500000>;
+								regulator-max-microvolt = <3000000>;
+							};
+
+							vgen1_reg: mc13892__vgen1 {
+								regulator-min-microvolt = <1200000>;
+								regulator-max-microvolt = <1200000>;
+							};
+
+							vgen2_reg: mc13892__vgen2 {
+								regulator-min-microvolt = <1200000>;
+								regulator-max-microvolt = <3150000>;
+								regulator-always-on;
+							};
+
+							vgen3_reg: mc13892__vgen3 {
+								regulator-min-microvolt = <1800000>;
+								regulator-max-microvolt = <2900000>;
+								regulator-always-on;
+							};
+
+							vusb_reg: mc13892__vusb {
+								regulator-boot-on;
+								regulator-always-on;
+							};
+						};
 					};
 
 					flash: at45db321d at 1 {
-- 
1.7.4.1