[PATCH 00/15] DT enablement for Snowball

[PATCH 00/15] DT enablement for Snowball

[Lee Jones]

Here's your next back of DT related doings for the ux500,
along with some bugs encountered fixed along the way.

 arch/arm/boot/dts/db8500.dtsi      |  103 +++++++++++++-
 arch/arm/boot/dts/snowball.dts     |    3 +
 arch/arm/configs/u8500_defconfig   |    1 +
 arch/arm/mach-ux500/board-mop500.c |   55 ++------
 drivers/i2c/busses/i2c-nomadik.c   |   53 ++++++-
 drivers/mfd/Makefile               |    5 +-
 drivers/mfd/ab8500-core.c          |  165 +++++++++++++++++++---
 drivers/mfd/ab8500-i2c.c           |  128 -----------------
 drivers/mfd/db8500-prcmu.c         |   30 ++--
 drivers/power/ab8500_btemp.c       |   12 +-
 drivers/power/ab8500_charger.c     |   12 +-
 drivers/power/ab8500_fg.c          |   12 +-
 drivers/regulator/ab8500.c         |  273 +++++++++++++++++++++++-------------

[PATCH 01/15] i2c/busses: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

Here we move the i2c-nomadik's default settings into the driver
rather than specifying them from platform code. At the time of
this writing we only have one user, the u8500. As new users are
added, it is expected that they will be Device Tree compliant.
If this is the case, we will look up their initialisation values
by compatible entry, then apply them forthwith.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   50 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 5267ab9..8803467 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -899,15 +899,57 @@ static const struct i2c_algorithm nmk_i2c_algo = {
 	.functionality	= nmk_i2c_functionality
 };
 
+static const struct nmk_i2c_controller u8500_i2c = {
+       /*
+        * slave data setup time, which is
+        * 250 ns,100ns,10ns which is 14,6,2
+        * respectively for a 48 Mhz
+        * i2c clock
+        */
+       .slsu           = 0xe,
+       /* Tx FIFO threshold */
+       .tft            = 1,
+       /* Rx FIFO threshold */
+       .rft            = 8,
+       /* std. mode operation */
+       .clk_freq       = 100000,
+       /* Slave response timeout(ms) */
+       .timeout        = 200,
+       .sm             = I2C_FREQ_MODE_FAST,
+};
+
+static const struct nmk_i2c_controller *
+nmk_i2c_find_pdata_from_compatible(struct device_node *np)
+{
+	/*
+	 * The u8500 is currently our only user. As more SoCs are added,
+	 * search for the correct value set using of_machine_is_compatible
+	 * and return a 'struct nmk_i2c_controller *' which contains the
+	 * correct information for the given SoC, whilst leaving u8500_i2c
+	 * as the default/fall-back value set.
+	 */
+	return &u8500_i2c;
+}
+
 static int __devinit nmk_i2c_probe(struct platform_device *pdev)
 {
 	int ret = 0;
 	struct resource *res;
-	struct nmk_i2c_controller *pdata =
+	const struct nmk_i2c_controller *pdata =
 			pdev->dev.platform_data;
+	struct device_node *np = pdev->dev.of_node;
 	struct nmk_i2c_dev	*dev;
 	struct i2c_adapter *adap;
 
+	if (!pdata) {
+		if (!np) {
+			dev_err(&pdev->dev,
+				"Neither platform_data nor Device Tree not found\n");
+			return -EINVAL;
+		}
+		pdata = nmk_i2c_find_pdata_from_compatible(np);
+	}
+
 	dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
 	if (!dev) {
 		dev_err(&pdev->dev, "cannot allocate memory\n");
@@ -1039,11 +1081,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id nmk_gpio_match[] = {
+	{ .compatible = "st,nomadik-i2c", },
+	{}
+};
+
 static struct platform_driver nmk_i2c_driver = {
 	.driver = {
 		.owner = THIS_MODULE,
 		.name = DRIVER_NAME,
 		.pm = &nmk_i2c_pm,
+		.of_match_table = nmk_gpio_match,
 	},
 	.probe = nmk_i2c_probe,
 	.remove = __devexit_p(nmk_i2c_remove),
-- 
1.7.9.5

[PATCH 01/15] i2c/busses: Add Device Tree support to the Nomadik I2C driver

[Arnd Bergmann]

On Friday 04 May 2012, Lee Jones wrote:
> +static const struct nmk_i2c_controller *
> +nmk_i2c_find_pdata_from_compatible(struct device_node *np)
> +{
> +       /*
> +        * The u8500 is currently our only user. As more SoCs are added,
> +        * search for the correct value set using of_machine_is_compatible
> +        * and return a 'struct nmk_i2c_controller *' which contains the
> +        * correct information for the given SoC, whilst leaving u8500_i2c
> +        * as the default/fall-back value set.
> +        */
> +       return &u8500_i2c;
> +}

Why not just put this pointer ...

> +static const struct of_device_id nmk_gpio_match[] = {
> +       { .compatible = "st,nomadik-i2c", },
> +       {}
> +};

into the .data field after the .compatible match, and make it more specific
to the soc, i.e.

static const struct of_device_id nmk_gpio_match[] = {
	{ .compatible = "st-ericsson,u8500-i2c", .data = &u8500_i2c },
	{ .compatible = "st,nomadik-i2c", .data = &default_i2c_controller },
};

	Arnd

[PATCH 01/15] i2c/busses: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On 04/05/12 21:02, Arnd Bergmann wrote:
> On Friday 04 May 2012, Lee Jones wrote:
>> +static const struct nmk_i2c_controller *
>> +nmk_i2c_find_pdata_from_compatible(struct device_node *np)
>> +{
>> +       /*
>> +        * The u8500 is currently our only user. As more SoCs are added,
>> +        * search for the correct value set using of_machine_is_compatible
>> +        * and return a 'struct nmk_i2c_controller *' which contains the
>> +        * correct information for the given SoC, whilst leaving u8500_i2c
>> +        * as the default/fall-back value set.
>> +        */
>> +       return&u8500_i2c;
>> +}
>
> Why not just put this pointer ...
>
>> +static const struct of_device_id nmk_gpio_match[] = {
>> +       { .compatible = "st,nomadik-i2c", },
>> +       {}
>> +};
>
> into the .data field after the .compatible match, and make it more specific
> to the soc, i.e.
>
> static const struct of_device_id nmk_gpio_match[] = {
> 	{ .compatible = "st-ericsson,u8500-i2c", .data =&u8500_i2c },
> 	{ .compatible = "st,nomadik-i2c", .data =&default_i2c_controller },
> };

Because;
a) I only learned about .data way after this patch was written
and
b) _You_ told me to do it like this. ;)


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 01/15] i2c/busses: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

I haven't tested it yet (it builds with no warnings/errors, but I
haven't booted it), as I don't have my board immediately to hand,
but does this kind of thing better suit what you're looking for?

From: Lee Jones <lee.jones@linaro.org>
Date: Tue, 17 Apr 2012 16:04:13 +0100
Subject: [PATCH 01/15] i2c/busses: Add Device Tree support to the Nomadik I2C
 driver

Here we move the i2c-nomadik's default settings into the driver
rather than specifying them from platform code. At the time of
this writing we only have one user, the u8500. As new users are
added, it is expected that they will be Device Tree compliant.
If this is the case, we will look up their initialisation values
by compatible entry, then apply them forthwith.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   42 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 5267ab9..3478a13 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_device.h>
 
 #include <plat/i2c.h>
 
@@ -899,15 +900,53 @@ static const struct i2c_algorithm nmk_i2c_algo = {
 	.functionality	= nmk_i2c_functionality
 };
 
+static const struct nmk_i2c_controller u8500_i2c = {
+       /*
+        * slave data setup time, which is
+        * 250 ns,100ns,10ns which is 14,6,2
+        * respectively for a 48 Mhz
+        * i2c clock
+        */
+       .slsu           = 0xe,
+       /* Tx FIFO threshold */
+       .tft            = 1,
+       /* Rx FIFO threshold */
+       .rft            = 8,
+       /* std. mode operation */
+       .clk_freq       = 100000,
+       /* Slave response timeout(ms) */
+       .timeout        = 200,
+       .sm             = I2C_FREQ_MODE_FAST,
+};
+
+static const struct of_device_id nmk_gpio_match[] = {
+	{ .compatible = "stericsson,db8500-i2c", .data = (void *)&u8500_i2c, },
+	{}
+};
+
 static int __devinit nmk_i2c_probe(struct platform_device *pdev)
 {
 	int ret = 0;
 	struct resource *res;
-	struct nmk_i2c_controller *pdata =
+	const struct nmk_i2c_controller *pdata =
 			pdev->dev.platform_data;
+	struct device_node *np = pdev->dev.of_node;
 	struct nmk_i2c_dev	*dev;
 	struct i2c_adapter *adap;
 
+	if (!pdata) {
+		if (!np) {
+			dev_err(&pdev->dev,
+				"Neither platform_data nor Device Tree not found\n");
+			return -EINVAL;
+		}
+		/* Try to fetch supplied i2c configuration from the match table. */
+		pdata = of_match_device(nmk_gpio_match, &pdev->dev)->data;
+		/* If no configuration was supplied, use the default. */
+		if (!pdata)
+			pdata = &u8500_i2c;
+	}
+
 	dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
 	if (!dev) {
 		dev_err(&pdev->dev, "cannot allocate memory\n");
@@ -1044,6 +1083,7 @@ static struct platform_driver nmk_i2c_driver = {
 		.owner = THIS_MODULE,
 		.name = DRIVER_NAME,
 		.pm = &nmk_i2c_pm,
+		.of_match_table = nmk_gpio_match,
 	},
 	.probe = nmk_i2c_probe,
 	.remove = __devexit_p(nmk_i2c_remove),
-- 
1.7.9.5

[PATCH 01/15] i2c/busses: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Sat, May 5, 2012 at 8:17 AM, Lee Jones <lee.jones@linaro.org> wrote:

> I haven't tested it yet (it builds with no warnings/errors, but I
> haven't booted it), as I don't have my board immediately to hand,
> but does this kind of thing better suit what you're looking for?
>
> From: Lee Jones <lee.jones@linaro.org>
> Date: Tue, 17 Apr 2012 16:04:13 +0100
> Subject: [PATCH 01/15] i2c/busses: Add Device Tree support to the Nomadik I2C
> ?driver
>
> Here we move the i2c-nomadik's default settings into the driver
> rather than specifying them from platform code. At the time of
> this writing we only have one user, the u8500. As new users are
> added, it is expected that they will be Device Tree compliant.
> If this is the case, we will look up their initialisation values
> by compatible entry, then apply them forthwith.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Make sure you test it first though.

Yours,
Linus Walleij

[PATCH 02/15] ARM: ux500: Remove unused i2c platform_data initialisation code

[Lee Jones]

Now that u5500 is obsolete, u8500 is the only user of the Nomadik
i2c driver. As such there is no requirement to differentiate between
initialisation values. By the time a new SoC is released, almost all
of the ux500 platform will be DT:ed, so we can make decisions based
on the compatible property instead.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |   39 ++++--------------------------------
 drivers/i2c/busses/i2c-nomadik.c   |    3 +--
 2 files changed, 5 insertions(+), 37 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index af4910d..215d212 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -330,43 +330,12 @@ static struct i2c_board_info __initdata mop500_i2c2_devices[] = {
 	},
 };
 
-#define U8500_I2C_CONTROLLER(id, _slsu, _tft, _rft, clk, t_out, _sm)	\
-static struct nmk_i2c_controller u8500_i2c##id##_data = { \
-	/*				\
-	 * slave data setup time, which is	\
-	 * 250 ns,100ns,10ns which is 14,6,2	\
-	 * respectively for a 48 Mhz	\
-	 * i2c clock			\
-	 */				\
-	.slsu		= _slsu,	\
-	/* Tx FIFO threshold */		\
-	.tft		= _tft,		\
-	/* Rx FIFO threshold */		\
-	.rft		= _rft,		\
-	/* std. mode operation */	\
-	.clk_freq	= clk,		\
-	/* Slave response timeout(ms) */\
-	.timeout	= t_out,	\
-	.sm		= _sm,		\
-}
-
-/*
- * The board uses 4 i2c controllers, initialize all of
- * them with slave data setup time of 250 ns,
- * Tx & Rx FIFO threshold values as 8 and standard
- * mode of operation
- */
-U8500_I2C_CONTROLLER(0, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
-U8500_I2C_CONTROLLER(1, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
-U8500_I2C_CONTROLLER(2,	0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
-U8500_I2C_CONTROLLER(3,	0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
-
 static void __init mop500_i2c_init(struct device *parent)
 {
-	db8500_add_i2c0(parent, &u8500_i2c0_data);
-	db8500_add_i2c1(parent, &u8500_i2c1_data);
-	db8500_add_i2c2(parent, &u8500_i2c2_data);
-	db8500_add_i2c3(parent, &u8500_i2c3_data);
+	db8500_add_i2c0(parent, NULL);
+	db8500_add_i2c1(parent, NULL);
+	db8500_add_i2c2(parent, NULL);
+	db8500_add_i2c3(parent, NULL);
 }
 
 static struct gpio_keys_button mop500_gpio_keys[] = {
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 8803467..a89af68 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -1007,8 +1007,7 @@ static int __devinit nmk_i2c_probe(struct platform_device *pdev)
 	adap->owner	= THIS_MODULE;
 	adap->class	= I2C_CLASS_HWMON | I2C_CLASS_SPD;
 	adap->algo	= &nmk_i2c_algo;
-	adap->timeout	= pdata->timeout ? msecs_to_jiffies(pdata->timeout) :
-		msecs_to_jiffies(20000);
+	adap->timeout	= msecs_to_jiffies(pdata->timeout);
 	snprintf(adap->name, sizeof(adap->name),
 		 "Nomadik I2C%d at %lx", pdev->id, (unsigned long)res->start);
 
-- 
1.7.9.5

[PATCH 02/15] ARM: ux500: Remove unused i2c platform_data initialisation code

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> Now that u5500 is obsolete, u8500 is the only user of the Nomadik
> i2c driver.

Well actually arch/arm/mach-nomadik has this block too,
but it's not making use of it right now, so why not.

> As such there is no requirement to differentiate between
> initialisation values. By the time a new SoC is released, almost all
> of the ux500 platform will be DT:ed, so we can make decisions based
> on the compatible property instead.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

So why not copy the contents of arch/arm/plat-nomadik/include/plat/i2c.h
into drivers/i2c/busses/i2c-nomadik.c and delete that header from
plat at the same time?

Then there sure is no way for platforms to sneak in any platform
data any other way.

Yours,
Linus Walleij

[PATCH 02/15] ARM: ux500: Remove unused i2c platform_data initialisation code

[Lee Jones]

On 09/05/12 09:46, Linus Walleij wrote:
> On Fri, May 4, 2012 at 8:23 PM, Lee Jones<lee.jones@linaro.org>  wrote:
>
>> Now that u5500 is obsolete, u8500 is the only user of the Nomadik
>> i2c driver.
>
> Well actually arch/arm/mach-nomadik has this block too,
> but it's not making use of it right now, so why not.
>
>> As such there is no requirement to differentiate between
>> initialisation values. By the time a new SoC is released, almost all
>> of the ux500 platform will be DT:ed, so we can make decisions based
>> on the compatible property instead.
>>
>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
>
> So why not copy the contents of arch/arm/plat-nomadik/include/plat/i2c.h
> into drivers/i2c/busses/i2c-nomadik.c and delete that header from
> plat at the same time?
>
> Then there sure is no way for platforms to sneak in any platform
> data any other way.

I have no problem in doing that. Can I submit a separate patch?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 02/15] ARM: ux500: Remove unused i2c platform_data initialisation code

[Linus Walleij]

On Wed, May 9, 2012 at 12:22 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On 09/05/12 09:46, Linus Walleij wrote:
>> So why not copy the contents of arch/arm/plat-nomadik/include/plat/i2c.h
>> into drivers/i2c/busses/i2c-nomadik.c and delete that header from
>> plat at the same time?
>>
>> Then there sure is no way for platforms to sneak in any platform
>> data any other way.
>
> I have no problem in doing that. Can I submit a separate patch?

Sure. Acked-by on this then...

Yours,
Linus Walleij

[PATCH 03/15] ARM: ux500: Provide auxdata to be used as name base clock search for nmk-i2c

[Lee Jones]

This patch provides a way for find_clk() to complete its name-based
searches and still find the nmk-i2c clocks. As Device Tree has a
different naming structure to that of more traditional initialisation
methods, find_clk() not succeed without these bindings.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 215d212..4b847cb 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -733,6 +733,11 @@ struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("st,nomadik-gpio", 0x8011e000, "gpio.6", NULL),
 	OF_DEV_AUXDATA("st,nomadik-gpio", 0x8011e080, "gpio.7", NULL),
 	OF_DEV_AUXDATA("st,nomadik-gpio", 0xa03fe000, "gpio.8", NULL),
+	OF_DEV_AUXDATA("st,nomadik-i2c", 0x80004000, "nmk-i2c.0", NULL),
+	OF_DEV_AUXDATA("st,nomadik-i2c", 0x80122000, "nmk-i2c.1", NULL),
+	OF_DEV_AUXDATA("st,nomadik-i2c", 0x80128000, "nmk-i2c.2", NULL),
+	OF_DEV_AUXDATA("st,nomadik-i2c", 0x80110000, "nmk-i2c.3", NULL),
+	OF_DEV_AUXDATA("st,nomadik-i2c", 0x8012a000, "nmk-i2c.4", NULL),
 	{},
 };
 
@@ -807,7 +812,6 @@ static void __init u8500_init_machine(void)
 		i2c_register_board_info(2, mop500_i2c2_devices,
 					ARRAY_SIZE(mop500_i2c2_devices));
 	}
-	mop500_i2c_init(parent);
 
 	/* This board has full regulator constraints */
 	regulator_has_full_constraints();
-- 
1.7.9.5

[PATCH 03/15] ARM: ux500: Provide auxdata to be used as name base clock search for nmk-i2c

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> This patch provides a way for find_clk() to complete its name-based
> searches and still find the nmk-i2c clocks. As Device Tree has a
> different naming structure to that of more traditional initialisation
> methods, find_clk() not succeed without these bindings.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

[PATCH 04/15] ARM: ux500: CONFIG: Compile in support for leds-gpio

[Lee Jones]

Code exists in the mop500 board file (used for HREF and
Snowball) to initialise Snowball's user LED via the
leds-gpio driver. However, the driver isn't currently
built when using the default configuration file. This
patch aims to change that behavior.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/configs/u8500_defconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/u8500_defconfig b/arch/arm/configs/u8500_defconfig
index 7e84f45..00571b7 100644
--- a/arch/arm/configs/u8500_defconfig
+++ b/arch/arm/configs/u8500_defconfig
@@ -85,6 +85,7 @@ CONFIG_NEW_LEDS=y
 CONFIG_LEDS_CLASS=y
 CONFIG_LEDS_LM3530=y
 CONFIG_LEDS_LP5521=y
+CONFIG_LEDS_GPIO=y
 CONFIG_RTC_CLASS=y
 CONFIG_RTC_DRV_AB8500=y
 CONFIG_RTC_DRV_PL031=y
-- 
1.7.9.5

[PATCH 04/15] ARM: ux500: CONFIG: Compile in support for leds-gpio

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> Code exists in the mop500 board file (used for HREF and
> Snowball) to initialise Snowball's user LED via the
> leds-gpio driver. However, the driver isn't currently
> built when using the default configuration file. This
> patch aims to change that behavior.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
(...)
> +CONFIG_LEDS_GPIO=y
> ?CONFIG_RTC_CLASS=y
> ?CONFIG_RTC_DRV_AB8500=y
> ?CONFIG_RTC_DRV_PL031=y

Why not just edit arch/arm/mach-ux500/Kconfig like this:

config MACH_SNOWBALL
        bool "U8500 Snowball platform"
        select MACH_MOP500
+        select LEDS_GPIO

Yours,
Linus Walleij

[PATCH 04/15] ARM: ux500: CONFIG: Compile in support for leds-gpio

[Lee Jones]

On 09/05/12 09:49, Linus Walleij wrote:
> On Fri, May 4, 2012 at 8:23 PM, Lee Jones<lee.jones@linaro.org>  wrote:
>
>> Code exists in the mop500 board file (used for HREF and
>> Snowball) to initialise Snowball's user LED via the
>> leds-gpio driver. However, the driver isn't currently
>> built when using the default configuration file. This
>> patch aims to change that behavior.
>>
>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
> (...)
>> +CONFIG_LEDS_GPIO=y
>>   CONFIG_RTC_CLASS=y
>>   CONFIG_RTC_DRV_AB8500=y
>>   CONFIG_RTC_DRV_PL031=y
>
> Why not just edit arch/arm/mach-ux500/Kconfig like this:
>
> config MACH_SNOWBALL
>          bool "U8500 Snowball platform"
>          select MACH_MOP500
> +        select LEDS_GPIO

I can do that. Is it not useful for any of the other platforms?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 04/15] ARM: ux500: CONFIG: Compile in support for leds-gpio

[Linus Walleij]

On Wed, May 9, 2012 at 12:23 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On 09/05/12 09:49, Linus Walleij wrote:
>> On Fri, May 4, 2012 at 8:23 PM, Lee Jones<lee.jones@linaro.org> ?wrote:
>>
>>> Code exists in the mop500 board file (used for HREF and
>>> Snowball) to initialise Snowball's user LED via the
>>> leds-gpio driver. However, the driver isn't currently
>>> built when using the default configuration file. This
>>> patch aims to change that behavior.
>>>
>>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
>>
>> (...)
>>>
>>> +CONFIG_LEDS_GPIO=y
>>> ?CONFIG_RTC_CLASS=y
>>> ?CONFIG_RTC_DRV_AB8500=y
>>> ?CONFIG_RTC_DRV_PL031=y
>>
>>
>> Why not just edit arch/arm/mach-ux500/Kconfig like this:
>>
>> config MACH_SNOWBALL
>> ? ? ? ? bool "U8500 Snowball platform"
>> ? ? ? ? select MACH_MOP500
>> + ? ? ? ?select LEDS_GPIO
>
>
> I can do that. Is it not useful for any of the other platforms?

Thanks and oo, the others have dedicated LED controllers and does not use
GPIO for this.

Yours,
Linus Walleij

[PATCH 05/15] ARM: ux500: Enable the user LED on Snowball via Device Tree

[Lee Jones]

This patch enables and illuminates the user_led on the
Snowball low-cost development board using DT. It also
removes initialisation carried out from platform code.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/snowball.dts     |    1 +
 arch/arm/mach-ux500/board-mop500.c |    3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/snowball.dts b/arch/arm/boot/dts/snowball.dts
index 56f8a76..579c304 100644
--- a/arch/arm/boot/dts/snowball.dts
+++ b/arch/arm/boot/dts/snowball.dts
@@ -67,6 +67,7 @@
 		used-led {
 			label = "user_led";
 			gpios = <&gpio4 14 0x4>;
+			default-state = "on";
 		};
 	};
 
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 4b847cb..7810334 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -579,7 +579,6 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
 };
 
 static struct platform_device *snowball_of_platform_devs[] __initdata = {
-	&snowball_led_dev,
 	&snowball_key_dev,
 	&ab8500_device,
 };
