[RC FIXES] Fix broken MMC devices on Snowball

[RC FIXES] Fix broken MMC devices on Snowball

[Lee Jones]

The newly introduced ab8500 external regulator driver causes an
unexpected quirk when used with the Snowball low-cost development
board. When the 3 ab8500 external regulators are registered, the
ones which can be freed, are. Unfortunately, on Snowball one of
them is connected to the MMC power line. Once freed, this turns
off the MMC device. External regulators are not used on Snowball,
so it's pointless registering them in the first place.

These two patches ensure external regulators aren't registered 
on Snowball or any other platforms which do not require them.

[RC FIXES] Fix broken MMC devices on Snowball

[Lee Jones]

The newly introduced ab8500 external regulator driver causes an
unexpected quirk when used with the Snowball low-cost development
board. When the 3 ab8500 external regulators are registered, the
ones which can be freed, are. Unfortunately, on Snowball one of
them is connected to the MMC power line. Once freed, this turns
off the MMC device. External regulators are not used on Snowball,
so it's pointless registering them in the first place.

These two patches ensure external regulators aren't registered 
on Snowball or any other platforms which do not require them.

[PATCH] ARM: ux500: Allow Snowball to pass its own regulator settings

[Lee Jones]

This patch fixes a bug introduced in the v3.10 merge window.

Currently the external regulator settings are passed though to the AB8500
regulator driver regards of whether they can or should be used on any
ux500 based platform. They are not applicable for the u8505 and when
enabled, they turn off AUX2 which adversely affects the eMMC on Snowball.

The driver already knows that the external regulator settings are not
appropriate for the u8505, however it's more difficult for driver code to
distinguish between two different u8500 based platforms.

Using the new semantics, we can just not pass the external regulator
initialisation values from platform code and the external regulator
driver will know not to register them.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-regulators.c |    8 ++++++++
 arch/arm/mach-ux500/board-mop500-regulators.h |    1 +
 arch/arm/mach-ux500/board-mop500.c            |    2 ++
 3 files changed, 11 insertions(+)

diff --git a/arch/arm/mach-ux500/board-mop500-regulators.c b/arch/arm/mach-ux500/board-mop500-regulators.c
index b34441b..7399e90 100644
--- a/arch/arm/mach-ux500/board-mop500-regulators.c
+++ b/arch/arm/mach-ux500/board-mop500-regulators.c
@@ -996,6 +996,14 @@ struct ab8500_regulator_platform_data ab8500_regulator_plat_data = {
 	.num_ext_regulator      = ARRAY_SIZE(ab8500_ext_regulators),
 };
 
