[PATCH 1/3] regulator: tps65910: fix device initialisation

[PATCH 1/3] regulator: tps65910: fix device initialisation

[Johan Hovold]

Fix regression introduced by commit
a320e3c3d6351814afa5182159df88d2637e0f6f (regulator: tps65911: Add new
chip version) which broke probe for all devices:

	"Invalid tps chip version"

Cc: stable@kernel.org
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/regulator/tps65910-regulator.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 55dd4e6..bc5ed25 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -898,9 +898,11 @@ static __devinit int tps65910_probe(struct platform_device *pdev)
 	case TPS65910:
 		pmic->get_ctrl_reg = &tps65910_get_ctrl_register;
 		info = tps65910_regs;
+		break;
 	case TPS65911:
 		pmic->get_ctrl_reg = &tps65911_get_ctrl_register;
 		info = tps65911_regs;
+		break;
 	default:
 		pr_err("Invalid tps chip version\n");
 		return -ENODEV;
-- 
1.7.6

[PATCH 2/3] regulator: tps65910: fix list_voltage for VDD1 and VDD2

[Johan Hovold]

Fix regression introduced by commit
a320e3c3d6351814afa5182159df88d2637e0f6f (regulator: tps65911: Add new
chip version) which broke list_voltage for VDD1 and VDD2.

Cc: stable@kernel.org
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/regulator/tps65910-regulator.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index bc5ed25..2c61e48 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -759,6 +759,7 @@ static int tps65910_list_voltage_dcdc(struct regulator_dev *dev,
 		mult = (selector / VDD1_2_NUM_VOLTS) + 1;
 		volt = VDD1_2_MIN_VOLT +
 				(selector % VDD1_2_NUM_VOLTS) * VDD1_2_OFFSET;
+		break;
 	case TPS65911_REG_VDDCTRL:
 		volt = VDDCTRL_MIN_VOLT + (selector * VDDCTRL_OFFSET);
 	}
-- 
1.7.6

[PATCH 3/3] regulator: tps65910: verify dcdc regulator ids

[Johan Hovold]

Add default case to switches in dcdc voltage functions.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/regulator/tps65910-regulator.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 2c61e48..991dfa4 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -534,6 +534,8 @@ static int tps65910_get_voltage_dcdc(struct regulator_dev *dev)
 		srvsel &= VDDCTRL_SR_SEL_MASK;
 		vselmax = 64;
 		break;
+	default:
+		return -EINVAL;
 	}
 
 	/* multiplier 0 == 1 but 2,3 normal */
@@ -688,6 +690,9 @@ static int tps65910_set_voltage_dcdc(struct regulator_dev *dev,
 	case TPS65911_REG_VDDCTRL:
 		vsel = selector;
 		tps65910_reg_write(pmic, TPS65911_VDDCTRL_OP, vsel);
+		break;
+	default:
+		return -EINVAL;
 	}
 
 	return 0;
@@ -762,6 +767,9 @@ static int tps65910_list_voltage_dcdc(struct regulator_dev *dev,
 		break;
 	case TPS65911_REG_VDDCTRL:
 		volt = VDDCTRL_MIN_VOLT + (selector * VDDCTRL_OFFSET);
+		break;
+	default:
+		return -EINVAL;
 	}
 
 	return  volt * 100 * mult;
-- 
1.7.6

Re: [PATCH 1/3] regulator: tps65910: fix device initialisation

[Mark Brown]

On Mon, Aug 15, 2011 at 12:45:25PM +0200, Johan Hovold wrote:
> Fix regression introduced by commit
> a320e3c3d6351814afa5182159df88d2637e0f6f (regulator: tps65911: Add new
> chip version) which broke probe for all devices:
> 
> 	"Invalid tps chip version"

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

Re: [PATCH 2/3] regulator: tps65910: fix list_voltage for VDD1 and VDD2

[Mark Brown]

On Mon, Aug 15, 2011 at 12:45:26PM +0200, Johan Hovold wrote:
> Fix regression introduced by commit
> a320e3c3d6351814afa5182159df88d2637e0f6f (regulator: tps65911: Add new
> chip version) which broke list_voltage for VDD1 and VDD2.

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

Why is this a separate patch to the previous one?

Re: [PATCH 3/3] regulator: tps65910: verify dcdc regulator ids

[Mark Brown]

