regulator: max8925: fix enabled/disabled judgement mistake

PATCH New Haojian Zhuang 3 participants 7 messages Dec 22, 2011 – Dec 23, 2011 Voltage and current regulator framework

lore

Share & tools

lore

Haojian Zhuang

author

2011-12-22 08:51 UTC #0 lore raw

PATCH 1/1 regulator: max8925: fix enabled/disabled judgement mistake

From: Kevin Liu <[email protected]>

The judgement should depend on the power up/down sequence select. With flexible power sequence, regulator is always enabled after boot up. With i2c enabled, regulator enable/disable according to output enable bit.

Signed-off-by Kevin Liu <[email protected]>

---
 drivers/regulator/max8925-regulator.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

drivers/regulator/max8925-regulator.c +9 -3

index cc9ec0e..9e99420 100644
@@ -27,6 +27,9 @@
 /* bit definitions in SD & LDO control registers */
 #define OUT_ENABLE   		0x1f		/* Power U/D sequence as I2C */
 #define OUT_DISABLE		0x1e		/* Power U/D sequence as I2C */
+#define LDO_SEQ_MASK		0x7		/* Power U/D sequence mask */
+#define LDO_SEQ_SHIFT		2		/* Power U/D sequence offset */
+#define LDO_SEQ_I2C_EN		0x7		/* Power U/D sequence as I2C_EN */
 
 struct max8925_regulator_info {
 	struct regulator_desc	desc;

@@ -114,13 +117,16 @@ static int max8925_disable(struct regulator_dev *rdev)
 static int max8925_is_enabled(struct regulator_dev *rdev)
 {
 	struct max8925_regulator_info *info = rdev_get_drvdata(rdev);
-	int ret;
+	int ldo_seq, ret;
 
 	ret = max8925_reg_read(info->i2c, info->enable_reg);
 	if (ret < 0)
 		return ret;
-
-	return ret & (1 << info->enable_bit);
+	ldo_seq = (ret >> LDO_SEQ_SHIFT) & LDO_SEQ_MASK;
+	if (ldo_seq != LDO_SEQ_I2C_EN)
+		return 1 << info->enable_bit;
+	else
+		return ret & (1 << info->enable_bit);
 }
 
 static int max8925_set_dvm_voltage(struct regulator_dev *rdev, int uV)

Signature · 1 line

1.7.0.4

Mark Brown

2011-12-22 10:53 UTC #1 lore raw

PATCH 1/1 regulator: max8925: fix enabled/disabled judgement mistake

Quoted 5 lines · Haojian Zhuang

From: Kevin Liu <[email protected]>
The judgement should depend on the power up/down sequence select. With flexible power sequence, regulator is always enabled after boot up. With i2c enabled, regulator enable/disable according to output enable bit.

I'm sorry but I'm having a hard time understanding what this changelog means, please clarify.

drivers/regulator/max8925-regulator.c static int max8925_disable(struct regulator_dev *rdev) · line 129

Quoted 6 lines

drivers/regulator/max8925-regulator.c +5 -1

-	return ret & (1 << info->enable_bit);
+	ldo_seq = (ret >> LDO_SEQ_SHIFT) & LDO_SEQ_MASK;
+	if (ldo_seq != LDO_SEQ_I2C_EN)
+		return 1 << info->enable_bit;
+	else
+		return ret & (1 << info->enable_bit);

Does this mean that the register for enabling and disabling the register doesn't always work? If it does then doesn't that mean that the enable and disable functions need to be updated as well to either put the regulator into I2C mode or return an error when it isn't in I2C mode?

Haojian Zhuang

2011-12-22 13:31 UTC #2 lore raw

PATCH 1/1 regulator: max8925: fix enabled/disabled judgement mistake

Quoted 10 lines · Mark Brown

Quoted 5 lines · Haojian Zhuang
From: Kevin Liu <[email protected]>
The judgement should depend on the power up/down sequence select. With flexible power sequence, regulator is always enabled after boot up. With i2c enabled, regulator enable/disable according to output enable bit.
I'm sorry but I'm having a hard time understanding what this changelog means, please clarify.

There're two registers to control LDO enable or disable. They're power-sequence register
and control register.

There're eight modes in power-sequence register. 0x7 means I2C mode. The rest means
sequence in power-on.

The hardware logic of enabling/disabling LDO is in below.

If power-sequence is power-on sequence:
enable LDO
else if power-sequence is i2c mode:
if enable bit of control register is on:
enable LDO
else:
disable LDO
else:
disable LDO

If the LDO default state is OFF, software needs to set sequence to I2C mode first. Then software can enable LDO via control register.

Quoted 7 lines

Quoted 6 lines
- ? ? return ret & (1 << info->enable_bit);
+ ? ? ldo_seq = (ret >> LDO_SEQ_SHIFT) & LDO_SEQ_MASK;
+ ? ? if (ldo_seq != LDO_SEQ_I2C_EN)
+ ? ? ? ? ? ? return 1 << info->enable_bit;
+ ? ? else
+ ? ? ? ? ? ? return ret & (1 << info->enable_bit);

Quoted 5 lines

Does this mean that the register for enabling and disabling the register doesn't always work? ?If it does then doesn't that mean that the enable and disable functions need to be updated as well to either put the regulator into I2C mode or return an error when it isn't in I2C mode?

According to the logic in above, we need check both registers to figure out whether LDO is on or off.
Return error if it isn't in I2C mode doesn't make sense. If sequence is in one of power-on sequence, the LDO is already ON by default.

Mark Brown

2011-12-22 13:44 UTC #3 lore raw

PATCH 1/1 regulator: max8925: fix enabled/disabled judgement mistake

Quoted 11 lines · Haojian Zhuang

On Thu, Dec 22, 2011 at 6:53 PM, Mark Brown
Quoted 4 lines
Does this mean that the register for enabling and disabling the register doesn't always work? ?If it does then doesn't that mean that the enable and disable functions need to be updated as well to either put the regulator into I2C mode or return an error when it isn't in I2C mode?
According to the logic in above, we need check both registers to figure out whether LDO is on or off.
Return error if it isn't in I2C mode doesn't make sense. If sequence is in one of power-on sequence, the LDO is already ON by default.

I don't understand how your reply follows on from my comment here? If the enable register doesn't control if the regulator is enabled and disabled and it's always on then it seems fairly clear that the enable and disable functions ought to know about this.

Haojian Zhuang

2011-12-22 14:22 UTC #4 lore raw

PATCH 1/1 regulator: max8925: fix enabled/disabled judgement mistake

Quoted 13 lines · Mark Brown

Quoted 11 lines · Haojian Zhuang
On Thu, Dec 22, 2011 at 6:53 PM, Mark Brown
Quoted 4 lines
Does this mean that the register for enabling and disabling the register doesn't always work? ?If it does then doesn't that mean that the enable and disable functions need to be updated as well to either put the regulator into I2C mode or return an error when it isn't in I2C mode?
According to the logic in above, we need check both registers to figure out whether LDO is on or off.
Return error if it isn't in I2C mode doesn't make sense. If sequence is in one of power-on sequence, the LDO is already ON by default.

Quoted 4 lines

I don't understand how your reply follows on from my comment here? ?If the enable register doesn't control if the regulator is enabled and disabled and it's always on then it seems fairly clear that the enable and disable functions ought to know about this.

Mark,

The logic I mentioned is pasted by me. In order to make it clear, let me copy the logic again.

Quoted 10 lines

If power-sequence is power-on sequence:
enable LDO
else if power-sequence is i2c mode:
if enable bit of control register is on:
enable LDO
else:
disable LDO
else:
disable LDO

Quoted 2 lines

If the LDO default state is OFF, software needs to set sequence to I2C mode first. Then software can enable LDO via control register.

I'm sorry that there's an error in the comments. There's only one enable register. Power sequence is bit[4:2], and enable bit is bit[0].

While we enable/disable the LDO in software, we'll force LDO into I2C mode and enable/disable the control bit. So there's no risk in the enable/disable function.

There's only potential issue of checking whether LDO is enabled. If sequence matches
the power sequence (not I2C mode), LDO will be enabled even the control bit is OFF.
So this patch is necessary.

Best Regards
Haojian

Mark Brown

2011-12-22 14:38 UTC #5 lore raw

PATCH 1/1 regulator: max8925: fix enabled/disabled judgement mistake

Quoted 3 lines · Haojian Zhuang

While we enable/disable the LDO in software, we'll force LDO into I2C mode and enable/disable the control bit. So there's no risk in the enable/disable function.

Oh, dear. That code really is obscure - it's got a shift called enable_bit but actually it's mostly working with bitfields not just a single bit. Reading the enable() and disable() functions it looks like they're setting a single bit but that's not the case.

Please send a patch fixing this.

Quoted 5 lines

There's only potential issue of checking whether LDO is enabled. If sequence matches
the power sequence (not I2C mode), LDO will be enabled even the control bit is OFF.
So this patch is necessary.

I'm not saying the patch isn't required, I'm saying it's very hard to work out what the patch is supposed to do and why it's only an issue in the is_enabled() function.

Please resend with an improved changelog.

Haojian Zhuang

2011-12-23 14:42 UTC #6 lore raw

PATCH 1/1 regulator: max8925: fix enabled/disabled judgement mistake

Quoted 19 lines · Mark Brown

Quoted 3 lines · Haojian Zhuang
While we enable/disable the LDO in software, we'll force LDO into I2C mode and enable/disable the control bit. So there's no risk in the enable/disable function.
Oh, dear. ?That code really is obscure - it's got a shift called enable_bit but actually it's mostly working with bitfields not just a single bit. ?Reading the enable() and disable() functions it looks like they're setting a single bit but that's not the case.
Please send a patch fixing this.
Quoted 5 lines
There's only potential issue of checking whether LDO is enabled. If sequence matches
the power sequence (not I2C mode), LDO will be enabled even the control bit is OFF.
So this patch is necessary.

Quoted 5 lines

I'm not saying the patch isn't required, I'm saying it's very hard to work out what the patch is supposed to do and why it's only an issue in the is_enabled() function.
Please resend with an improved changelog.

Sure. We'll format a new version.