@@ -783,7 +782,7 @@ static void __init u8500_init_machine(void)
 
 		/*
 		 * Devices to be DT:ed:
-		 *   snowball_led_dev   = todo
+		 *   snowball_led_dev   = done
 		 *   snowball_key_dev   = todo
 		 *   snowball_sbnet_dev = done
 		 *   ab8500_device      = todo
-- 
1.7.9.5

[PATCH 05/15] ARM: ux500: Enable the user LED on Snowball via Device Tree

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> This patch enables and illuminates the user_led on the
> Snowball low-cost development board using DT. It also
> removes initialisation carried out from platform code.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

[PATCH 06/15] mfd/ab8500: Remove confusing ab8500-i2c file and merge into ab8500-core

[Lee Jones]

ab8500-i2c is used as core code to register the ab8500 device.
After allocating ab8500 memory, it immediately calls into
ab8500-core where the real initialisation takes place. This
patch moves all core registration and memory allocation into
the true ab8500-core file and removes ab8500-i2c completely.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    2 +-
 drivers/mfd/Makefile               |    5 +-
 drivers/mfd/ab8500-core.c          |  105 +++++++++++++++++++++++++++--
 drivers/mfd/ab8500-i2c.c           |  128 ------------------------------------
 4 files changed, 103 insertions(+), 137 deletions(-)
 delete mode 100644 drivers/mfd/ab8500-i2c.c

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 7810334..bc8a0d7 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -205,7 +205,7 @@ static struct resource ab8500_resources[] = {
 };
 
 struct platform_device ab8500_device = {
-	.name = "ab8500-i2c",
+	.name = "ab8500-core",
 	.id = 0,
 	.dev = {
 		.platform_data = &ab8500_platdata,
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 05fa538..c29c68c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -89,12 +89,11 @@ obj-$(CONFIG_AB3100_CORE)	+= ab3100-core.o
 obj-$(CONFIG_AB3100_OTP)	+= ab3100-otp.o
 obj-$(CONFIG_AB5500_CORE)	+= ab5500-core.o
 obj-$(CONFIG_AB5500_DEBUG)	+= ab5500-debugfs.o
-obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o ab8500-sysctrl.o
 obj-$(CONFIG_AB8500_DEBUG)	+= ab8500-debugfs.o
 obj-$(CONFIG_AB8500_GPADC)	+= ab8500-gpadc.o
 obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
-# ab8500-i2c need to come after db8500-prcmu (which provides the channel)
-obj-$(CONFIG_AB8500_I2C_CORE)	+= ab8500-i2c.o
+# ab8500-core needs to come after db8500-prcmu (which provides the channel)
+obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o ab8500-sysctrl.o
 obj-$(CONFIG_MFD_DB5500_PRCMU)	+= db5500-prcmu.o
 obj-$(CONFIG_MFD_TIMBERDALE)    += timberdale.o
 obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 1f08704..74e6661 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -18,6 +18,7 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/abx500.h>
 #include <linux/mfd/abx500/ab8500.h>
+#include <linux/mfd/dbx500-prcmu.h>
 #include <linux/regulator/ab8500.h>
 
 /*
@@ -125,6 +126,41 @@ static const char ab8500_version_str[][7] = {
 	[AB8500_VERSION_AB8540] = "AB8540",
 };
 
+static int ab8500_i2c_write(struct ab8500 *ab8500, u16 addr, u8 data)
+{
+	int ret;
+
+	ret = prcmu_abb_write((u8)(addr >> 8), (u8)(addr & 0xFF), &data, 1);
+	if (ret < 0)
+		dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
+	return ret;
+}
+
+static int ab8500_i2c_write_masked(struct ab8500 *ab8500, u16 addr, u8 mask,
+	u8 data)
+{
+	int ret;
+
+	ret = prcmu_abb_write_masked((u8)(addr >> 8), (u8)(addr & 0xFF), &data,
+		&mask, 1);
+	if (ret < 0)
+		dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
+	return ret;
+}
+
+static int ab8500_i2c_read(struct ab8500 *ab8500, u16 addr)
+{
+	int ret;
+	u8 data;
+
+	ret = prcmu_abb_read((u8)(addr >> 8), (u8)(addr & 0xFF), &data, 1);
+	if (ret < 0) {
+		dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
+		return ret;
+	}
+	return (int)data;
+}
+
 static int ab8500_get_chip_id(struct device *dev)
 {
 	struct ab8500 *ab8500;
@@ -1003,26 +1039,50 @@ static struct attribute_group ab9540_attr_group = {
 	.attrs	= ab9540_sysfs_entries,
 };
 
-int __devinit ab8500_init(struct ab8500 *ab8500, enum ab8500_version version)
+static int __devinit ab8500_probe(struct platform_device *pdev)
 {
-	struct ab8500_platform_data *plat = dev_get_platdata(ab8500->dev);
+	struct ab8500_platform_data *plat = dev_get_platdata(&pdev->dev);
+	const struct platform_device_id *platid = platform_get_device_id(pdev);
+	enum ab8500_version version = platid->driver_data;
+	struct ab8500 *ab8500;
+	struct resource *resource;
 	int ret;
 	int i;
 	u8 value;
 
+	ab8500 = kzalloc(sizeof *ab8500, GFP_KERNEL);
+	if (!ab8500)
+		return -ENOMEM;
+
 	if (plat)
 		ab8500->irq_base = plat->irq_base;
 
+	ab8500->dev = &pdev->dev;
+
+	resource = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!resource) {
+		ret = -ENODEV;
+		goto out_free_ab8500;
+	}
+
+	ab8500->irq = resource->start;
+
+	ab8500->read = ab8500_i2c_read;
+	ab8500->write = ab8500_i2c_write;
+	ab8500->write_masked = ab8500_i2c_write_masked;
+
 	mutex_init(&ab8500->lock);
 	mutex_init(&ab8500->irq_lock);
 
+	platform_set_drvdata(pdev, ab8500);
+
 	if (version != AB8500_VERSION_UNDEFINED)
 		ab8500->version = version;
 	else {
 		ret = get_register_interruptible(ab8500, AB8500_MISC,
 			AB8500_IC_NAME_REG, &value);
 		if (ret < 0)
-			return ret;
+			goto out_free_ab8500;
 
 		ab8500->version = value;
 	}
@@ -1030,7 +1090,7 @@ int __devinit ab8500_init(struct ab8500 *ab8500, enum ab8500_version version)
 	ret = get_register_interruptible(ab8500, AB8500_MISC,
 		AB8500_REV_REG, &value);
 	if (ret < 0)
-		return ret;
+		goto out_free_ab8500;
 
 	ab8500->chip_id = value;
 
@@ -1151,12 +1211,16 @@ out_freeoldmask:
 	kfree(ab8500->oldmask);
 out_freemask:
 	kfree(ab8500->mask);
+out_free_ab8500:
+	kfree(ab8500);
 
 	return ret;
 }
 
-int __devexit ab8500_exit(struct ab8500 *ab8500)
+static int __devexit ab8500_remove(struct platform_device *pdev)
 {
+	struct ab8500 *ab8500 = platform_get_drvdata(pdev);
+
 	if (is_ab9540(ab8500))
 		sysfs_remove_group(&ab8500->dev->kobj, &ab9540_attr_group);
 	else
@@ -1168,10 +1232,41 @@ int __devexit ab8500_exit(struct ab8500 *ab8500)
 	}
 	kfree(ab8500->oldmask);
 	kfree(ab8500->mask);
+	kfree(ab8500);
 
 	return 0;
 }
 
+static const struct platform_device_id ab8500_id[] = {
+	{ "ab8500-core", AB8500_VERSION_AB8500 },
+	{ "ab8505-i2c", AB8500_VERSION_AB8505 },
+	{ "ab9540-i2c", AB8500_VERSION_AB9540 },
+	{ "ab8540-i2c", AB8500_VERSION_AB8540 },
+	{ }
+};
+
+static struct platform_driver ab8500_core_driver = {
+	.driver = {
+		.name = "ab8500-core",
+		.owner = THIS_MODULE,
+	},
+	.probe	= ab8500_probe,
+	.remove	= __devexit_p(ab8500_remove),
+	.id_table = ab8500_id,
+};
+
+static int __init ab8500_core_init(void)
+{
+	return platform_driver_register(&ab8500_core_driver);
+}
+
+static void __exit ab8500_core_exit(void)
+{
+	platform_driver_unregister(&ab8500_core_driver);
+}
+arch_initcall(ab8500_core_init);
+module_exit(ab8500_core_exit);
+
 MODULE_AUTHOR("Mattias Wallin, Srinidhi Kasagar, Rabin Vincent");
 MODULE_DESCRIPTION("AB8500 MFD core");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/mfd/ab8500-i2c.c b/drivers/mfd/ab8500-i2c.c
deleted file mode 100644
index b83045f..0000000
--- a/drivers/mfd/ab8500-i2c.c
+++ /dev/null
@@ -1,128 +0,0 @@
-/*
- * Copyright (C) ST-Ericsson SA 2010
- * Author: Mattias Wallin <mattias.wallin@stericsson.com> for ST-Ericsson.
- * License Terms: GNU General Public License v2
- * This file was based on drivers/mfd/ab8500-spi.c
- */
-
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/mfd/abx500/ab8500.h>
-#include <linux/mfd/dbx500-prcmu.h>
-
-static int ab8500_i2c_write(struct ab8500 *ab8500, u16 addr, u8 data)
-{
-	int ret;
-
-	ret = prcmu_abb_write((u8)(addr >> 8), (u8)(addr & 0xFF), &data, 1);
-	if (ret < 0)
-		dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
-	return ret;
-}
-
-static int ab8500_i2c_write_masked(struct ab8500 *ab8500, u16 addr, u8 mask,
-	u8 data)
-{
-	int ret;
-
-	ret = prcmu_abb_write_masked((u8)(addr >> 8), (u8)(addr & 0xFF), &data,
-		&mask, 1);
-	if (ret < 0)
-		dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
-	return ret;
-}
-
-static int ab8500_i2c_read(struct ab8500 *ab8500, u16 addr)
-{
-	int ret;
-	u8 data;
-
-	ret = prcmu_abb_read((u8)(addr >> 8), (u8)(addr & 0xFF), &data, 1);
-	if (ret < 0) {
-		dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
-		return ret;
-	}
-	return (int)data;
-}
-
-static int __devinit ab8500_i2c_probe(struct platform_device *plf)
-{
-	const struct platform_device_id *platid = platform_get_device_id(plf);
-	struct ab8500 *ab8500;
-	struct resource *resource;
-	int ret;
-
-	ab8500 = kzalloc(sizeof *ab8500, GFP_KERNEL);
-	if (!ab8500)
-		return -ENOMEM;
-
-	ab8500->dev = &plf->dev;
-
-	resource = platform_get_resource(plf, IORESOURCE_IRQ, 0);
-	if (!resource) {
-		kfree(ab8500);
-		return -ENODEV;
-	}
-
-	ab8500->irq = resource->start;
-
-	ab8500->read = ab8500_i2c_read;
-	ab8500->write = ab8500_i2c_write;
-	ab8500->write_masked = ab8500_i2c_write_masked;
-
-	platform_set_drvdata(plf, ab8500);
-
-	ret = ab8500_init(ab8500, platid->driver_data);
-	if (ret)
-		kfree(ab8500);
-
-
-	return ret;
-}
-
-static int __devexit ab8500_i2c_remove(struct platform_device *plf)
-{
-	struct ab8500 *ab8500 = platform_get_drvdata(plf);
-
-	ab8500_exit(ab8500);
-	kfree(ab8500);
-
-	return 0;
-}
-
-static const struct platform_device_id ab8500_id[] = {
-	{ "ab8500-i2c", AB8500_VERSION_AB8500 },
-	{ "ab8505-i2c", AB8500_VERSION_AB8505 },
-	{ "ab9540-i2c", AB8500_VERSION_AB9540 },
-	{ "ab8540-i2c", AB8500_VERSION_AB8540 },
-	{ }
-};
-
-static struct platform_driver ab8500_i2c_driver = {
-	.driver = {
-		.name = "ab8500-i2c",
-		.owner = THIS_MODULE,
-	},
-	.probe	= ab8500_i2c_probe,
-	.remove	= __devexit_p(ab8500_i2c_remove),
-	.id_table = ab8500_id,
-};
-
-static int __init ab8500_i2c_init(void)
-{
-	return platform_driver_register(&ab8500_i2c_driver);
-}
-
-static void __exit ab8500_i2c_exit(void)
-{
-	platform_driver_unregister(&ab8500_i2c_driver);
-}
-arch_initcall(ab8500_i2c_init);
-module_exit(ab8500_i2c_exit);
-
-MODULE_AUTHOR("Mattias WALLIN <mattias.wallin@stericsson.com");
-MODULE_DESCRIPTION("AB8500 Core access via PRCMU I2C");
-MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[PATCH 06/15] mfd/ab8500: Remove confusing ab8500-i2c file and merge into ab8500-core

