[PATCH] regulator: max8925: fix enabled/disabled judgement mistake

[PATCH] regulator: max8925: fix enabled/disabled judgement mistake

[Haojian Zhuang]

From: Kevin Liu <kliu5@marvell.com>

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 <kliu5@marvell.com>
---
 drivers/regulator/max8925-regulator.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/max8925-regulator.c b/drivers/regulator/max8925-regulator.c
index cc9ec0e..9e99420 100644
--- a/drivers/regulator/max8925-regulator.c
+++ b/drivers/regulator/max8925-regulator.c
@@ -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)
-- 
1.7.0.4

[PATCH] regulator: max8925: fix enabled/disabled judgement mistake

[Mark Brown]

On Thu, Dec 22, 2011 at 04:51:32PM +0800, Haojian Zhuang wrote:
> From: Kevin Liu <kliu5@marvell.com>
> 
> 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.

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

[PATCH] regulator: max8925: fix enabled/disabled judgement mistake

[Haojian Zhuang]

On Thu, Dec 22, 2011 at 6:53 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Dec 22, 2011 at 04:51:32PM +0800, Haojian Zhuang wrote:
>> From: Kevin Liu <kliu5@marvell.com>
>>
>> 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.

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

[PATCH] regulator: max8925: fix enabled/disabled judgement mistake

[Mark Brown]

On Thu, Dec 22, 2011 at 09:31:22PM +0800, Haojian Zhuang wrote:
> On Thu, Dec 22, 2011 at 6:53 PM, Mark Brown

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

[PATCH] regulator: max8925: fix enabled/disabled judgement mistake

[Haojian Zhuang]

On Thu, Dec 22, 2011 at 9:44 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Dec 22, 2011 at 09:31:22PM +0800, Haojian Zhuang wrote:
>> On Thu, Dec 22, 2011 at 6:53 PM, Mark Brown
>
>> > 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.

Mark,

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

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

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

[PATCH] regulator: max8925: fix enabled/disabled judgement mistake

[Mark Brown]

On Thu, Dec 22, 2011 at 10:22:17PM +0800, Haojian Zhuang wrote:

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

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

[PATCH] regulator: max8925: fix enabled/disabled judgement mistake

[Haojian Zhuang]

On Thu, Dec 22, 2011 at 10:38 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Dec 22, 2011 at 10:22:17PM +0800, Haojian Zhuang wrote:
>
>> 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.
>
>> 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.

Sure. We'll format a new version.

[PATCH] regulator: max8925: fix enabled/disabled judgement mistake

[Haojian Zhuang]

From: Kevin Liu <kliu5@marvell.com>

Max8925 ldo status should be determined by two factors:
1. power up/down sequence selection(LDOCTL[4:2]).
2. i2c enable bit(LDOCTL[0]).

Max8925 ldo support two types of power up/down sequence:
1. flexible sequence(LDOCTL[4:2] = 0~6).
2. i2c sequence(LDOCTL[4:2] = 7).

With flexible sequence selected, the ldo is enabled during power up by default.
With i2c sequence selected, the ldo is controlled by the i2c enable bit(LDOCTL[0]).

Signed-off-by: Kevin Liu <kliu5@marvell.com>
Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
---
 drivers/regulator/max8925-regulator.c |   33 ++++++++++++++++++++-------------
 1 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/regulator/max8925-regulator.c b/drivers/regulator/max8925-regulator.c
index cc9ec0e..9537f43 100644
--- a/drivers/regulator/max8925-regulator.c
+++ b/drivers/regulator/max8925-regulator.c
@@ -24,9 +24,13 @@
 #define SD1_DVM_SHIFT		5		/* SDCTL1 bit5 */
 #define SD1_DVM_EN		6		/* SDV1 bit 6 */
 
-/* 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 */
+/* bit definitions in LDO control registers */
+#define LDO_SEQ_I2C		0x7		/* Power U/D by i2c */
+#define LDO_SEQ_MASK		0x7		/* Power U/D sequence mask */
+#define LDO_SEQ_SHIFT		2		/* Power U/D sequence offset */
+#define LDO_I2C_EN		0x1		/* Enable by i2c */
+#define LDO_I2C_EN_MASK		0x1		/* Enable mask by i2c */
+#define LDO_I2C_EN_SHIFT	0		/* Enable offset by i2c */
 
 struct max8925_regulator_info {
 	struct regulator_desc	desc;
@@ -40,7 +44,6 @@ struct max8925_regulator_info {
 	int	vol_reg;
 	int	vol_shift;
 	int	vol_nbits;
-	int	enable_bit;
 	int	enable_reg;
 };
 
@@ -98,8 +101,10 @@ static int max8925_enable(struct regulator_dev *rdev)
 	struct max8925_regulator_info *info = rdev_get_drvdata(rdev);
 
 	return max8925_set_bits(info->i2c, info->enable_reg,
-				OUT_ENABLE << info->enable_bit,
-				OUT_ENABLE << info->enable_bit);
+				LDO_SEQ_MASK << LDO_SEQ_SHIFT |
+				LDO_I2C_EN_MASK << LDO_I2C_EN_SHIFT,
+				LDO_SEQ_I2C << LDO_SEQ_SHIFT |
+				LDO_I2C_EN << LDO_I2C_EN_SHIFT);
 }
 
 static int max8925_disable(struct regulator_dev *rdev)
@@ -107,20 +112,24 @@ static int max8925_disable(struct regulator_dev *rdev)
 	struct max8925_regulator_info *info = rdev_get_drvdata(rdev);
 
 	return max8925_set_bits(info->i2c, info->enable_reg,
-				OUT_ENABLE << info->enable_bit,
-				OUT_DISABLE << info->enable_bit);
+				LDO_SEQ_MASK << LDO_SEQ_SHIFT |
+				LDO_I2C_EN_MASK << LDO_I2C_EN_SHIFT,
+				LDO_SEQ_I2C << LDO_SEQ_SHIFT);
 }
 
 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)
+		return 1;
+	else
+		return ret & (LDO_I2C_EN_MASK << LDO_I2C_EN_SHIFT);
 }
 
 static int max8925_set_dvm_voltage(struct regulator_dev *rdev, int uV)
@@ -188,7 +197,6 @@ static struct regulator_ops max8925_regulator_ldo_ops = {
 	.vol_shift	= 0,					\
 	.vol_nbits	= 6,					\
 	.enable_reg	= MAX8925_SDCTL##_id,			\
-	.enable_bit	= 0,					\
 }
 
 #define MAX8925_LDO(_id, min, max, step)			\
@@ -207,7 +215,6 @@ static struct regulator_ops max8925_regulator_ldo_ops = {
 	.vol_shift	= 0,					\
 	.vol_nbits	= 6,					\
 	.enable_reg	= MAX8925_LDOCTL##_id,			\
-	.enable_bit	= 0,					\
 }
 
 static struct max8925_regulator_info max8925_regulator_info[] = {
-- 
1.7.0.4