On Mon, Aug 15, 2011 at 12:45:27PM +0200, Johan Hovold wrote:
> Add default case to switches in dcdc voltage functions.
> 
> Signed-off-by: Johan Hovold <jhovold@gmail.com>

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

Re: [PATCH 2/3] regulator: tps65910: fix list_voltage for VDD1 and VDD2

[Johan Hovold]

On Mon, Aug 15, 2011 at 10:09:13PM +0900, Mark Brown wrote:
> On Mon, Aug 15, 2011 at 12:45:26PM +0200, Johan Hovold wrote:
> > Fix regression introduced by commit
> > a320e3c3d6351814afa5182159df88d2637e0f6f (regulator: tps65911: Add new
> > chip version) which broke list_voltage for VDD1 and VDD2.
> 
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> Why is this a separate patch to the previous one?

Because it fixes a different bug (although introduced by the same commit
and the same kind of programming error).

Thanks,
Johan

Re: [PATCH 2/3] regulator: tps65910: fix list_voltage for VDD1 and VDD2

[Mark Brown]

On Mon, Aug 15, 2011 at 04:29:48PM +0200, Johan Hovold wrote:
> On Mon, Aug 15, 2011 at 10:09:13PM +0900, Mark Brown wrote:

> > Why is this a separate patch to the previous one?

> Because it fixes a different bug (although introduced by the same commit
> and the same kind of programming error).

You've got a bunch of missing break statements introduced by the same
commit in the same driver causing the same runtime issue...

Re: [PATCH 2/3] regulator: tps65910: fix list_voltage for VDD1 and VDD2

[Johan Hovold]

On Mon, Aug 15, 2011 at 11:49:13PM +0900, Mark Brown wrote:
> On Mon, Aug 15, 2011 at 04:29:48PM +0200, Johan Hovold wrote:
> > On Mon, Aug 15, 2011 at 10:09:13PM +0900, Mark Brown wrote:
> 
> > > Why is this a separate patch to the previous one?
> 
> > Because it fixes a different bug (although introduced by the same commit
> > and the same kind of programming error).
> 
> You've got a bunch of missing break statements introduced by the same
> commit in the same driver causing the same runtime issue...

Well, the first bug prevents any device from being used at all (as probe
always fails). The second bug makes list_voltage return the wrong value
for two regulators.

I simply prefer fixing them separately as it will be more clear from the
changelog what behaviours are being fixed -- especially as they are
marked for stable.

Thanks,
Johan

Re: [PATCH 2/3] regulator: tps65910: fix list_voltage for VDD1 and VDD2

[Mark Brown]

On Mon, Aug 15, 2011 at 05:12:53PM +0200, Johan Hovold wrote:
> On Mon, Aug 15, 2011 at 11:49:13PM +0900, Mark Brown wrote:

> > You've got a bunch of missing break statements introduced by the same
> > commit in the same driver causing the same runtime issue...

> Well, the first bug prevents any device from being used at all (as probe
> always fails). The second bug makes list_voltage return the wrong value
> for two regulators.

> I simply prefer fixing them separately as it will be more clear from the
> changelog what behaviours are being fixed -- especially as they are
> marked for stable.

As a reviewer I did a double take as I wasn't immediately sure if this
was actually a different patch or not.

Re: [PATCH 1/3] regulator: tps65910: fix device initialisation

[Liam Girdwood]

Hi Johan,

On Mon, 2011-08-15 at 12:45 +0200, Johan Hovold wrote:
> Fix regression introduced by commit
> a320e3c3d6351814afa5182159df88d2637e0f6f (regulator: tps65911: Add new
> chip version) which broke probe for all devices:
> 
> 	"Invalid tps chip version"
> 
> Cc: stable@kernel.org
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
>  drivers/regulator/tps65910-regulator.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
> index 55dd4e6..bc5ed25 100644
> --- a/drivers/regulator/tps65910-regulator.c
> +++ b/drivers/regulator/tps65910-regulator.c
> @@ -898,9 +898,11 @@ static __devinit int tps65910_probe(struct platform_device *pdev)
>  	case TPS65910:
>  		pmic->get_ctrl_reg = &tps65910_get_ctrl_register;
>  		info = tps65910_regs;
> +		break;
>  	case TPS65911:
>  		pmic->get_ctrl_reg = &tps65911_get_ctrl_register;
>  		info = tps65911_regs;
> +		break;
>  	default:
>  		pr_err("Invalid tps chip version\n");
>  		return -ENODEV;