+/* External regulators turn off VAUX2 when in use on Snowball. */
+struct ab8500_regulator_platform_data snowball_regulator_plat_data = {
+	.reg_init               = ab8500_reg_init,
+	.num_reg_init           = ARRAY_SIZE(ab8500_reg_init),
+	.regulator              = ab8500_regulators,
+	.num_regulator          = ARRAY_SIZE(ab8500_regulators),
+};
+
 struct ab8500_regulator_platform_data ab8505_regulator_plat_data = {
 	.reg_init               = ab8505_reg_init,
 	.num_reg_init           = ARRAY_SIZE(ab8505_reg_init),
diff --git a/arch/arm/mach-ux500/board-mop500-regulators.h b/arch/arm/mach-ux500/board-mop500-regulators.h
index 039f513..6300e79 100644
--- a/arch/arm/mach-ux500/board-mop500-regulators.h
+++ b/arch/arm/mach-ux500/board-mop500-regulators.h
@@ -16,6 +16,7 @@
 
 extern struct ab8500_regulator_platform_data ab8500_regulator_plat_data;
 extern struct ab8500_regulator_platform_data ab8505_regulator_plat_data;
+extern struct ab8500_regulator_platform_data snowball_regulator_plat_data;
 extern struct regulator_init_data tps61052_regulator;
 extern struct regulator_init_data gpio_en_3v3_regulator;
 extern struct regulator_init_data sdi0_reg_init_data;
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 0b018c4..2d609d4 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -615,6 +615,8 @@ static void __init snowball_init_machine(void)
 	struct device *parent = NULL;
 	int i;
 
+	ab8500_platdata.regulator = &snowball_regulator_plat_data;
+
 	platform_device_register(&db8500_prcmu_device);
 
 	sdi0_reg_info.enable_gpio = SNOWBALL_SDMMC_EN_GPIO;
-- 
1.7.10.4

[PATCH] ARM: ux500: Allow Snowball to pass its own regulator settings

[Lee Jones]

This patch fixes a bug introduced in the v3.10 merge window.

Currently the external regulator settings are passed though to the AB8500
regulator driver regards of whether they can or should be used on any
ux500 based platform. They are not applicable for the u8505 and when
enabled, they turn off AUX2 which adversely affects the eMMC on Snowball.

The driver already knows that the external regulator settings are not
appropriate for the u8505, however it's more difficult for driver code to
distinguish between two different u8500 based platforms.

Using the new semantics, we can just not pass the external regulator
initialisation values from platform code and the external regulator
driver will know not to register them.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-regulators.c |    8 ++++++++
 arch/arm/mach-ux500/board-mop500-regulators.h |    1 +
 arch/arm/mach-ux500/board-mop500.c            |    2 ++
 3 files changed, 11 insertions(+)

diff --git a/arch/arm/mach-ux500/board-mop500-regulators.c b/arch/arm/mach-ux500/board-mop500-regulators.c
index b34441b..7399e90 100644
--- a/arch/arm/mach-ux500/board-mop500-regulators.c
+++ b/arch/arm/mach-ux500/board-mop500-regulators.c
@@ -996,6 +996,14 @@ struct ab8500_regulator_platform_data ab8500_regulator_plat_data = {
 	.num_ext_regulator      = ARRAY_SIZE(ab8500_ext_regulators),
 };
 
+/* External regulators turn off VAUX2 when in use on Snowball. */
+struct ab8500_regulator_platform_data snowball_regulator_plat_data = {
+	.reg_init               = ab8500_reg_init,
+	.num_reg_init           = ARRAY_SIZE(ab8500_reg_init),
+	.regulator              = ab8500_regulators,
+	.num_regulator          = ARRAY_SIZE(ab8500_regulators),
+};
+
 struct ab8500_regulator_platform_data ab8505_regulator_plat_data = {
 	.reg_init               = ab8505_reg_init,
 	.num_reg_init           = ARRAY_SIZE(ab8505_reg_init),
diff --git a/arch/arm/mach-ux500/board-mop500-regulators.h b/arch/arm/mach-ux500/board-mop500-regulators.h
index 039f513..6300e79 100644
--- a/arch/arm/mach-ux500/board-mop500-regulators.h
+++ b/arch/arm/mach-ux500/board-mop500-regulators.h
@@ -16,6 +16,7 @@
 
 extern struct ab8500_regulator_platform_data ab8500_regulator_plat_data;
 extern struct ab8500_regulator_platform_data ab8505_regulator_plat_data;
+extern struct ab8500_regulator_platform_data snowball_regulator_plat_data;
 extern struct regulator_init_data tps61052_regulator;
 extern struct regulator_init_data gpio_en_3v3_regulator;
 extern struct regulator_init_data sdi0_reg_init_data;
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 0b018c4..2d609d4 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -615,6 +615,8 @@ static void __init snowball_init_machine(void)
 	struct device *parent = NULL;
 	int i;
 
+	ab8500_platdata.regulator = &snowball_regulator_plat_data;
+
 	platform_device_register(&db8500_prcmu_device);
 
 	sdi0_reg_info.enable_gpio = SNOWBALL_SDMMC_EN_GPIO;
-- 
1.7.10.4

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

This patch fixes a bug introduced in the v3.10 merge window.

Some platforms will not want external registers. Rather than setting up
lots of different clauses in the core ab8500 regulator driver not to
call ab8500-ext init() we just won't pass the initialisation data from
platform code. This patch checks for it and if it's missing, we won't
register the external regulators.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/regulator/ab8500-ext.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
index b4d4547..8421537 100644
--- a/drivers/regulator/ab8500-ext.c
+++ b/drivers/regulator/ab8500-ext.c
@@ -334,6 +334,12 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	/* have any external regulators been specified? */
+	if (pdata->num_ext_regulator == 0) {
+		dev_warn(&pdev->dev, "Not using external regulators\n");
+		return 0;
+	}
+
 	/* make sure the platform data has the correct size */
 	if (pdata->num_ext_regulator != ARRAY_SIZE(ab8500_ext_regulator_info)) {
 		dev_err(&pdev->dev, "Configuration error: size mismatch.\n");
-- 
1.7.10.4

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

This patch fixes a bug introduced in the v3.10 merge window.

Some platforms will not want external registers. Rather than setting up
lots of different clauses in the core ab8500 regulator driver not to
call ab8500-ext init() we just won't pass the initialisation data from
platform code. This patch checks for it and if it's missing, we won't
register the external regulators.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/regulator/ab8500-ext.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
index b4d4547..8421537 100644
--- a/drivers/regulator/ab8500-ext.c
+++ b/drivers/regulator/ab8500-ext.c
@@ -334,6 +334,12 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	/* have any external regulators been specified? */
+	if (pdata->num_ext_regulator == 0) {
+		dev_warn(&pdev->dev, "Not using external regulators\n");
+		return 0;
+	}
+
 	/* make sure the platform data has the correct size */
 	if (pdata->num_ext_regulator != ARRAY_SIZE(ab8500_ext_regulator_info)) {
 		dev_err(&pdev->dev, "Configuration error: size mismatch.\n");
-- 
1.7.10.4

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Linus Walleij]

On Wed, May 22, 2013 at 2:47 PM, Lee Jones <lee.jones@linaro.org> wrote:

> This patch fixes a bug introduced in the v3.10 merge window.
>
> Some platforms will not want external registers. Rather than setting up
> lots of different clauses in the core ab8500 regulator driver not to
> call ab8500-ext init() we just won't pass the initialisation data from
> platform code. This patch checks for it and if it's missing, we won't
> register the external regulators.
>
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/regulator/ab8500-ext.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> index b4d4547..8421537 100644
> --- a/drivers/regulator/ab8500-ext.c
> +++ b/drivers/regulator/ab8500-ext.c
> @@ -334,6 +334,12 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> +       /* have any external regulators been specified? */
> +       if (pdata->num_ext_regulator == 0) {
> +               dev_warn(&pdev->dev, "Not using external regulators\n");
> +               return 0;
> +       }

dev_warn() is really when we warn about something really nasty, worse
than dev_err(). It seems dev_info() is more apropriate.

Anyway since it's an urgent regression:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Linus

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Linus Walleij]

On Wed, May 22, 2013 at 2:47 PM, Lee Jones <lee.jones@linaro.org> wrote:

> This patch fixes a bug introduced in the v3.10 merge window.
>
> Some platforms will not want external registers. Rather than setting up
> lots of different clauses in the core ab8500 regulator driver not to
> call ab8500-ext init() we just won't pass the initialisation data from
> platform code. This patch checks for it and if it's missing, we won't
> register the external regulators.
>
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/regulator/ab8500-ext.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> index b4d4547..8421537 100644
> --- a/drivers/regulator/ab8500-ext.c
> +++ b/drivers/regulator/ab8500-ext.c
> @@ -334,6 +334,12 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> +       /* have any external regulators been specified? */
> +       if (pdata->num_ext_regulator == 0) {
> +               dev_warn(&pdev->dev, "Not using external regulators\n");
> +               return 0;
> +       }

dev_warn() is really when we warn about something really nasty, worse
than dev_err(). It seems dev_info() is more apropriate.

Anyway since it's an urgent regression:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Linus

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Arnd Bergmann]

On Wednesday 22 May 2013, Linus Walleij wrote:
> > diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> > index b4d4547..8421537 100644
> > --- a/drivers/regulator/ab8500-ext.c
> > +++ b/drivers/regulator/ab8500-ext.c
> > @@ -334,6 +334,12 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
> >                 return -EINVAL;
> >         }
> >
> > +       /* have any external regulators been specified? */
> > +       if (pdata->num_ext_regulator == 0) {
> > +               dev_warn(&pdev->dev, "Not using external regulators\n");
> > +               return 0;
> > +       }
> 
> dev_warn() is really when we warn about something really nasty, worse
> than dev_err(). It seems dev_info() is more apropriate.

