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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [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

[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 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

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...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1952 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [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

Re: [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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 635 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [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

Re: [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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]