Couldn't apply these as it seems there may already be fixes to some of
the issues fixed by your patches. Could you update and resend.

Thanks !

Liam

Re: [PATCH 1/3] regulator: tps65910: fix device initialisation

[Johan Hovold]

On Sun, Aug 28, 2011 at 05:32:57PM +0100, Liam Girdwood wrote:
> Hi Johan,
> 
> On Mon, 2011-08-15 at 12:45 +0200, Johan Hovold wrote:
> > Fix regression introduced by commit
> > a320e3c3d6351814afa5182159df88d2637e0f6f (regulator: tps65911: Add new
> > chip version) which broke probe for all devices:
> > 
> > 	"Invalid tps chip version"
> > 
> > Cc: stable@kernel.org
> > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > ---
> >  drivers/regulator/tps65910-regulator.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
> > index 55dd4e6..bc5ed25 100644
> > --- a/drivers/regulator/tps65910-regulator.c
> > +++ b/drivers/regulator/tps65910-regulator.c
> > @@ -898,9 +898,11 @@ static __devinit int tps65910_probe(struct platform_device *pdev)
> >  	case TPS65910:
> >  		pmic->get_ctrl_reg = &tps65910_get_ctrl_register;
> >  		info = tps65910_regs;
> > +		break;
> >  	case TPS65911:
> >  		pmic->get_ctrl_reg = &tps65911_get_ctrl_register;
> >  		info = tps65911_regs;
> > +		break;
> >  	default:
> >  		pr_err("Invalid tps chip version\n");
> >  		return -ENODEV;
> 
> Couldn't apply these as it seems there may already be fixes to some of
> the issues fixed by your patches. Could you update and resend.

It seems patches 1 and 2 are subsumed by Axel Lin's
d04156bca629740a661fd0738cd69ba1f08b2b20 (regulator: tps65910: Add
missing breaks in switch/case), but this one was not marked for stable
even though it fixes a regression for tps65910 devices.

I'll send an updated patch 3 in a minute.

Thanks,
Johan

[PATCH 3/3 v2] regulator: tps65910: verify dcdc regulator ids

[Johan Hovold]

Add default case to switches in dcdc voltage functions.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/regulator/tps65910-regulator.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 66d2d60..633ecd4 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -534,6 +534,8 @@ static int tps65910_get_voltage_dcdc(struct regulator_dev *dev)
 		srvsel &= VDDCTRL_SR_SEL_MASK;
 		vselmax = 64;
 		break;
+	default:
+		return -EINVAL;
 	}
 
 	/* multiplier 0 == 1 but 2,3 normal */
@@ -688,6 +690,9 @@ static int tps65910_set_voltage_dcdc(struct regulator_dev *dev,
 	case TPS65911_REG_VDDCTRL:
 		vsel = selector;
 		tps65910_reg_write(pmic, TPS65911_VDDCTRL_OP, vsel);
+		break;
+	default:
+		return -EINVAL;
 	}
 
 	return 0;
-- 
1.7.6

Re: [PATCH 3/3 v2] regulator: tps65910: verify dcdc regulator ids

[Mark Brown]

On Tue, Aug 30, 2011 at 06:07:06PM +0200, Johan Hovold wrote:
> Add default case to switches in dcdc voltage functions.
> 
> Signed-off-by: Johan Hovold <jhovold@gmail.com>

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

Re: [PATCH 3/3 v2] regulator: tps65910: verify dcdc regulator ids

[Liam Girdwood]

On 30/08/11 17:07, Johan Hovold wrote:
> Add default case to switches in dcdc voltage functions.
> 
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
>  drivers/regulator/tps65910-regulator.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
> index 66d2d60..633ecd4 100644
> --- a/drivers/regulator/tps65910-regulator.c
> +++ b/drivers/regulator/tps65910-regulator.c
> @@ -534,6 +534,8 @@ static int tps65910_get_voltage_dcdc(struct regulator_dev *dev)
>  		srvsel &= VDDCTRL_SR_SEL_MASK;
>  		vselmax = 64;
>  		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
>  	/* multiplier 0 == 1 but 2,3 normal */
> @@ -688,6 +690,9 @@ static int tps65910_set_voltage_dcdc(struct regulator_dev *dev,
>  	case TPS65911_REG_VDDCTRL:
>  		vsel = selector;
>  		tps65910_reg_write(pmic, TPS65911_VDDCTRL_OP, vsel);
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
>  	return 0;

Applied.

Thanks

Liam