Are you confusing it with dev_crit()?

This is how the levels are defined, and "warning" is less urgent than "error":

#define KERN_EMERG      KERN_SOH "0"    /* system is unusable */
#define KERN_ALERT      KERN_SOH "1"    /* action must be taken immediately */
#define KERN_CRIT       KERN_SOH "2"    /* critical conditions */
#define KERN_ERR        KERN_SOH "3"    /* error conditions */
#define KERN_WARNING    KERN_SOH "4"    /* warning conditions */
#define KERN_NOTICE     KERN_SOH "5"    /* normal but significant condition */
#define KERN_INFO       KERN_SOH "6"    /* informational */
#define KERN_DEBUG      KERN_SOH "7"    /* debug-level messages */

	Arnd

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Arnd Bergmann]

On Wednesday 22 May 2013, Linus Walleij wrote:
> > diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> > index b4d4547..8421537 100644
> > --- a/drivers/regulator/ab8500-ext.c
> > +++ b/drivers/regulator/ab8500-ext.c
> > @@ -334,6 +334,12 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
> >                 return -EINVAL;
> >         }
> >
> > +       /* have any external regulators been specified? */
> > +       if (pdata->num_ext_regulator == 0) {
> > +               dev_warn(&pdev->dev, "Not using external regulators\n");
> > +               return 0;
> > +       }
> 
> dev_warn() is really when we warn about something really nasty, worse
> than dev_err(). It seems dev_info() is more apropriate.

Are you confusing it with dev_crit()?

This is how the levels are defined, and "warning" is less urgent than "error":

#define KERN_EMERG      KERN_SOH "0"    /* system is unusable */
#define KERN_ALERT      KERN_SOH "1"    /* action must be taken immediately */
#define KERN_CRIT       KERN_SOH "2"    /* critical conditions */
#define KERN_ERR        KERN_SOH "3"    /* error conditions */
#define KERN_WARNING    KERN_SOH "4"    /* warning conditions */
#define KERN_NOTICE     KERN_SOH "5"    /* normal but significant condition */
#define KERN_INFO       KERN_SOH "6"    /* informational */
#define KERN_DEBUG      KERN_SOH "7"    /* debug-level messages */

	Arnd

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Mark Brown]

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

On Wed, May 22, 2013 at 01:47:33PM +0100, Lee Jones wrote:
> This patch fixes a bug introduced in the v3.10 merge window.
> 
> Some platforms will not want external registers. Rather than setting up
> lots of different clauses in the core ab8500 regulator driver not to
> call ab8500-ext init() we just won't pass the initialisation data from
> platform code. This patch checks for it and if it's missing, we won't
> register the external regulators.

This seems problematic - if the regulators are unused then they
shouldn't have anything enabled in the constraints and the regulator
driver ought to be read only in which case it doesn't matter if it's
running or not, the worst that should happen is that the state can be
read back.  I'd therefore expect the fix here to be in the board side
code that enables the regulators.

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

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Mark Brown]

On Wed, May 22, 2013 at 01:47:33PM +0100, Lee Jones wrote:
> This patch fixes a bug introduced in the v3.10 merge window.
> 
> Some platforms will not want external registers. Rather than setting up
> lots of different clauses in the core ab8500 regulator driver not to
> call ab8500-ext init() we just won't pass the initialisation data from
> platform code. This patch checks for it and if it's missing, we won't
> register the external regulators.

This seems problematic - if the regulators are unused then they
shouldn't have anything enabled in the constraints and the regulator
driver ought to be read only in which case it doesn't matter if it's
running or not, the worst that should happen is that the state can be
read back.  I'd therefore expect the fix here to be in the board side
code that enables 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/20130522/9e4f7de8/attachment.sig>

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

On Wed, 22 May 2013, Mark Brown wrote:

> On Wed, May 22, 2013 at 01:47:33PM +0100, Lee Jones wrote:
> > This patch fixes a bug introduced in the v3.10 merge window.
> > 
> > Some platforms will not want external registers. Rather than setting up
> > lots of different clauses in the core ab8500 regulator driver not to
> > call ab8500-ext init() we just won't pass the initialisation data from
> > platform code. This patch checks for it and if it's missing, we won't
> > register the external regulators.
> 
> This seems problematic - if the regulators are unused then they
> shouldn't have anything enabled in the constraints

Right, that's what this does.

"we just won't pass the initialisation data from platform code."

'initialisation data' == 'constraints'.

> and the regulator
> driver ought to be read only in which case it doesn't matter if it's
> running or not, the worst that should happen is that the state can be
> read back.  I'd therefore expect the fix here to be in the board side
> code that enables the regulators.

It's both.

>From the platform side we have stopped passing the external regulator
driver's constraints for Snowball:

https://lkml.org/lkml/2013/5/22/280

All I've done here is stop the ext driver from crapping out with the
misleading error "Configuration error: size mismatch", because a)
it's not an error, it's expected and b) It's not a size mismatch,
instead we have chosen to keep the constraints back in the Snowball
(and u8505) case.

Strictly speaking 'this' part of the fix is not an -rc candidate, but
without it someone could waste a great deal of time finding out why
this 'error' has appeared when it's not actually an error at all. Thus,
it would be more than helpful if it made it into the -rcs along with
the real platform fix.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

On Wed, 22 May 2013, Mark Brown wrote:

> On Wed, May 22, 2013 at 01:47:33PM +0100, Lee Jones wrote:
> > This patch fixes a bug introduced in the v3.10 merge window.
> > 
> > Some platforms will not want external registers. Rather than setting up
> > lots of different clauses in the core ab8500 regulator driver not to
> > call ab8500-ext init() we just won't pass the initialisation data from
> > platform code. This patch checks for it and if it's missing, we won't
> > register the external regulators.
> 
> This seems problematic - if the regulators are unused then they
> shouldn't have anything enabled in the constraints

Right, that's what this does.

"we just won't pass the initialisation data from platform code."

'initialisation data' == 'constraints'.

> and the regulator
> driver ought to be read only in which case it doesn't matter if it's
> running or not, the worst that should happen is that the state can be
> read back.  I'd therefore expect the fix here to be in the board side
> code that enables the regulators.

It's both.

>From the platform side we have stopped passing the external regulator
driver's constraints for Snowball:

https://lkml.org/lkml/2013/5/22/280

All I've done here is stop the ext driver from crapping out with the
misleading error "Configuration error: size mismatch", because a)
it's not an error, it's expected and b) It's not a size mismatch,
instead we have chosen to keep the constraints back in the Snowball
(and u8505) case.

Strictly speaking 'this' part of the fix is not an -rc candidate, but
without it someone could waste a great deal of time finding out why
this 'error' has appeared when it's not actually an error at all. Thus,
it would be more than helpful if it made it into the -rcs along with
the real platform fix.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Mark Brown]

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

On Wed, May 22, 2013 at 05:53:53PM +0100, Lee Jones wrote:
> On Wed, 22 May 2013, Mark Brown wrote:

> > running or not, the worst that should happen is that the state can be
> > read back.  I'd therefore expect the fix here to be in the board side
> > code that enables the regulators.

> It's both.

> From the platform side we have stopped passing the external regulator
> driver's constraints for Snowball:

> https://lkml.org/lkml/2013/5/22/280

> All I've done here is stop the ext driver from crapping out with the
> misleading error "Configuration error: size mismatch", because a)
> it's not an error, it's expected and b) It's not a size mismatch,
> instead we have chosen to keep the constraints back in the Snowball
> (and u8505) case.

OK, so the regulator driver is buggy and insisting on having constraints
passed in for every regulator then?

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

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Mark Brown]

On Wed, May 22, 2013 at 05:53:53PM +0100, Lee Jones wrote:
> On Wed, 22 May 2013, Mark Brown wrote:

> > running or not, the worst that should happen is that the state can be
> > read back.  I'd therefore expect the fix here to be in the board side
> > code that enables the regulators.

> It's both.

> From the platform side we have stopped passing the external regulator
> driver's constraints for Snowball:

> https://lkml.org/lkml/2013/5/22/280

> All I've done here is stop the ext driver from crapping out with the
> misleading error "Configuration error: size mismatch", because a)
> it's not an error, it's expected and b) It's not a size mismatch,
> instead we have chosen to keep the constraints back in the Snowball
> (and u8505) case.

OK, so the regulator driver is buggy and insisting on having constraints
passed in for every regulator then?
-------------- 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/20130522/7ecf3ecc/attachment.sig>

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

On Wed, 22 May 2013, Mark Brown wrote:

> On Wed, May 22, 2013 at 05:53:53PM +0100, Lee Jones wrote:
> > On Wed, 22 May 2013, Mark Brown wrote:
> 
> > > running or not, the worst that should happen is that the state can be
> > > read back.  I'd therefore expect the fix here to be in the board side
> > > code that enables the regulators.
> 
> > It's both.
> 
> > From the platform side we have stopped passing the external regulator
> > driver's constraints for Snowball:
> 
> > https://lkml.org/lkml/2013/5/22/280
> 
> > All I've done here is stop the ext driver from crapping out with the
> > misleading error "Configuration error: size mismatch", because a)
> > it's not an error, it's expected and b) It's not a size mismatch,
> > instead we have chosen to keep the constraints back in the Snowball
> > (and u8505) case.
> 
> OK, so the regulator driver is buggy and insisting on having constraints
> passed in for every regulator then?

If that's not how it's meant to work then, yes.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

On Wed, 22 May 2013, Mark Brown wrote:

> On Wed, May 22, 2013 at 05:53:53PM +0100, Lee Jones wrote:
> > On Wed, 22 May 2013, Mark Brown wrote:
> 
> > > running or not, the worst that should happen is that the state can be
> > > read back.  I'd therefore expect the fix here to be in the board side
> > > code that enables the regulators.
> 
> > It's both.
> 
> > From the platform side we have stopped passing the external regulator
> > driver's constraints for Snowball:
> 
> > https://lkml.org/lkml/2013/5/22/280
> 
> > All I've done here is stop the ext driver from crapping out with the
> > misleading error "Configuration error: size mismatch", because a)
> > it's not an error, it's expected and b) It's not a size mismatch,
> > instead we have chosen to keep the constraints back in the Snowball
> > (and u8505) case.
> 
> OK, so the regulator driver is buggy and insisting on having constraints
> passed in for every regulator then?

If that's not how it's meant to work then, yes.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Linus Walleij]

On Wed, May 22, 2013 at 4:08 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 22 May 2013, Linus Walleij wrote:
>> > diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
>> > index b4d4547..8421537 100644
>> > --- a/drivers/regulator/ab8500-ext.c
>> > +++ b/drivers/regulator/ab8500-ext.c
>> > @@ -334,6 +334,12 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
>> >                 return -EINVAL;
>> >         }
>> >
>> > +       /* have any external regulators been specified? */
>> > +       if (pdata->num_ext_regulator == 0) {
>> > +               dev_warn(&pdev->dev, "Not using external regulators\n");
>> > +               return 0;
>> > +       }
>>
>> dev_warn() is really when we warn about something really nasty, worse
>> than dev_err(). It seems dev_info() is more apropriate.
>
> Are you confusing it with dev_crit()?
>
> This is how the levels are defined, and "warning" is less urgent than "error":

Yeah :-/

But I still think this should be just dev_info().

No big deal though.

Yours,
Linus Walleij

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Linus Walleij]

On Wed, May 22, 2013 at 4:08 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 22 May 2013, Linus Walleij wrote:
>> > diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
>> > index b4d4547..8421537 100644
>> > --- a/drivers/regulator/ab8500-ext.c
>> > +++ b/drivers/regulator/ab8500-ext.c
>> > @@ -334,6 +334,12 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
>> >                 return -EINVAL;
>> >         }
>> >
>> > +       /* have any external regulators been specified? */
>> > +       if (pdata->num_ext_regulator == 0) {
>> > +               dev_warn(&pdev->dev, "Not using external regulators\n");
>> > +               return 0;
>> > +       }
>>
>> dev_warn() is really when we warn about something really nasty, worse
>> than dev_err(). It seems dev_info() is more apropriate.
>
> Are you confusing it with dev_crit()?
>
> This is how the levels are defined, and "warning" is less urgent than "error":

Yeah :-/

But I still think this should be just dev_info().

No big deal though.

Yours,
Linus Walleij

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Mark Brown]

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

On Thu, May 23, 2013 at 08:09:05AM +0100, Lee Jones wrote:
> On Wed, 22 May 2013, Mark Brown wrote:

> > OK, so the regulator driver is buggy and insisting on having constraints
> > passed in for every regulator then?

> If that's not how it's meant to work then, yes.

Yes, that's the intention - it should be possible to just not pass in
anything in and have the driver start up in read only mode (partly to
save having to do things like what you're doing).

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

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Mark Brown]

On Thu, May 23, 2013 at 08:09:05AM +0100, Lee Jones wrote:
> On Wed, 22 May 2013, Mark Brown wrote:

> > OK, so the regulator driver is buggy and insisting on having constraints
> > passed in for every regulator then?

> If that's not how it's meant to work then, yes.

Yes, that's the intention - it should be possible to just not pass in
anything in and have the driver start up in read only mode (partly to
save having to do things like what you're doing).
-------------- 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/20130523/3b48a1f1/attachment.sig>

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

On Thu, 23 May 2013, Mark Brown wrote:

> On Thu, May 23, 2013 at 08:09:05AM +0100, Lee Jones wrote:
> > On Wed, 22 May 2013, Mark Brown wrote:
> 
> > > OK, so the regulator driver is buggy and insisting on having constraints
> > > passed in for every regulator then?
> 
> > If that's not how it's meant to work then, yes.
> 
> Yes, that's the intention - it should be possible to just not pass in
> anything in and have the driver start up in read only mode (partly to
> save having to do things like what you're doing).

Okay, noted.

Well, whatever happens I'm going to be able to refactor the driver in
the -rc:s, so we still need this fix, or the world will end.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

On Thu, 23 May 2013, Mark Brown wrote:

> On Thu, May 23, 2013 at 08:09:05AM +0100, Lee Jones wrote:
> > On Wed, 22 May 2013, Mark Brown wrote:
> 
> > > OK, so the regulator driver is buggy and insisting on having constraints
> > > passed in for every regulator then?
> 
> > If that's not how it's meant to work then, yes.
> 
> Yes, that's the intention - it should be possible to just not pass in
> anything in and have the driver start up in read only mode (partly to
> save having to do things like what you're doing).

Okay, noted.

Well, whatever happens I'm going to be able to refactor the driver in
the -rc:s, so we still need this fix, or the world will end.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Mark Brown]

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

On Thu, May 23, 2013 at 03:44:53PM +0100, Lee Jones wrote:

> Well, whatever happens I'm going to be able to refactor the driver in
> the -rc:s, so we still need this fix, or the world will end.

Why is it so invasive to have the driver cope with absent constraints?

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

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Mark Brown]

On Thu, May 23, 2013 at 03:44:53PM +0100, Lee Jones wrote:

> Well, whatever happens I'm going to be able to refactor the driver in
> the -rc:s, so we still need this fix, or the world will end.

Why is it so invasive to have the driver cope with absent constraints?
-------------- 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/20130523/991bec63/attachment.sig>

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

On Thu, 23 May 2013, Mark Brown wrote:

> On Thu, May 23, 2013 at 03:44:53PM +0100, Lee Jones wrote:
> 
> > Well, whatever happens I'm going to be able to refactor the driver in
> > the -rc:s, so we still need this fix, or the world will end.
> 
> Why is it so invasive to have the driver cope with absent constraints?

It probably isn't. Are you asking me to code that up for the -rc:s?

I would be happier if I could stick it onto my TODO and address it
at another time for inclusion into -next, as I have a list of priority
items to attend to in the immediate future.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

On Thu, 23 May 2013, Mark Brown wrote:

> On Thu, May 23, 2013 at 03:44:53PM +0100, Lee Jones wrote:
> 
> > Well, whatever happens I'm going to be able to refactor the driver in
> > the -rc:s, so we still need this fix, or the world will end.
> 
> Why is it so invasive to have the driver cope with absent constraints?

It probably isn't. Are you asking me to code that up for the -rc:s?

I would be happier if I could stick it onto my TODO and address it
at another time for inclusion into -next, as I have a list of priority
items to attend to in the immediate future.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

On Wed, 22 May 2013, Lee Jones wrote:

> This patch fixes a bug introduced in the v3.10 merge window.
> 
> Some platforms will not want external registers. Rather than setting up
> lots of different clauses in the core ab8500 regulator driver not to
> call ab8500-ext init() we just won't pass the initialisation data from
> platform code. This patch checks for it and if it's missing, we won't
> register the external regulators.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/regulator/ab8500-ext.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> index b4d4547..8421537 100644
> --- a/drivers/regulator/ab8500-ext.c
> +++ b/drivers/regulator/ab8500-ext.c
> @@ -334,6 +334,12 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	/* have any external regulators been specified? */
> +	if (pdata->num_ext_regulator == 0) {
> +		dev_warn(&pdev->dev, "Not using external regulators\n");
> +		return 0;
> +	}
> +
>  	/* make sure the platform data has the correct size */
>  	if (pdata->num_ext_regulator != ARRAY_SIZE(ab8500_ext_regulator_info)) {
>  		dev_err(&pdev->dev, "Configuration error: size mismatch.\n");

Okay drop this patch, I have a better solution.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH] regulator: ab8500-ext: Don't register without initialisation data

[Lee Jones]

On Wed, 22 May 2013, Lee Jones wrote:

> This patch fixes a bug introduced in the v3.10 merge window.
> 
> Some platforms will not want external registers. Rather than setting up
> lots of different clauses in the core ab8500 regulator driver not to
> call ab8500-ext init() we just won't pass the initialisation data from
> platform code. This patch checks for it and if it's missing, we won't
> register the external regulators.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/regulator/ab8500-ext.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> index b4d4547..8421537 100644
> --- a/drivers/regulator/ab8500-ext.c
> +++ b/drivers/regulator/ab8500-ext.c
> @@ -334,6 +334,12 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	/* have any external regulators been specified? */
> +	if (pdata->num_ext_regulator == 0) {
> +		dev_warn(&pdev->dev, "Not using external regulators\n");
> +		return 0;
> +	}
> +
>  	/* make sure the platform data has the correct size */
>  	if (pdata->num_ext_regulator != ARRAY_SIZE(ab8500_ext_regulator_info)) {
>  		dev_err(&pdev->dev, "Configuration error: size mismatch.\n");

Okay drop this patch, I have a better solution.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] ARM: ux500: Allow Snowball to pass its own regulator settings

[Lee Jones]

On Wed, 22 May 2013, Lee Jones wrote:

> This patch fixes a bug introduced in the v3.10 merge window.
> 
> Currently the external regulator settings are passed though to the AB8500
> regulator driver regards of whether they can or should be used on any
> ux500 based platform. They are not applicable for the u8505 and when
> enabled, they turn off AUX2 which adversely affects the eMMC on Snowball.
> 
> The driver already knows that the external regulator settings are not
> appropriate for the u8505, however it's more difficult for driver code to
> distinguish between two different u8500 based platforms.
> 
> Using the new semantics, we can just not pass the external regulator
> initialisation values from platform code and the external regulator
> driver will know not to register them.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/mach-ux500/board-mop500-regulators.c |    8 ++++++++
>  arch/arm/mach-ux500/board-mop500-regulators.h |    1 +
>  arch/arm/mach-ux500/board-mop500.c            |    2 ++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/arch/arm/mach-ux500/board-mop500-regulators.c b/arch/arm/mach-ux500/board-mop500-regulators.c
> index b34441b..7399e90 100644
> --- a/arch/arm/mach-ux500/board-mop500-regulators.c
> +++ b/arch/arm/mach-ux500/board-mop500-regulators.c
> @@ -996,6 +996,14 @@ struct ab8500_regulator_platform_data ab8500_regulator_plat_data = {
>  	.num_ext_regulator      = ARRAY_SIZE(ab8500_ext_regulators),
>  };
>  
> +/* External regulators turn off VAUX2 when in use on Snowball. */
> +struct ab8500_regulator_platform_data snowball_regulator_plat_data = {
> +	.reg_init               = ab8500_reg_init,
> +	.num_reg_init           = ARRAY_SIZE(ab8500_reg_init),
> +	.regulator              = ab8500_regulators,
> +	.num_regulator          = ARRAY_SIZE(ab8500_regulators),
> +};
> +
>  struct ab8500_regulator_platform_data ab8505_regulator_plat_data = {
>  	.reg_init               = ab8505_reg_init,
>  	.num_reg_init           = ARRAY_SIZE(ab8505_reg_init),
> diff --git a/arch/arm/mach-ux500/board-mop500-regulators.h b/arch/arm/mach-ux500/board-mop500-regulators.h
> index 039f513..6300e79 100644
> --- a/arch/arm/mach-ux500/board-mop500-regulators.h
> +++ b/arch/arm/mach-ux500/board-mop500-regulators.h
> @@ -16,6 +16,7 @@
>  
>  extern struct ab8500_regulator_platform_data ab8500_regulator_plat_data;
>  extern struct ab8500_regulator_platform_data ab8505_regulator_plat_data;
> +extern struct ab8500_regulator_platform_data snowball_regulator_plat_data;
>  extern struct regulator_init_data tps61052_regulator;
>  extern struct regulator_init_data gpio_en_3v3_regulator;
>  extern struct regulator_init_data sdi0_reg_init_data;
> diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
> index 0b018c4..2d609d4 100644
> --- a/arch/arm/mach-ux500/board-mop500.c
> +++ b/arch/arm/mach-ux500/board-mop500.c
> @@ -615,6 +615,8 @@ static void __init snowball_init_machine(void)
>  	struct device *parent = NULL;
>  	int i;
>  
> +	ab8500_platdata.regulator = &snowball_regulator_plat_data;
> +
>  	platform_device_register(&db8500_prcmu_device);
>  
>  	sdi0_reg_info.enable_gpio = SNOWBALL_SDMMC_EN_GPIO;

Please drop this patch, I have sent a better solution.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH] ARM: ux500: Allow Snowball to pass its own regulator settings

[Lee Jones]

On Wed, 22 May 2013, Lee Jones wrote:

> This patch fixes a bug introduced in the v3.10 merge window.
> 
> Currently the external regulator settings are passed though to the AB8500
> regulator driver regards of whether they can or should be used on any
> ux500 based platform. They are not applicable for the u8505 and when
> enabled, they turn off AUX2 which adversely affects the eMMC on Snowball.
> 
> The driver already knows that the external regulator settings are not
> appropriate for the u8505, however it's more difficult for driver code to
> distinguish between two different u8500 based platforms.
> 
> Using the new semantics, we can just not pass the external regulator
> initialisation values from platform code and the external regulator
> driver will know not to register them.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/mach-ux500/board-mop500-regulators.c |    8 ++++++++
>  arch/arm/mach-ux500/board-mop500-regulators.h |    1 +
>  arch/arm/mach-ux500/board-mop500.c            |    2 ++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/arch/arm/mach-ux500/board-mop500-regulators.c b/arch/arm/mach-ux500/board-mop500-regulators.c
> index b34441b..7399e90 100644
> --- a/arch/arm/mach-ux500/board-mop500-regulators.c
> +++ b/arch/arm/mach-ux500/board-mop500-regulators.c
> @@ -996,6 +996,14 @@ struct ab8500_regulator_platform_data ab8500_regulator_plat_data = {
>  	.num_ext_regulator      = ARRAY_SIZE(ab8500_ext_regulators),
>  };
>  
> +/* External regulators turn off VAUX2 when in use on Snowball. */
> +struct ab8500_regulator_platform_data snowball_regulator_plat_data = {
> +	.reg_init               = ab8500_reg_init,
> +	.num_reg_init           = ARRAY_SIZE(ab8500_reg_init),
> +	.regulator              = ab8500_regulators,
> +	.num_regulator          = ARRAY_SIZE(ab8500_regulators),
> +};
> +
>  struct ab8500_regulator_platform_data ab8505_regulator_plat_data = {
>  	.reg_init               = ab8505_reg_init,
>  	.num_reg_init           = ARRAY_SIZE(ab8505_reg_init),
> diff --git a/arch/arm/mach-ux500/board-mop500-regulators.h b/arch/arm/mach-ux500/board-mop500-regulators.h
> index 039f513..6300e79 100644
> --- a/arch/arm/mach-ux500/board-mop500-regulators.h
> +++ b/arch/arm/mach-ux500/board-mop500-regulators.h
> @@ -16,6 +16,7 @@
>  
>  extern struct ab8500_regulator_platform_data ab8500_regulator_plat_data;
>  extern struct ab8500_regulator_platform_data ab8505_regulator_plat_data;
> +extern struct ab8500_regulator_platform_data snowball_regulator_plat_data;
>  extern struct regulator_init_data tps61052_regulator;
>  extern struct regulator_init_data gpio_en_3v3_regulator;
>  extern struct regulator_init_data sdi0_reg_init_data;
> diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
> index 0b018c4..2d609d4 100644
> --- a/arch/arm/mach-ux500/board-mop500.c
> +++ b/arch/arm/mach-ux500/board-mop500.c
> @@ -615,6 +615,8 @@ static void __init snowball_init_machine(void)
>  	struct device *parent = NULL;
>  	int i;
>  
> +	ab8500_platdata.regulator = &snowball_regulator_plat_data;
> +
>  	platform_device_register(&db8500_prcmu_device);
>  
>  	sdi0_reg_info.enable_gpio = SNOWBALL_SDMMC_EN_GPIO;

Please drop this patch, I have sent a better solution.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog