[RFC 0/7] MFD: twl6040: Conversion to i2c driver

[RFC 0/7] MFD: twl6040: Conversion to i2c driver

[Peter Ujfalusi]

Hello,

This series will convert the twl6040 MFD driver to an i2c driver.
Compared to older twl4030/5030/TPS the twl6040 is a standalone audio IC. It is
better if the twl6040-core (and all of it's child devices) does not depend on
the twl-core since it has nothing to with it.

With this conversion the dependency on twl can be dropped from the twl6040
driver stack (core, vibra, audio).

Between the first and second patch the audio will not probe on OMAP4, but I felt
it is better this way at least for the first RFC series to not have too big
change within one patch. The two patch can be squashed together later if no
objections.

Regards,
Peter
---
Peter Ujfalusi (7):
  MFD: twl-core: Detach twl6040 from the pmic mfd driver
  MFD: twl6040: Convert to i2c driver, and separate it from twl core
  ASoC: twl6040: Remove dependency on twl4030 from Kconfig
  OMAP: 4430sdp: Correct fixed regulator device ID
  OMAP: sdp4430: Add fixed regulator for twl6040 needs
  OMAP: omap4panda: Add fixed regulator for twl6040 needs
  MFD: TWL6040: Add regulator support for VIO, V2V1 supplies

 arch/arm/mach-omap2/board-4430sdp.c    |   77 +++++++++++++--
 arch/arm/mach-omap2/board-generic.c    |    2 +-
 arch/arm/mach-omap2/board-omap4panda.c |   75 +++++++++++++--
 arch/arm/mach-omap2/twl-common.c       |   37 ++++++-
 arch/arm/mach-omap2/twl-common.h       |   10 +-
 drivers/input/misc/Kconfig             |    1 -
 drivers/input/misc/twl6040-vibra.c     |    4 +-
 drivers/mfd/Kconfig                    |    2 +-
 drivers/mfd/twl-core.c                 |   58 +++++++-----
 drivers/mfd/twl6040-core.c             |  165 ++++++++++++++++++++++++++------
 include/linux/i2c/twl.h                |   12 ---
 include/linux/mfd/twl6040.h            |   27 +++++
 sound/soc/codecs/Kconfig               |    2 +-
 sound/soc/codecs/twl6040.c             |    3 +-
 14 files changed, 375 insertions(+), 100 deletions(-)

-- 
1.7.8.4

[RFC 1/7] MFD: twl-core: Detach twl6040 from the pmic mfd driver

[Peter Ujfalusi]

On OMAP4 platform audio has separate IC, it is no longer part
of the pmic chip.
Prevent twl-core to claim the 0x4b address, which belongs to
the twl6040 audio IC.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/mfd/twl-core.c |   58 +++++++++++++++++++++++++++--------------------
 1 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index e04e04d..ac6fdd93 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -115,8 +115,8 @@
 #define twl_has_watchdog()        false
 #endif
 
-#if defined(CONFIG_MFD_TWL4030_AUDIO) || defined(CONFIG_MFD_TWL4030_AUDIO_MODULE) ||\
-	defined(CONFIG_TWL6040_CORE) || defined(CONFIG_TWL6040_CORE_MODULE)
+#if defined(CONFIG_MFD_TWL4030_AUDIO) || \
+	defined(CONFIG_MFD_TWL4030_AUDIO_MODULE)
 #define twl_has_codec()	true
 #else
 #define twl_has_codec()	false
@@ -146,6 +146,7 @@
 #define SUB_CHIP_ID1 1
 #define SUB_CHIP_ID2 2
 #define SUB_CHIP_ID3 3
+#define SUB_CHIP_ID_INVAL 0xff
 
 #define TWL_MODULE_LAST TWL4030_MODULE_LAST
 
@@ -315,7 +316,7 @@ static struct twl_mapping twl6030_map[] = {
 	 * so they continue to match the order in this table.
 	 */
 	{ SUB_CHIP_ID1, TWL6030_BASEADD_USB },
-	{ SUB_CHIP_ID3, TWL6030_BASEADD_AUDIO },
+	{ SUB_CHIP_ID_INVAL, TWL6030_BASEADD_AUDIO },
 	{ SUB_CHIP_ID2, TWL6030_BASEADD_DIEID },
 	{ SUB_CHIP_ID2, TWL6030_BASEADD_RSV },
 	{ SUB_CHIP_ID1, TWL6030_BASEADD_PIH },
@@ -377,6 +378,11 @@ int twl_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes)
 		return -EPERM;
 	}
 	sid = twl_map[mod_no].sid;
+	if (unlikely(sid == SUB_CHIP_ID_INVAL)) {
+		pr_err("%s: module %d is not part of the pmic\n",
+		       DRIVER_NAME, mod_no);
+		return -EINVAL;
+	}
 	twl = &twl_modules[sid];
 
 	mutex_lock(&twl->xfer_lock);
@@ -434,6 +440,11 @@ int twl_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes)
 		return -EPERM;
 	}
 	sid = twl_map[mod_no].sid;
+	if (unlikely(sid == SUB_CHIP_ID_INVAL)) {
+		pr_err("%s: module %d is not part of the pmic\n",
+		       DRIVER_NAME, mod_no);
+		return -EINVAL;
+	}
 	twl = &twl_modules[sid];
 
 	mutex_lock(&twl->xfer_lock);
@@ -834,15 +845,6 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
 			return PTR_ERR(child);
 	}
 
-	if (twl_has_codec() && pdata->audio && twl_class_is_6030()) {
-		sub_chip_id = twl_map[TWL_MODULE_AUDIO_VOICE].sid;
-		child = add_child(sub_chip_id, "twl6040",
-				pdata->audio, sizeof(*pdata->audio),
-				false, 0, 0);
-		if (IS_ERR(child))
-			return PTR_ERR(child);
-	}
-
 	/* twl4030 regulators */
 	if (twl_has_regulator() && twl_class_is_4030()) {
 		child = add_regulator(TWL4030_REG_VPLL1, pdata->vpll1,
@@ -1163,18 +1165,21 @@ int twl6030_exit_irq(void);
 
 static int twl_remove(struct i2c_client *client)
 {
-	unsigned i;
+	unsigned i, num_slaves;
 	int status;
 
-	if (twl_class_is_4030())
+	if (twl_class_is_4030()) {
 		status = twl4030_exit_irq();
-	else
+		num_slaves = TWL_NUM_SLAVES;
+	} else {
 		status = twl6030_exit_irq();
+		num_slaves = TWL_NUM_SLAVES - 1;
+	}
 
 	if (status < 0)
 		return status;
 
-	for (i = 0; i < TWL_NUM_SLAVES; i++) {
+	for (i = 0; i < num_slaves; i++) {
 		struct twl_client	*twl = &twl_modules[i];
 
 		if (twl->client && twl->client != client)
@@ -1190,7 +1195,7 @@ static int __devinit
 twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	int				status;
-	unsigned			i;
+	unsigned			i, num_slaves;
 	struct twl4030_platform_data	*pdata = client->dev.platform_data;
 	struct device_node		*node = client->dev.of_node;
 	u8 temp;
@@ -1244,7 +1249,17 @@ twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return -EBUSY;
 	}
 
-	for (i = 0; i < TWL_NUM_SLAVES; i++) {
+	if ((id->driver_data) & TWL6030_CLASS) {
+		twl_id = TWL6030_CLASS_ID;
+		twl_map = &twl6030_map[0];
+		num_slaves = TWL_NUM_SLAVES - 1;
+	} else {
+		twl_id = TWL4030_CLASS_ID;
+		twl_map = &twl4030_map[0];
+		num_slaves = TWL_NUM_SLAVES;
+	}
+
+	for (i = 0; i < num_slaves; i++) {
 		struct twl_client	*twl = &twl_modules[i];
 
 		twl->address = client->addr + i;
@@ -1263,13 +1278,6 @@ twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		mutex_init(&twl->xfer_lock);
 	}
 	inuse = true;
-	if ((id->driver_data) & TWL6030_CLASS) {
-		twl_id = TWL6030_CLASS_ID;
-		twl_map = &twl6030_map[0];
-	} else {
-		twl_id = TWL4030_CLASS_ID;
-		twl_map = &twl4030_map[0];
-	}
 
 	/* setup clock framework */
 	clocks_init(&client->dev, pdata->clock);
-- 
1.7.8.4

[RFC 4/7] OMAP: 4430sdp: Correct fixed regulator device ID

[Mark Brown]

On Thu, Feb 02, 2012 at 02:16:56PM +0200, Peter Ujfalusi wrote:
> The board has two fixed voltage regulator. Correct the
> device ID for the vbat, which used -1.
> Create defines for the fixed regulator IDs so when adding new
> regulators we know the next available ID to use for them.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
-------------- 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/20120202/80d0def8/attachment.sig>

[RFC 5/7] OMAP: sdp4430: Add fixed regulator for twl6040 needs

[Mark Brown]

On Thu, Feb 02, 2012 at 02:16:57PM +0200, Peter Ujfalusi wrote:
> The twl6040 VIO power is connected to SMPS V1V8, the
> V2V1 power is coming from SMPS V2V1 of twl6030.
> These are fixed, always on regulators.
> Create fixed voltage regulators for these supplies.

Shouldn't the PMIC be creating the relevant fixed voltage regulators?
It's a bit clearer given that the supplies actually come from the PMIC
and it would cut down a bit on the boilerplate in boards (though much
less so with device tree, sadly).
-------------- 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/20120202/bd01398d/attachment.sig>

[RFC 5/7] OMAP: sdp4430: Add fixed regulator for twl6040 needs

[Peter Ujfalusi]

On 02/02/2012 02:40 PM, Mark Brown wrote:
> On Thu, Feb 02, 2012 at 02:16:57PM +0200, Peter Ujfalusi wrote:
>> The twl6040 VIO power is connected to SMPS V1V8, the
>> V2V1 power is coming from SMPS V2V1 of twl6030.
>> These are fixed, always on regulators.
>> Create fixed voltage regulators for these supplies.
> 
> Shouldn't the PMIC be creating the relevant fixed voltage regulators?
> It's a bit clearer given that the supplies actually come from the PMIC
> and it would cut down a bit on the boilerplate in boards (though much
> less so with device tree, sadly).

Yes these are coming from twl6030, so it might be better to create the
fixed regulators in the PMIC driver. I'll take a look.

Thanks,
P?ter

[RFC 6/7] OMAP: omap4panda: Add fixed regulator for twl6040 needs

[Mark Brown]

On Thu, Feb 02, 2012 at 02:16:58PM +0200, Peter Ujfalusi wrote:
> The twl6040 VIO power is connected to SMPS V1V8, the
> V2V1 power is coming from SMPS V2V1 of twl6030.
> These are fixed, always on regulators.
> Create fixed voltage regulators for these supplies.

Same issue as previously, shouldn't the PMIC create the regulators?
-------------- 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/20120202/2a44d37e/attachment.sig>

[RFC 3/7] ASoC: twl6040: Remove dependency on twl4030 from Kconfig

[Mark Brown]

On Thu, Feb 02, 2012 at 02:16:55PM +0200, Peter Ujfalusi wrote:
> twl6040 no longer needs twl4030.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Not applying since this needs to follow on from (or be integrated with)
the patch splitting the twl6040 to avoid build breaks.
-------------- 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/20120202/be79ae3e/attachment.sig>

[RFC 2/7] MFD: twl6040: Convert to i2c driver, and separate it from twl core

[Mark Brown]

On Thu, Feb 02, 2012 at 02:16:54PM +0200, Peter Ujfalusi wrote:

> +static int twl6040_i2c_read(struct i2c_client *i2c, u8 *value, u8 reg)
> +{
> +	struct i2c_msg msg[2];
> +	int ret;

May as well convert to regmap while you're at it, saves some code and
will get you access to the regmap features - you have to make updates in
all the relevant places anyway.  We should be pushing to remove use of
the ASoC level code for register cache and whatnot to cut down on code
duplication (especially for MFDs where it has a few issues interacting
with the MFD) and this seems like a good opportunity.

The calling convention here seems a bit weird too, you've got value then
register but normally we have register then value for I2C/SPI devices.
Except when we don't :(

> -module_platform_driver(twl6040_driver);
> +static int __devinit twl6040_init(void)
> +{
> +	return i2c_add_driver(&twl6040_driver);
> +}
> +module_init(twl6040_init);
> +
> +static void __devexit twl6040_exit(void)
> +{
> +	i2c_del_driver(&twl6040_driver);
> +}
> +
> +module_exit(twl6040_exit);

There's module_i2c_driver() in mainline as of the last merge window.
-------------- 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/20120202/1319fb4b/attachment-0001.sig>

[RFC 2/7] MFD: twl6040: Convert to i2c driver, and separate it from twl core

[Peter Ujfalusi]

On 02/02/2012 02:48 PM, Mark Brown wrote:
> On Thu, Feb 02, 2012 at 02:16:54PM +0200, Peter Ujfalusi wrote:
> 
>> +static int twl6040_i2c_read(struct i2c_client *i2c, u8 *value, u8 reg)
>> +{
>> +	struct i2c_msg msg[2];
>> +	int ret;
> 
> May as well convert to regmap while you're at it, saves some code and
> will get you access to the regmap features - you have to make updates in
> all the relevant places anyway.  We should be pushing to remove use of
> the ASoC level code for register cache and whatnot to cut down on code
> duplication (especially for MFDs where it has a few issues interacting
> with the MFD) and this seems like a good opportunity.

True.
I have done the bulk of this set back in June, 2011 and thought that I
will do the move to regmap as increment, but you are right. I'm anyway
touching the relevant places, so why not do it properly?
I'll take the regmap into use for  v2.

> The calling convention here seems a bit weird too, you've got value then
> register but normally we have register then value for I2C/SPI devices.

I have based these part on the twl-core's implementation. I should not
have done that, I know.. Let's see how it will look like with regmap.

> Except when we don't :(
> 
>> -module_platform_driver(twl6040_driver);
>> +static int __devinit twl6040_init(void)
>> +{
>> +	return i2c_add_driver(&twl6040_driver);
>> +}
>> +module_init(twl6040_init);
>> +
>> +static void __devexit twl6040_exit(void)
>> +{
>> +	i2c_del_driver(&twl6040_driver);
>> +}
>> +
>> +module_exit(twl6040_exit);
> 
> There's module_i2c_driver() in mainline as of the last merge window.

Oh, good to know. Thanks

-- 
P?ter

[RFC 2/7] MFD: twl6040: Convert to i2c driver, and separate it from twl core

[Mark Brown]

On Thu, Feb 02, 2012 at 03:01:06PM +0200, Peter Ujfalusi wrote:
> On 02/02/2012 02:48 PM, Mark Brown wrote:

> > The calling convention here seems a bit weird too, you've got value then
> > register but normally we have register then value for I2C/SPI devices.

> I have based these part on the twl-core's implementation. I should not
> have done that, I know.. Let's see how it will look like with regmap.

Yeah, like I say...

> > Except when we don't :(

...it's not like we've been consistent so far.  I'm not that fussed
either way, I probably wouldn't have mentioned it if I hadn't otherwise
been commenting.
-------------- 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/20120202/f81919fa/attachment.sig>

[RFC 7/7] MFD: TWL6040: Add regulator support for VIO, V2V1 supplies

[Mark Brown]

On Thu, Feb 02, 2012 at 02:16:59PM +0200, Peter Ujfalusi wrote:

> +	twl6040->vio = regulator_get(&client->dev, "vio");
> +	if (IS_ERR(twl6040->vio)) {
> +		ret = PTR_ERR(twl6040->vio);
> +		dev_err(&client->dev, "Failed to request VIO supply: %d\n",
> +			ret);
> +		goto regulator_get_err;
> +	}
> +	twl6040->v2v1 = regulator_get(&client->dev, "v2v1");
> +	if (IS_ERR(twl6040->v2v1)) {
> +		ret = PTR_ERR(twl6040->v2v1);
> +		dev_err(&client->dev, "Failed to request V2V1 supply: %d\n",
> +			ret);
> +		regulator_put(twl6040->vio);
> +		goto regulator_get_err;
> +	}

Looks like you want regulator_bulk_get() here.  Or (better yet though
it'd be a potential issue for merge via MFD and the benefits aren't that
exciting since you still need to disable) devm_regulator_bulk_get().

> +	ret = regulator_enable(twl6040->vio);
> +	if (ret != 0) {
> +		dev_err(&client->dev, "Failed to enable VIO: %d\n", ret);
> +		goto power_err;
> +	}
> +	ret = regulator_enable(twl6040->v2v1);
> +	if (ret != 0) {
> +		dev_err(&client->dev, "Failed to enable V2V1: %d\n", ret);
> +		regulator_disable(twl6040->vio);
> +		goto power_err;
> +	}

Similarly regulator_bulk_enable() here, and it'll fix...

>  gpio1_err:
> +	regulator_disable(twl6040->v2v1);
> +	regulator_disable(twl6040->vio);

...the fact that if you fail to enable the v2.1 regulator you don't
disable vio.
-------------- 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/20120202/42b9ecc2/attachment.sig>

[RFC 7/7] MFD: TWL6040: Add regulator support for VIO, V2V1 supplies

[Peter Ujfalusi]

On 02/02/2012 02:52 PM, Mark Brown wrote:
> On Thu, Feb 02, 2012 at 02:16:59PM +0200, Peter Ujfalusi wrote:
> 
>> +	twl6040->vio = regulator_get(&client->dev, "vio");
>> +	if (IS_ERR(twl6040->vio)) {
>> +		ret = PTR_ERR(twl6040->vio);
>> +		dev_err(&client->dev, "Failed to request VIO supply: %d\n",
>> +			ret);
>> +		goto regulator_get_err;
>> +	}
>> +	twl6040->v2v1 = regulator_get(&client->dev, "v2v1");
>> +	if (IS_ERR(twl6040->v2v1)) {
>> +		ret = PTR_ERR(twl6040->v2v1);
>> +		dev_err(&client->dev, "Failed to request V2V1 supply: %d\n",
>> +			ret);
>> +		regulator_put(twl6040->vio);
>> +		goto regulator_get_err;
>> +	}
> 
> Looks like you want regulator_bulk_get() here.  Or (better yet though
> it'd be a potential issue for merge via MFD and the benefits aren't that
> exciting since you still need to disable) devm_regulator_bulk_get().

I need separate control for the two power source since we can hit
different power levels depending on which is powered/not powered:
power down: vio, v2v1 is OFF
deep sleep: vio is ON, v2v1 is OFF
sleep/power on: vio, v2v1 is ON

And we have certain sequence to move between power states.

> 
>> +	ret = regulator_enable(twl6040->vio);
>> +	if (ret != 0) {
>> +		dev_err(&client->dev, "Failed to enable VIO: %d\n", ret);
>> +		goto power_err;
>> +	}
>> +	ret = regulator_enable(twl6040->v2v1);
>> +	if (ret != 0) {
>> +		dev_err(&client->dev, "Failed to enable V2V1: %d\n", ret);
>> +		regulator_disable(twl6040->vio);

I disable the vio here, if the v2v1 enable fails.

>> +		goto power_err;
>> +	}
> 
> Similarly regulator_bulk_enable() here, and it'll fix...
> 
>>  gpio1_err:
>> +	regulator_disable(twl6040->v2v1);
>> +	regulator_disable(twl6040->vio);
> 
> ...the fact that if you fail to enable the v2.1 regulator you don't
> disable vio.

It is handled within the if(){}
With the devm_regulator this will be much nicer for sure.

-- 
P?ter

[RFC 7/7] MFD: TWL6040: Add regulator support for VIO, V2V1 supplies

[Mark Brown]

On Thu, Feb 02, 2012 at 03:18:09PM +0200, Peter Ujfalusi wrote:
> On 02/02/2012 02:52 PM, Mark Brown wrote:

> > Looks like you want regulator_bulk_get() here.  Or (better yet though
> > it'd be a potential issue for merge via MFD and the benefits aren't that
> > exciting since you still need to disable) devm_regulator_bulk_get().

> I need separate control for the two power source since we can hit
> different power levels depending on which is powered/not powered:
> power down: vio, v2v1 is OFF
> deep sleep: vio is ON, v2v1 is OFF
> sleep/power on: vio, v2v1 is ON

> And we have certain sequence to move between power states.

That's not a problem for using the bulk get - the array is part of the
API so you can use regulator_bulk_get() and still look at individual
supplies within the array later on when enabling and disabling them.

> >> +	ret = regulator_enable(twl6040->vio);
> >> +	if (ret != 0) {
> >> +		dev_err(&client->dev, "Failed to enable VIO: %d\n", ret);
> >> +		goto power_err;
> >> +	}
> >> +	ret = regulator_enable(twl6040->v2v1);
> >> +	if (ret != 0) {
> >> +		dev_err(&client->dev, "Failed to enable V2V1: %d\n", ret);
> >> +		regulator_disable(twl6040->vio);

> I disable the vio here, if the v2v1 enable fails.

Oh, that's quite confusing when mixed in with the goto/unwind - it'd be
clearer to have the extra lable to jump to.
-------------- 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/20120202/f817d7b4/attachment.sig>

[RFC 7/7] MFD: TWL6040: Add regulator support for VIO, V2V1 supplies

[Peter Ujfalusi]

On 02/02/2012 03:32 PM, Mark Brown wrote:
> That's not a problem for using the bulk get - the array is part of the
> API so you can use regulator_bulk_get() and still look at individual
> supplies within the array later on when enabling and disabling them.

For some reason I have associated the use of regulator_bulk_get with the
use of regulator_bulk_enable/disable. It did not even crossed my mind
that I can still use regulator_enable on the individual regulators.
Will convert the regulator_get/put to bulk operations. It will make the
code a bit cleaner.
Thanks

>>>> +	ret = regulator_enable(twl6040->vio);
>>>> +	if (ret != 0) {
>>>> +		dev_err(&client->dev, "Failed to enable VIO: %d\n", ret);
>>>> +		goto power_err;
>>>> +	}
>>>> +	ret = regulator_enable(twl6040->v2v1);
>>>> +	if (ret != 0) {
>>>> +		dev_err(&client->dev, "Failed to enable V2V1: %d\n", ret);
>>>> +		regulator_disable(twl6040->vio);
> 
>> I disable the vio here, if the v2v1 enable fails.
> 
> Oh, that's quite confusing when mixed in with the goto/unwind - it'd be
> clearer to have the extra lable to jump to.

I guess this is a matter of taste.
However I have found some inconsistent naming with the exit labels,
which I'm going to fix for the next series.

-- 
P?ter

[RFC 7/7] MFD: TWL6040: Add regulator support for VIO, V2V1 supplies

[Mark Brown]

On Thu, Feb 02, 2012 at 03:59:20PM +0200, Peter Ujfalusi wrote:
> On 02/02/2012 03:32 PM, Mark Brown wrote:
> > That's not a problem for using the bulk get - the array is part of the
> > API so you can use regulator_bulk_get() and still look at individual
> > supplies within the array later on when enabling and disabling them.

> For some reason I have associated the use of regulator_bulk_get with the
> use of regulator_bulk_enable/disable. It did not even crossed my mind
> that I can still use regulator_enable on the individual regulators.

They do normally end up going together since a very large proportion of
devices need all their supplies on when active but don't care about
sequencing so you end up with a common pattern.  It's not required,
though.

> Will convert the regulator_get/put to bulk operations. It will make the
> code a bit cleaner.

Good stuff.
-------------- 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/20120202/73519ed4/attachment-0001.sig>