[Arnd Bergmann]

On Friday 04 May 2012, Lee Jones wrote:
> 
> ab8500-i2c is used as core code to register the ab8500 device.
> After allocating ab8500 memory, it immediately calls into
> ab8500-core where the real initialisation takes place. This
> patch moves all core registration and memory allocation into
> the true ab8500-core file and removes ab8500-i2c completely.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

These changes all look good, but I think I would go further here.
I believe we discussed this and I agreed that we could leave that
for later, but upon reading this code, I think now that it's getting
rather silly.

> @@ -125,6 +126,41 @@ static const char ab8500_version_str[][7] = {
>         [AB8500_VERSION_AB8540] = "AB8540",
>  };
>  
> +static int ab8500_i2c_write(struct ab8500 *ab8500, u16 addr, u8 data)
> +{
> +       int ret;
> +
> +       ret = prcmu_abb_write((u8)(addr >> 8), (u8)(addr & 0xFF), &data, 1);
> +       if (ret < 0)
> +               dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
> +       return ret;
> +}
> +
> +static int ab8500_i2c_write_masked(struct ab8500 *ab8500, u16 addr, u8 mask,
> +       u8 data)
> +{
> +       int ret;
> +
> +       ret = prcmu_abb_write_masked((u8)(addr >> 8), (u8)(addr & 0xFF), &data,
> +               &mask, 1);
> +       if (ret < 0)
> +               dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
> +       return ret;
> +}
> +
> +static int ab8500_i2c_read(struct ab8500 *ab8500, u16 addr)
> +{
> +       int ret;
> +       u8 data;
> +
> +       ret = prcmu_abb_read((u8)(addr >> 8), (u8)(addr & 0xFF), &data, 1);
> +       if (ret < 0) {
> +               dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
> +               return ret;
> +       }
> +       return (int)data;
> +}
> +
>  static int ab8500_get_chip_id(struct device *dev)
>  {
>         struct ab8500 *ab8500;

Each of these functions is called only from a single location, and through an indirect
function pointer.

> +       ab8500->read = ab8500_i2c_read;
> +       ab8500->write = ab8500_i2c_write;
> +       ab8500->write_masked = ab8500_i2c_write_masked;

Which you unconditionally assign here.

If you apply this patch below, then there is no reason to add any of those.
There is room for additional simplification even, but this is the most
important one. Note that the ab8500 mutex was only needed to support the
case where write_masked is not present, and that the debug output
on error is pointless because the prcmu driver already writes the same
output. The next step would be to remove all the {get,set}_register functions
from ab8500 and just call the prcmu directly.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---

 drivers/mfd/ab8500-core.c         |   72 +++---------------------------------------------------------------------
 include/linux/mfd/abx500/ab8500.h |    1 -
 2 files changed, 3 insertions(+), 70 deletions(-)

diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 1f08704..b10bd2f 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -138,24 +138,8 @@ static int ab8500_get_chip_id(struct device *dev)
 static int set_register_interruptible(struct ab8500 *ab8500, u8 bank,
 	u8 reg, u8 data)
 {
-	int ret;
-	/*
-	 * Put the u8 bank and u8 register together into a an u16.
-	 * The bank on higher 8 bits and register in lower 8 bits.
-	 * */
-	u16 addr = ((u16)bank) << 8 | reg;
-
 	dev_vdbg(ab8500->dev, "wr: addr %#x <= %#x\n", addr, data);
-
-	mutex_lock(&ab8500->lock);
-
-	ret = ab8500->write(ab8500, addr, data);
-	if (ret < 0)
-		dev_err(ab8500->dev, "failed to write reg %#x: %d\n",
-			addr, ret);
-	mutex_unlock(&ab8500->lock);
-
-	return ret;
+	return prcmu_abb_write(bank, reg, &data, 1);
 }
 
 static int ab8500_set_register(struct device *dev, u8 bank,
@@ -169,21 +153,7 @@ static int ab8500_set_register(struct device *dev, u8 bank,
 static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
 	u8 reg, u8 *value)
 {
-	int ret;
-	/* put the u8 bank and u8 reg together into a an u16.
-	 * bank on higher 8 bits and reg in lower */
-	u16 addr = ((u16)bank) << 8 | reg;
-
-	mutex_lock(&ab8500->lock);
-
-	ret = ab8500->read(ab8500, addr);
-	if (ret < 0)
-		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
-			addr, ret);
-	else
-		*value = ret;
-
-	mutex_unlock(&ab8500->lock);
+	ret = prcmu_abb_read(bank, addr, value, 1);
 	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
 
 	return ret;
@@ -200,42 +170,7 @@ static int ab8500_get_register(struct device *dev, u8 bank,
 static int mask_and_set_register_interruptible(struct ab8500 *ab8500, u8 bank,
 	u8 reg, u8 bitmask, u8 bitvalues)
 {
-	int ret;
-	/* put the u8 bank and u8 reg together into a an u16.
-	 * bank on higher 8 bits and reg in lower */
-	u16 addr = ((u16)bank) << 8 | reg;
-
-	mutex_lock(&ab8500->lock);
-
-	if (ab8500->write_masked == NULL) {
-		u8 data;
-
-		ret = ab8500->read(ab8500, addr);
-		if (ret < 0) {
-			dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
-				addr, ret);
-			goto out;
-		}
-
-		data = (u8)ret;
-		data = (~bitmask & data) | (bitmask & bitvalues);
-
-		ret = ab8500->write(ab8500, addr, data);
-		if (ret < 0)
-			dev_err(ab8500->dev, "failed to write reg %#x: %d\n",
-				addr, ret);
-
-		dev_vdbg(ab8500->dev, "mask: addr %#x => data %#x\n", addr,
-			data);
-		goto out;
-	}
-	ret = ab8500->write_masked(ab8500, addr, bitmask, bitvalues);
-	if (ret < 0)
-		dev_err(ab8500->dev, "failed to modify reg %#x: %d\n", addr,
-			ret);
-out:
-	mutex_unlock(&ab8500->lock);
-	return ret;
+	return prcmu_abb_write_masked(bank, reg, &bitmask, &bitvalues, 1);
 }
 
 static int ab8500_mask_and_set_register(struct device *dev,
@@ -1013,7 +948,6 @@ int __devinit ab8500_init(struct ab8500 *ab8500, enum ab8500_version version)
 	if (plat)
 		ab8500->irq_base = plat->irq_base;
 
-	mutex_init(&ab8500->lock);
 	mutex_init(&ab8500->irq_lock);
 
 	if (version != AB8500_VERSION_UNDEFINED)
diff --git a/include/linux/mfd/abx500/ab8500.h b/include/linux/mfd/abx500/ab8500.h
index fccc300..5f70328 100644
--- a/include/linux/mfd/abx500/ab8500.h
+++ b/include/linux/mfd/abx500/ab8500.h
@@ -232,7 +232,6 @@ enum ab8500_version {
  */
 struct ab8500 {
 	struct device	*dev;
-	struct mutex	lock;
 	struct mutex	irq_lock;
 
 	int		irq_base;

[PATCH 06/15] mfd/ab8500: Remove confusing ab8500-i2c file and merge into ab8500-core

[Lee Jones]

On 04/05/12 21:25, Arnd Bergmann wrote:
> On Friday 04 May 2012, Lee Jones wrote:
>>
>> ab8500-i2c is used as core code to register the ab8500 device.
>> After allocating ab8500 memory, it immediately calls into
>> ab8500-core where the real initialisation takes place. This
>> patch moves all core registration and memory allocation into
>> the true ab8500-core file and removes ab8500-i2c completely.
>>
>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
>
> These changes all look good, but I think I would go further here.
> I believe we discussed this and I agreed that we could leave that
> for later, but upon reading this code, I think now that it's getting
> rather silly.
>
>> @@ -125,6 +126,41 @@ static const char ab8500_version_str[][7] = {
>>          [AB8500_VERSION_AB8540] = "AB8540",
>>   };
>>
>> +static int ab8500_i2c_write(struct ab8500 *ab8500, u16 addr, u8 data)
>> +{
>> +       int ret;
>> +
>> +       ret = prcmu_abb_write((u8)(addr>>  8), (u8)(addr&  0xFF),&data, 1);
>> +       if (ret<  0)
>> +               dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
>> +       return ret;
>> +}
>> +
>> +static int ab8500_i2c_write_masked(struct ab8500 *ab8500, u16 addr, u8 mask,
>> +       u8 data)
>> +{
>> +       int ret;
>> +
>> +       ret = prcmu_abb_write_masked((u8)(addr>>  8), (u8)(addr&  0xFF),&data,
>> +&mask, 1);
>> +       if (ret<  0)
>> +               dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
>> +       return ret;
>> +}
>> +
>> +static int ab8500_i2c_read(struct ab8500 *ab8500, u16 addr)
>> +{
>> +       int ret;
>> +       u8 data;
>> +
>> +       ret = prcmu_abb_read((u8)(addr>>  8), (u8)(addr&  0xFF),&data, 1);
>> +       if (ret<  0) {
>> +               dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
>> +               return ret;
>> +       }
>> +       return (int)data;
>> +}
>> +
>>   static int ab8500_get_chip_id(struct device *dev)
>>   {
>>          struct ab8500 *ab8500;
>
> Each of these functions is called only from a single location, and through an indirect
> function pointer.
>
>> +       ab8500->read = ab8500_i2c_read;
>> +       ab8500->write = ab8500_i2c_write;
>> +       ab8500->write_masked = ab8500_i2c_write_masked;
>
> Which you unconditionally assign here.
>
> If you apply this patch below, then there is no reason to add any of those.
> There is room for additional simplification even, but this is the most
> important one. Note that the ab8500 mutex was only needed to support the
> case where write_masked is not present, and that the debug output
> on error is pointless because the prcmu driver already writes the same
> output. The next step would be to remove all the {get,set}_register functions
> from ab8500 and just call the prcmu directly.

It's something I'm happy to do, but wasn't the point of the patch. I 
don't know much about this code, as I didn't write it.

> Signed-off-by: Arnd Bergmann<arnd@arndb.de>
> ---
>
>   drivers/mfd/ab8500-core.c         |   72 +++---------------------------------------------------------------------
>   include/linux/mfd/abx500/ab8500.h |    1 -
>   2 files changed, 3 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> index 1f08704..b10bd2f 100644
> --- a/drivers/mfd/ab8500-core.c
> +++ b/drivers/mfd/ab8500-core.c
> @@ -138,24 +138,8 @@ static int ab8500_get_chip_id(struct device *dev)
>   static int set_register_interruptible(struct ab8500 *ab8500, u8 bank,
>   	u8 reg, u8 data)
>   {
> -	int ret;
> -	/*
> -	 * Put the u8 bank and u8 register together into a an u16.
> -	 * The bank on higher 8 bits and register in lower 8 bits.
> -	 * */
> -	u16 addr = ((u16)bank)<<  8 | reg;
> -
>   	dev_vdbg(ab8500->dev, "wr: addr %#x<= %#x\n", addr, data);
> -
> -	mutex_lock(&ab8500->lock);
> -
> -	ret = ab8500->write(ab8500, addr, data);
> -	if (ret<  0)
> -		dev_err(ab8500->dev, "failed to write reg %#x: %d\n",
> -			addr, ret);
> -	mutex_unlock(&ab8500->lock);
> -
> -	return ret;
> +	return prcmu_abb_write(bank, reg,&data, 1);
>   }
>
>   static int ab8500_set_register(struct device *dev, u8 bank,
> @@ -169,21 +153,7 @@ static int ab8500_set_register(struct device *dev, u8 bank,
>   static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
>   	u8 reg, u8 *value)
>   {
> -	int ret;
> -	/* put the u8 bank and u8 reg together into a an u16.
> -	 * bank on higher 8 bits and reg in lower */
> -	u16 addr = ((u16)bank)<<  8 | reg;
> -
> -	mutex_lock(&ab8500->lock);
> -
> -	ret = ab8500->read(ab8500, addr);
> -	if (ret<  0)
> -		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
> -			addr, ret);
> -	else
> -		*value = ret;
> -
> -	mutex_unlock(&ab8500->lock);
> +	ret = prcmu_abb_read(bank, addr, value, 1);
>   	dev_vdbg(ab8500->dev, "rd: addr %#x =>  data %#x\n", addr, ret);
>
>   	return ret;
> @@ -200,42 +170,7 @@ static int ab8500_get_register(struct device *dev, u8 bank,
>   static int mask_and_set_register_interruptible(struct ab8500 *ab8500, u8 bank,
>   	u8 reg, u8 bitmask, u8 bitvalues)
>   {
> -	int ret;
> -	/* put the u8 bank and u8 reg together into a an u16.
> -	 * bank on higher 8 bits and reg in lower */
> -	u16 addr = ((u16)bank)<<  8 | reg;
> -
> -	mutex_lock(&ab8500->lock);
> -
> -	if (ab8500->write_masked == NULL) {
> -		u8 data;
> -
> -		ret = ab8500->read(ab8500, addr);
> -		if (ret<  0) {
> -			dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
> -				addr, ret);
> -			goto out;
> -		}
> -
> -		data = (u8)ret;
> -		data = (~bitmask&  data) | (bitmask&  bitvalues);
> -
> -		ret = ab8500->write(ab8500, addr, data);
> -		if (ret<  0)
> -			dev_err(ab8500->dev, "failed to write reg %#x: %d\n",
> -				addr, ret);
> -
> -		dev_vdbg(ab8500->dev, "mask: addr %#x =>  data %#x\n", addr,
> -			data);
> -		goto out;
> -	}
> -	ret = ab8500->write_masked(ab8500, addr, bitmask, bitvalues);
> -	if (ret<  0)
> -		dev_err(ab8500->dev, "failed to modify reg %#x: %d\n", addr,
> -			ret);
> -out:
> -	mutex_unlock(&ab8500->lock);
> -	return ret;
> +	return prcmu_abb_write_masked(bank, reg,&bitmask,&bitvalues, 1);
>   }
>
>   static int ab8500_mask_and_set_register(struct device *dev,
> @@ -1013,7 +948,6 @@ int __devinit ab8500_init(struct ab8500 *ab8500, enum ab8500_version version)
>   	if (plat)
>   		ab8500->irq_base = plat->irq_base;
>
> -	mutex_init(&ab8500->lock);
>   	mutex_init(&ab8500->irq_lock);
>
>   	if (version != AB8500_VERSION_UNDEFINED)
> diff --git a/include/linux/mfd/abx500/ab8500.h b/include/linux/mfd/abx500/ab8500.h
> index fccc300..5f70328 100644
> --- a/include/linux/mfd/abx500/ab8500.h
> +++ b/include/linux/mfd/abx500/ab8500.h
> @@ -232,7 +232,6 @@ enum ab8500_version {
>    */
>   struct ab8500 {
>   	struct device	*dev;
> -	struct mutex	lock;
>   	struct mutex	irq_lock;
>
>   	int		irq_base;


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 06/15] mfd/ab8500: Remove confusing ab8500-i2c file and merge into ab8500-core

[Lee Jones]

Whoops, I put that comment in the wrong place. Let me amend:

On 04/05/12 22:24, Lee Jones wrote:
> On 04/05/12 21:25, Arnd Bergmann wrote:
>> On Friday 04 May 2012, Lee Jones wrote:
>>>
>>> ab8500-i2c is used as core code to register the ab8500 device.
>>> After allocating ab8500 memory, it immediately calls into
>>> ab8500-core where the real initialisation takes place. This
>>> patch moves all core registration and memory allocation into
>>> the true ab8500-core file and removes ab8500-i2c completely.
>>>
>>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
>>
>> These changes all look good, but I think I would go further here.
>> I believe we discussed this and I agreed that we could leave that
>> for later, but upon reading this code, I think now that it's getting
>> rather silly.

It's something I'm happy to do, but wasn't the point of the patch. I
don't know much about this code, as I didn't write it. I can spend more 
time on clean-ups such as these after my DT task has been completed.

<snip>

>> If you apply this patch below, then there is no reason to add any of
>> those.
 >>
>> There is room for additional simplification even, but this is the most
>> important one. Note that the ab8500 mutex was only needed to support the
>> case where write_masked is not present, and that the debug output
>> on error is pointless because the prcmu driver already writes the same
>> output. The next step would be to remove all the {get,set}_register
>> functions
>> from ab8500 and just call the prcmu directly.
>>
>> Signed-off-by: Arnd Bergmann<arnd@arndb.de>

Patch looks good. I'll test it when I have my board. If it doesn't cause 
any issues I'll apply it to the patch-set ready for pull-request.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 06/15] mfd/ab8500: Remove confusing ab8500-i2c file and merge into ab8500-core

[Mark Brown]

On Fri, May 04, 2012 at 07:23:16PM +0100, Lee Jones wrote:
> ab8500-i2c is used as core code to register the ab8500 device.
> After allocating ab8500 memory, it immediately calls into
> ab8500-core where the real initialisation takes place. This
> patch moves all core registration and memory allocation into
> the true ab8500-core file and removes ab8500-i2c completely.

This idiom is commonly used as many devices support both I2C and SPI
control interfaces.  Does this apply here, or does the device only
support I2C?
-------------- 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/20120507/1039d0a7/attachment.sig>

[PATCH 06/15] mfd/ab8500: Remove confusing ab8500-i2c file and merge into ab8500-core

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> ab8500-i2c is used as core code to register the ab8500 device.
> After allocating ab8500 memory, it immediately calls into
> ab8500-core where the real initialisation takes place. This
> patch moves all core registration and memory allocation into
> the true ab8500-core file and removes ab8500-i2c completely.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

This code is in a separate file for a reason:

git checkout v2.6.37
$ ls drivers/mfd/
ab8500-core.c
ab8500-debugfs.c
ab8500-i2c.c
ab8500-spi.c

So the driver supports different backendsm and at one point
there was both the SPI and "I2C" (actually PRCMU) backend.

And the physical component has an SPI port as well.

However nowadays it's not used, since the component is design to
be used by the PRCMU I2C channel and nothing else, the SPI
mode was just for prototyping and testing. On newer boards
and the Snowball the SPI isn't even routed.

(...)
+static int ab8500_i2c_write(struct ab8500 *ab8500, u16 addr, u8 data)
+static int ab8500_i2c_read(struct ab8500 *ab8500, u16 addr)

As you might have noticed, many people have been confused by
the "i2c" naming of these things. Yes, there is physically an I2C
carrying these out but in Linux terms that's just confusing, it's mixing
hardware and software terminology. Take this opportunity to
rename them:

static int ab8500_prcmu_write(struct ab8500 *ab8500, u16 addr, u8 data)
static int ab8500_prcmu_read(struct ab8500 *ab8500, u16 addr)

etc, so they have a meaningful name.

Yours,
Linus Walleij

[PATCH 06/15] mfd/ab8500: Remove confusing ab8500-i2c file and merge into ab8500-core

[Lee Jones]

On 09/05/12 13:20, Linus Walleij wrote:
> On Fri, May 4, 2012 at 8:23 PM, Lee Jones<lee.jones@linaro.org>  wrote:
>
>> ab8500-i2c is used as core code to register the ab8500 device.
>> After allocating ab8500 memory, it immediately calls into
>> ab8500-core where the real initialisation takes place. This
>> patch moves all core registration and memory allocation into
>> the true ab8500-core file and removes ab8500-i2c completely.
>>
>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
>
> This code is in a separate file for a reason:
>
> git checkout v2.6.37
> $ ls drivers/mfd/
> ab8500-core.c
> ab8500-debugfs.c
> ab8500-i2c.c
> ab8500-spi.c
>
> So the driver supports different backendsm and at one point
> there was both the SPI and "I2C" (actually PRCMU) backend.
>
> And the physical component has an SPI port as well.
>
> However nowadays it's not used, since the component is design to
> be used by the PRCMU I2C channel and nothing else, the SPI
> mode was just for prototyping and testing. On newer boards
> and the Snowball the SPI isn't even routed.
>
> (...)
> +static int ab8500_i2c_write(struct ab8500 *ab8500, u16 addr, u8 data)
> +static int ab8500_i2c_read(struct ab8500 *ab8500, u16 addr)
>
> As you might have noticed, many people have been confused by
> the "i2c" naming of these things. Yes, there is physically an I2C
> carrying these out but in Linux terms that's just confusing, it's mixing
> hardware and software terminology. Take this opportunity to
> rename them:
>
> static int ab8500_prcmu_write(struct ab8500 *ab8500, u16 addr, u8 data)
> static int ab8500_prcmu_read(struct ab8500 *ab8500, u16 addr)
>
> etc, so they have a meaningful name.

Again, something that I'm happy to do, and again something I'd like to 
do as a separate patch (I'll create a patch-set with this and your other 
'clean-up' requests if that's okay with you?)

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 06/15] mfd/ab8500: Remove confusing ab8500-i2c file and merge into ab8500-core

[Linus Walleij]

On Mon, May 14, 2012 at 10:41 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On 09/05/12 13:20, Linus Walleij wrote:

>> (...)
>> +static int ab8500_i2c_write(struct ab8500 *ab8500, u16 addr, u8 data)
>> +static int ab8500_i2c_read(struct ab8500 *ab8500, u16 addr)
>>
>> As you might have noticed, many people have been confused by
>> the "i2c" naming of these things. Yes, there is physically an I2C
>> carrying these out but in Linux terms that's just confusing, it's mixing
>> hardware and software terminology. Take this opportunity to
>> rename them:
>>
>> static int ab8500_prcmu_write(struct ab8500 *ab8500, u16 addr, u8 data)
>> static int ab8500_prcmu_read(struct ab8500 *ab8500, u16 addr)
>>
>> etc, so they have a meaningful name.
>
>
> Again, something that I'm happy to do, and again something I'd like to do as
> a separate patch (I'll create a patch-set with this and your other
> 'clean-up' requests if that's okay with you?)

Sure, what I worry about is that it needs to happen, whether it's done
in 1,2 or 50 patches is just mechanics, as long as the patches are sent.

And Sam can always squash them if they get too plentiful :-)

Yours,
Linus Walleij

[PATCH 07/15] drivers/power: Carry out platform_data error checking on ab8500 devices

[Lee Jones]

In their current state the ab8500 power devices explode if platform data
isn't provided. This patch prevents this from happening and informs the
user of what has happened.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/power/ab8500_btemp.c   |   12 ++++++++----
 drivers/power/ab8500_charger.c |   12 ++++++++----
 drivers/power/ab8500_fg.c      |   12 ++++++++----
 3 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/power/ab8500_btemp.c b/drivers/power/ab8500_btemp.c
index d8bb993..a2ebcb4 100644
--- a/drivers/power/ab8500_btemp.c
+++ b/drivers/power/ab8500_btemp.c
@@ -964,10 +964,15 @@ static int __devinit ab8500_btemp_probe(struct platform_device *pdev)
 {
 	int irq, i, ret = 0;
 	u8 val;
-	struct abx500_bm_plat_data *plat_data;
+	struct abx500_bm_plat_data *plat_data = pdev->dev.platform_data;
+	struct ab8500_btemp *di;
+
+	if (!plat_data) {
+		dev_err(&pdev->dev, "No platform data\n");
+		return -EINVAL;
+	}
 
-	struct ab8500_btemp *di =
-		kzalloc(sizeof(struct ab8500_btemp), GFP_KERNEL);
+	di = kzalloc(sizeof(struct ab8500_btemp), GFP_KERNEL);
 	if (!di)
 		return -ENOMEM;
 
@@ -977,7 +982,6 @@ static int __devinit ab8500_btemp_probe(struct platform_device *pdev)
 	di->gpadc = ab8500_gpadc_get("ab8500-gpadc.0");
 
 	/* get btemp specific platform data */
-	plat_data = pdev->dev.platform_data;
 	di->pdata = plat_data->btemp;
 	if (!di->pdata) {
 		dev_err(di->dev, "no btemp platform data supplied\n");
diff --git a/drivers/power/ab8500_charger.c b/drivers/power/ab8500_charger.c
index e2b4acc..a4e37a8 100644
--- a/drivers/power/ab8500_charger.c
+++ b/drivers/power/ab8500_charger.c
@@ -2534,10 +2534,15 @@ static int __devexit ab8500_charger_remove(struct platform_device *pdev)
 static int __devinit ab8500_charger_probe(struct platform_device *pdev)
 {
 	int irq, i, charger_status, ret = 0;
-	struct abx500_bm_plat_data *plat_data;
+	struct abx500_bm_plat_data *plat_data = pdev->dev.platform_data;
+	struct ab8500_charger *di;
 
-	struct ab8500_charger *di =
-		kzalloc(sizeof(struct ab8500_charger), GFP_KERNEL);
+	if (!plat_data) {
+		dev_err(&pdev->dev, "No platform data\n");
+		return -EINVAL;
+	}
+
+	di = kzalloc(sizeof(struct ab8500_charger), GFP_KERNEL);
 	if (!di)
 		return -ENOMEM;
 
@@ -2550,7 +2555,6 @@ static int __devinit ab8500_charger_probe(struct platform_device *pdev)
 	spin_lock_init(&di->usb_state.usb_lock);
 
 	/* get charger specific platform data */
-	plat_data = pdev->dev.platform_data;
 	di->pdata = plat_data->charger;
 
 	if (!di->pdata) {
diff --git a/drivers/power/ab8500_fg.c b/drivers/power/ab8500_fg.c
index c22f2f0..d88bc47 100644
--- a/drivers/power/ab8500_fg.c
+++ b/drivers/power/ab8500_fg.c
@@ -2446,10 +2446,15 @@ static int __devinit ab8500_fg_probe(struct platform_device *pdev)
 {
 	int i, irq;
 	int ret = 0;
-	struct abx500_bm_plat_data *plat_data;
+	struct abx500_bm_plat_data *plat_data = pdev->dev.platform_data;
+	struct ab8500_fg *di;
+
+	if (!plat_data) {
+		dev_err(&pdev->dev, "No platform data\n");
+		return -EINVAL;
+	}
 
-	struct ab8500_fg *di =
-		kzalloc(sizeof(struct ab8500_fg), GFP_KERNEL);
+	di = kzalloc(sizeof(struct ab8500_fg), GFP_KERNEL);
 	if (!di)
 		return -ENOMEM;
 
@@ -2461,7 +2466,6 @@ static int __devinit ab8500_fg_probe(struct platform_device *pdev)
 	di->gpadc = ab8500_gpadc_get("ab8500-gpadc.0");
 
 	/* get fg specific platform data */
-	plat_data = pdev->dev.platform_data;
 	di->pdata = plat_data->fg;
 	if (!di->pdata) {
 		dev_err(di->dev, "no fg platform data supplied\n");
-- 
1.7.9.5

[PATCH 07/15] drivers/power: Carry out platform_data error checking on ab8500 devices

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> In their current state the ab8500 power devices explode if platform data
> isn't provided. This patch prevents this from happening and informs the
> user of what has happened.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

This or equivalent is already submitted and merged by Anton into the
power tree I think?

Yours,
Linus Walleij

[PATCH 07/15] drivers/power: Carry out platform_data error checking on ab8500 devices

[Lee Jones]

On 09/05/12 09:51, Linus Walleij wrote:
> On Fri, May 4, 2012 at 8:23 PM, Lee Jones<lee.jones@linaro.org>  wrote:
>
>> In their current state the ab8500 power devices explode if platform data
>> isn't provided. This patch prevents this from happening and informs the
>> user of what has happened.
>>
>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
>
> This or equivalent is already submitted and merged by Anton into the
> power tree I think?

It is, and I sent him a derivative of this patch which he has also 
applied, hence his can now be ignored.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 08/15] ARM: ux500: PRCMU related configuration and layout corrections for Device Tree

[Lee Jones]

Apply db8500 related PRCMU Device Tree settings and clean up some formatting
errors. We also remove one of the PRCMU assigned IRQs, as it is currently not
used.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/db8500.dtsi |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
index 9a62f86..1188925 100644
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -187,12 +187,12 @@
 		prcmu at 80157000 {
 			compatible = "stericsson,db8500-prcmu";
 			reg = <0x80157000 0x1000>;
-			interrupts = <46 47>;
+			interrupts = <0 47 0x4>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
 
-				prcmu-timer-4 at 80157450 {
+			prcmu-timer-4 at 80157450 {
 				compatible = "stericsson,db8500-prcmu-timer-4";
 				reg = <0x80157450 0xC>;
 			};
@@ -201,6 +201,7 @@
 				compatible = "stericsson,ab8500";
 				reg = <5>; /* mailbox 5 is i2c */
 				interrupts = <0 40 0x4>;
+				stericsson,irq-base = <520>;
 			};
 		};
 
-- 
1.7.9.5

[PATCH 08/15] ARM: ux500: PRCMU related configuration and layout corrections for Device Tree

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> Apply db8500 related PRCMU Device Tree settings and clean up some formatting
> errors. We also remove one of the PRCMU assigned IRQs, as it is currently not
> used.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
(...)
> - ? ? ? ? ? ? ? ? ? ? ? interrupts = <46 47>;
> + ? ? ? ? ? ? ? ? ? ? ? interrupts = <0 47 0x4>;

Sorry I don't get it, why is two values in decimal and one in hex?

Please choose one and stick to it...

Yours,
Linus Walleij

[PATCH 08/15] ARM: ux500: PRCMU related configuration and layout corrections for Device Tree

[Lee Jones]

On 09/05/12 09:53, Linus Walleij wrote:
> On Fri, May 4, 2012 at 8:23 PM, Lee Jones<lee.jones@linaro.org>  wrote:
>
>> Apply db8500 related PRCMU Device Tree settings and clean up some formatting
>> errors. We also remove one of the PRCMU assigned IRQs, as it is currently not
>> used.
>>
>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
> (...)
>> -                       interrupts =<46 47>;
>> +                       interrupts =<0 47 0x4>;
>
> Sorry I don't get it, why is two values in decimal and one in hex?
>
> Please choose one and stick to it...

Because the one on the end is a mask.

<[PPI|SPI] [IRQ Number] [Configuration mask]>


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 08/15] ARM: ux500: PRCMU related configuration and layout corrections for Device Tree

[Linus Walleij]

On Wed, May 9, 2012 at 12:27 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On 09/05/12 09:53, Linus Walleij wrote:
>>
>> On Fri, May 4, 2012 at 8:23 PM, Lee Jones<lee.jones@linaro.org> ?wrote:
>>
>>> Apply db8500 related PRCMU Device Tree settings and clean up some
>>> formatting
>>> errors. We also remove one of the PRCMU assigned IRQs, as it is currently
>>> not
>>> used.
>>>
>>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
>>
>> (...)
>>>
>>> - ? ? ? ? ? ? ? ? ? ? ? interrupts =<46 47>;
>>> + ? ? ? ? ? ? ? ? ? ? ? interrupts =<0 47 0x4>;
>>
>>
>> Sorry I don't get it, why is two values in decimal and one in hex?
>>
>> Please choose one and stick to it...
>
> Because the one on the end is a mask.
>
> <[PPI|SPI] [IRQ Number] [Configuration mask]>

...and I guess there must be a precedent for using hex only
for any mask value. OK then, I'm not a DT person anyway.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 09/15] drivers/mfd: Enable Device Tree support for the db8500-prcmu

[Lee Jones]

This patch will enable probing to occur during a Device Tree enabled
boot. The IRQ base is expected to be located in and will be fetched
from the DT itself. We also prevent any of the db8500 regulators
from being registered here, as they will be enabled via DT instead.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/db8500-prcmu.c |   29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 7a83bf0..9bc75a8 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -2960,7 +2960,8 @@ static struct mfd_cell db8500_prcmu_devs[] = {
  */
 static int __devinit db8500_prcmu_probe(struct platform_device *pdev)
 {
-	int err = 0;
+	struct device_node *np = pdev->dev.of_node;
+	int irq = 0, err = 0;
 
 	if (ux500_is_svp())
 		return -ENODEV;
@@ -2970,8 +2971,14 @@ static int __devinit db8500_prcmu_probe(struct platform_device *pdev)
 	/* Clean up the mailbox interrupts after pre-kernel code. */
 	writel(ALL_MBOX_BITS, PRCM_ARM_IT1_CLR);
 
-	err = request_threaded_irq(IRQ_DB8500_PRCMU1, prcmu_irq_handler,
-		prcmu_irq_thread_fn, IRQF_NO_SUSPEND, "prcmu", NULL);
+	if (np)
+		irq = platform_get_irq(pdev, 0);
+
+	if (!np || irq <= 0)
+		irq = IRQ_DB8500_PRCMU1;
+
+	err = request_threaded_irq(irq, prcmu_irq_handler,
+	        prcmu_irq_thread_fn, IRQF_NO_SUSPEND, "prcmu", NULL);
 	if (err < 0) {
 		pr_err("prcmu: Failed to allocate IRQ_DB8500_PRCMU1.\n");
 		err = -EBUSY;
@@ -2981,14 +2988,16 @@ static int __devinit db8500_prcmu_probe(struct platform_device *pdev)
 	if (cpu_is_u8500v20_or_later())
 		prcmu_config_esram0_deep_sleep(ESRAM0_DEEP_SLEEP_STATE_RET);
 
-	err = mfd_add_devices(&pdev->dev, 0, db8500_prcmu_devs,
-			      ARRAY_SIZE(db8500_prcmu_devs), NULL,
-			      0);
+	if (!np) {
+		err = mfd_add_devices(&pdev->dev, 0, db8500_prcmu_devs,
+				ARRAY_SIZE(db8500_prcmu_devs), NULL, 0);
+		if (err) {
+			pr_err("prcmu: Failed to add subdevices\n");
+			return err;
+		}
+	}
 
-	if (err)
-		pr_err("prcmu: Failed to add subdevices\n");
-	else
-		pr_info("DB8500 PRCMU initialized\n");
+	pr_info("DB8500 PRCMU initialized\n");
 
 no_irq_return:
 	return err;
-- 
1.7.9.5

[PATCH 09/15] drivers/mfd: Enable Device Tree support for the db8500-prcmu

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> This patch will enable probing to occur during a Device Tree enabled
> boot. The IRQ base is expected to be located in and will be fetched
> from the DT itself. We also prevent any of the db8500 regulators
> from being registered here, as they will be enabled via DT instead.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 09/15] drivers/mfd: Enable Device Tree support for the db8500-prcmu

[Samuel Ortiz]

Hi Lee,

On Fri, May 04, 2012 at 07:23:19PM +0100, Lee Jones wrote:
> This patch will enable probing to occur during a Device Tree enabled
> boot. The IRQ base is expected to be located in and will be fetched
> from the DT itself. We also prevent any of the db8500 regulators
> from being registered here, as they will be enabled via DT instead.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mfd/db8500-prcmu.c |   29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
Patch applied, thanks.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 10/15] drivers/mfd: db8500-prcmu: Add support for regulator supply for nmk-i2c.4

[Lee Jones]

This applies a supply alias for the db8500's fifth Nomadik i2c port.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/db8500-prcmu.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 9bc75a8..d534449 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -2720,6 +2720,7 @@ static struct regulator_consumer_supply db8500_vape_consumers[] = {
 	REGULATOR_SUPPLY("v-i2c", "nmk-i2c.1"),
 	REGULATOR_SUPPLY("v-i2c", "nmk-i2c.2"),
 	REGULATOR_SUPPLY("v-i2c", "nmk-i2c.3"),
+	REGULATOR_SUPPLY("v-i2c", "nmk-i2c.4"),
 	/* "v-mmc" changed to "vcore" in the mainline kernel */
 	REGULATOR_SUPPLY("vcore", "sdi0"),
 	REGULATOR_SUPPLY("vcore", "sdi1"),
-- 
1.7.9.5

[PATCH 10/15] drivers/mfd: db8500-prcmu: Add support for regulator supply for nmk-i2c.4

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> This applies a supply alias for the db8500's fifth Nomadik i2c port.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 10/15] drivers/mfd: db8500-prcmu: Add support for regulator supply for nmk-i2c.4

[Samuel Ortiz]

Hi Lee,

On Fri, May 04, 2012 at 07:23:20PM +0100, Lee Jones wrote:
> This applies a supply alias for the db8500's fifth Nomadik i2c port.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mfd/db8500-prcmu.c |    1 +
>  1 file changed, 1 insertion(+)
Applied as well, thanks.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver

[Lee Jones]

This patch will allow the ab8500-core driver to be probed and set up
when booting when Device Tree is enabled. This includes platform ID
look-up which identifies the machine it is currently running on. If
we are undergoing a DT enabled boot, we will refuse to setup each of
the other ab8500-* devices, as they will be probed individually by DT.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/ab8500-core.c |   62 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 74e6661..9f8547e 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -20,6 +20,8 @@
 #include <linux/mfd/abx500/ab8500.h>
 #include <linux/mfd/dbx500-prcmu.h>
 #include <linux/regulator/ab8500.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 /*
  * Interrupt register offsets
@@ -1039,11 +1041,20 @@ static struct attribute_group ab9540_attr_group = {
 	.attrs	= ab9540_sysfs_entries,
 };
 
+static const struct of_device_id ab8500_match[] = {
+	{
+		.compatible = "stericsson,ab8500",
+		.data = (void *)AB8500_VERSION_AB8500,
+	},
+	{},
+};
+
 static int __devinit ab8500_probe(struct platform_device *pdev)
 {
 	struct ab8500_platform_data *plat = dev_get_platdata(&pdev->dev);
 	const struct platform_device_id *platid = platform_get_device_id(pdev);
-	enum ab8500_version version = platid->driver_data;
+	enum ab8500_version version = AB8500_VERSION_UNDEFINED;
+	struct device_node *np = pdev->dev.of_node;
 	struct ab8500 *ab8500;
 	struct resource *resource;
 	int ret;
@@ -1056,6 +1067,14 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
 
 	if (plat)
 		ab8500->irq_base = plat->irq_base;
+	else if (np)
+		ret = of_property_read_u32(np, "stericsson,irq-base", &ab8500->irq_base);
+
+	if (ab8500->irq_base == 0) {
+		dev_info(&pdev->dev, "couldn't find irq-base\n");
+		ret = -EINVAL;
+		goto out_free_ab8500;
+	}
 
 	ab8500->dev = &pdev->dev;
 
@@ -1076,6 +1095,12 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ab8500);
 
+	if (platid)
+		version = platid->driver_data;
+	else if (np)
+		version = (unsigned int)
+			of_match_device(ab8500_match, &pdev->dev)->data;
+
 	if (version != AB8500_VERSION_UNDEFINED)
 		ab8500->version = version;
 	else {
@@ -1172,23 +1197,25 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
 			goto out_removeirq;
 	}
 
-	ret = mfd_add_devices(ab8500->dev, 0, abx500_common_devs,
-			      ARRAY_SIZE(abx500_common_devs), NULL,
-			      ab8500->irq_base);
+	if (!np) {
+		ret = mfd_add_devices(ab8500->dev, 0, abx500_common_devs,
+				ARRAY_SIZE(abx500_common_devs), NULL,
+				ab8500->irq_base);
 
-	if (ret)
-		goto out_freeirq;
-
-	if (is_ab9540(ab8500))
-		ret = mfd_add_devices(ab8500->dev, 0, ab9540_devs,
-			      ARRAY_SIZE(ab9540_devs), NULL,
-			      ab8500->irq_base);
-	else
-		ret = mfd_add_devices(ab8500->dev, 0, ab8500_devs,
-			      ARRAY_SIZE(ab9540_devs), NULL,
-			      ab8500->irq_base);
-	if (ret)
-		goto out_freeirq;
+		if (ret)
+			goto out_freeirq;
+
+		if (is_ab9540(ab8500))
+			ret = mfd_add_devices(ab8500->dev, 0, ab9540_devs,
+					ARRAY_SIZE(ab9540_devs), NULL,
+					ab8500->irq_base);
+		else
+			ret = mfd_add_devices(ab8500->dev, 0, ab8500_devs,
+					ARRAY_SIZE(ab9540_devs), NULL,
+					ab8500->irq_base);
+		if (ret)
+			goto out_freeirq;
+	}
 
 	if (is_ab9540(ab8500))
 		ret = sysfs_create_group(&ab8500->dev->kobj,
@@ -1249,6 +1276,7 @@ static struct platform_driver ab8500_core_driver = {
 	.driver = {
 		.name = "ab8500-core",
 		.owner = THIS_MODULE,
+		.of_match_table = ab8500_match,
 	},
 	.probe	= ab8500_probe,
 	.remove	= __devexit_p(ab8500_remove),
-- 
1.7.9.5

[PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> This patch will allow the ab8500-core driver to be probed and set up
> when booting when Device Tree is enabled. This includes platform ID
> look-up which identifies the machine it is currently running on. If
> we are undergoing a DT enabled boot, we will refuse to setup each of
> the other ab8500-* devices, as they will be probed individually by DT.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
(...)
> + ? ? ? else if (np)
> + ? ? ? ? ? ? ? ret = of_property_read_u32(np, "stericsson,irq-base", &ab8500->irq_base);
> +
> + ? ? ? if (ab8500->irq_base == 0) {

Shouldn't this be (av8500->irq_base == NO_IRQ) now that we're tranisitioning to
use 0 as NO_IRQ?

Apart from that it looks good to me:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver

[Lee Jones]

On 09/05/12 10:02, Linus Walleij wrote:
> On Fri, May 4, 2012 at 8:23 PM, Lee Jones<lee.jones@linaro.org>  wrote:
>
>> This patch will allow the ab8500-core driver to be probed and set up
>> when booting when Device Tree is enabled. This includes platform ID
>> look-up which identifies the machine it is currently running on. If
>> we are undergoing a DT enabled boot, we will refuse to setup each of
>> the other ab8500-* devices, as they will be probed individually by DT.
>>
>> Signed-off-by: Lee Jones<lee.jones@linaro.org>
> (...)
>> +       else if (np)
>> +               ret = of_property_read_u32(np, "stericsson,irq-base",&ab8500->irq_base);
>> +
>> +       if (ab8500->irq_base == 0) {
>
> Shouldn't this be (av8500->irq_base == NO_IRQ) now that we're tranisitioning to
> use 0 as NO_IRQ?

Sure, why not.

> Apart from that it looks good to me:
> Acked-by: Linus Walleij<linus.walleij@linaro.org>


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver

[Mark Brown]

On Wed, May 09, 2012 at 11:28:49AM +0100, Lee Jones wrote:

> >Shouldn't this be (av8500->irq_base == NO_IRQ) now that we're tranisitioning to
> >use 0 as NO_IRQ?

> Sure, why not.

I think the endgame is to phase out NO_IRQ entirely...
-------------- 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/20120509/cbb73bbc/attachment-0001.sig>

[PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver

[Arnd Bergmann]

On Wednesday 09 May 2012, Linus Walleij wrote:
> > +       else if (np)
> > +               ret = of_property_read_u32(np, "stericsson,irq-base", &ab8500->irq_base);
> > +
> > +       if (ab8500->irq_base == 0) {
> 
> Shouldn't this be (av8500->irq_base == NO_IRQ) now that we're tranisitioning to
> use 0 as NO_IRQ?

Since we're into bike-shedding already, why not make it :? :-)

	if (!ab8500->irq_base)

I usually prefer this syntax for testing if something has been assigned,
while I use a comparison with 0 only when it is a meant as a numeric value
rather than meant as validity test.

	Arnd

[PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver

[Russell King - ARM Linux]

On Wed, May 09, 2012 at 11:02:27AM +0200, Linus Walleij wrote:
> On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > This patch will allow the ab8500-core driver to be probed and set up
> > when booting when Device Tree is enabled. This includes platform ID
> > look-up which identifies the machine it is currently running on. If
> > we are undergoing a DT enabled boot, we will refuse to setup each of
> > the other ab8500-* devices, as they will be probed individually by DT.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> (...)
> > + ? ? ? else if (np)
> > + ? ? ? ? ? ? ? ret = of_property_read_u32(np, "stericsson,irq-base", &ab8500->irq_base);
> > +
> > + ? ? ? if (ab8500->irq_base == 0) {
> 
> Shouldn't this be (av8500->irq_base == NO_IRQ) now that we're tranisitioning to
> use 0 as NO_IRQ?

No it shouldn't.  We want new drivers to be rejecting on explicitly zero
IRQ numbers and not using NO_IRQ at all.  We want to get rid of NO_IRQ
entirely.

At the moment, NO_IRQ is a marker for "this is a mistake which needs
fixing" and allows them to be found by grep.

What we need is more of an effort so that platforms do not start numbering
IRQs at zero, but start at one.

(I could fix up SA11x0, which would then allow the SA1111 code to be fixed,
but I've not had time to look at that during this last cycle.  Others need
their maintainers who know the code to fix them.)

I brought up the issue of the new introduction of NO_IRQ in the previously
clean versatile express code, and failed to get any reply.

So, I think what I'll do when I've eliminated all those that I can do is
to remove the definition from our asm/irq.h and cause a load of build
errors in linux-next.  This seems to be the _only_ way to motivate people
into fixing these kinds of issues.  Which is awfully sad.  I'd much
rather people just had the carrot instead but as ever its proven many
times that people just ignore such things.

[PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver

[Linus Walleij]

On Thu, May 10, 2012 at 12:26 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, May 09, 2012 at 11:02:27AM +0200, Linus Walleij wrote:

>> Shouldn't this be (av8500->irq_base == NO_IRQ) now that we're tranisitioning to
>> use 0 as NO_IRQ?
>
> No it shouldn't. ?We want new drivers to be rejecting on explicitly zero
> IRQ numbers and not using NO_IRQ at all. ?We want to get rid of NO_IRQ
> entirely.

Yes of course, it's strange how I can go astray sometimes ..
(!av8500->irq_base) it is.

Thanks Russell,
Linus Walleij

[PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver

[Samuel Ortiz]

Hi Lee,

On Fri, May 04, 2012 at 07:23:21PM +0100, Lee Jones wrote:
> This patch will allow the ab8500-core driver to be probed and set up
> when booting when Device Tree is enabled. This includes platform ID
> look-up which identifies the machine it is currently running on. If
> we are undergoing a DT enabled boot, we will refuse to setup each of
> the other ab8500-* devices, as they will be probed individually by DT.
This one doesn't apply to my for-next branch. Which tree/branch did you use to
generate it ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver

[Lee Jones]

Hi Sam,

> On Fri, May 04, 2012 at 07:23:21PM +0100, Lee Jones wrote:
>> This patch will allow the ab8500-core driver to be probed and set up
>> when booting when Device Tree is enabled. This includes platform ID
>> look-up which identifies the machine it is currently running on. If
>> we are undergoing a DT enabled boot, we will refuse to setup each of
>> the other ab8500-* devices, as they will be probed individually by DT.
> This one doesn't apply to my for-next branch. Which tree/branch did you use to
> generate it ?

I've been using the latest Mainline -rc, but one of my first jobs today 
is to rebase this patch-set against -next and resubmit.

Sorry for any confusion caused.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 12/15] drivers/regulator: ab8500: Split up probe() into manageable pieces

[Lee Jones]

ab8500's probe() function is becoming quite large, so in the lead
up to Device Tree enablement which will fork the thread of execution
this patch splits it into 3 main areas; basic error checking will
remain in probe(), but regulator register initialisation and regulator
registration have been moved to their own functions which will
be called in sequence by probe() and the DT equivalent.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/regulator/ab8500.c |  144 ++++++++++++++++++++++++++------------------
 1 file changed, 85 insertions(+), 59 deletions(-)

diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
index c7ee4c1..e403a0f 100644
--- a/drivers/regulator/ab8500.c
+++ b/drivers/regulator/ab8500.c
@@ -735,6 +735,86 @@ static struct ab8500_reg_init ab8500_reg_init[] = {
 	REG_INIT(AB8500_REGUCTRLDISCH2,		0x04, 0x44, 0x16),
 };
 
+static __devinit int
+ab8500_regulator_init_registers(struct platform_device *pdev, int id, int value)
+{
+	int err;
+
+	if (value & ~ab8500_reg_init[id].mask) {
+		dev_err(&pdev->dev,
+			"Configuration error: value outside mask.\n");
+		return -EINVAL;
+	}
+
+	err = abx500_mask_and_set_register_interruptible(
+		&pdev->dev,
+		ab8500_reg_init[id].bank,
+		ab8500_reg_init[id].addr,
+		ab8500_reg_init[id].mask,
+		value);
+	if (err < 0) {
+		dev_err(&pdev->dev,
+			"Failed to initialize 0x%02x, 0x%02x.\n",
+			ab8500_reg_init[id].bank,
+			ab8500_reg_init[id].addr);
+		return err;
+	}
+
+	dev_vdbg(&pdev->dev,
+		"init: 0x%02x, 0x%02x, 0x%02x, 0x%02x\n",
+		ab8500_reg_init[id].bank,
+		ab8500_reg_init[id].addr,
+		ab8500_reg_init[id].mask,
+		value);
+
+	return 0;
+}
+
+static __devinit int ab8500_regulator_register(struct platform_device *pdev,
+					struct regulator_init_data *init_data,
+					int id,
+					struct device_node *np)
+{
+	struct ab8500_regulator_info *info = NULL;
+	int err;
+
+	/* assign per-regulator data */
+	info = &ab8500_regulator_info[id];
+	info->dev = &pdev->dev;
+
+	/* fix for hardware before ab8500v2.0 */
+	if (abx500_get_chip_id(info->dev) < 0x20) {
+		if (info->desc.id == AB8500_LDO_AUX3) {
+			info->desc.n_voltages =
+				ARRAY_SIZE(ldo_vauxn_voltages);
+			info->voltages = ldo_vauxn_voltages;
+			info->voltages_len =
+				ARRAY_SIZE(ldo_vauxn_voltages);
+			info->voltage_mask = 0xf;
+		}
+	}
+
+	/* register regulator with framework */
+	info->regulator = regulator_register(&info->desc, &pdev->dev,
+					init_data, info, np);
+	if (IS_ERR(info->regulator)) {
+		err = PTR_ERR(info->regulator);
+		pr_err("failed to register regulator %s\n",
+			info->desc.name);
+		/* when we fail, un-register all earlier regulators */
+		while (--id >= 0) {
+			info = &ab8500_regulator_info[id];
+			regulator_unregister(info->regulator);
+		}
+		return err;
+	}
+
+	dev_vdbg(rdev_get_dev(info->regulator),
+		"%s-probed\n", info->desc.name);
+
+	return 0;
+}
+
 static __devinit int ab8500_regulator_probe(struct platform_device *pdev)
 {
 	struct ab8500 *ab8500 = dev_get_drvdata(pdev->dev.parent);
@@ -759,8 +839,7 @@ static __devinit int ab8500_regulator_probe(struct platform_device *pdev)
 
 	/* initialize registers */
 	for (i = 0; i < pdata->num_regulator_reg_init; i++) {
-		int id;
-		u8 value;
+		int id, value;
 
 		id = pdata->regulator_reg_init[i].id;
 		value = pdata->regulator_reg_init[i].value;
@@ -771,70 +850,17 @@ static __devinit int ab8500_regulator_probe(struct platform_device *pdev)
 				"Configuration error: id outside range.\n");
 			return -EINVAL;
 		}
-		if (value & ~ab8500_reg_init[id].mask) {
-			dev_err(&pdev->dev,
-				"Configuration error: value outside mask.\n");
-			return -EINVAL;
-		}
 
-		/* initialize register */
-		err = abx500_mask_and_set_register_interruptible(&pdev->dev,
-			ab8500_reg_init[id].bank,
-			ab8500_reg_init[id].addr,
-			ab8500_reg_init[id].mask,
-			value);
-		if (err < 0) {
-			dev_err(&pdev->dev,
-				"Failed to initialize 0x%02x, 0x%02x.\n",
-				ab8500_reg_init[id].bank,
-				ab8500_reg_init[id].addr);
+		err = ab8500_regulator_init_registers(pdev, id, value);
+		if (err < 0)
 			return err;
-		}
-		dev_vdbg(&pdev->dev,
-			"  init: 0x%02x, 0x%02x, 0x%02x, 0x%02x\n",
-			ab8500_reg_init[id].bank,
-			ab8500_reg_init[id].addr,
-			ab8500_reg_init[id].mask,
-			value);
 	}
 
 	/* register all regulators */
 	for (i = 0; i < ARRAY_SIZE(ab8500_regulator_info); i++) {
-		struct ab8500_regulator_info *info = NULL;
-
-		/* assign per-regulator data */
-		info = &ab8500_regulator_info[i];
-		info->dev = &pdev->dev;
-
-		/* fix for hardware before ab8500v2.0 */
-		if (abx500_get_chip_id(info->dev) < 0x20) {
-			if (info->desc.id == AB8500_LDO_AUX3) {
-				info->desc.n_voltages =
-					ARRAY_SIZE(ldo_vauxn_voltages);
-				info->voltages = ldo_vauxn_voltages;
-				info->voltages_len =
-					ARRAY_SIZE(ldo_vauxn_voltages);
-				info->voltage_mask = 0xf;
-			}
-		}
-
-		/* register regulator with framework */
-		info->regulator = regulator_register(&info->desc, &pdev->dev,
-				&pdata->regulator[i], info, NULL);
-		if (IS_ERR(info->regulator)) {
-			err = PTR_ERR(info->regulator);
-			dev_err(&pdev->dev, "failed to register regulator %s\n",
-					info->desc.name);
-			/* when we fail, un-register all earlier regulators */
-			while (--i >= 0) {
-				info = &ab8500_regulator_info[i];
-				regulator_unregister(info->regulator);
-			}
+		err = ab8500_regulator_register(pdev, &pdata->regulator[i], i, NULL);
+		if (err < 0)
 			return err;
-		}
-
-		dev_vdbg(rdev_get_dev(info->regulator),
-			"%s-probed\n", info->desc.name);
 	}
 
 	return 0;
-- 
1.7.9.5

[PATCH 12/15] drivers/regulator: ab8500: Split up probe() into manageable pieces

[Mark Brown]

On Fri, May 04, 2012 at 07:23:22PM +0100, Lee Jones wrote:
> ab8500's probe() function is becoming quite large, so in the lead
> up to Device Tree enablement which will fork the thread of execution

This doesn't apply against current regulator code.  Please also at least
try to use changelogs that match the subsystem you're submitting
against.
-------------- 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/20120507/6b2965d7/attachment.sig>

[PATCH 12/15] drivers/regulator: ab8500: Split up probe() into manageable pieces

[Mark Brown]

On Mon, May 07, 2012 at 07:04:59PM +0100, Lee Jones wrote:
> On 7 May 2012 17:58, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> > This doesn't apply against current regulator code.

> It doesn't? I rebased it onto -rc5 which was the latest at time of
> submission.

This isn't a bugfix that should be rushed into Linus' tree for 3.4!
You're missing the best part of a release cycle of development here,
including at least one incompatible API change.  

If you're developing new code you should always submit against the tree
it's supposed to be applied to; as a rule of thumb -next is a good
approximation though subsystems do things slightly differently,
especially in the creation of topic branches.  Even where the code will
get applied to Linus' tree then merged as a new topic branch you should
verify that there aren't any other changes in -next that affect it (eg,
an API you rely on having changed or other changes to the same file
which conflict with yours).

> > Please also at least
> > try to use changelogs that match the subsystem you're submitting
> > against.

> What do you mean? Isn't "drivers/regulator" in the subject line enough?

As I said you should use changelogs that match the subsystem you are
submitting against.  Essentially nothing in the regulator tree uses
this, as you should be able to see from inspection of the changelogs:

$ git shortlog next/master drivers/regulator | wc -l
1059
$ git shortlog drivers/regulator | grep drivers/regulator | wc -l
6

Generally everything uses "regulator: ".  You should always make an
effort to do this for whatever subsystem you're working with.
-------------- 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/20120507/5d89fce0/attachment.sig>

[PATCH 12/15] drivers/regulator: ab8500: Split up probe() into manageable pieces

[Lee Jones]

On 07/05/12 19:44, Mark Brown wrote:
> On Mon, May 07, 2012 at 07:04:59PM +0100, Lee Jones wrote:
>> On 7 May 2012 17:58, Mark Brown<broonie@opensource.wolfsonmicro.com>  wrote:
>
>>> This doesn't apply against current regulator code.
>
>> It doesn't? I rebased it onto -rc5 which was the latest at time of
>> submission.
>
> This isn't a bugfix that should be rushed into Linus' tree for 3.4!
> You're missing the best part of a release cycle of development here,
> including at least one incompatible API change.
>
> If you're developing new code you should always submit against the tree
> it's supposed to be applied to; as a rule of thumb -next is a good
> approximation though subsystems do things slightly differently,
> especially in the creation of topic branches.  Even where the code will
> get applied to Linus' tree then merged as a new topic branch you should
> verify that there aren't any other changes in -next that affect it (eg,
> an API you rely on having changed or other changes to the same file
> which conflict with yours).

Okay, I will pull these patches out of the patch-set and rebase them 
against -next before re-sending.

>>> Please also at least
>>> try to use changelogs that match the subsystem you're submitting
>>> against.
>
>> What do you mean? Isn't "drivers/regulator" in the subject line enough?
>
> As I said you should use changelogs that match the subsystem you are
> submitting against.  Essentially nothing in the regulator tree uses
> this, as you should be able to see from inspection of the changelogs:
>
> $ git shortlog next/master drivers/regulator | wc -l
> 1059
> $ git shortlog drivers/regulator | grep drivers/regulator | wc -l
> 6
>
> Generally everything uses "regulator: ".  You should always make an
> effort to do this for whatever subsystem you're working with.

I am happy to change the subject line to make you happy. :)

I'll make the requested changes and re-send early next week - as I'm off 
this week.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 13/15] ARM: ux500: Add support for ab8500 regulators into the Device Tree

[Lee Jones]

Here we supply the information required to setup regulators
successfully on Snowball and other db8500 variants which use
the ab8500 regulators. This information includes values used
for regulator related register initialisation and platform
specific regulator setup information.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/db8500.dtsi  |   98 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/snowball.dts |    2 +
 2 files changed, 100 insertions(+)

diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
index 1188925..2609f53 100644
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -202,6 +202,104 @@
 				reg = <5>; /* mailbox 5 is i2c */
 				interrupts = <0 40 0x4>;
 				stericsson,irq-base = <520>;
+
+				ab8500-regulators {
+					compatible = "stericsson,ab8500-regulator";
+					stericsson,regurequestctrl2 = <0x00>;
+					stericsson,regurequestctrl3 = <0x00>;
+					stericsson,regurequestctrl4 = <0x00>;
+					stericsson,regusysclkreq1hpvalid1 = <0x00>;
+					stericsson,regusysclkreq1hpvalid2 = <0x40>;
+					stericsson,reguhwhpreq1valid1 = <0x00>;
+					stericsson,reguhwhpreq1valid2 = <0x00>;
+					stericsson,reguhwhpreq2valid1 = <0x00>;
+					stericsson,reguhwhpreq2valid2 = <0x04>;
+					stericsson,reguswhpreqvalid1 = <0x00>;
+					stericsson,reguswhpreqvalid2 = <0x00>;
+					stericsson,regusysclkreqvalid1 = <0x2a>;
+					stericsson,regusysclkreqvalid2 = <0x20>;
+					stericsson,regumisc1 = <0x10>;
+					stericsson,vaudiosupply = <0x00>;
+					stericsson,reguctrl1vamic = <0x00>;
+					stericsson,vpllvanaregu = <0x02>;
+					stericsson,vrefddr = <0x00>;
+					stericsson,extsupplyregu = <0x2a>;
+					stericsson,vaux12regu = <0x01>;
+					stericsson,vrf1vaux3regu = <0x00>;
+					stericsson,vsmps1sel1 = <0x24>;
+					stericsson,vaux1sel = <0x08>;
+					stericsson,vaux2sel = <0x0d>;
+					stericsson,vrf1vaux3sel = <0x07>;
+					stericsson,reguctrl2spare = <0x00>;
+					stericsson,reguctrldisch = <0x00>;
+					stericsson,reguctrldisch2 = <0x00>;
+
+					/* It's imperative that these remain in order. */
+
+					// supplies to the display/camera
+					ab8500_ldo_aux1_reg: ab8500_ldo_aux1 {
+						regulator-name = "V-DISPLAY";
+						regulator-min-microvolt = <2500000>;
+						regulator-max-microvolt = <2900000>;
+						regulator-boot-on;
+						/* BUG: If turned off MMC will be affected. */
+						regulator-always-on;
+					};
+
+					// supplies to the on-board eMMC
+					ab8500_ldo_aux2_reg: ab8500_ldo_aux2 {
+						regulator-name = "V-eMMC1";
+						regulator-min-microvolt = <1100000>;
+						regulator-max-microvolt = <3300000>;
+					};
+
+					// supply for VAUX3; SDcard slots
+					ab8500_ldo_aux3_reg: ab8500_ldo_aux3 {
+						regulator-name = "V-MMC-SD";
+						regulator-min-microvolt = <1100000>;
+						regulator-max-microvolt = <3300000>;
+					};
+
+					// supply for v-intcore12; VINTCORE12 LDO
+					ab8500_ldo_initcore_reg: ab8500_ldo_initcore {
+						regulator-name = "V-INTCORE";
+					};
+
+					// supply for tvout; gpadc; TVOUT LDO
+					ab8500_ldo_tvout_reg: ab8500_ldo_tvout {
+						regulator-name = "V-TVOUT";
+					};
+
+					// supply for ab8500-usb; USB LDO
+					ab8500_ldo_usb_reg: ab8500_ldo_usb {
+						regulator-name = "dummy";
+					};
+
+					// supply for ab8500-vaudio; VAUDIO LDO
+					ab8500_ldo_audio_reg: ab8500_ldo_audio {
+						regulator-name = "V-AUD";
+					};
+
+					// supply for v-anamic1 VAMic1-LDO
+					ab8500_ldo_anamic1_reg: ab8500_ldo_anamic1 {
+						regulator-name = "V-AMIC1";
+					};
+
+					// supply for v-amic2; VAMIC2 LDO; reuse constants for AMIC1
+					ab8500_ldo_amamic2_reg: ab8500_ldo_amamic2 {
+						regulator-name = "V-AMIC2";
+					};
+
+					// supply for v-dmic; VDMIC LDO
+					ab8500_ldo_dmic_reg: ab8500_ldo_dmic {
+						regulator-name = "V-DMIC";
+					};
+
+					// supply for U8500 CSI/DSI; VANA LDO
+					ab8500_ldo_ana_reg: ab8500_ldo_ana {
+						regulator-name = "V-CSI/DSI";
+					};
+				};
 			};
 		};
 
diff --git a/arch/arm/boot/dts/snowball.dts b/arch/arm/boot/dts/snowball.dts
index 579c304..6b00350 100644
--- a/arch/arm/boot/dts/snowball.dts
+++ b/arch/arm/boot/dts/snowball.dts
@@ -94,6 +94,7 @@
 			arm,primecell-periphid = <0x10480180>;
 			max-frequency = <50000000>;
 			bus-width = <8>;
+			vmmc-supply = <&ab8500_ldo_aux3_reg>;
 			mmc-cap-mmc-highspeed;
 
 			cd-gpios  = <&gpio6 26 0x4>; // 218
@@ -107,6 +108,7 @@
 			arm,primecell-periphid = <0x10480180>;
 		        max-frequency = <50000000>;
 			bus-width = <8>;
+			vmmc-supply = <&ab8500_ldo_aux2_reg>;
 			mmc-cap-mmc-highspeed;
 
 			status = "okay";
-- 
1.7.9.5

[PATCH 13/15] ARM: ux500: Add support for ab8500 regulators into the Device Tree

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> Here we supply the information required to setup regulators
> successfully on Snowball and other db8500 variants which use
> the ab8500 regulators. This information includes values used
> for regulator related register initialisation and platform
> specific regulator setup information.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Lee Jones]

Here we setup the ab8500 regulator driver for DT. We first do
this in the normal way, by providing a match structure during
initialisation, but then we provide information so that
whilst probing we can use existing data structures to do DT
look-ups. We do that by embedding DT property names into
ab8500_reg_init, so that we may look-up initial register data
values directly.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/regulator/ab8500.c |  129 ++++++++++++++++++++++++++++++++------------
 1 file changed, 94 insertions(+), 35 deletions(-)

diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
index e403a0f..67de2a6 100644
--- a/drivers/regulator/ab8500.c
+++ b/drivers/regulator/ab8500.c
@@ -18,9 +18,12 @@
 #include <linux/platform_device.h>
 #include <linux/mfd/abx500.h>
 #include <linux/mfd/abx500/ab8500.h>
+#include <linux/of.h>
+#include <linux/regulator/of_regulator.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/regulator/ab8500.h>
+#include <linux/slab.h>
 
 /**
  * struct ab8500_regulator_info - ab8500 regulator information
@@ -556,16 +559,18 @@ static struct ab8500_regulator_info
 };
 
 struct ab8500_reg_init {
+	const char *of_name;
 	u8 bank;
 	u8 addr;
 	u8 mask;
 };
 
-#define REG_INIT(_id, _bank, _addr, _mask)	\
-	[_id] = {				\
-		.bank = _bank,			\
-		.addr = _addr,			\
-		.mask = _mask,			\
+#define REG_INIT(_id, _of_name, _bank, _addr, _mask)	\
+	[_id] = {					\
+		.bank    = _bank,			\
+		.of_name = _of_name,			\
+		.addr    = _addr,			\
+		.mask    = _mask,			\
 	}
 
 static struct ab8500_reg_init ab8500_reg_init[] = {
@@ -574,63 +579,63 @@ static struct ab8500_reg_init ab8500_reg_init[] = {
 	 * 0x0C, VpllRequestCtrl
 	 * 0xc0, VextSupply1RequestCtrl
 	 */
-	REG_INIT(AB8500_REGUREQUESTCTRL2,	0x03, 0x04, 0xfc),
+	REG_INIT(AB8500_REGUREQUESTCTRL2, "stericsson,regurequestctrl2", 0x03, 0x04, 0xfc),
 	/*
 	 * 0x03, VextSupply2RequestCtrl
 	 * 0x0c, VextSupply3RequestCtrl
 	 * 0x30, Vaux1RequestCtrl
 	 * 0xc0, Vaux2RequestCtrl
 	 */
-	REG_INIT(AB8500_REGUREQUESTCTRL3,	0x03, 0x05, 0xff),
+	REG_INIT(AB8500_REGUREQUESTCTRL3, "stericsson,regurequestctrl3", 0x03, 0x05, 0xff),
 	/*
 	 * 0x03, Vaux3RequestCtrl
 	 * 0x04, SwHPReq
 	 */
-	REG_INIT(AB8500_REGUREQUESTCTRL4,	0x03, 0x06, 0x07),
+	REG_INIT(AB8500_REGUREQUESTCTRL4, "stericsson,regurequestctrl4", 0x03, 0x06, 0x07),
 	/*
 	 * 0x08, VanaSysClkReq1HPValid
 	 * 0x20, Vaux1SysClkReq1HPValid
 	 * 0x40, Vaux2SysClkReq1HPValid
 	 * 0x80, Vaux3SysClkReq1HPValid
 	 */
-	REG_INIT(AB8500_REGUSYSCLKREQ1HPVALID1,	0x03, 0x07, 0xe8),
+	REG_INIT(AB8500_REGUSYSCLKREQ1HPVALID1,"stericsson,regusysclkreq1hpvalid1", 0x03, 0x07, 0xe8),
 	/*
 	 * 0x10, VextSupply1SysClkReq1HPValid
 	 * 0x20, VextSupply2SysClkReq1HPValid
 	 * 0x40, VextSupply3SysClkReq1HPValid
 	 */
-	REG_INIT(AB8500_REGUSYSCLKREQ1HPVALID2,	0x03, 0x08, 0x70),
+	REG_INIT(AB8500_REGUSYSCLKREQ1HPVALID2,	 "stericsson,regusysclkreq1hpvalid2", 0x03, 0x08, 0x70),
 	/*
 	 * 0x08, VanaHwHPReq1Valid
 	 * 0x20, Vaux1HwHPReq1Valid
 	 * 0x40, Vaux2HwHPReq1Valid
 	 * 0x80, Vaux3HwHPReq1Valid
 	 */
-	REG_INIT(AB8500_REGUHWHPREQ1VALID1,	0x03, 0x09, 0xe8),
+	REG_INIT(AB8500_REGUHWHPREQ1VALID1, "stericsson,reguhwhpreq1valid1", 0x03, 0x09, 0xe8),
 	/*
 	 * 0x01, VextSupply1HwHPReq1Valid
 	 * 0x02, VextSupply2HwHPReq1Valid
 	 * 0x04, VextSupply3HwHPReq1Valid
 	 */
-	REG_INIT(AB8500_REGUHWHPREQ1VALID2,	0x03, 0x0a, 0x07),
+	REG_INIT(AB8500_REGUHWHPREQ1VALID2, "stericsson,reguhwhpreq1valid2", 0x03, 0x0a, 0x07),
 	/*
 	 * 0x08, VanaHwHPReq2Valid
 	 * 0x20, Vaux1HwHPReq2Valid
 	 * 0x40, Vaux2HwHPReq2Valid
 	 * 0x80, Vaux3HwHPReq2Valid
 	 */
-	REG_INIT(AB8500_REGUHWHPREQ2VALID1,	0x03, 0x0b, 0xe8),
+	REG_INIT(AB8500_REGUHWHPREQ2VALID1, "stericsson,reguhwhpreq2valid1", 0x03, 0x0b, 0xe8),
 	/*
 	 * 0x01, VextSupply1HwHPReq2Valid
 	 * 0x02, VextSupply2HwHPReq2Valid
 	 * 0x04, VextSupply3HwHPReq2Valid
 	 */
-	REG_INIT(AB8500_REGUHWHPREQ2VALID2,	0x03, 0x0c, 0x07),
+	REG_INIT(AB8500_REGUHWHPREQ2VALID2, "stericsson,reguhwhpreq2valid2", 0x03, 0x0c, 0x07),
 	/*
 	 * 0x20, VanaSwHPReqValid
 	 * 0x80, Vaux1SwHPReqValid
 	 */
-	REG_INIT(AB8500_REGUSWHPREQVALID1,	0x03, 0x0d, 0xa0),
+	REG_INIT(AB8500_REGUSWHPREQVALID1, "stericsson,reguswhpreqvalid1", 0x03, 0x0d, 0xa0),
 	/*
 	 * 0x01, Vaux2SwHPReqValid
 	 * 0x02, Vaux3SwHPReqValid
@@ -638,19 +643,19 @@ static struct ab8500_reg_init ab8500_reg_init[] = {
 	 * 0x08, VextSupply2SwHPReqValid
 	 * 0x10, VextSupply3SwHPReqValid
 	 */
-	REG_INIT(AB8500_REGUSWHPREQVALID2,	0x03, 0x0e, 0x1f),
+	REG_INIT(AB8500_REGUSWHPREQVALID2, "stericsson,reguswhpreqvalid2", 0x03, 0x0e, 0x1f),
 	/*
 	 * 0x02, SysClkReq2Valid1
 	 * ...
 	 * 0x80, SysClkReq8Valid1
 	 */
-	REG_INIT(AB8500_REGUSYSCLKREQVALID1,	0x03, 0x0f, 0xfe),
+	REG_INIT(AB8500_REGUSYSCLKREQVALID1, "stericsson,regusysclkreqvalid1", 0x03, 0x0f, 0xfe),
 	/*
 	 * 0x02, SysClkReq2Valid2
 	 * ...
 	 * 0x80, SysClkReq8Valid2
 	 */
-	REG_INIT(AB8500_REGUSYSCLKREQVALID2,	0x03, 0x10, 0xfe),
+	REG_INIT(AB8500_REGUSYSCLKREQVALID2, "stericsson,regusysclkreqvalid2", 0x03, 0x10, 0xfe),
 	/*
 	 * 0x02, VTVoutEna
 	 * 0x04, Vintcore12Ena
@@ -658,29 +663,29 @@ static struct ab8500_reg_init ab8500_reg_init[] = {
 	 * 0x40, Vintcore12LP
 	 * 0x80, VTVoutLP
 	 */
-	REG_INIT(AB8500_REGUMISC1,		0x03, 0x80, 0xfe),
+	REG_INIT(AB8500_REGUMISC1, "stericsson,regumisc1", 0x03, 0x80, 0xfe),
 	/*
 	 * 0x02, VaudioEna
 	 * 0x04, VdmicEna
 	 * 0x08, Vamic1Ena
 	 * 0x10, Vamic2Ena
 	 */
-	REG_INIT(AB8500_VAUDIOSUPPLY,		0x03, 0x83, 0x1e),
+	REG_INIT(AB8500_VAUDIOSUPPLY, "stericsson,vaudiosupply", 0x03, 0x83, 0x1e),
 	/*
 	 * 0x01, Vamic1_dzout
 	 * 0x02, Vamic2_dzout
 	 */
-	REG_INIT(AB8500_REGUCTRL1VAMIC,		0x03, 0x84, 0x03),
+	REG_INIT(AB8500_REGUCTRL1VAMIC, "stericsson,reguctrl1vamic", 0x03, 0x84, 0x03),
 	/*
 	 * 0x0c, VanaRegu
 	 * 0x03, VpllRegu
 	 */
-	REG_INIT(AB8500_VPLLVANAREGU,		0x04, 0x06, 0x0f),
+	REG_INIT(AB8500_VPLLVANAREGU, "stericsson,vpllvanaregu", 0x04, 0x06, 0x0f),
 	/*
 	 * 0x01, VrefDDREna
 	 * 0x02, VrefDDRSleepMode
 	 */
-	REG_INIT(AB8500_VREFDDR,		0x04, 0x07, 0x03),
+	REG_INIT(AB8500_VREFDDR, "stericsson,vrefddr", 0x04, 0x07, 0x03),
 	/*
 	 * 0x03, VextSupply1Regu
 	 * 0x0c, VextSupply2Regu
@@ -688,36 +693,36 @@ static struct ab8500_reg_init ab8500_reg_init[] = {
 	 * 0x40, ExtSupply2Bypass
 	 * 0x80, ExtSupply3Bypass
 	 */
-	REG_INIT(AB8500_EXTSUPPLYREGU,		0x04, 0x08, 0xff),
+	REG_INIT(AB8500_EXTSUPPLYREGU, "stericsson,extsupplyregu", 0x04, 0x08, 0xff),
 	/*
 	 * 0x03, Vaux1Regu
 	 * 0x0c, Vaux2Regu
 	 */
-	REG_INIT(AB8500_VAUX12REGU,		0x04, 0x09, 0x0f),
+	REG_INIT(AB8500_VAUX12REGU, "stericsson,vaux12regu", 0x04, 0x09, 0x0f),
 	/*
 	 * 0x03, Vaux3Regu
 	 */
-	REG_INIT(AB8500_VRF1VAUX3REGU,		0x04, 0x0a, 0x03),
+	REG_INIT(AB8500_VRF1VAUX3REGU, "stericsson,vrf1vaux3regu", 0x04, 0x0a, 0x03),
 	/*
 	 * 0x3f, Vsmps1Sel1
 	 */
-	REG_INIT(AB8500_VSMPS1SEL1,		0x04, 0x13, 0x3f),
+	REG_INIT(AB8500_VSMPS1SEL1, "stericsson,vsmps1sel1", 0x04, 0x13, 0x3f),
 	/*
 	 * 0x0f, Vaux1Sel
 	 */
-	REG_INIT(AB8500_VAUX1SEL,		0x04, 0x1f, 0x0f),
+	REG_INIT(AB8500_VAUX1SEL, "stericsson,vaux1sel", 0x04, 0x1f, 0x0f),
 	/*
 	 * 0x0f, Vaux2Sel
 	 */
-	REG_INIT(AB8500_VAUX2SEL,		0x04, 0x20, 0x0f),
+	REG_INIT(AB8500_VAUX2SEL, "stericsson,vaux2sel", 0x04, 0x20, 0x0f),
 	/*
 	 * 0x07, Vaux3Sel
 	 */
-	REG_INIT(AB8500_VRF1VAUX3SEL,		0x04, 0x21, 0x07),
+	REG_INIT(AB8500_VRF1VAUX3SEL, "stericsson,vrf1vaux3sel", 0x04, 0x21, 0x07),
 	/*
 	 * 0x01, VextSupply12LP
 	 */
-	REG_INIT(AB8500_REGUCTRL2SPARE,		0x04, 0x22, 0x01),
+	REG_INIT(AB8500_REGUCTRL2SPARE, "stericsson,reguctrl2spare", 0x04, 0x22, 0x01),
 	/*
 	 * 0x04, Vaux1Disch
 	 * 0x08, Vaux2Disch
@@ -726,13 +731,13 @@ static struct ab8500_reg_init ab8500_reg_init[] = {
 	 * 0x40, VTVoutDisch
 	 * 0x80, VaudioDisch
 	 */
-	REG_INIT(AB8500_REGUCTRLDISCH,		0x04, 0x43, 0xfc),
+	REG_INIT(AB8500_REGUCTRLDISCH, "stericsson,reguctrldisch", 0x04, 0x43, 0xfc),
 	/*
 	 * 0x02, VanaDisch
 	 * 0x04, VdmicPullDownEna
 	 * 0x10, VdmicDisch
 	 */
-	REG_INIT(AB8500_REGUCTRLDISCH2,		0x04, 0x44, 0x16),
+	REG_INIT(AB8500_REGUCTRLDISCH2, "stericsson,reguctrldisch2", 0x04, 0x44, 0x16),
 };
 
 static __devinit int
@@ -815,22 +820,70 @@ static __devinit int ab8500_regulator_register(struct platform_device *pdev,
 	return 0;
 }
 
+static __devinit int
+ab8500_regulator_of_probe(struct platform_device *pdev, struct device_node *np)
+{
+	struct regulator_init_data *ab8500_regulator;
+	struct device_node *child;
+	int err, value, i, id = 0;
+
+	/* Initialise regulator registers to platform specific values. */
+	for (i = 0; i < ARRAY_SIZE(ab8500_reg_init); i++) {
+		err = of_property_read_u32(np, ab8500_reg_init[i].of_name, &value);
+		if (err < 0)
+			return err;
+
+		err = ab8500_regulator_init_registers(pdev, i, value);
+		if (err < 0)
+			return err;
+	}
+
+	/* Register each ab8500 regulator found in the Device Tree. */
+	for_each_child_of_node(np, child) {
+		ab8500_regulator = of_get_regulator_init_data(&pdev->dev, child);
+		if (!ab8500_regulator) {
+			dev_err(&pdev->dev,
+				"failed to fetch regulator data for child %s\n", child->full_name);
+			return -EINVAL;
+		}
+
+		if (strcmp(ab8500_regulator->constraints.name, "dummy"))
+			ab8500_regulator_register(pdev, ab8500_regulator, id, child);
+
+		id++;
+	}
+
+	return 0;
+}
+
 static __devinit int ab8500_regulator_probe(struct platform_device *pdev)
 {
 	struct ab8500 *ab8500 = dev_get_drvdata(pdev->dev.parent);
 	struct ab8500_platform_data *pdata;
+	struct device_node *np = pdev->dev.of_node;
 	int i, err;
 
 	if (!ab8500) {
 		dev_err(&pdev->dev, "null mfd parent\n");
 		return -EINVAL;
 	}
+
+	if (!ab8500->dev) {
+		dev_err(&pdev->dev, "no device data for parent found\n");
+		return -EINVAL;
+	}
+
 	pdata = dev_get_platdata(ab8500->dev);
-	if (!pdata) {
-		dev_err(&pdev->dev, "null pdata\n");
+	if (!pdata && !np) {
+		dev_err(&pdev->dev, "null pdata and no device tree found\n");
 		return -EINVAL;
 	}
 
+	if (!pdata) {
+		err = ab8500_regulator_of_probe(pdev, np);
+		return err;
+	}
+
 	/* make sure the platform data has the correct size */
 	if (pdata->num_regulator != ARRAY_SIZE(ab8500_regulator_info)) {
 		dev_err(&pdev->dev, "Configuration error: size mismatch.\n");
@@ -883,12 +936,18 @@ static __devexit int ab8500_regulator_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id ab8500_regulator_match[] = {
+        { .compatible = "stericsson,ab8500-regulator", },
+        {}
+};
+
 static struct platform_driver ab8500_regulator_driver = {
 	.probe = ab8500_regulator_probe,
 	.remove = __devexit_p(ab8500_regulator_remove),
 	.driver         = {
 		.name   = "ab8500-regulator",
 		.owner  = THIS_MODULE,
+		.of_match_table = ab8500_regulator_match,
 	},
 };
 
-- 
1.7.9.5

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Mark Brown]

On Fri, May 04, 2012 at 07:23:24PM +0100, Lee Jones wrote:

Once again, please try to make sure your changelog matches the
subsystem.  This also isn't consistent with the other regulator change
further up the series :(

You've also not included any binding documenation here, binding
documentation should be included for new bindings.

>  
> +static __devinit int
> +ab8500_regulator_of_probe(struct platform_device *pdev, struct device_node *np)
> +{
> +	struct regulator_init_data *ab8500_regulator;
> +	struct device_node *child;
> +	int err, value, i, id = 0;
> +
> +	/* Initialise regulator registers to platform specific values. */
> +	for (i = 0; i < ARRAY_SIZE(ab8500_reg_init); i++) {
> +		err = of_property_read_u32(np, ab8500_reg_init[i].of_name, &value);
> +		if (err < 0)
> +			return err;
> +
> +		err = ab8500_regulator_init_registers(pdev, i, value);
> +		if (err < 0)
> +			return err;

You should be using of_regulator_match() for this (I think it's supposed
to do an equivalent job...) rather than open coding.

> +	/* Register each ab8500 regulator found in the Device Tree. */
> +	for_each_child_of_node(np, child) {
> +		ab8500_regulator = of_get_regulator_init_data(&pdev->dev, child);

Definitely don't do this - you should unconditionally register all the
regulators that physically exist, this allows users to inspect their
state even if they aren't used.

It's possible the original driver has this bug (I didn't check but 

> +		if (strcmp(ab8500_regulator->constraints.name, "dummy"))
> +			ab8500_regulator_register(pdev, ab8500_regulator, id, child);

This is really broken - the whole purpose of allowing users to specify a
name in the constraints is to allow them to assign a name that's
meaningful for their board so they can tie the software in with the
schematic which we will display in diagnostics.  Forcing them to specify
magic strings as the supply name defeats this and makes diagnostics from
the kernel more obscure.

>  	pdata = dev_get_platdata(ab8500->dev);
> -	if (!pdata) {
> -		dev_err(&pdev->dev, "null pdata\n");
> +	if (!pdata && !np) {
> +		dev_err(&pdev->dev, "null pdata and no device tree found\n");
>  		return -EINVAL;
>  	}

Neither should be mandatory.
-------------- 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/20120507/1b7b0ae8/attachment.sig>

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Lee Jones]

On 07/05/12 18:08, Mark Brown wrote:
> On Fri, May 04, 2012 at 07:23:24PM +0100, Lee Jones wrote:
>
> Once again, please try to make sure your changelog matches the
> subsystem.  This also isn't consistent with the other regulator change
> further up the series :(
>
> You've also not included any binding documenation here, binding
> documentation should be included for new bindings.
>
>>
>> +static __devinit int
>> +ab8500_regulator_of_probe(struct platform_device *pdev, struct device_node *np)
>> +{
>> +	struct regulator_init_data *ab8500_regulator;
>> +	struct device_node *child;
>> +	int err, value, i, id = 0;
>> +
>> +	/* Initialise regulator registers to platform specific values. */
>> +	for (i = 0; i<  ARRAY_SIZE(ab8500_reg_init); i++) {
>> +		err = of_property_read_u32(np, ab8500_reg_init[i].of_name,&value);
>> +		if (err<  0)
>> +			return err;
>> +
>> +		err = ab8500_regulator_init_registers(pdev, i, value);
>> +		if (err<  0)
>> +			return err;
>
> You should be using of_regulator_match() for this (I think it's supposed
> to do an equivalent job...) rather than open coding.

of_regulator_match() didn't exist when I wrote this. In fact, it only 
made it into -next a couple of days ago. Besides, it doesn't satisfy the 
needs of this code segment. of_regulator_match() is a(nother) wrapper 
around of_get_regulation_constraints(), which is only used to populate 
'struct regulation_constraints constraints' after being provided with a 
selection of .compatible strings.

I'd be happy to use an API for what this is trying to achieve, however 
there aren't any as far as I know.

>> +	/* Register each ab8500 regulator found in the Device Tree. */
>> +	for_each_child_of_node(np, child) {
>> +		ab8500_regulator = of_get_regulator_init_data(&pdev->dev, child);
>
> Definitely don't do this - you should unconditionally register all the
> regulators that physically exist, this allows users to inspect their
> state even if they aren't used.

No problem. Thanks for the information. I'll change that and re-submit.

> It's possible the original driver has this bug (I didn't check but
>
>> +		if (strcmp(ab8500_regulator->constraints.name, "dummy"))
>> +			ab8500_regulator_register(pdev, ab8500_regulator, id, child);
>
> This is really broken - the whole purpose of allowing users to specify a
> name in the constraints is to allow them to assign a name that's
> meaningful for their board so they can tie the software in with the
> schematic which we will display in diagnostics.  Forcing them to specify
> magic strings as the supply name defeats this and makes diagnostics from
> the kernel more obscure.

Noted. I'll have that changed to. Thanks.

>>   	pdata = dev_get_platdata(ab8500->dev);
>> -	if (!pdata) {
>> -		dev_err(&pdev->dev, "null pdata\n");
>> +	if (!pdata&&  !np) {
>> +		dev_err(&pdev->dev, "null pdata and no device tree found\n");
>>   		return -EINVAL;
>>   	}
>
> Neither should be mandatory.

Okay.

Thanks for the review Mark. I'll get it fixed up and supply early next week.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Mark Brown]

On Tue, May 08, 2012 at 01:04:49PM +0100, Lee Jones wrote:
> On 07/05/12 18:08, Mark Brown wrote:

> >You should be using of_regulator_match() for this (I think it's supposed
> >to do an equivalent job...) rather than open coding.

> of_regulator_match() didn't exist when I wrote this. In fact, it
> only made it into -next a couple of days ago. Besides, it doesn't

It's been kicking around for review for a little while longer than that
(it was waiting for review while Rhyland was on holiday), and in any
case half the reason for adding infrastructure is to avoid adding
repeated code.

> satisfy the needs of this code segment. of_regulator_match() is
> a(nother) wrapper around of_get_regulation_constraints(), which is
> only used to populate 'struct regulation_constraints constraints'
> after being provided with a selection of .compatible strings.

I suspect that what you're trying to achieve isn't a good regulator
binding but I'm not entirely sure what you're trying to do so perhaps
not.  You haven't documented the binding at all which might make things
clearer...
-------------- 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/20120508/5436d4b0/attachment-0001.sig>

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Lee Jones]

On 08/05/12 13:19, Mark Brown wrote:
> On Tue, May 08, 2012 at 01:04:49PM +0100, Lee Jones wrote:
>> On 07/05/12 18:08, Mark Brown wrote:
>
>>> You should be using of_regulator_match() for this (I think it's supposed
>>> to do an equivalent job...) rather than open coding.
>
>> of_regulator_match() didn't exist when I wrote this. In fact, it
>> only made it into -next a couple of days ago. Besides, it doesn't
>
> It's been kicking around for review for a little while longer than that
> (it was waiting for review while Rhyland was on holiday), and in any
> case half the reason for adding infrastructure is to avoid adding
> repeated code.

I'm sorry Mark, but I just don't have the time to read all of the 
mailing lists in order to keep up with, and in-turn use all of the new 
features which might make it upstream. I only use what I see in the 
kernel at time of writing, as I have an entire platform to enable and 
very little time in which to do it.

>> satisfy the needs of this code segment. of_regulator_match() is
>> a(nother) wrapper around of_get_regulation_constraints(), which is
>> only used to populate 'struct regulation_constraints constraints'
>> after being provided with a selection of .compatible strings.
>
> I suspect that what you're trying to achieve isn't a good regulator
> binding but I'm not entirely sure what you're trying to do so perhaps
> not.  You haven't documented the binding at all which might make things
> clearer...

Right, I agree with you. I certainly will knock up some documentation 
for them.

This piece of code plucks pre-defined initialisation values and from the 
Device Tree and uses them to set-up regulator related registers on the 
u8500. See 'struct ab8500_regulator_reg_init ab8500_regulator_reg_init' 
in arch/arm/mach-ux500/board-mop500-regulators.c for reference.

I did run this past Arnd before writing the code and he agreed that this 
would be suitable; however, if you know of a better way in which I can 
do this, I'd be pleased to hear of it.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Mark Brown]

On Tue, May 08, 2012 at 01:38:18PM +0100, Lee Jones wrote:
> On 08/05/12 13:19, Mark Brown wrote:

> >It's been kicking around for review for a little while longer than that
> >(it was waiting for review while Rhyland was on holiday), and in any
> >case half the reason for adding infrastructure is to avoid adding
> >repeated code.

> I'm sorry Mark, but I just don't have the time to read all of the
> mailing lists in order to keep up with, and in-turn use all of the
> new features which might make it upstream. I only use what I see in
> the kernel at time of writing, as I have an entire platform to
> enable and very little time in which to do it.

If you're going to do this you really need to track -next rather than
-rc for actively developed subsystems - it's not just that you're not
using the latest APIs here you're submitting code that won't even
compile against the current subsystem.

> This piece of code plucks pre-defined initialisation values and from
> the Device Tree and uses them to set-up regulator related registers
> on the u8500. See 'struct ab8500_regulator_reg_init
> ab8500_regulator_reg_init' in
> arch/arm/mach-ux500/board-mop500-regulators.c for reference.

Oh, dear.  It's quite hard to associate this with the code especially
not without the binding document.  

Looking at the usage here it looks like most of this stuff shouldn't be
there even with non-DT stuff, we probably don't want to add DT bindings
for those bits.  All the voltage setting is not at all device specific
and can be done using the generic regulator bindings, the forcing on or
off is similarly generic.  There looks to be a large chunk of mode
setting too.  We probably need to scrub the existing magic number stuff
prior to trying to do this.

While looking for the original patch I also noticed that you're not CCing
the mailing list either...  please always CC the subsystem mailing list
on patches.
-------------- 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/20120508/db5f3efc/attachment.sig>

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Lee Jones]

On 08/05/12 14:34, Mark Brown wrote:
> On Tue, May 08, 2012 at 01:38:18PM +0100, Lee Jones wrote:
>> On 08/05/12 13:19, Mark Brown wrote:
>
>>> It's been kicking around for review for a little while longer than that
>>> (it was waiting for review while Rhyland was on holiday), and in any
>>> case half the reason for adding infrastructure is to avoid adding
>>> repeated code.
>
>> I'm sorry Mark, but I just don't have the time to read all of the
>> mailing lists in order to keep up with, and in-turn use all of the
>> new features which might make it upstream. I only use what I see in
>> the kernel at time of writing, as I have an entire platform to
>> enable and very little time in which to do it.
>
> If you're going to do this you really need to track -next rather than
> -rc for actively developed subsystems - it's not just that you're not
> using the latest APIs here you're submitting code that won't even
> compile against the current subsystem.

I plan to do that now, but that still wouldn't have helped in this 
instance, as the API you mentioned wasn't in -next at the time.

>> This piece of code plucks pre-defined initialisation values and from
>> the Device Tree and uses them to set-up regulator related registers
>> on the u8500. See 'struct ab8500_regulator_reg_init
>> ab8500_regulator_reg_init' in
>> arch/arm/mach-ux500/board-mop500-regulators.c for reference.
>
> Oh, dear.  It's quite hard to associate this with the code especially
> not without the binding document.

Take a look at: "[PATCH 13/15] ARM: ux500: Add support for ab8500 
regulators into the Device Tree" and compare it with the *.c file and 
I'm sure all will become apparent. It can't be complicated - I wrote it. :D

> Looking at the usage here it looks like most of this stuff shouldn't be
> there even with non-DT stuff, we probably don't want to add DT bindings
> for those bits.All the voltage setting is not at all device specific
> and can be done using the generic regulator bindings, the forcing on or
> off is similarly generic.

All the generic properties _are_ set using the generic bindings. The 
only vendor specific values are the initialisation register values 
referenced above. I'll see what happens when I remove those from DT. I 
have a feeling that the regulators will just fail though.

 > There looks to be a large chunk of mode
> setting too.  We probably need to scrub the existing magic number stuff
> prior to trying to do this.
>
> While looking for the original patch I also noticed that you're not CCing
> the mailing list either...  please always CC the subsystem mailing list
> on patches.

You don't appear to have one. I ran get_maintainer.pl on the patch and 
the only ML it came up with was LKML. If you do have one, you may need 
to update the MAINTAINERS file.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Mark Brown]

On Tue, May 08, 2012 at 03:54:09PM +0100, Lee Jones wrote:
> On 08/05/12 14:34, Mark Brown wrote:

> >Looking at the usage here it looks like most of this stuff shouldn't be
> >there even with non-DT stuff, we probably don't want to add DT bindings
> >for those bits.All the voltage setting is not at all device specific
> >and can be done using the generic regulator bindings, the forcing on or
> >off is similarly generic.

> All the generic properties _are_ set using the generic bindings. The
> only vendor specific values are the initialisation register values
> referenced above. I'll see what happens when I remove those from DT.
> I have a feeling that the regulators will just fail though.

The comments in the arch/arm file indicate otherwise - they were talking
about enabling and disabling regulators, and about setting voltages.  It
may be that the comments in the arch/arm code are inaccurate but with it
being magic numbers you'd really hope they're accurate...

> >While looking for the original patch I also noticed that you're not CCing
> >the mailing list either...  please always CC the subsystem mailing list
> >on patches.

> You don't appear to have one. I ran get_maintainer.pl on the patch
> and the only ML it came up with was LKML. If you do have one, you
> may need to update the MAINTAINERS file.

LKML is the relevant list here.
-------------- 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/20120508/0d149ee3/attachment.sig>

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Lee Jones]

On 08/05/12 15:57, Mark Brown wrote:
> On Tue, May 08, 2012 at 03:54:09PM +0100, Lee Jones wrote:
>> On 08/05/12 14:34, Mark Brown wrote:
>
>>> Looking at the usage here it looks like most of this stuff shouldn't be
>>> there even with non-DT stuff, we probably don't want to add DT bindings
>>> for those bits.All the voltage setting is not at all device specific
>>> and can be done using the generic regulator bindings, the forcing on or
>>> off is similarly generic.
>
>> All the generic properties _are_ set using the generic bindings. The
>> only vendor specific values are the initialisation register values
>> referenced above. I'll see what happens when I remove those from DT.
>> I have a feeling that the regulators will just fail though.
>
> The comments in the arch/arm file indicate otherwise - they were talking
> about enabling and disabling regulators, and about setting voltages.  It
> may be that the comments in the arch/arm code are inaccurate but with it
> being magic numbers you'd really hope they're accurate...

Fingers crossed. I'll let you know how I get on.

>>> While looking for the original patch I also noticed that you're not CCing
>>> the mailing list either...  please always CC the subsystem mailing list
>>> on patches.
>
>> You don't appear to have one. I ran get_maintainer.pl on the patch
>> and the only ML it came up with was LKML. If you do have one, you
>> may need to update the MAINTAINERS file.
>
> LKML is the relevant list here.

Ah, okay. In retrospect I don't know why I didn't add it to be honest. 
I'll endeavor to do so next time.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Arnd Bergmann]

On Tuesday 08 May 2012, Lee Jones wrote:
> This piece of code plucks pre-defined initialisation values and from the 
> Device Tree and uses them to set-up regulator related registers on the 
> u8500. See 'struct ab8500_regulator_reg_init ab8500_regulator_reg_init' 
> in arch/arm/mach-ux500/board-mop500-regulators.c for reference.
> 
> I did run this past Arnd before writing the code and he agreed that this 
> would be suitable; however, if you know of a better way in which I can 
> do this, I'd be pleased to hear of it.

It was the best approach that I could think of for this, but I'm also
a total newbie on regulators.

	Arnd

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Mark Brown]

On Tue, May 08, 2012 at 01:48:14PM +0000, Arnd Bergmann wrote:
> On Tuesday 08 May 2012, Lee Jones wrote:

> > I did run this past Arnd before writing the code and he agreed that this 
> > would be suitable; however, if you know of a better way in which I can 
> > do this, I'd be pleased to hear of it.

> It was the best approach that I could think of for this, but I'm also
> a total newbie on regulators.

It's not really a regulator thing, what the code is doing is blasting in
a bunch of magic register writes to set things up, some of which are
configuring things that the subsystem already knows how to configure.
-------------- 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/20120508/e9ec091a/attachment.sig>

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Arnd Bergmann]

On Tuesday 08 May 2012, Mark Brown wrote:
> On Tue, May 08, 2012 at 01:48:14PM +0000, Arnd Bergmann wrote:
> > On Tuesday 08 May 2012, Lee Jones wrote:
> 
> > > I did run this past Arnd before writing the code and he agreed that this 
> > > would be suitable; however, if you know of a better way in which I can 
> > > do this, I'd be pleased to hear of it.
> 
> > It was the best approach that I could think of for this, but I'm also
> > a total newbie on regulators.
> 
> It's not really a regulator thing, what the code is doing is blasting in
> a bunch of magic register writes to set things up, some of which are
> configuring things that the subsystem already knows how to configure.

Right, which is what the driver has done since 79568b9412 "regulator:
initialization for ab8500 regulators" with your ack, so we decided not
to change that and simply move the init data from platform code
to the device tree.

	Arnd

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Mark Brown]

On Tue, May 08, 2012 at 02:36:46PM +0000, Arnd Bergmann wrote:

> Right, which is what the driver has done since 79568b9412 "regulator:
> initialization for ab8500 regulators" with your ack, so we decided not
> to change that and simply move the init data from platform code
> to the device tree.

Yes, I've never seen the arch/arm bit of it before to see what the magic
writes actually do before - I did review the original code which just
used the regulator API normally but not this magic number stuff which
was just done in the platform.  The magic numbers might be OK for things
where it's device specific enough but the comments here make it clear
that some of the magic number setup is duplicating framework features.
-------------- 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/20120508/578f5479/attachment.sig>

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Lee Jones]

Hi Mark,

Looking at this now.

>> +static __devinit int
>> +ab8500_regulator_of_probe(struct platform_device *pdev, struct device_node *np)
>> +{
>> +	struct regulator_init_data *ab8500_regulator;
>> +	struct device_node *child;
>> +	int err, value, i, id = 0;
>> +
>> +	/* Initialise regulator registers to platform specific values. */
>> +	for (i = 0; i<  ARRAY_SIZE(ab8500_reg_init); i++) {
>> +		err = of_property_read_u32(np, ab8500_reg_init[i].of_name,&value);
>> +		if (err<  0)
>> +			return err;
>> +
>> +		err = ab8500_regulator_init_registers(pdev, i, value);
>> +		if (err<  0)
>> +			return err;
>
> You should be using of_regulator_match() for this (I think it's supposed
> to do an equivalent job...) rather than open coding.

I've ripped this out completely and the code appears to continue be 
fully functional. Happy days! :)

>> +	/* Register each ab8500 regulator found in the Device Tree. */
>> +	for_each_child_of_node(np, child) {
>> +		ab8500_regulator = of_get_regulator_init_data(&pdev->dev, child);
>
> Definitely don't do this - you should unconditionally register all the
> regulators that physically exist, this allows users to inspect their
> state even if they aren't used.
>
> It's possible the original driver has this bug (I didn't check but

The original driver places each of the registers inside a structure 
within the driver itself and recursively registers them from there. The 
constraints are united with the correct element using #defines.

Can't we just assume that all of the regulators will be put into the 
Device Tree? As this is what I'll be doing.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Arnd Bergmann]

On Monday 14 May 2012, Lee Jones wrote:
> > You should be using of_regulator_match() for this (I think it's supposed
> > to do an equivalent job...) rather than open coding.
> 
> I've ripped this out completely and the code appears to continue be 
> fully functional. Happy days! :)

Ok, very good!

Of course it would still be helpful to understand the reasons of
the person putting that code in originally, but I'm at least comfortable
with leaving it out for now, and fixing any problems we see in a proper
way after your code is merged.


	Arnd

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Mark Brown]

On Mon, May 14, 2012 at 04:49:21PM +0100, Lee Jones wrote:

> >You should be using of_regulator_match() for this (I think it's supposed
> >to do an equivalent job...) rather than open coding.

> I've ripped this out completely and the code appears to continue be
> fully functional. Happy days! :)

Great!

> The original driver places each of the registers inside a structure
> within the driver itself and recursively registers them from there.
> The constraints are united with the correct element using #defines.

> Can't we just assume that all of the regulators will be put into the
> Device Tree? As this is what I'll be doing.

Part of the idea here is to help with diagnostics, especially during
board bringup, so we really shouldn't be relying on the user to have set
things up reliably.  This is also used to enabled features like powering
off any unused regulators that were left enabled in late init so things
that weren't set up turn out to be moderately important.
-------------- 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/20120514/cccade9e/attachment.sig>

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Lee Jones]

>>   	pdata = dev_get_platdata(ab8500->dev);
>> -	if (!pdata) {
>> -		dev_err(&pdev->dev, "null pdata\n");
>> +	if (!pdata&&  !np) {
>> +		dev_err(&pdev->dev, "null pdata and no device tree found\n");
>>   		return -EINVAL;
>>   	}
>
> Neither should be mandatory.

Unfortunately, this has to reside, as without it I'd have to re-write 
the driver. Something with is completely out of scope of what I'm trying 
to achieve right now. It's something I'd be happy to do in the future, 
but not in this patch-set.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Mark Brown]

On Mon, May 14, 2012 at 04:57:39PM +0100, Lee Jones wrote:
> >>  	pdata = dev_get_platdata(ab8500->dev);
> >>-	if (!pdata) {
> >>-		dev_err(&pdev->dev, "null pdata\n");
> >>+	if (!pdata&&  !np) {
> >>+		dev_err(&pdev->dev, "null pdata and no device tree found\n");
> >>  		return -EINVAL;
> >>  	}

> >Neither should be mandatory.

> Unfortunately, this has to reside, as without it I'd have to
> re-write the driver. Something with is completely out of scope of
> what I'm trying to achieve right now. It's something I'd be happy to
> do in the future, but not in this patch-set.

Why do you need to rewrite the driver?  That doesn't sound terribly
plausible...   The normal approach to this stuff is to allocate a
platform data struct unconditionally, why is this impossible in your
case?
-------------- 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/20120514/1788ebec/attachment.sig>

[PATCH 15/15] ARM: ux500: Disable platform setup of the ab8500 when DT is enabled

[Lee Jones]

The final piece of the ab8500 puzzle. Here we prevent any of the ab8500-*
drivers from being registered from platform code when Device Tree is
enabled, as we expect DT do probe each of these individually. We also
provide the relevant compatible strings, so that DT knows which nodes
it needs to pay attention to during population.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index bc8a0d7..7bfd8d0 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -580,7 +580,6 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
 
 static struct platform_device *snowball_of_platform_devs[] __initdata = {
 	&snowball_key_dev,
-	&ab8500_device,
 };
 
 static void __init mop500_init_machine(void)
@@ -743,6 +742,9 @@ struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = {
 static const struct of_device_id u8500_local_bus_nodes[] = {
 	/* only create devices below soc node */
 	{ .compatible = "stericsson,db8500", },
+	{ .compatible = "stericsson,db8500-prcmu", },
+	{ .compatible = "stericsson,ab8500", },
+	{ .compatible = "stericsson,ab8500-regulator", },
 	{ .compatible = "simple-bus"},
 	{ },
 };
@@ -771,7 +773,6 @@ static void __init u8500_init_machine(void)
 				ARRAY_SIZE(mop500_platform_devs));
 
 		mop500_sdi_init(parent);
-
 		i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 		i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
 		i2c_register_board_info(2, mop500_i2c2_devices,
-- 
1.7.9.5

[PATCH 15/15] ARM: ux500: Disable platform setup of the ab8500 when DT is enabled

[Linus Walleij]

On Fri, May 4, 2012 at 8:23 PM, Lee Jones <lee.jones@linaro.org> wrote:

> The final piece of the ab8500 puzzle. Here we prevent any of the ab8500-*
> drivers from being registered from platform code when Device Tree is
> enabled, as we expect DT do probe each of these individually. We also
> provide the relevant compatible strings, so that DT knows which nodes
> it needs to pay attention to during population.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 00/15] DT enablement for Snowball

[Arnd Bergmann]

On Friday 04 May 2012, Lee Jones wrote:
> 
> Here's your next back of DT related doings for the ux500,
> along with some bugs encountered fixed along the way.
> 
>  arch/arm/boot/dts/db8500.dtsi      |  103 +++++++++++++-
>  arch/arm/boot/dts/snowball.dts     |    3 +
>  arch/arm/configs/u8500_defconfig   |    1 +
>  arch/arm/mach-ux500/board-mop500.c |   55 ++------
>  drivers/i2c/busses/i2c-nomadik.c   |   53 ++++++-
>  drivers/mfd/Makefile               |    5 +-
>  drivers/mfd/ab8500-core.c          |  165 +++++++++++++++++++---
>  drivers/mfd/ab8500-i2c.c           |  128 -----------------
>  drivers/mfd/db8500-prcmu.c         |   30 ++--
>  drivers/power/ab8500_btemp.c       |   12 +-
>  drivers/power/ab8500_charger.c     |   12 +-
>  drivers/power/ab8500_fg.c          |   12 +-
>  drivers/regulator/ab8500.c         |  273 +++++++++++++++++++++++-------------

Looks all good, but I commented on two patches that I think can be done
in a simpler way.

	Arnd

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Mark Brown]

On Mon, May 14, 2012 at 06:39:00PM +0100, Lee Jones wrote:

> Okay, so can you suggest a way to marry up the constraints found in DT to
> the remainder of the settings found in the driver without pointlessly
> applying it all to the DT itself (which is was some of the other drives
> have done)?

That's what people are using regulator_of_match() for - it's for mapping
the init data in DT array to a handle the driver can use for its
internal data (and vice versa).  See how tps65910 is using it for an
example.
-------------- 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/20120514/c5fc4d96/attachment.sig>

[PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Lee Jones]

On 14/05/12 19:06, Mark Brown wrote:
> On Mon, May 14, 2012 at 06:39:00PM +0100, Lee Jones wrote:
>
>> Okay, so can you suggest a way to marry up the constraints found in DT to
>> the remainder of the settings found in the driver without pointlessly
>> applying it all to the DT itself (which is was some of the other drives
>> have done)?
>
> That's what people are using regulator_of_match() for - it's for mapping
> the init data in DT array to a handle the driver can use for its
> internal data (and vice versa).  See how tps65910 is using it for an
> example.

Ah, I see it now. Looks like that's what I'm after.

I'll sort it out and try to get the new patch off tomorrow.

Thanks Mark.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog