[PATCH 1/6] Bugfixes and clean-ups bound for the v3.6 RCs

[PATCH 1/6] Bugfixes and clean-ups bound for the v3.6 RCs

[Lee Jones]

This patch-set includes some important changes which should make
their way to the Mainline Release Candidates for the v3.6 release.
Without them Audio doesn't even probe (at all) when booting with
Device Tree selected. The kernel can't compile with ux500 audio
enabled. Even if we can get that far SoC Core will assume we're
using regmaps and attempt to use them; however, none exist for
this driver, so the kernel will oops.

 arch/arm/mach-ux500/Kconfig            |    1 +
 arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
 arch/arm/mach-ux500/board-mop500.c     |    9 ++++-----
 sound/soc/codecs/ab8500-codec.c        |    4 ++++
 sound/soc/soc-dapm.c                   |    2 --
 5 files changed, 10 insertions(+), 8 deletions(-)

[PATCH 1/6] Bugfixes and clean-ups bound for the v3.6 RCs

[Lee Jones]

This patch-set includes some important changes which should make
their way to the Mainline Release Candidates for the v3.6 release.
Without them Audio doesn't even probe (at all) when booting with
Device Tree selected. The kernel can't compile with ux500 audio
enabled. Even if we can get that far SoC Core will assume we're
using regmaps and attempt to use them; however, none exist for
this driver, so the kernel will oops.

 arch/arm/mach-ux500/Kconfig            |    1 +
 arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
 arch/arm/mach-ux500/board-mop500.c     |    9 ++++-----
 sound/soc/codecs/ab8500-codec.c        |    4 ++++
 sound/soc/soc-dapm.c                   |    2 --
 5 files changed, 10 insertions(+), 8 deletions(-)

[PATCH 1/6] Bugfixes and clean-ups bound for the v3.6 RCs

[Lee Jones]

This patch-set includes some important changes which should make
their way to the Mainline Release Candidates for the v3.6 release.
Without them Audio doesn't even probe (at all) when booting with
Device Tree selected. The kernel can't compile with ux500 audio
enabled. Even if we can get that far SoC Core will assume we're
using regmaps and attempt to use them; however, none exist for
this driver, so the kernel will oops.

 arch/arm/mach-ux500/Kconfig            |    1 +
 arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
 arch/arm/mach-ux500/board-mop500.c     |    9 ++++-----
 sound/soc/codecs/ab8500-codec.c        |    4 ++++
 sound/soc/soc-dapm.c                   |    2 --
 5 files changed, 10 insertions(+), 8 deletions(-)

[PATCH 1/6] ASoC: ab8500: Inform SoC Core that we have our own I/O arrangements

[Lee Jones]

If codec->control_data is not populated SoC Core assumes we want to
use regmap, which fails catastrophically, as we don't have one:

Unable to handle kernel NULL pointer dereference at virtual address 00000080
pgd = c0004000
[00000080] *pgd=00000000
Internal error: Oops: 17 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1    Not tainted  (3.5.0-rc6-00884-g0b2419e-dirty #130)
PC is at regmap_read+0x10/0x5c
LR is at hw_read+0x80/0x90
pc : [<c01a91b8>]    lr : [<c0216804>]    psr: 60000013

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 sound/soc/codecs/ab8500-codec.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
index 3c79592..23b4018 100644
--- a/sound/soc/codecs/ab8500-codec.c
+++ b/sound/soc/codecs/ab8500-codec.c
@@ -2406,6 +2406,10 @@ static int ab8500_codec_probe(struct snd_soc_codec *codec)
 
 	/* Setup AB8500 according to board-settings */
 	pdata = (struct ab8500_platform_data *)dev_get_platdata(dev->parent);
+
+	/* Inform SoC Core that we have our own I/O arrangements. */
+	codec->control_data = (void *)true;
+
 	status = ab8500_audio_setup_mics(codec, &pdata->codec->amics);
 	if (status < 0) {
 		pr_err("%s: Failed to setup mics (%d)!\n", __func__, status);
-- 
1.7.9.5

[PATCH 1/6] ASoC: ab8500: Inform SoC Core that we have our own I/O arrangements

[Lee Jones]

If codec->control_data is not populated SoC Core assumes we want to
use regmap, which fails catastrophically, as we don't have one:

Unable to handle kernel NULL pointer dereference at virtual address 00000080
pgd = c0004000
[00000080] *pgd=00000000
Internal error: Oops: 17 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1    Not tainted  (3.5.0-rc6-00884-g0b2419e-dirty #130)
PC is at regmap_read+0x10/0x5c
LR is at hw_read+0x80/0x90
pc : [<c01a91b8>]    lr : [<c0216804>]    psr: 60000013

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 sound/soc/codecs/ab8500-codec.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
index 3c79592..23b4018 100644
--- a/sound/soc/codecs/ab8500-codec.c
+++ b/sound/soc/codecs/ab8500-codec.c
@@ -2406,6 +2406,10 @@ static int ab8500_codec_probe(struct snd_soc_codec *codec)
 
 	/* Setup AB8500 according to board-settings */
 	pdata = (struct ab8500_platform_data *)dev_get_platdata(dev->parent);
+
+	/* Inform SoC Core that we have our own I/O arrangements. */
+	codec->control_data = (void *)true;
+
 	status = ab8500_audio_setup_mics(codec, &pdata->codec->amics);
 	if (status < 0) {
 		pr_err("%s: Failed to setup mics (%d)!\n", __func__, status);
-- 
1.7.9.5

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

If a list of widgets is provided and one of them fails to be added as
a control, the present semantics fail all subsequent widgets. A better
solution would be to only fail that widget, but pursue in attempting
to add the rest of the list.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 sound/soc/soc-dapm.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index eded657..7d9c154 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3095,8 +3095,6 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm,
 			dev_err(dapm->dev,
 				"ASoC: Failed to create DAPM control %s\n",
 				widget->name);
-			ret = -ENOMEM;
-			break;
 		}
 		widget++;
 	}
-- 
1.7.9.5

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

If a list of widgets is provided and one of them fails to be added as
a control, the present semantics fail all subsequent widgets. A better
solution would be to only fail that widget, but pursue in attempting
to add the rest of the list.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 sound/soc/soc-dapm.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index eded657..7d9c154 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3095,8 +3095,6 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm,
 			dev_err(dapm->dev,
 				"ASoC: Failed to create DAPM control %s\n",
 				widget->name);
-			ret = -ENOMEM;
-			break;
 		}
 		widget++;
 	}
-- 
1.7.9.5

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

If a list of widgets is provided and one of them fails to be added as
a control, the present semantics fail all subsequent widgets. A better
solution would be to only fail that widget, but pursue in attempting
to add the rest of the list.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 sound/soc/soc-dapm.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index eded657..7d9c154 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3095,8 +3095,6 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm,
 			dev_err(dapm->dev,
 				"ASoC: Failed to create DAPM control %s\n",
 				widget->name);
-			ret = -ENOMEM;
-			break;
 		}
 		widget++;
 	}
-- 
1.7.9.5

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
> If a list of widgets is provided and one of them fails to be added as
> a control, the present semantics fail all subsequent widgets. A better
> solution would be to only fail that widget, but pursue in attempting
> to add the rest of the list.

You're posting this *again* without bothering to respond to my review
comments.

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
> If a list of widgets is provided and one of them fails to be added as
> a control, the present semantics fail all subsequent widgets. A better
> solution would be to only fail that widget, but pursue in attempting
> to add the rest of the list.

You're posting this *again* without bothering to respond to my review
comments.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 14:42, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
>> If a list of widgets is provided and one of them fails to be added as
>> a control, the present semantics fail all subsequent widgets. A better
>> solution would be to only fail that widget, but pursue in attempting
>> to add the rest of the list.
>
> You're posting this *again* without bothering to respond to my review
> comments.


I didn't see any comments on this.

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

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 14:42, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
>> If a list of widgets is provided and one of them fails to be added as
>> a control, the present semantics fail all subsequent widgets. A better
>> solution would be to only fail that widget, but pursue in attempting
>> to add the rest of the list.
>
> You're posting this *again* without bothering to respond to my review
> comments.


I didn't see any comments on this.

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

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 14:42, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
>> If a list of widgets is provided and one of them fails to be added as
>> a control, the present semantics fail all subsequent widgets. A better
>> solution would be to only fail that widget, but pursue in attempting
>> to add the rest of the list.
>
> You're posting this *again* without bothering to respond to my review
> comments.


I didn't see any comments on this.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 03:25:07PM +0100, Lee Jones wrote:
> On 31/07/12 14:42, Mark Brown wrote:

> >You're posting this *again* without bothering to respond to my review
> >comments.

> I didn't see any comments on this.

Read your email.  <20120726115450.GE3099@opensource.wolfsonmicro.com>
and <20120729202510.GB4384@opensource.wolfsonmicro.com>, the second one
even includes a complaint about you ignoring the first mail.

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 03:25:07PM +0100, Lee Jones wrote:
> On 31/07/12 14:42, Mark Brown wrote:

> >You're posting this *again* without bothering to respond to my review
> >comments.

> I didn't see any comments on this.

Read your email.  <20120726115450.GE3099@opensource.wolfsonmicro.com>
and <20120729202510.GB4384@opensource.wolfsonmicro.com>, the second one
even includes a complaint about you ignoring the first mail.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 03:25:07PM +0100, Lee Jones wrote:
> On 31/07/12 14:42, Mark Brown wrote:

> >You're posting this *again* without bothering to respond to my review
> >comments.

> I didn't see any comments on this.

Read your email.  <20120726115450.GE3099@opensource.wolfsonmicro.com>
and <20120729202510.GB4384@opensource.wolfsonmicro.com>, the second one
even includes a complaint about you ignoring the first mail.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 15:28, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 03:25:07PM +0100, Lee Jones wrote:
>> On 31/07/12 14:42, Mark Brown wrote:
>
>>> You're posting this *again* without bothering to respond to my review
>>> comments.
>
>> I didn't see any comments on this.
>
> Read your email.  <20120726115450.GE3099@opensource.wolfsonmicro.com>
> and <20120729202510.GB4384@opensource.wolfsonmicro.com>, the second one
> even includes a complaint about you ignoring the first mail.

Neither of those are in my Inbox. Blame Mozilla. :)

It's better because the whole audio system doesn't fail in the case of 
minor failure. It'd be like calling off a football game (or whatever 
you're into) because one of the substitutes ruptured an eyelash.

During start-up the ux500 has a couple of very unimportant widgets fail. 
It's the wrong behavior to force failure on the everything audio just 
because of that.

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

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 15:28, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 03:25:07PM +0100, Lee Jones wrote:
>> On 31/07/12 14:42, Mark Brown wrote:
>
>>> You're posting this *again* without bothering to respond to my review
>>> comments.
>
>> I didn't see any comments on this.
>
> Read your email.  <20120726115450.GE3099@opensource.wolfsonmicro.com>
> and <20120729202510.GB4384@opensource.wolfsonmicro.com>, the second one
> even includes a complaint about you ignoring the first mail.

Neither of those are in my Inbox. Blame Mozilla. :)

It's better because the whole audio system doesn't fail in the case of 
minor failure. It'd be like calling off a football game (or whatever 
you're into) because one of the substitutes ruptured an eyelash.

During start-up the ux500 has a couple of very unimportant widgets fail. 
It's the wrong behavior to force failure on the everything audio just 
because of that.

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

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 15:28, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 03:25:07PM +0100, Lee Jones wrote:
>> On 31/07/12 14:42, Mark Brown wrote:
>
>>> You're posting this *again* without bothering to respond to my review
>>> comments.
>
>> I didn't see any comments on this.
>
> Read your email.  <20120726115450.GE3099@opensource.wolfsonmicro.com>
> and <20120729202510.GB4384@opensource.wolfsonmicro.com>, the second one
> even includes a complaint about you ignoring the first mail.

Neither of those are in my Inbox. Blame Mozilla. :)

It's better because the whole audio system doesn't fail in the case of 
minor failure. It'd be like calling off a football game (or whatever 
you're into) because one of the substitutes ruptured an eyelash.

During start-up the ux500 has a couple of very unimportant widgets fail. 
It's the wrong behavior to force failure on the everything audio just 
because of that.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 03:38:02PM +0100, Lee Jones wrote:

> Neither of those are in my Inbox. Blame Mozilla. :)

You might want to look at a better mail program.

> It's better because the whole audio system doesn't fail in the case
> of minor failure. It'd be like calling off a football game (or
> whatever you're into) because one of the substitutes ruptured an
> eyelash.

It shouldn't make any difference to startup - we should still be
checking errors and failing the init if we're failing to add links, 
this isn't something that's likely to randomly break on a particular
boot, it's more something that indicates nobody bothered testing.

It's certainly totally inappropriate for an "urgent" bugfix.

> During start-up the ux500 has a couple of very unimportant widgets
> fail. It's the wrong behavior to force failure on the everything
> audio just because of that.

Fixes for those errors, however...

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 03:38:02PM +0100, Lee Jones wrote:

> Neither of those are in my Inbox. Blame Mozilla. :)

You might want to look at a better mail program.

> It's better because the whole audio system doesn't fail in the case
> of minor failure. It'd be like calling off a football game (or
> whatever you're into) because one of the substitutes ruptured an
> eyelash.

It shouldn't make any difference to startup - we should still be
checking errors and failing the init if we're failing to add links, 
this isn't something that's likely to randomly break on a particular
boot, it's more something that indicates nobody bothered testing.

It's certainly totally inappropriate for an "urgent" bugfix.

> During start-up the ux500 has a couple of very unimportant widgets
> fail. It's the wrong behavior to force failure on the everything
> audio just because of that.

Fixes for those errors, however...

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 03:38:02PM +0100, Lee Jones wrote:

> Neither of those are in my Inbox. Blame Mozilla. :)

You might want to look at a better mail program.

> It's better because the whole audio system doesn't fail in the case
> of minor failure. It'd be like calling off a football game (or
> whatever you're into) because one of the substitutes ruptured an
> eyelash.

It shouldn't make any difference to startup - we should still be
checking errors and failing the init if we're failing to add links, 
this isn't something that's likely to randomly break on a particular
boot, it's more something that indicates nobody bothered testing.

It's certainly totally inappropriate for an "urgent" bugfix.

> During start-up the ux500 has a couple of very unimportant widgets
> fail. It's the wrong behavior to force failure on the everything
> audio just because of that.

Fixes for those errors, however...

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 15:54, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 03:38:02PM +0100, Lee Jones wrote:
>
>> Neither of those are in my Inbox. Blame Mozilla. :)
>
> You might want to look at a better mail program.

Willingly. Any good suggestions?

>> It's better because the whole audio system doesn't fail in the case
>> of minor failure. It'd be like calling off a football game (or
>> whatever you're into) because one of the substitutes ruptured an
>> eyelash.
>
> It shouldn't make any difference to startup - we should still be
> checking errors and failing the init if we're failing to add links,
> this isn't something that's likely to randomly break on a particular
> boot, it's more something that indicates nobody bothered testing.
>
> It's certainly totally inappropriate for an "urgent" bugfix.

Well it just means that audio won't work for the ux500 for this kernel 
release, but as we're waiting on clocks, this isn't a big issue for us. 
If you do take it (with or without the return code), feel free to add it 
to for-next instead of the -rc:s

>> During start-up the ux500 has a couple of very unimportant widgets
>> fail. It's the wrong behavior to force failure on the everything
>> audio just because of that.
>
> Fixes for those errors, however...

I'll leave that to the audio expert. I'd like to move to to something 
else (that you maintain - perhaps I'll go and mess-up regulators next). :)

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

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 15:54, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 03:38:02PM +0100, Lee Jones wrote:
>
>> Neither of those are in my Inbox. Blame Mozilla. :)
>
> You might want to look at a better mail program.

Willingly. Any good suggestions?

>> It's better because the whole audio system doesn't fail in the case
>> of minor failure. It'd be like calling off a football game (or
>> whatever you're into) because one of the substitutes ruptured an
>> eyelash.
>
> It shouldn't make any difference to startup - we should still be
> checking errors and failing the init if we're failing to add links,
> this isn't something that's likely to randomly break on a particular
> boot, it's more something that indicates nobody bothered testing.
>
> It's certainly totally inappropriate for an "urgent" bugfix.

Well it just means that audio won't work for the ux500 for this kernel 
release, but as we're waiting on clocks, this isn't a big issue for us. 
If you do take it (with or without the return code), feel free to add it 
to for-next instead of the -rc:s

>> During start-up the ux500 has a couple of very unimportant widgets
>> fail. It's the wrong behavior to force failure on the everything
>> audio just because of that.
>
> Fixes for those errors, however...

I'll leave that to the audio expert. I'd like to move to to something 
else (that you maintain - perhaps I'll go and mess-up regulators next). :)

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

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 15:54, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 03:38:02PM +0100, Lee Jones wrote:
>
>> Neither of those are in my Inbox. Blame Mozilla. :)
>
> You might want to look at a better mail program.

Willingly. Any good suggestions?

>> It's better because the whole audio system doesn't fail in the case
>> of minor failure. It'd be like calling off a football game (or
>> whatever you're into) because one of the substitutes ruptured an
>> eyelash.
>
> It shouldn't make any difference to startup - we should still be
> checking errors and failing the init if we're failing to add links,
> this isn't something that's likely to randomly break on a particular
> boot, it's more something that indicates nobody bothered testing.
>
> It's certainly totally inappropriate for an "urgent" bugfix.

Well it just means that audio won't work for the ux500 for this kernel 
release, but as we're waiting on clocks, this isn't a big issue for us. 
If you do take it (with or without the return code), feel free to add it 
to for-next instead of the -rc:s

>> During start-up the ux500 has a couple of very unimportant widgets
>> fail. It's the wrong behavior to force failure on the everything
>> audio just because of that.
>
> Fixes for those errors, however...

I'll leave that to the audio expert. I'd like to move to to something 
else (that you maintain - perhaps I'll go and mess-up regulators next). :)

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 04:15:23PM +0100, Lee Jones wrote:
> On 31/07/12 15:54, Mark Brown wrote:

> >You might want to look at a better mail program.

> Willingly. Any good suggestions?

mutt works for me.

> >It's certainly totally inappropriate for an "urgent" bugfix.

> Well it just means that audio won't work for the ux500 for this
> kernel release, but as we're waiting on clocks, this isn't a big
> issue for us. If you do take it (with or without the return code),
> feel free to add it to for-next instead of the -rc:s

I'm not going to apply this patch.  This isn't a vendor BSP, we
shouldn't be putting random hacks like this in core code.

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 04:15:23PM +0100, Lee Jones wrote:
> On 31/07/12 15:54, Mark Brown wrote:

> >You might want to look at a better mail program.

> Willingly. Any good suggestions?

mutt works for me.

> >It's certainly totally inappropriate for an "urgent" bugfix.

> Well it just means that audio won't work for the ux500 for this
> kernel release, but as we're waiting on clocks, this isn't a big
> issue for us. If you do take it (with or without the return code),
> feel free to add it to for-next instead of the -rc:s

I'm not going to apply this patch.  This isn't a vendor BSP, we
shouldn't be putting random hacks like this in core code.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Tue, Jul 31, 2012 at 04:15:23PM +0100, Lee Jones wrote:
> On 31/07/12 15:54, Mark Brown wrote:

> >You might want to look at a better mail program.

> Willingly. Any good suggestions?

mutt works for me.

> >It's certainly totally inappropriate for an "urgent" bugfix.

> Well it just means that audio won't work for the ux500 for this
> kernel release, but as we're waiting on clocks, this isn't a big
> issue for us. If you do take it (with or without the return code),
> feel free to add it to for-next instead of the -rc:s

I'm not going to apply this patch.  This isn't a vendor BSP, we
shouldn't be putting random hacks like this in core code.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 16:18, Mark Brown wrote:
>>> It's certainly totally inappropriate for an "urgent" bugfix.
>
>> Well it just means that audio won't work for the ux500 for this
>> kernel release, but as we're waiting on clocks, this isn't a big
>> issue for us. If you do take it (with or without the return code),
>> feel free to add it to for-next instead of the -rc:s
>
> I'm not going to apply this patch.  This isn't a vendor BSP, we
> shouldn't be putting random hacks like this in core code.

BSP kernel or otherwise, it still seems wrong to me to fail and entire 
audio driver just because of a broken link.

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

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 16:18, Mark Brown wrote:
>>> It's certainly totally inappropriate for an "urgent" bugfix.
>
>> Well it just means that audio won't work for the ux500 for this
>> kernel release, but as we're waiting on clocks, this isn't a big
>> issue for us. If you do take it (with or without the return code),
>> feel free to add it to for-next instead of the -rc:s
>
> I'm not going to apply this patch.  This isn't a vendor BSP, we
> shouldn't be putting random hacks like this in core code.

BSP kernel or otherwise, it still seems wrong to me to fail and entire 
audio driver just because of a broken link.

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

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 31/07/12 16:18, Mark Brown wrote:
>>> It's certainly totally inappropriate for an "urgent" bugfix.
>
>> Well it just means that audio won't work for the ux500 for this
>> kernel release, but as we're waiting on clocks, this isn't a big
>> issue for us. If you do take it (with or without the return code),
>> feel free to add it to for-next instead of the -rc:s
>
> I'm not going to apply this patch.  This isn't a vendor BSP, we
> shouldn't be putting random hacks like this in core code.

BSP kernel or otherwise, it still seems wrong to me to fail and entire 
audio driver just because of a broken link.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
> On 31/07/12 16:18, Mark Brown wrote:

> >I'm not going to apply this patch.  This isn't a vendor BSP, we
> >shouldn't be putting random hacks like this in core code.

> BSP kernel or otherwise, it still seems wrong to me to fail and
> entire audio driver just because of a broken link.

No, really.  Random disconnections in the DAPM graph are just endless
pain from a support and debug point of view.  This isn't something that
randomly breaks on specific hardware where we'd expect random errors at
runtime, it's something that will never have worked - it seems clear
nobody tested the mainline submission.

It's very disappointing to see such an error exist, and even more
disappointing that there's no interest in fixing the driver.

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
> On 31/07/12 16:18, Mark Brown wrote:

> >I'm not going to apply this patch.  This isn't a vendor BSP, we
> >shouldn't be putting random hacks like this in core code.

> BSP kernel or otherwise, it still seems wrong to me to fail and
> entire audio driver just because of a broken link.

No, really.  Random disconnections in the DAPM graph are just endless
pain from a support and debug point of view.  This isn't something that
randomly breaks on specific hardware where we'd expect random errors at
runtime, it's something that will never have worked - it seems clear
nobody tested the mainline submission.

It's very disappointing to see such an error exist, and even more
disappointing that there's no interest in fixing the driver.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
> On 31/07/12 16:18, Mark Brown wrote:

> >I'm not going to apply this patch.  This isn't a vendor BSP, we
> >shouldn't be putting random hacks like this in core code.

> BSP kernel or otherwise, it still seems wrong to me to fail and
> entire audio driver just because of a broken link.

No, really.  Random disconnections in the DAPM graph are just endless
pain from a support and debug point of view.  This isn't something that
randomly breaks on specific hardware where we'd expect random errors at
runtime, it's something that will never have worked - it seems clear
nobody tested the mainline submission.

It's very disappointing to see such an error exist, and even more
disappointing that there's no interest in fixing the driver.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 01/08/12 14:20, Mark Brown wrote:
> On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
>> On 31/07/12 16:18, Mark Brown wrote:
>
>>> I'm not going to apply this patch.  This isn't a vendor BSP, we
>>> shouldn't be putting random hacks like this in core code.
>
>> BSP kernel or otherwise, it still seems wrong to me to fail and
>> entire audio driver just because of a broken link.
>
> No, really.  Random disconnections in the DAPM graph are just endless
> pain from a support and debug point of view.  This isn't something that
> randomly breaks on specific hardware where we'd expect random errors at
> runtime, it's something that will never have worked - it seems clear
> nobody tested the mainline submission.

I am under the impression that it was tested. Perhaps before it was 
rebased, but tests were completed. Ola was very surprised when I told 
him there were link failures. The only issue is that I only found out 
and told him a day before he was due to take his Summer leave.

> It's very disappointing to see such an error exist, and even more
> disappointing that there's no interest in fixing the driver.

This is incorrect. I'm sure the driver will be fixed post-haste when Ola 
returns back from vacation. If I can find some time I might dabble in 
the mean-time also.

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

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On 01/08/12 14:20, Mark Brown wrote:
> On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
>> On 31/07/12 16:18, Mark Brown wrote:
>
>>> I'm not going to apply this patch.  This isn't a vendor BSP, we
>>> shouldn't be putting random hacks like this in core code.
>
>> BSP kernel or otherwise, it still seems wrong to me to fail and
>> entire audio driver just because of a broken link.
>
> No, really.  Random disconnections in the DAPM graph are just endless
> pain from a support and debug point of view.  This isn't something that
> randomly breaks on specific hardware where we'd expect random errors at
> runtime, it's something that will never have worked - it seems clear
> nobody tested the mainline submission.

I am under the impression that it was tested. Perhaps before it was 
rebased, but tests were completed. Ola was very surprised when I told 
him there were link failures. The only issue is that I only found out 
and told him a day before he was due to take his Summer leave.

> It's very disappointing to see such an error exist, and even more
> disappointing that there's no interest in fixing the driver.

This is incorrect. I'm sure the driver will be fixed post-haste when Ola 
returns back from vacation. If I can find some time I might dabble in 
the mean-time also.

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

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Wed, Aug 01, 2012 at 02:50:32PM +0100, Lee Jones wrote:

> I am under the impression that it was tested. Perhaps before it was
> rebased, but tests were completed. Ola was very surprised when I
> told him there were link failures. The only issue is that I only
> found out and told him a day before he was due to take his Summer
> leave.

That's really surprising.  What are the actual issues here, you've never
shown the actual errors?

> >It's very disappointing to see such an error exist, and even more
> >disappointing that there's no interest in fixing the driver.

> This is incorrect. I'm sure the driver will be fixed post-haste when
> Ola returns back from vacation. If I can find some time I might
> dabble in the mean-time also.

It may not be what you're intending but it's unfortuantely what's I'm
seeing.

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Wed, Aug 01, 2012 at 02:50:32PM +0100, Lee Jones wrote:

> I am under the impression that it was tested. Perhaps before it was
> rebased, but tests were completed. Ola was very surprised when I
> told him there were link failures. The only issue is that I only
> found out and told him a day before he was due to take his Summer
> leave.

That's really surprising.  What are the actual issues here, you've never
shown the actual errors?

> >It's very disappointing to see such an error exist, and even more
> >disappointing that there's no interest in fixing the driver.

> This is incorrect. I'm sure the driver will be fixed post-haste when
> Ola returns back from vacation. If I can find some time I might
> dabble in the mean-time also.

It may not be what you're intending but it's unfortuantely what's I'm
seeing.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Wed, Aug 01, 2012 at 02:50:32PM +0100, Lee Jones wrote:

> I am under the impression that it was tested. Perhaps before it was
> rebased, but tests were completed. Ola was very surprised when I
> told him there were link failures. The only issue is that I only
> found out and told him a day before he was due to take his Summer
> leave.

That's really surprising.  What are the actual issues here, you've never
shown the actual errors?

> >It's very disappointing to see such an error exist, and even more
> >disappointing that there's no interest in fixing the driver.

> This is incorrect. I'm sure the driver will be fixed post-haste when
> Ola returns back from vacation. If I can find some time I might
> dabble in the mean-time also.

It may not be what you're intending but it's unfortuantely what's I'm
seeing.

Re: [alsa-devel] [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Wed, Aug 01, 2012 at 05:08:24PM +0100, Mark Brown wrote:
> On Wed, Aug 01, 2012 at 02:50:32PM +0100, Lee Jones wrote:

> > >It's very disappointing to see such an error exist, and even more
> > >disappointing that there's no interest in fixing the driver.

> > This is incorrect. I'm sure the driver will be fixed post-haste when
> > Ola returns back from vacation. If I can find some time I might
> > dabble in the mean-time also.

> It may not be what you're intending but it's unfortuantely what's I'm
> seeing.

Just to expand on this a bit since I've found myself pushing back in
this sort of way far too often with these recent serieses and it's
making everyone grumpy:

What I'm seeing here is a lot of patches getting sent with problems that
I'd really not expect from someone sending such a high volume of
patches.  Things like the lack of documentation for the DT bindings for
example, it's something that's mandatory and which people doing lots of
device tree work really ought to be aware of.  There's also noticable
pushback with fixing some of these issues, and like I say this happens
often enough to be really noticeable.

This isn't awesome from a review point of view, it's not nice to find
issues in things and when it happens a lot for the wrong sort of thing
it ends up seeming like the time spent doing the reviews isn't valued.

[alsa-devel] [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Wed, Aug 01, 2012 at 05:08:24PM +0100, Mark Brown wrote:
> On Wed, Aug 01, 2012 at 02:50:32PM +0100, Lee Jones wrote:

> > >It's very disappointing to see such an error exist, and even more
> > >disappointing that there's no interest in fixing the driver.

> > This is incorrect. I'm sure the driver will be fixed post-haste when
> > Ola returns back from vacation. If I can find some time I might
> > dabble in the mean-time also.

> It may not be what you're intending but it's unfortuantely what's I'm
> seeing.

Just to expand on this a bit since I've found myself pushing back in
this sort of way far too often with these recent serieses and it's
making everyone grumpy:

What I'm seeing here is a lot of patches getting sent with problems that
I'd really not expect from someone sending such a high volume of
patches.  Things like the lack of documentation for the DT bindings for
example, it's something that's mandatory and which people doing lots of
device tree work really ought to be aware of.  There's also noticable
pushback with fixing some of these issues, and like I say this happens
often enough to be really noticeable.

This isn't awesome from a review point of view, it's not nice to find
issues in things and when it happens a lot for the wrong sort of thing
it ends up seeming like the time spent doing the reviews isn't valued.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Wed, Aug 01, 2012 at 05:08:24PM +0100, Mark Brown wrote:
> On Wed, Aug 01, 2012 at 02:50:32PM +0100, Lee Jones wrote:

> > >It's very disappointing to see such an error exist, and even more
> > >disappointing that there's no interest in fixing the driver.

> > This is incorrect. I'm sure the driver will be fixed post-haste when
> > Ola returns back from vacation. If I can find some time I might
> > dabble in the mean-time also.

> It may not be what you're intending but it's unfortuantely what's I'm
> seeing.

Just to expand on this a bit since I've found myself pushing back in
this sort of way far too often with these recent serieses and it's
making everyone grumpy:

What I'm seeing here is a lot of patches getting sent with problems that
I'd really not expect from someone sending such a high volume of
patches.  Things like the lack of documentation for the DT bindings for
example, it's something that's mandatory and which people doing lots of
device tree work really ought to be aware of.  There's also noticable
pushback with fixing some of these issues, and like I say this happens
often enough to be really noticeable.

This isn't awesome from a review point of view, it's not nice to find
issues in things and when it happens a lot for the wrong sort of thing
it ends up seeming like the time spent doing the reviews isn't valued.

Re: [alsa-devel] [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On Wed, Aug 01, 2012 at 08:41:34PM +0100, Mark Brown wrote:
> On Wed, Aug 01, 2012 at 05:08:24PM +0100, Mark Brown wrote:
> > On Wed, Aug 01, 2012 at 02:50:32PM +0100, Lee Jones wrote:
> 
> > > >It's very disappointing to see such an error exist, and even more
> > > >disappointing that there's no interest in fixing the driver.
> 
> > > This is incorrect. I'm sure the driver will be fixed post-haste when
> > > Ola returns back from vacation. If I can find some time I might
> > > dabble in the mean-time also.
> 
> > It may not be what you're intending but it's unfortuantely what's I'm
> > seeing.
> 
> Just to expand on this a bit since I've found myself pushing back in
> this sort of way far too often with these recent serieses and it's
> making everyone grumpy:
> 
> What I'm seeing here is a lot of patches getting sent with problems that
> I'd really not expect from someone sending such a high volume of
> patches.  Things like the lack of documentation for the DT bindings for
> example, it's something that's mandatory and which people doing lots of
> device tree work really ought to be aware of.  There's also noticable
> pushback with fixing some of these issues, and like I say this happens
> often enough to be really noticeable.
> 
> This isn't awesome from a review point of view, it's not nice to find
> issues in things and when it happens a lot for the wrong sort of thing
> it ends up seeming like the time spent doing the reviews isn't valued.

Okay, seeing as we're lying our cards on the table, here's my hand:

I'm in a very difficult position here. My initial task was to enable
Device Tree on all drivers associated with the Snowball low-cost 
development board. I was working very closely with Arnd, who was regularly
requesting code restructuring, both within the drivers and in platform
code. Something I was only too pleased to do. Then some of the original
authors noticed the restructuring and I subsequently spent a great deal of
time defending my actions. Now we have some systems in place to keep
everyone informed and happy.

Over time, the requests for Maintainers have Snowballed (pun intended). My 
task now seems to be enabling drivers for Device Tree _and_ fix all 
associated driver bugs, including any requested restructuring and API
adoption. What you fail to notice is that I am only one person, and hopping
all over the code-base trying to do everyone's bidding is no mean feat. In
reality I am no more obliged to fix driver bugs than you are. In fact as
the Maintainer of some of these subsystems, perhaps you could even help out
a bit?

With regards to the documentation, I am perfectly aware that any new binding
needs to be documented. Leaving it out was intentional until we can agree on
the bindings. After you told me you review the documentation rather than
the code bindings themselves ( pffft... ;) ) I added the documentation to
the patch-set. I always had every intention of writing them!

On your last point about feeling undervalued, that's not the case at all. I
do respect your opinion and value your reviews. They are always completed
quickly and are very thorough. If I had one complaint it's that you are
_too_ stringent. Some of this stuff really doesn't matter.

We are all trying to do good things here. Please try not to be too over-
critical. We all know Mainlining is a good thing. Perhaps less people
would be so frightened of it, thus more people would do it if the reviews
weren't such a ball-ache ( for want of a better expression :) ).

[alsa-devel] [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On Wed, Aug 01, 2012 at 08:41:34PM +0100, Mark Brown wrote:
> On Wed, Aug 01, 2012 at 05:08:24PM +0100, Mark Brown wrote:
> > On Wed, Aug 01, 2012 at 02:50:32PM +0100, Lee Jones wrote:
> 
> > > >It's very disappointing to see such an error exist, and even more
> > > >disappointing that there's no interest in fixing the driver.
> 
> > > This is incorrect. I'm sure the driver will be fixed post-haste when
> > > Ola returns back from vacation. If I can find some time I might
> > > dabble in the mean-time also.
> 
> > It may not be what you're intending but it's unfortuantely what's I'm
> > seeing.
> 
> Just to expand on this a bit since I've found myself pushing back in
> this sort of way far too often with these recent serieses and it's
> making everyone grumpy:
> 
> What I'm seeing here is a lot of patches getting sent with problems that
> I'd really not expect from someone sending such a high volume of
> patches.  Things like the lack of documentation for the DT bindings for
> example, it's something that's mandatory and which people doing lots of
> device tree work really ought to be aware of.  There's also noticable
> pushback with fixing some of these issues, and like I say this happens
> often enough to be really noticeable.
> 
> This isn't awesome from a review point of view, it's not nice to find
> issues in things and when it happens a lot for the wrong sort of thing
> it ends up seeming like the time spent doing the reviews isn't valued.

Okay, seeing as we're lying our cards on the table, here's my hand:

I'm in a very difficult position here. My initial task was to enable
Device Tree on all drivers associated with the Snowball low-cost 
development board. I was working very closely with Arnd, who was regularly
requesting code restructuring, both within the drivers and in platform
code. Something I was only too pleased to do. Then some of the original
authors noticed the restructuring and I subsequently spent a great deal of
time defending my actions. Now we have some systems in place to keep
everyone informed and happy.

Over time, the requests for Maintainers have Snowballed (pun intended). My 
task now seems to be enabling drivers for Device Tree _and_ fix all 
associated driver bugs, including any requested restructuring and API
adoption. What you fail to notice is that I am only one person, and hopping
all over the code-base trying to do everyone's bidding is no mean feat. In
reality I am no more obliged to fix driver bugs than you are. In fact as
the Maintainer of some of these subsystems, perhaps you could even help out
a bit?

With regards to the documentation, I am perfectly aware that any new binding
needs to be documented. Leaving it out was intentional until we can agree on
the bindings. After you told me you review the documentation rather than
the code bindings themselves ( pffft... ;) ) I added the documentation to
the patch-set. I always had every intention of writing them!

On your last point about feeling undervalued, that's not the case at all. I
do respect your opinion and value your reviews. They are always completed
quickly and are very thorough. If I had one complaint it's that you are
_too_ stringent. Some of this stuff really doesn't matter.

We are all trying to do good things here. Please try not to be too over-
critical. We all know Mainlining is a good thing. Perhaps less people
would be so frightened of it, thus more people would do it if the reviews
weren't such a ball-ache ( for want of a better expression :) ).

Re: [alsa-devel] [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Thu, Aug 02, 2012 at 08:45:18AM +0100, Lee Jones wrote:

> Over time, the requests for Maintainers have Snowballed (pun intended). My 
> task now seems to be enabling drivers for Device Tree _and_ fix all 
> associated driver bugs, including any requested restructuring and API

One thing to bear in mind with device tree is that it's all about
defining an ABI - it's supposed to be stable and OS agnostic so it puts
a lot of pressure on to really address quality problems that become
visible in the bindings.  This stuff is much less critical with platform
data, it's relatively easy to change.

> adoption. What you fail to notice is that I am only one person, and hopping
> all over the code-base trying to do everyone's bidding is no mean feat. In

It's fairly obvious that it's only you, or at least only you posting
stuff (I know sometimes there are bigger teams behind people) - the
pressure you're under to get something in is very clear.  A big part of
what I'm saying here is that it would be really helpful if could you
slow down a bit, discuss problems more and avoid cutting corners so
much.  This is likely to save you time overall, you'll have a much
higher success rate and you'll get much better feedback if it's clear
what's going on and that there's an interest in understanding the
issues.

If you need to focus on device tree enablement and there are underlying
problems in the subsystems you're looking at perhaps you need to push
back on whoever's asking you to do the work and say you need the domain
experts to pitch in and help you out.

> reality I am no more obliged to fix driver bugs than you are. In fact as
> the Maintainer of some of these subsystems, perhaps you could even help out
> a bit?

You're not telling us about the problems you see so it's very difficult
for anyone to help you.

For example with this patch the only information you've sent is the
patch and the fact that you're seeing the error you're ignoring going
off on the system you're working with (which I had to ask to find out
about...).  You've not shown the error message or provided any other
hint which would help anyone understand why the error might be occuring
and what a good solution to the problem is.  Ola's guess seems very
likely but nobody's got enough information to confirm that unless
there's been some off-list ST/Linaro communication.

Obviously with the stuff that's device specific you also have to contend
with the fact that you're working on hardware which just isn't all that
widely available and quite possibly has closed datasheets.

> We are all trying to do good things here. Please try not to be too over-
> critical. We all know Mainlining is a good thing. Perhaps less people
> would be so frightened of it, thus more people would do it if the reviews
> weren't such a ball-ache ( for want of a better expression :) ).

This is a two way thing - one of the tools that maintainers have to try
to drive up the quality of the code is to push back on poor quality
submissions and devote less bandwith to sources of those submissions.

It is true that there's a bunch of people who seem to view review as
being just an annoying obstacle to be dealt with with the minimum
possible effort but in practice all that does is make things more
painful for everyone, they do tend to be more noticable because
"applied, thanks" doesn't make for a big thread.

[alsa-devel] [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Thu, Aug 02, 2012 at 08:45:18AM +0100, Lee Jones wrote:

> Over time, the requests for Maintainers have Snowballed (pun intended). My 
> task now seems to be enabling drivers for Device Tree _and_ fix all 
> associated driver bugs, including any requested restructuring and API

One thing to bear in mind with device tree is that it's all about
defining an ABI - it's supposed to be stable and OS agnostic so it puts
a lot of pressure on to really address quality problems that become
visible in the bindings.  This stuff is much less critical with platform
data, it's relatively easy to change.

> adoption. What you fail to notice is that I am only one person, and hopping
> all over the code-base trying to do everyone's bidding is no mean feat. In

It's fairly obvious that it's only you, or at least only you posting
stuff (I know sometimes there are bigger teams behind people) - the
pressure you're under to get something in is very clear.  A big part of
what I'm saying here is that it would be really helpful if could you
slow down a bit, discuss problems more and avoid cutting corners so
much.  This is likely to save you time overall, you'll have a much
higher success rate and you'll get much better feedback if it's clear
what's going on and that there's an interest in understanding the
issues.

If you need to focus on device tree enablement and there are underlying
problems in the subsystems you're looking at perhaps you need to push
back on whoever's asking you to do the work and say you need the domain
experts to pitch in and help you out.

> reality I am no more obliged to fix driver bugs than you are. In fact as
> the Maintainer of some of these subsystems, perhaps you could even help out
> a bit?

You're not telling us about the problems you see so it's very difficult
for anyone to help you.

For example with this patch the only information you've sent is the
patch and the fact that you're seeing the error you're ignoring going
off on the system you're working with (which I had to ask to find out
about...).  You've not shown the error message or provided any other
hint which would help anyone understand why the error might be occuring
and what a good solution to the problem is.  Ola's guess seems very
likely but nobody's got enough information to confirm that unless
there's been some off-list ST/Linaro communication.

Obviously with the stuff that's device specific you also have to contend
with the fact that you're working on hardware which just isn't all that
widely available and quite possibly has closed datasheets.

> We are all trying to do good things here. Please try not to be too over-
> critical. We all know Mainlining is a good thing. Perhaps less people
> would be so frightened of it, thus more people would do it if the reviews
> weren't such a ball-ache ( for want of a better expression :) ).

This is a two way thing - one of the tools that maintainers have to try
to drive up the quality of the code is to push back on poor quality
submissions and devote less bandwith to sources of those submissions.

It is true that there's a bunch of people who seem to view review as
being just an annoying obstacle to be dealt with with the minimum
possible effort but in practice all that does is make things more
painful for everyone, they do tend to be more noticable because
"applied, thanks" doesn't make for a big thread.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Thu, Aug 02, 2012 at 08:45:18AM +0100, Lee Jones wrote:

> Over time, the requests for Maintainers have Snowballed (pun intended). My 
> task now seems to be enabling drivers for Device Tree _and_ fix all 
> associated driver bugs, including any requested restructuring and API

One thing to bear in mind with device tree is that it's all about
defining an ABI - it's supposed to be stable and OS agnostic so it puts
a lot of pressure on to really address quality problems that become
visible in the bindings.  This stuff is much less critical with platform
data, it's relatively easy to change.

> adoption. What you fail to notice is that I am only one person, and hopping
> all over the code-base trying to do everyone's bidding is no mean feat. In

It's fairly obvious that it's only you, or at least only you posting
stuff (I know sometimes there are bigger teams behind people) - the
pressure you're under to get something in is very clear.  A big part of
what I'm saying here is that it would be really helpful if could you
slow down a bit, discuss problems more and avoid cutting corners so
much.  This is likely to save you time overall, you'll have a much
higher success rate and you'll get much better feedback if it's clear
what's going on and that there's an interest in understanding the
issues.

If you need to focus on device tree enablement and there are underlying
problems in the subsystems you're looking at perhaps you need to push
back on whoever's asking you to do the work and say you need the domain
experts to pitch in and help you out.

> reality I am no more obliged to fix driver bugs than you are. In fact as
> the Maintainer of some of these subsystems, perhaps you could even help out
> a bit?

You're not telling us about the problems you see so it's very difficult
for anyone to help you.

For example with this patch the only information you've sent is the
patch and the fact that you're seeing the error you're ignoring going
off on the system you're working with (which I had to ask to find out
about...).  You've not shown the error message or provided any other
hint which would help anyone understand why the error might be occuring
and what a good solution to the problem is.  Ola's guess seems very
likely but nobody's got enough information to confirm that unless
there's been some off-list ST/Linaro communication.

Obviously with the stuff that's device specific you also have to contend
with the fact that you're working on hardware which just isn't all that
widely available and quite possibly has closed datasheets.

> We are all trying to do good things here. Please try not to be too over-
> critical. We all know Mainlining is a good thing. Perhaps less people
> would be so frightened of it, thus more people would do it if the reviews
> weren't such a ball-ache ( for want of a better expression :) ).

This is a two way thing - one of the tools that maintainers have to try
to drive up the quality of the code is to push back on poor quality
submissions and devote less bandwith to sources of those submissions.

It is true that there's a bunch of people who seem to view review as
being just an annoying obstacle to be dealt with with the minimum
possible effort but in practice all that does is make things more
painful for everyone, they do tend to be more noticable because
"applied, thanks" doesn't make for a big thread.

Re: [alsa-devel] [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On Thu, Aug 02, 2012 at 06:56:04PM +0100, Mark Brown wrote:
> On Thu, Aug 02, 2012 at 08:45:18AM +0100, Lee Jones wrote:
> 
> > Over time, the requests for Maintainers have Snowballed (pun intended). My 
> > task now seems to be enabling drivers for Device Tree _and_ fix all 
> > associated driver bugs, including any requested restructuring and API
> 
> One thing to bear in mind with device tree is that it's all about
> defining an ABI - it's supposed to be stable and OS agnostic so it puts
> a lot of pressure on to really address quality problems that become
> visible in the bindings.  This stuff is much less critical with platform
> data, it's relatively easy to change.
>
> > adoption. What you fail to notice is that I am only one person, and hopping
> > all over the code-base trying to do everyone's bidding is no mean feat. In
> 
> It's fairly obvious that it's only you, or at least only you posting
> stuff (I know sometimes there are bigger teams behind people) - the
> pressure you're under to get something in is very clear.  A big part of
> what I'm saying here is that it would be really helpful if could you
> slow down a bit, discuss problems more and avoid cutting corners so
> much.  This is likely to save you time overall, you'll have a much
> higher success rate and you'll get much better feedback if it's clear
> what's going on and that there's an interest in understanding the
> issues.

I do agree that it should be correct, but the difference between getting
it 90% correct and absolutely perfect increases the effort at least x2.
With so much left to do, I think it would be better to get everything in
and functioning, then fix the minor issues as we come across them later.
This is what I've been doing from the start and it's actually looking
pretty good. I also agree that DT should be OS agnostic, but breaking up
MFDs into their functions shouldn't be an issue for other OSes. Either
they can choose to break them up and use the individual child compatible
strings, or only use the parent node.

My personal opinion is if we'd sat around discussing how it should look
prior to doing any work a) the project probably wouldn't have even got
off the ground yet and b) we would have most likely got it all wrong, as
most of our learning and knowledge has been done/gained by actually doing
the work. Instead I've been using the JFDI (Just Frickin' Do It) 
philosophy, and we've actually made some amazing headway. Being blocked
on minor issues and easy to change, orthogonal issues such as vendor
defined properties (i2c) isn't helpful to anyone.
 
> If you need to focus on device tree enablement and there are underlying
> problems in the subsystems you're looking at perhaps you need to push
> back on whoever's asking you to do the work and say you need the domain
> experts to pitch in and help you out.

If only it were that easy. We're not bursting at the seems with resources
here. I'm working in a very customer focused ecosystem. If they don't 
request it, or file a bug about it, there's no resource allocation to fix
it. However, the future is looking up from that point of view. We've just 
started a new project, which I'm hoping will attract some new resources. 
Watch this space.

> > reality I am no more obliged to fix driver bugs than you are. In fact as
> > the Maintainer of some of these subsystems, perhaps you could even help out
> > a bit?
> 
> You're not telling us about the problems you see so it's very difficult
> for anyone to help you.
> 
> For example with this patch the only information you've sent is the
> patch and the fact that you're seeing the error you're ignoring going
> off on the system you're working with (which I had to ask to find out
> about...).  You've not shown the error message or provided any other
> hint which would help anyone understand why the error might be occuring
> and what a good solution to the problem is.  Ola's guess seems very
> likely but nobody's got enough information to confirm that unless
> there's been some off-list ST/Linaro communication.

I only went off what I knew. Some objects (which wouldn't have
prevented playing audio) were failing. It seemed wrong to shut down the
entire audio system because for instance, 'headset mute' or the 'vibrator'
links were broken. As I said to you before, time is a big factor and I
have a massive TODO list. Fixing audio links a) isn't my subject of
expertise, so it would take me much longer to fix than someone with
a good knowledge of the system and b) isn't really my responsibility.
I've tested the DT stuff and it works well. Now I should move onto
something else, like providing the PRCMU with a IRQ Domain, which is
currently blocking the mainlining of other (i.e. thermal) drivers.

> Obviously with the stuff that's device specific you also have to contend
> with the fact that you're working on hardware which just isn't all that
> widely available and quite possibly has closed datasheets.
> 
> > We are all trying to do good things here. Please try not to be too over-
> > critical. We all know Mainlining is a good thing. Perhaps less people
> > would be so frightened of it, thus more people would do it if the reviews
> > weren't such a ball-ache ( for want of a better expression :) ).
> 
> This is a two way thing - one of the tools that maintainers have to try
> to drive up the quality of the code is to push back on poor quality
> submissions and devote less bandwith to sources of those submissions.
> 
> It is true that there's a bunch of people who seem to view review as
> being just an annoying obstacle to be dealt with with the minimum
> possible effort but in practice all that does is make things more
> painful for everyone, they do tend to be more noticable because
> "applied, thanks" doesn't make for a big thread.

Well I know my submissions are not always 100% perfect, but I hope 
you're not implying that they're poor quality. Writing code and fixing
things you view as bugs in code you have no prior knowledge of isn't the 
easiest task in the world. All I can do is attempt to fix the issues that
I see, which get things off the ground or make drivers work again and
submit the changes. If they're wrong they're wrong, but I don't think this
should be viewed as poor quality code!

I didn't say reviews in general. I personally think that reviews are a
very handy way of ensuring code is as the correct level for entry into
Mainline. It's also vital for the learning process, and is where most
of my knowledge has been gained. It's the type of review which defines
the experience. Some Maintainers say things like, "That's wrong. This
is wrong. Why are you doing this?" etc without explaining what the
issues are. That's not a good review, and will put people off trying
again. Equally being too overzealous and nit-picky about stuff that a)
really doesn't matter or b) can be changed really easily _if_ in the
rare case there's an issue. I've also submitted to some Maintainers
who are a pleasure to work with. They explain what's wrong and why
and encourage resubmission. I know Maintainers aren't school teachers,
or life coaches, but they should be encouraging more people to share
their good ( after some fixup ;) ) code and not playing the role of 
an incredibly hard to please boss, or impenetrable brick wall.

Let's just drop this now, or we'll be going round and round forever.
I need to move on to this PRCMU stuff, as it's blocking driver
submission. If you can lend a hand with the audioclk stubs that would
be grand. If not, me might just have to wait for Ola to come back from
his well earned vacation.

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

[alsa-devel] [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On Thu, Aug 02, 2012 at 06:56:04PM +0100, Mark Brown wrote:
> On Thu, Aug 02, 2012 at 08:45:18AM +0100, Lee Jones wrote:
> 
> > Over time, the requests for Maintainers have Snowballed (pun intended). My 
> > task now seems to be enabling drivers for Device Tree _and_ fix all 
> > associated driver bugs, including any requested restructuring and API
> 
> One thing to bear in mind with device tree is that it's all about
> defining an ABI - it's supposed to be stable and OS agnostic so it puts
> a lot of pressure on to really address quality problems that become
> visible in the bindings.  This stuff is much less critical with platform
> data, it's relatively easy to change.
>
> > adoption. What you fail to notice is that I am only one person, and hopping
> > all over the code-base trying to do everyone's bidding is no mean feat. In
> 
> It's fairly obvious that it's only you, or at least only you posting
> stuff (I know sometimes there are bigger teams behind people) - the
> pressure you're under to get something in is very clear.  A big part of
> what I'm saying here is that it would be really helpful if could you
> slow down a bit, discuss problems more and avoid cutting corners so
> much.  This is likely to save you time overall, you'll have a much
> higher success rate and you'll get much better feedback if it's clear
> what's going on and that there's an interest in understanding the
> issues.

I do agree that it should be correct, but the difference between getting
it 90% correct and absolutely perfect increases the effort at least x2.
With so much left to do, I think it would be better to get everything in
and functioning, then fix the minor issues as we come across them later.
This is what I've been doing from the start and it's actually looking
pretty good. I also agree that DT should be OS agnostic, but breaking up
MFDs into their functions shouldn't be an issue for other OSes. Either
they can choose to break them up and use the individual child compatible
strings, or only use the parent node.

My personal opinion is if we'd sat around discussing how it should look
prior to doing any work a) the project probably wouldn't have even got
off the ground yet and b) we would have most likely got it all wrong, as
most of our learning and knowledge has been done/gained by actually doing
the work. Instead I've been using the JFDI (Just Frickin' Do It) 
philosophy, and we've actually made some amazing headway. Being blocked
on minor issues and easy to change, orthogonal issues such as vendor
defined properties (i2c) isn't helpful to anyone.
 
> If you need to focus on device tree enablement and there are underlying
> problems in the subsystems you're looking at perhaps you need to push
> back on whoever's asking you to do the work and say you need the domain
> experts to pitch in and help you out.

If only it were that easy. We're not bursting at the seems with resources
here. I'm working in a very customer focused ecosystem. If they don't 
request it, or file a bug about it, there's no resource allocation to fix
it. However, the future is looking up from that point of view. We've just 
started a new project, which I'm hoping will attract some new resources. 
Watch this space.

> > reality I am no more obliged to fix driver bugs than you are. In fact as
> > the Maintainer of some of these subsystems, perhaps you could even help out
> > a bit?
> 
> You're not telling us about the problems you see so it's very difficult
> for anyone to help you.
> 
> For example with this patch the only information you've sent is the
> patch and the fact that you're seeing the error you're ignoring going
> off on the system you're working with (which I had to ask to find out
> about...).  You've not shown the error message or provided any other
> hint which would help anyone understand why the error might be occuring
> and what a good solution to the problem is.  Ola's guess seems very
> likely but nobody's got enough information to confirm that unless
> there's been some off-list ST/Linaro communication.

I only went off what I knew. Some objects (which wouldn't have
prevented playing audio) were failing. It seemed wrong to shut down the
entire audio system because for instance, 'headset mute' or the 'vibrator'
links were broken. As I said to you before, time is a big factor and I
have a massive TODO list. Fixing audio links a) isn't my subject of
expertise, so it would take me much longer to fix than someone with
a good knowledge of the system and b) isn't really my responsibility.
I've tested the DT stuff and it works well. Now I should move onto
something else, like providing the PRCMU with a IRQ Domain, which is
currently blocking the mainlining of other (i.e. thermal) drivers.

> Obviously with the stuff that's device specific you also have to contend
> with the fact that you're working on hardware which just isn't all that
> widely available and quite possibly has closed datasheets.
> 
> > We are all trying to do good things here. Please try not to be too over-
> > critical. We all know Mainlining is a good thing. Perhaps less people
> > would be so frightened of it, thus more people would do it if the reviews
> > weren't such a ball-ache ( for want of a better expression :) ).
> 
> This is a two way thing - one of the tools that maintainers have to try
> to drive up the quality of the code is to push back on poor quality
> submissions and devote less bandwith to sources of those submissions.
> 
> It is true that there's a bunch of people who seem to view review as
> being just an annoying obstacle to be dealt with with the minimum
> possible effort but in practice all that does is make things more
> painful for everyone, they do tend to be more noticable because
> "applied, thanks" doesn't make for a big thread.

Well I know my submissions are not always 100% perfect, but I hope 
you're not implying that they're poor quality. Writing code and fixing
things you view as bugs in code you have no prior knowledge of isn't the 
easiest task in the world. All I can do is attempt to fix the issues that
I see, which get things off the ground or make drivers work again and
submit the changes. If they're wrong they're wrong, but I don't think this
should be viewed as poor quality code!

I didn't say reviews in general. I personally think that reviews are a
very handy way of ensuring code is as the correct level for entry into
Mainline. It's also vital for the learning process, and is where most
of my knowledge has been gained. It's the type of review which defines
the experience. Some Maintainers say things like, "That's wrong. This
is wrong. Why are you doing this?" etc without explaining what the
issues are. That's not a good review, and will put people off trying
again. Equally being too overzealous and nit-picky about stuff that a)
really doesn't matter or b) can be changed really easily _if_ in the
rare case there's an issue. I've also submitted to some Maintainers
who are a pleasure to work with. They explain what's wrong and why
and encourage resubmission. I know Maintainers aren't school teachers,
or life coaches, but they should be encouraging more people to share
their good ( after some fixup ;) ) code and not playing the role of 
an incredibly hard to please boss, or impenetrable brick wall.

Let's just drop this now, or we'll be going round and round forever.
I need to move on to this PRCMU stuff, as it's blocking driver
submission. If you can lend a hand with the audioclk stubs that would
be grand. If not, me might just have to wait for Ola to come back from
his well earned vacation.

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

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

On Thu, Aug 02, 2012 at 06:56:04PM +0100, Mark Brown wrote:
> On Thu, Aug 02, 2012 at 08:45:18AM +0100, Lee Jones wrote:
> 
> > Over time, the requests for Maintainers have Snowballed (pun intended). My 
> > task now seems to be enabling drivers for Device Tree _and_ fix all 
> > associated driver bugs, including any requested restructuring and API
> 
> One thing to bear in mind with device tree is that it's all about
> defining an ABI - it's supposed to be stable and OS agnostic so it puts
> a lot of pressure on to really address quality problems that become
> visible in the bindings.  This stuff is much less critical with platform
> data, it's relatively easy to change.
>
> > adoption. What you fail to notice is that I am only one person, and hopping
> > all over the code-base trying to do everyone's bidding is no mean feat. In
> 
> It's fairly obvious that it's only you, or at least only you posting
> stuff (I know sometimes there are bigger teams behind people) - the
> pressure you're under to get something in is very clear.  A big part of
> what I'm saying here is that it would be really helpful if could you
> slow down a bit, discuss problems more and avoid cutting corners so
> much.  This is likely to save you time overall, you'll have a much
> higher success rate and you'll get much better feedback if it's clear
> what's going on and that there's an interest in understanding the
> issues.

I do agree that it should be correct, but the difference between getting
it 90% correct and absolutely perfect increases the effort at least x2.
With so much left to do, I think it would be better to get everything in
and functioning, then fix the minor issues as we come across them later.
This is what I've been doing from the start and it's actually looking
pretty good. I also agree that DT should be OS agnostic, but breaking up
MFDs into their functions shouldn't be an issue for other OSes. Either
they can choose to break them up and use the individual child compatible
strings, or only use the parent node.

My personal opinion is if we'd sat around discussing how it should look
prior to doing any work a) the project probably wouldn't have even got
off the ground yet and b) we would have most likely got it all wrong, as
most of our learning and knowledge has been done/gained by actually doing
the work. Instead I've been using the JFDI (Just Frickin' Do It) 
philosophy, and we've actually made some amazing headway. Being blocked
on minor issues and easy to change, orthogonal issues such as vendor
defined properties (i2c) isn't helpful to anyone.
 
> If you need to focus on device tree enablement and there are underlying
> problems in the subsystems you're looking at perhaps you need to push
> back on whoever's asking you to do the work and say you need the domain
> experts to pitch in and help you out.

If only it were that easy. We're not bursting at the seems with resources
here. I'm working in a very customer focused ecosystem. If they don't 
request it, or file a bug about it, there's no resource allocation to fix
it. However, the future is looking up from that point of view. We've just 
started a new project, which I'm hoping will attract some new resources. 
Watch this space.

> > reality I am no more obliged to fix driver bugs than you are. In fact as
> > the Maintainer of some of these subsystems, perhaps you could even help out
> > a bit?
> 
> You're not telling us about the problems you see so it's very difficult
> for anyone to help you.
> 
> For example with this patch the only information you've sent is the
> patch and the fact that you're seeing the error you're ignoring going
> off on the system you're working with (which I had to ask to find out
> about...).  You've not shown the error message or provided any other
> hint which would help anyone understand why the error might be occuring
> and what a good solution to the problem is.  Ola's guess seems very
> likely but nobody's got enough information to confirm that unless
> there's been some off-list ST/Linaro communication.

I only went off what I knew. Some objects (which wouldn't have
prevented playing audio) were failing. It seemed wrong to shut down the
entire audio system because for instance, 'headset mute' or the 'vibrator'
links were broken. As I said to you before, time is a big factor and I
have a massive TODO list. Fixing audio links a) isn't my subject of
expertise, so it would take me much longer to fix than someone with
a good knowledge of the system and b) isn't really my responsibility.
I've tested the DT stuff and it works well. Now I should move onto
something else, like providing the PRCMU with a IRQ Domain, which is
currently blocking the mainlining of other (i.e. thermal) drivers.

> Obviously with the stuff that's device specific you also have to contend
> with the fact that you're working on hardware which just isn't all that
> widely available and quite possibly has closed datasheets.
> 
> > We are all trying to do good things here. Please try not to be too over-
> > critical. We all know Mainlining is a good thing. Perhaps less people
> > would be so frightened of it, thus more people would do it if the reviews
> > weren't such a ball-ache ( for want of a better expression :) ).
> 
> This is a two way thing - one of the tools that maintainers have to try
> to drive up the quality of the code is to push back on poor quality
> submissions and devote less bandwith to sources of those submissions.
> 
> It is true that there's a bunch of people who seem to view review as
> being just an annoying obstacle to be dealt with with the minimum
> possible effort but in practice all that does is make things more
> painful for everyone, they do tend to be more noticable because
> "applied, thanks" doesn't make for a big thread.

Well I know my submissions are not always 100% perfect, but I hope 
you're not implying that they're poor quality. Writing code and fixing
things you view as bugs in code you have no prior knowledge of isn't the 
easiest task in the world. All I can do is attempt to fix the issues that
I see, which get things off the ground or make drivers work again and
submit the changes. If they're wrong they're wrong, but I don't think this
should be viewed as poor quality code!

I didn't say reviews in general. I personally think that reviews are a
very handy way of ensuring code is as the correct level for entry into
Mainline. It's also vital for the learning process, and is where most
of my knowledge has been gained. It's the type of review which defines
the experience. Some Maintainers say things like, "That's wrong. This
is wrong. Why are you doing this?" etc without explaining what the
issues are. That's not a good review, and will put people off trying
again. Equally being too overzealous and nit-picky about stuff that a)
really doesn't matter or b) can be changed really easily _if_ in the
rare case there's an issue. I've also submitted to some Maintainers
who are a pleasure to work with. They explain what's wrong and why
and encourage resubmission. I know Maintainers aren't school teachers,
or life coaches, but they should be encouraging more people to share
their good ( after some fixup ;) ) code and not playing the role of 
an incredibly hard to please boss, or impenetrable brick wall.

Let's just drop this now, or we'll be going round and round forever.
I need to move on to this PRCMU stuff, as it's blocking driver
submission. If you can lend a hand with the audioclk stubs that would
be grand. If not, me might just have to wait for Ola to come back from
his well earned vacation.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Fri, Aug 03, 2012 at 09:30:10AM +0100, Lee Jones wrote:

> I do agree that it should be correct, but the difference between getting
> it 90% correct and absolutely perfect increases the effort at least x2.
> With so much left to do, I think it would be better to get everything in
> and functioning, then fix the minor issues as we come across them later.

If you're going to do this the usual way is to do it by leaving bits
out, and see below.

> If only it were that easy. We're not bursting at the seems with resources
> here. I'm working in a very customer focused ecosystem. If they don't 
> request it, or file a bug about it, there's no resource allocation to fix

Right, I work in the same industry - but this shouldn't be a problem,
if it's not urgent for people to help it's probably not urgent to do
whatever's blocked by it either.

> > You're not telling us about the problems you see so it's very difficult
> > for anyone to help you.

> > For example with this patch the only information you've sent is the
> > patch and the fact that you're seeing the error you're ignoring going
> > off on the system you're working with (which I had to ask to find out

> I only went off what I knew. Some objects (which wouldn't have
> prevented playing audio) were failing. It seemed wrong to shut down the
> entire audio system because for instance, 'headset mute' or the 'vibrator'
> links were broken. As I said to you before, time is a big factor and I
> have a massive TODO list. Fixing audio links a) isn't my subject of

This isn't the point, and it's a *very* important point which is the
main reason I'm replying here.

The immediate point here is that you're not communicating about what
you're trying to which is the source of a lot of problems.  Things would
run a lot more smoothly if when you try to cut corners you were explicit
about the corners you cut, and if when you run into problems you report
those problems as well as sending whatever code you're using to work
around things.  Set people's expecations about what they're seeing and
provide them with context.

Consider the patch that's in the subject line here - it took me a couple
of goes before you even said you'd seen an issue on your system which
you were working around (I still don't know what the actual errors are).
As far as I could tell looking at the patch description it was something
done for taste reasons which was being sent as a bug fix.

The usual approach for things like this is a changelog or cover mail
which says something like "I'm seeing this error, here's the code I'm
using to get things working on my system and I think this is a good idea
because..." (or "...but that can't be right", or whatever).  This works
a whole lot better, it makes it clear what the underlying motivation for
the change is and understand the submitter's expecation for the quality
of the patch.

Similarly with the missing device tree binding documentation, had you
said something about the patches not being complete and writing the
binding documentation later that'd have helped a lot.  Having it there
is a basic checklist thing for new DT bindings which is easy to spot
from a diffstat, it's really not something a reviewer should ever need
to ask about especially from someone doing a lot of DT work and it's a
big red flag for the quality of the code.

Things like this are really important, especially for people doing lots
of work, as they have such a big impact on communication and so much of
what makes this thing tick is about communication.

> expertise, so it would take me much longer to fix than someone with
> a good knowledge of the system and b) isn't really my responsibility.

That's fine, just tell people about the problem and move on to
something else from what's probably a large task list if it's blocking
you (and start nagging people if it doesn't get fixed and it seems
important).  This happens fairly often, it works well most of the time.
Sending a fix is of course ideal but it's not essential.

> Well I know my submissions are not always 100% perfect, but I hope 
> you're not implying that they're poor quality. Writing code and fixing
> things you view as bugs in code you have no prior knowledge of isn't the 

This is process stuff more than code stuff, it's all about communication.

> easiest task in the world. All I can do is attempt to fix the issues that
> I see, which get things off the ground or make drivers work again and
> submit the changes. If they're wrong they're wrong, but I don't think this
> should be viewed as poor quality code!

What you can do here is to commmunicate about what you're doing more.
Don't just think about the code, think about the communication
surrounding the code - this is the core of the issue.

> the experience. Some Maintainers say things like, "That's wrong. This
> is wrong. Why are you doing this?" etc without explaining what the
> issues are. That's not a good review, and will put people off trying
> again.

Like I said in my previous mail this is one of the tools people have
available to them to drive up quality - if you watch a bit more closely
you'll often see that the quality of the review is scaled to factors in
the submission (and often the pattern of submissions from a contributor).
It's often not something that's done conciously, a lot of this is just
people conveying that they're annoyed.

>        Equally being too overzealous and nit-picky about stuff that a)
> really doesn't matter or b) can be changed really easily _if_ in the
> rare case there's an issue. I've also submitted to some Maintainers

This is a similar thing - it's part of the toolbox.

> who are a pleasure to work with. They explain what's wrong and why
> and encourage resubmission. I know Maintainers aren't school teachers,
> or life coaches, but they should be encouraging more people to share
> their good ( after some fixup ;) ) code and not playing the role of 
> an incredibly hard to please boss, or impenetrable brick wall.

Maintainer bandwidth is limited, and people will focus these efforts
where they think it'd be useful.  What I'm spending time doing here is
trying to convey that there's some fairly easily solvable process issues
here which are making everyone's life harder here.

[alsa-devel] [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Fri, Aug 03, 2012 at 09:30:10AM +0100, Lee Jones wrote:

> I do agree that it should be correct, but the difference between getting
> it 90% correct and absolutely perfect increases the effort at least x2.
> With so much left to do, I think it would be better to get everything in
> and functioning, then fix the minor issues as we come across them later.

If you're going to do this the usual way is to do it by leaving bits
out, and see below.

> If only it were that easy. We're not bursting at the seems with resources
> here. I'm working in a very customer focused ecosystem. If they don't 
> request it, or file a bug about it, there's no resource allocation to fix

Right, I work in the same industry - but this shouldn't be a problem,
if it's not urgent for people to help it's probably not urgent to do
whatever's blocked by it either.

> > You're not telling us about the problems you see so it's very difficult
> > for anyone to help you.

> > For example with this patch the only information you've sent is the
> > patch and the fact that you're seeing the error you're ignoring going
> > off on the system you're working with (which I had to ask to find out

> I only went off what I knew. Some objects (which wouldn't have
> prevented playing audio) were failing. It seemed wrong to shut down the
> entire audio system because for instance, 'headset mute' or the 'vibrator'
> links were broken. As I said to you before, time is a big factor and I
> have a massive TODO list. Fixing audio links a) isn't my subject of

This isn't the point, and it's a *very* important point which is the
main reason I'm replying here.

The immediate point here is that you're not communicating about what
you're trying to which is the source of a lot of problems.  Things would
run a lot more smoothly if when you try to cut corners you were explicit
about the corners you cut, and if when you run into problems you report
those problems as well as sending whatever code you're using to work
around things.  Set people's expecations about what they're seeing and
provide them with context.

Consider the patch that's in the subject line here - it took me a couple
of goes before you even said you'd seen an issue on your system which
you were working around (I still don't know what the actual errors are).
As far as I could tell looking at the patch description it was something
done for taste reasons which was being sent as a bug fix.

The usual approach for things like this is a changelog or cover mail
which says something like "I'm seeing this error, here's the code I'm
using to get things working on my system and I think this is a good idea
because..." (or "...but that can't be right", or whatever).  This works
a whole lot better, it makes it clear what the underlying motivation for
the change is and understand the submitter's expecation for the quality
of the patch.

Similarly with the missing device tree binding documentation, had you
said something about the patches not being complete and writing the
binding documentation later that'd have helped a lot.  Having it there
is a basic checklist thing for new DT bindings which is easy to spot
from a diffstat, it's really not something a reviewer should ever need
to ask about especially from someone doing a lot of DT work and it's a
big red flag for the quality of the code.

Things like this are really important, especially for people doing lots
of work, as they have such a big impact on communication and so much of
what makes this thing tick is about communication.

> expertise, so it would take me much longer to fix than someone with
> a good knowledge of the system and b) isn't really my responsibility.

That's fine, just tell people about the problem and move on to
something else from what's probably a large task list if it's blocking
you (and start nagging people if it doesn't get fixed and it seems
important).  This happens fairly often, it works well most of the time.
Sending a fix is of course ideal but it's not essential.

> Well I know my submissions are not always 100% perfect, but I hope 
> you're not implying that they're poor quality. Writing code and fixing
> things you view as bugs in code you have no prior knowledge of isn't the 

This is process stuff more than code stuff, it's all about communication.

> easiest task in the world. All I can do is attempt to fix the issues that
> I see, which get things off the ground or make drivers work again and
> submit the changes. If they're wrong they're wrong, but I don't think this
> should be viewed as poor quality code!

What you can do here is to commmunicate about what you're doing more.
Don't just think about the code, think about the communication
surrounding the code - this is the core of the issue.

> the experience. Some Maintainers say things like, "That's wrong. This
> is wrong. Why are you doing this?" etc without explaining what the
> issues are. That's not a good review, and will put people off trying
> again.

Like I said in my previous mail this is one of the tools people have
available to them to drive up quality - if you watch a bit more closely
you'll often see that the quality of the review is scaled to factors in
the submission (and often the pattern of submissions from a contributor).
It's often not something that's done conciously, a lot of this is just
people conveying that they're annoyed.

>        Equally being too overzealous and nit-picky about stuff that a)
> really doesn't matter or b) can be changed really easily _if_ in the
> rare case there's an issue. I've also submitted to some Maintainers

This is a similar thing - it's part of the toolbox.

> who are a pleasure to work with. They explain what's wrong and why
> and encourage resubmission. I know Maintainers aren't school teachers,
> or life coaches, but they should be encouraging more people to share
> their good ( after some fixup ;) ) code and not playing the role of 
> an incredibly hard to please boss, or impenetrable brick wall.

Maintainer bandwidth is limited, and people will focus these efforts
where they think it'd be useful.  What I'm spending time doing here is
trying to convey that there's some fairly easily solvable process issues
here which are making everyone's life harder here.

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Fri, Aug 03, 2012 at 09:30:10AM +0100, Lee Jones wrote:

> I do agree that it should be correct, but the difference between getting
> it 90% correct and absolutely perfect increases the effort at least x2.
> With so much left to do, I think it would be better to get everything in
> and functioning, then fix the minor issues as we come across them later.

If you're going to do this the usual way is to do it by leaving bits
out, and see below.

> If only it were that easy. We're not bursting at the seems with resources
> here. I'm working in a very customer focused ecosystem. If they don't 
> request it, or file a bug about it, there's no resource allocation to fix

Right, I work in the same industry - but this shouldn't be a problem,
if it's not urgent for people to help it's probably not urgent to do
whatever's blocked by it either.

> > You're not telling us about the problems you see so it's very difficult
> > for anyone to help you.

> > For example with this patch the only information you've sent is the
> > patch and the fact that you're seeing the error you're ignoring going
> > off on the system you're working with (which I had to ask to find out

> I only went off what I knew. Some objects (which wouldn't have
> prevented playing audio) were failing. It seemed wrong to shut down the
> entire audio system because for instance, 'headset mute' or the 'vibrator'
> links were broken. As I said to you before, time is a big factor and I
> have a massive TODO list. Fixing audio links a) isn't my subject of

This isn't the point, and it's a *very* important point which is the
main reason I'm replying here.

The immediate point here is that you're not communicating about what
you're trying to which is the source of a lot of problems.  Things would
run a lot more smoothly if when you try to cut corners you were explicit
about the corners you cut, and if when you run into problems you report
those problems as well as sending whatever code you're using to work
around things.  Set people's expecations about what they're seeing and
provide them with context.

Consider the patch that's in the subject line here - it took me a couple
of goes before you even said you'd seen an issue on your system which
you were working around (I still don't know what the actual errors are).
As far as I could tell looking at the patch description it was something
done for taste reasons which was being sent as a bug fix.

The usual approach for things like this is a changelog or cover mail
which says something like "I'm seeing this error, here's the code I'm
using to get things working on my system and I think this is a good idea
because..." (or "...but that can't be right", or whatever).  This works
a whole lot better, it makes it clear what the underlying motivation for
the change is and understand the submitter's expecation for the quality
of the patch.

Similarly with the missing device tree binding documentation, had you
said something about the patches not being complete and writing the
binding documentation later that'd have helped a lot.  Having it there
is a basic checklist thing for new DT bindings which is easy to spot
from a diffstat, it's really not something a reviewer should ever need
to ask about especially from someone doing a lot of DT work and it's a
big red flag for the quality of the code.

Things like this are really important, especially for people doing lots
of work, as they have such a big impact on communication and so much of
what makes this thing tick is about communication.

> expertise, so it would take me much longer to fix than someone with
> a good knowledge of the system and b) isn't really my responsibility.

That's fine, just tell people about the problem and move on to
something else from what's probably a large task list if it's blocking
you (and start nagging people if it doesn't get fixed and it seems
important).  This happens fairly often, it works well most of the time.
Sending a fix is of course ideal but it's not essential.

> Well I know my submissions are not always 100% perfect, but I hope 
> you're not implying that they're poor quality. Writing code and fixing
> things you view as bugs in code you have no prior knowledge of isn't the 

This is process stuff more than code stuff, it's all about communication.

> easiest task in the world. All I can do is attempt to fix the issues that
> I see, which get things off the ground or make drivers work again and
> submit the changes. If they're wrong they're wrong, but I don't think this
> should be viewed as poor quality code!

What you can do here is to commmunicate about what you're doing more.
Don't just think about the code, think about the communication
surrounding the code - this is the core of the issue.

> the experience. Some Maintainers say things like, "That's wrong. This
> is wrong. Why are you doing this?" etc without explaining what the
> issues are. That's not a good review, and will put people off trying
> again.

Like I said in my previous mail this is one of the tools people have
available to them to drive up quality - if you watch a bit more closely
you'll often see that the quality of the review is scaled to factors in
the submission (and often the pattern of submissions from a contributor).
It's often not something that's done conciously, a lot of this is just
people conveying that they're annoyed.

>        Equally being too overzealous and nit-picky about stuff that a)
> really doesn't matter or b) can be changed really easily _if_ in the
> rare case there's an issue. I've also submitted to some Maintainers

This is a similar thing - it's part of the toolbox.

> who are a pleasure to work with. They explain what's wrong and why
> and encourage resubmission. I know Maintainers aren't school teachers,
> or life coaches, but they should be encouraging more people to share
> their good ( after some fixup ;) ) code and not playing the role of 
> an incredibly hard to please boss, or impenetrable brick wall.

Maintainer bandwidth is limited, and people will focus these efforts
where they think it'd be useful.  What I'm spending time doing here is
trying to convey that there's some fairly easily solvable process issues
here which are making everyone's life harder here.

RE: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Ola Lilja]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: den 1 augusti 2012 15:20
> To: Lee Jones
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> STEricsson_nomadik_linux@list.st.com; linus.walleij@stericsson.com;
> arnd@arndb.de; olalilja@yahoo.se; ola.o.lilja@stericsson.com; alsa-
> devel@alsa-project.org; lrg@ti.com
> Subject: Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force
> all subsequent widgets to fail too
> 
> On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
> > On 31/07/12 16:18, Mark Brown wrote:
> 
> > >I'm not going to apply this patch.  This isn't a vendor BSP, we
> > >shouldn't be putting random hacks like this in core code.
> 
> > BSP kernel or otherwise, it still seems wrong to me to fail and
> entire
> > audio driver just because of a broken link.
> 
> No, really.  Random disconnections in the DAPM graph are just endless
> pain from a support and debug point of view.  This isn't something that
> randomly breaks on specific hardware where we'd expect random errors at
> runtime, it's something that will never have worked - it seems clear
> nobody tested the mainline submission.
> 
> It's very disappointing to see such an error exist, and even more
> disappointing that there's no interest in fixing the driver.

(Yes, I know this mailer isn't configured correctly, but I'm on vacation
and have no Linux-computer/community-mailer available. However I find it
important to answer this)
Mark, you very well know that I have put in a lot of effort in getting our
Ux500-driver mainlined. This is something I have driven without really
getting sanctioned directly at working, rendering it even harder to find
time for it.
Accusing me of having "no interest in fixing the driver" is just absurd
regarding the time I've spent on this. I'm also still driving for
mainlining our upcoming drivers, so there is no lack of interest, nor lack
of activity at our side. I really think you could afford a bit more polite
attitude when doing reviews. It is not easy to fulfill every single aspect
of mainlining directly and there is (most likely) no one that purposely do
break any community rules. At least not from my side.

Regarding the problem with the failing DAPM-widget I can probably guess
What is going wrong when Lee is trying it out. There will be two failing
clock-supply widgets due to the fact that on the mainline-code these
clocks simply is not there yet. I have, of course, tested this driver
before submitting it, and I wouldn't dream on submitting a driver where
there were failing widgets/routes. Internally, I have put a patch with our
clock-tree for Ux500 on, but this is not mainline-quality code and that is
why it is not submitted with the other patches I sent. The clocks are in
the moment of writing being worked on by other persons in ST-Ericsson, and
I would not have had any time to be doing all this which is not in the
scope of my responsibilities (which is the audio-domain).
Before you told me to create the clock-supply widget-type, I had only
warnings for these failing clocks, as an intermediate solution, before
the clock-tree was submitted, but now they are implemented with the clock-
supply-type and there will be route-errors instead.
Linus W. could probably shed some light of when the missing clocks are to
be submitted.

Regards,
Ola

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Ola Lilja]

> -----Original Message-----
> From: Mark Brown [mailto:broonie at opensource.wolfsonmicro.com]
> Sent: den 1 augusti 2012 15:20
> To: Lee Jones
> Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> STEricsson_nomadik_linux at list.st.com; linus.walleij at stericsson.com;
> arnd at arndb.de; olalilja at yahoo.se; ola.o.lilja at stericsson.com; alsa-
> devel at alsa-project.org; lrg at ti.com
> Subject: Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force
> all subsequent widgets to fail too
> 
> On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
> > On 31/07/12 16:18, Mark Brown wrote:
> 
> > >I'm not going to apply this patch.  This isn't a vendor BSP, we
> > >shouldn't be putting random hacks like this in core code.
> 
> > BSP kernel or otherwise, it still seems wrong to me to fail and
> entire
> > audio driver just because of a broken link.
> 
> No, really.  Random disconnections in the DAPM graph are just endless
> pain from a support and debug point of view.  This isn't something that
> randomly breaks on specific hardware where we'd expect random errors at
> runtime, it's something that will never have worked - it seems clear
> nobody tested the mainline submission.
> 
> It's very disappointing to see such an error exist, and even more
> disappointing that there's no interest in fixing the driver.

(Yes, I know this mailer isn't configured correctly, but I'm on vacation
and have no Linux-computer/community-mailer available. However I find it
important to answer this)
Mark, you very well know that I have put in a lot of effort in getting our
Ux500-driver mainlined. This is something I have driven without really
getting sanctioned directly at working, rendering it even harder to find
time for it.
Accusing me of having "no interest in fixing the driver" is just absurd
regarding the time I've spent on this. I'm also still driving for
mainlining our upcoming drivers, so there is no lack of interest, nor lack
of activity at our side. I really think you could afford a bit more polite
attitude when doing reviews. It is not easy to fulfill every single aspect
of mainlining directly and there is (most likely) no one that purposely do
break any community rules. At least not from my side.

Regarding the problem with the failing DAPM-widget I can probably guess
What is going wrong when Lee is trying it out. There will be two failing
clock-supply widgets due to the fact that on the mainline-code these
clocks simply is not there yet. I have, of course, tested this driver
before submitting it, and I wouldn't dream on submitting a driver where
there were failing widgets/routes. Internally, I have put a patch with our
clock-tree for Ux500 on, but this is not mainline-quality code and that is
why it is not submitted with the other patches I sent. The clocks are in
the moment of writing being worked on by other persons in ST-Ericsson, and
I would not have had any time to be doing all this which is not in the
scope of my responsibilities (which is the audio-domain).
Before you told me to create the clock-supply widget-type, I had only
warnings for these failing clocks, as an intermediate solution, before
the clock-tree was submitted, but now they are implemented with the clock-
supply-type and there will be route-errors instead.
Linus W. could probably shed some light of when the missing clocks are to
be submitted.

Regards,
Ola

RE: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Ola Lilja]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: den 1 augusti 2012 15:20
> To: Lee Jones
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> STEricsson_nomadik_linux@list.st.com; linus.walleij@stericsson.com;
> arnd@arndb.de; olalilja@yahoo.se; ola.o.lilja@stericsson.com; alsa-
> devel@alsa-project.org; lrg@ti.com
> Subject: Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force
> all subsequent widgets to fail too
> 
> On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
> > On 31/07/12 16:18, Mark Brown wrote:
> 
> > >I'm not going to apply this patch.  This isn't a vendor BSP, we
> > >shouldn't be putting random hacks like this in core code.
> 
> > BSP kernel or otherwise, it still seems wrong to me to fail and
> entire
> > audio driver just because of a broken link.
> 
> No, really.  Random disconnections in the DAPM graph are just endless
> pain from a support and debug point of view.  This isn't something that
> randomly breaks on specific hardware where we'd expect random errors at
> runtime, it's something that will never have worked - it seems clear
> nobody tested the mainline submission.
> 
> It's very disappointing to see such an error exist, and even more
> disappointing that there's no interest in fixing the driver.

(Yes, I know this mailer isn't configured correctly, but I'm on vacation
and have no Linux-computer/community-mailer available. However I find it
important to answer this)
Mark, you very well know that I have put in a lot of effort in getting our
Ux500-driver mainlined. This is something I have driven without really
getting sanctioned directly at working, rendering it even harder to find
time for it.
Accusing me of having "no interest in fixing the driver" is just absurd
regarding the time I've spent on this. I'm also still driving for
mainlining our upcoming drivers, so there is no lack of interest, nor lack
of activity at our side. I really think you could afford a bit more polite
attitude when doing reviews. It is not easy to fulfill every single aspect
of mainlining directly and there is (most likely) no one that purposely do
break any community rules. At least not from my side.

Regarding the problem with the failing DAPM-widget I can probably guess
What is going wrong when Lee is trying it out. There will be two failing
clock-supply widgets due to the fact that on the mainline-code these
clocks simply is not there yet. I have, of course, tested this driver
before submitting it, and I wouldn't dream on submitting a driver where
there were failing widgets/routes. Internally, I have put a patch with our
clock-tree for Ux500 on, but this is not mainline-quality code and that is
why it is not submitted with the other patches I sent. The clocks are in
the moment of writing being worked on by other persons in ST-Ericsson, and
I would not have had any time to be doing all this which is not in the
scope of my responsibilities (which is the audio-domain).
Before you told me to create the clock-supply widget-type, I had only
warnings for these failing clocks, as an intermediate solution, before
the clock-tree was submitted, but now they are implemented with the clock-
supply-type and there will be route-errors instead.
Linus W. could probably shed some light of when the missing clocks are to
be submitted.

Regards,
Ola

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Thu, Aug 02, 2012 at 07:58:24AM +0200, Ola Lilja wrote:

> Accusing me of having "no interest in fixing the driver" is just absurd
> regarding the time I've spent on this. I'm also still driving for

Sorry, this is more directed at the current round of fixes that are
being sent than the driver itself - the patches to bodge around the
issue are being pushed fairly aggressively as an urgent fix without
even mentioning what the issue is.  I don't have any concerns with
the driver, only with the way it's being fixed.

> Regarding the problem with the failing DAPM-widget I can probably guess
> What is going wrong when Lee is trying it out. There will be two failing
> clock-supply widgets due to the fact that on the mainline-code these
> clocks simply is not there yet. I have, of course, tested this driver
> before submitting it, and I wouldn't dream on submitting a driver where
> there were failing widgets/routes. Internally, I have put a patch with our
> clock-tree for Ux500 on, but this is not mainline-quality code and that is

This sort of reliance on out of tree patches which any real user of the
device would be expected to have sounds like exactly the sort of thing
I'd expect to have caused an issue like this, and obviously the fix here
is to get the clocks in place (even if just as stubs, though I'd expect
there to be a major impact on the functionality of the device if it's
not clocked...) rather than to just bodge out the error checking.

It does also suggest that the DT binding for this device will need to
include the setup of these clocks (I believe the clock bindings for DT
have just gone into mainline this merge window but I'd need to check).

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Thu, Aug 02, 2012 at 07:58:24AM +0200, Ola Lilja wrote:

> Accusing me of having "no interest in fixing the driver" is just absurd
> regarding the time I've spent on this. I'm also still driving for

Sorry, this is more directed at the current round of fixes that are
being sent than the driver itself - the patches to bodge around the
issue are being pushed fairly aggressively as an urgent fix without
even mentioning what the issue is.  I don't have any concerns with
the driver, only with the way it's being fixed.

> Regarding the problem with the failing DAPM-widget I can probably guess
> What is going wrong when Lee is trying it out. There will be two failing
> clock-supply widgets due to the fact that on the mainline-code these
> clocks simply is not there yet. I have, of course, tested this driver
> before submitting it, and I wouldn't dream on submitting a driver where
> there were failing widgets/routes. Internally, I have put a patch with our
> clock-tree for Ux500 on, but this is not mainline-quality code and that is

This sort of reliance on out of tree patches which any real user of the
device would be expected to have sounds like exactly the sort of thing
I'd expect to have caused an issue like this, and obviously the fix here
is to get the clocks in place (even if just as stubs, though I'd expect
there to be a major impact on the functionality of the device if it's
not clocked...) rather than to just bodge out the error checking.

It does also suggest that the DT binding for this device will need to
include the setup of these clocks (I believe the clock bindings for DT
have just gone into mainline this merge window but I'd need to check).

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Mark Brown]

On Thu, Aug 02, 2012 at 07:58:24AM +0200, Ola Lilja wrote:

> Accusing me of having "no interest in fixing the driver" is just absurd
> regarding the time I've spent on this. I'm also still driving for

Sorry, this is more directed at the current round of fixes that are
being sent than the driver itself - the patches to bodge around the
issue are being pushed fairly aggressively as an urgent fix without
even mentioning what the issue is.  I don't have any concerns with
the driver, only with the way it's being fixed.

> Regarding the problem with the failing DAPM-widget I can probably guess
> What is going wrong when Lee is trying it out. There will be two failing
> clock-supply widgets due to the fact that on the mainline-code these
> clocks simply is not there yet. I have, of course, tested this driver
> before submitting it, and I wouldn't dream on submitting a driver where
> there were failing widgets/routes. Internally, I have put a patch with our
> clock-tree for Ux500 on, but this is not mainline-quality code and that is

This sort of reliance on out of tree patches which any real user of the
device would be expected to have sounds like exactly the sort of thing
I'd expect to have caused an issue like this, and obviously the fix here
is to get the clocks in place (even if just as stubs, though I'd expect
there to be a major impact on the functionality of the device if it's
not clocked...) rather than to just bodge out the error checking.

It does also suggest that the DT binding for this device will need to
include the setup of these clocks (I believe the clock bindings for DT
have just gone into mainline this merge window but I'd need to check).

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Linus Walleij]

(As for the thread, which got flamy, let's put it to rest, and Ola:
 we are all impressed with your work on the ux500 ALSA SoC
 driver, no doubt about that, this was all ever about the DT
 patch set.)

On Thu, Aug 2, 2012 at 7:58 AM, Ola Lilja <olalilja@yahoo.se> wrote:

> Linus W. could probably shed some light of when the missing clocks are to
> be submitted.

Ulf Hansson is working on this but he's on vacation FTM.
However he has shown good progress and can probably present
a prototype clock driver when he arrives back. IIRC there was
some clock reparenting business left to sort out.

Yours,
Linus Walleij

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Linus Walleij]

(As for the thread, which got flamy, let's put it to rest, and Ola:
 we are all impressed with your work on the ux500 ALSA SoC
 driver, no doubt about that, this was all ever about the DT
 patch set.)

On Thu, Aug 2, 2012 at 7:58 AM, Ola Lilja <olalilja@yahoo.se> wrote:

> Linus W. could probably shed some light of when the missing clocks are to
> be submitted.

Ulf Hansson is working on this but he's on vacation FTM.
However he has shown good progress and can probably present
a prototype clock driver when he arrives back. IIRC there was
some clock reparenting business left to sort out.

Yours,
Linus Walleij

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Linus Walleij]

(As for the thread, which got flamy, let's put it to rest, and Ola:
 we are all impressed with your work on the ux500 ALSA SoC
 driver, no doubt about that, this was all ever about the DT
 patch set.)

On Thu, Aug 2, 2012 at 7:58 AM, Ola Lilja <olalilja@yahoo.se> wrote:

> Linus W. could probably shed some light of when the missing clocks are to
> be submitted.

Ulf Hansson is working on this but he's on vacation FTM.
However he has shown good progress and can probably present
a prototype clock driver when he arrives back. IIRC there was
some clock reparenting business left to sort out.

Yours,
Linus Walleij

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

Patch withdrawn.

On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
> If a list of widgets is provided and one of them fails to be added as
> a control, the present semantics fail all subsequent widgets. A better
> solution would be to only fail that widget, but pursue in attempting
> to add the rest of the list.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  sound/soc/soc-dapm.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index eded657..7d9c154 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -3095,8 +3095,6 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm,
>  			dev_err(dapm->dev,
>  				"ASoC: Failed to create DAPM control %s\n",
>  				widget->name);
> -			ret = -ENOMEM;
> -			break;
>  		}
>  		widget++;
>  	}
> -- 
> 1.7.9.5
> 

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

[PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

Patch withdrawn.

On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
> If a list of widgets is provided and one of them fails to be added as
> a control, the present semantics fail all subsequent widgets. A better
> solution would be to only fail that widget, but pursue in attempting
> to add the rest of the list.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  sound/soc/soc-dapm.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index eded657..7d9c154 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -3095,8 +3095,6 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm,
>  			dev_err(dapm->dev,
>  				"ASoC: Failed to create DAPM control %s\n",
>  				widget->name);
> -			ret = -ENOMEM;
> -			break;
>  		}
>  		widget++;
>  	}
> -- 
> 1.7.9.5
> 

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

Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too

[Lee Jones]

Patch withdrawn.

On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
> If a list of widgets is provided and one of them fails to be added as
> a control, the present semantics fail all subsequent widgets. A better
> solution would be to only fail that widget, but pursue in attempting
> to add the rest of the list.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  sound/soc/soc-dapm.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index eded657..7d9c154 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -3095,8 +3095,6 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm,
>  			dev_err(dapm->dev,
>  				"ASoC: Failed to create DAPM control %s\n",
>  				widget->name);
> -			ret = -ENOMEM;
> -			break;
>  		}
>  		widget++;
>  	}
> -- 
> 1.7.9.5
> 

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 2/6] ARM: ux500: Remove unused snowball_of_platform_devs struct

[Lee Jones]

This was left over during a recent clean-up which removed Device Tree
helper structs. There is no longer a requirement for it, so we can just
remove it.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 80b4f0e..e641003 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -726,11 +726,6 @@ MACHINE_END
 
 #ifdef CONFIG_MACH_UX500_DT
 
-static struct platform_device *snowball_of_platform_devs[] __initdata = {
-	&snowball_led_dev,
-	&snowball_key_dev,
-};
-
 struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = {
 	/* Requires call-back bindings. */
 	OF_DEV_AUXDATA("arm,cortex-a9-pmu", 0, "arm-pmu", &db8500_pmu_platdata),
-- 
1.7.9.5

[PATCH 2/6] ARM: ux500: Remove unused snowball_of_platform_devs struct

[Lee Jones]

This was left over during a recent clean-up which removed Device Tree
helper structs. There is no longer a requirement for it, so we can just
remove it.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 80b4f0e..e641003 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -726,11 +726,6 @@ MACHINE_END
 
 #ifdef CONFIG_MACH_UX500_DT
 
-static struct platform_device *snowball_of_platform_devs[] __initdata = {
-	&snowball_led_dev,
-	&snowball_key_dev,
-};
-
 struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = {
 	/* Requires call-back bindings. */
 	OF_DEV_AUXDATA("arm,cortex-a9-pmu", 0, "arm-pmu", &db8500_pmu_platdata),
-- 
1.7.9.5

[PATCH 2/6] ASoC: ab8500: Inform SoC Core that we have our own I/O arrangements

[Lee Jones]

If codec->control_data is not populated SoC Core assumes we want to
use regmap, which fails catastrophically, as we don't have one:

Unable to handle kernel NULL pointer dereference at virtual address 00000080
pgd = c0004000
[00000080] *pgd=00000000
Internal error: Oops: 17 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1    Not tainted  (3.5.0-rc6-00884-g0b2419e-dirty #130)
PC is at regmap_read+0x10/0x5c
LR is at hw_read+0x80/0x90
pc : [<c01a91b8>]    lr : [<c0216804>]    psr: 60000013

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 sound/soc/codecs/ab8500-codec.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
index 3c79592..23b4018 100644
--- a/sound/soc/codecs/ab8500-codec.c
+++ b/sound/soc/codecs/ab8500-codec.c
@@ -2406,6 +2406,10 @@ static int ab8500_codec_probe(struct snd_soc_codec *codec)
 
 	/* Setup AB8500 according to board-settings */
 	pdata = (struct ab8500_platform_data *)dev_get_platdata(dev->parent);
+
+	/* Inform SoC Core that we have our own I/O arrangements. */
+	codec->control_data = (void *)true;
+
 	status = ab8500_audio_setup_mics(codec, &pdata->codec->amics);
 	if (status < 0) {
 		pr_err("%s: Failed to setup mics (%d)!\n", __func__, status);
-- 
1.7.9.5

[PATCH 2/6] ASoC: ab8500: Inform SoC Core that we have our own I/O arrangements

[Lee Jones]

If codec->control_data is not populated SoC Core assumes we want to
use regmap, which fails catastrophically, as we don't have one:

Unable to handle kernel NULL pointer dereference at virtual address 00000080
pgd = c0004000
[00000080] *pgd=00000000
Internal error: Oops: 17 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1    Not tainted  (3.5.0-rc6-00884-g0b2419e-dirty #130)
PC is at regmap_read+0x10/0x5c
LR is at hw_read+0x80/0x90
pc : [<c01a91b8>]    lr : [<c0216804>]    psr: 60000013

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 sound/soc/codecs/ab8500-codec.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
index 3c79592..23b4018 100644
--- a/sound/soc/codecs/ab8500-codec.c
+++ b/sound/soc/codecs/ab8500-codec.c
@@ -2406,6 +2406,10 @@ static int ab8500_codec_probe(struct snd_soc_codec *codec)
 
 	/* Setup AB8500 according to board-settings */
 	pdata = (struct ab8500_platform_data *)dev_get_platdata(dev->parent);
+
+	/* Inform SoC Core that we have our own I/O arrangements. */
+	codec->control_data = (void *)true;
+
 	status = ab8500_audio_setup_mics(codec, &pdata->codec->amics);
 	if (status < 0) {
 		pr_err("%s: Failed to setup mics (%d)!\n", __func__, status);
-- 
1.7.9.5

[PATCH 2/6] ASoC: ab8500: Inform SoC Core that we have our own I/O arrangements

[Lee Jones]

If codec->control_data is not populated SoC Core assumes we want to
use regmap, which fails catastrophically, as we don't have one:

Unable to handle kernel NULL pointer dereference at virtual address 00000080
pgd = c0004000
[00000080] *pgd=00000000
Internal error: Oops: 17 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1    Not tainted  (3.5.0-rc6-00884-g0b2419e-dirty #130)
PC is at regmap_read+0x10/0x5c
LR is at hw_read+0x80/0x90
pc : [<c01a91b8>]    lr : [<c0216804>]    psr: 60000013

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 sound/soc/codecs/ab8500-codec.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
index 3c79592..23b4018 100644
--- a/sound/soc/codecs/ab8500-codec.c
+++ b/sound/soc/codecs/ab8500-codec.c
@@ -2406,6 +2406,10 @@ static int ab8500_codec_probe(struct snd_soc_codec *codec)
 
 	/* Setup AB8500 according to board-settings */
 	pdata = (struct ab8500_platform_data *)dev_get_platdata(dev->parent);
+
+	/* Inform SoC Core that we have our own I/O arrangements. */
+	codec->control_data = (void *)true;
+
 	status = ab8500_audio_setup_mics(codec, &pdata->codec->amics);
 	if (status < 0) {
 		pr_err("%s: Failed to setup mics (%d)!\n", __func__, status);
-- 
1.7.9.5

[PATCH 3/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

The platform attempts to register platform device 'snd_soc_u8500'
which doesn't actually exist. Here we change the reference to the
correct one 'snd_soc_mop500'.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
index 9960480..1b6a193 100644
--- a/arch/arm/mach-ux500/board-mop500-msp.c
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent)
 	struct platform_device *msp1;
 
 	pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
-	platform_device_register(&snd_soc_u8500);
+	platform_device_register(&snd_soc_mop500);
 
 	pr_info("Initialize MSP I2S-devices.\n");
 	db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
-- 
1.7.9.5

[PATCH 3/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

The platform attempts to register platform device 'snd_soc_u8500'
which doesn't actually exist. Here we change the reference to the
correct one 'snd_soc_mop500'.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
index 9960480..1b6a193 100644
--- a/arch/arm/mach-ux500/board-mop500-msp.c
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent)
 	struct platform_device *msp1;
 
 	pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
-	platform_device_register(&snd_soc_u8500);
+	platform_device_register(&snd_soc_mop500);
 
 	pr_info("Initialize MSP I2S-devices.\n");
 	db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
-- 
1.7.9.5

Re: [PATCH 3/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Sergei Shtylyov]

Hello.

On 07/31/2012 05:31 PM, Lee Jones wrote:

  Subject doesn't parse for me...

> The platform attempts to register platform device 'snd_soc_u8500'
> which doesn't actually exist. Here we change the reference to the
> correct one 'snd_soc_mop500'.

> Signed-off-by: Lee Jones <lee.jones@linaro.org>

WBR, Sergei

[PATCH 3/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Sergei Shtylyov]

Hello.

On 07/31/2012 05:31 PM, Lee Jones wrote:

  Subject doesn't parse for me...

> The platform attempts to register platform device 'snd_soc_u8500'
> which doesn't actually exist. Here we change the reference to the
> correct one 'snd_soc_mop500'.

> Signed-off-by: Lee Jones <lee.jones@linaro.org>

WBR, Sergei

Re: [PATCH 3/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

On 31/07/12 17:46, Sergei Shtylyov wrote:
> Hello.
> 
> On 07/31/2012 05:31 PM, Lee Jones wrote:
> 
>    Subject doesn't parse for me...
> 
>> The platform attempts to register platform device 'snd_soc_u8500'
>> which doesn't actually exist. Here we change the reference to the
>> correct one 'snd_soc_mop500'.
> 
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Actually, it does make sense?

As a sentence: "This patch fixes merge error: so such structure 'struct snd_soc_u8500'.


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

[PATCH 3/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

On 31/07/12 17:46, Sergei Shtylyov wrote:
> Hello.
> 
> On 07/31/2012 05:31 PM, Lee Jones wrote:
> 
>    Subject doesn't parse for me...
> 
>> The platform attempts to register platform device 'snd_soc_u8500'
>> which doesn't actually exist. Here we change the reference to the
>> correct one 'snd_soc_mop500'.
> 
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Actually, it does make sense?

As a sentence: "This patch fixes merge error: so such structure 'struct snd_soc_u8500'.


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

Re: [PATCH 3/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

On 31/07/12 17:46, Sergei Shtylyov wrote:
> Hello.
> 
> On 07/31/2012 05:31 PM, Lee Jones wrote:
> 
>    Subject doesn't parse for me...
> 
>> The platform attempts to register platform device 'snd_soc_u8500'
>> which doesn't actually exist. Here we change the reference to the
>> correct one 'snd_soc_mop500'.
> 
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Actually, it does make sense?

As a sentence: "This patch fixes merge error: so such structure 'struct snd_soc_u8500'.


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

On 01/08/12 08:37, Lee Jones wrote:
> On 31/07/12 17:46, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 07/31/2012 05:31 PM, Lee Jones wrote:
>>
>>     Subject doesn't parse for me...
>>
>>> The platform attempts to register platform device 'snd_soc_u8500'
>>> which doesn't actually exist. Here we change the reference to the
>>> correct one 'snd_soc_mop500'.
>>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>
> Actually, it does make sense?
>
> As a sentence: "This patch fixes merge error: so such structure 'struct snd_soc_u8500'.

That's interesting. Not only when I read it did it seem correct, I also 
wrote it wrong "as a sentence". Of course "so" should be "no".


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

[PATCH 3/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

On 01/08/12 08:37, Lee Jones wrote:
> On 31/07/12 17:46, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 07/31/2012 05:31 PM, Lee Jones wrote:
>>
>>     Subject doesn't parse for me...
>>
>>> The platform attempts to register platform device 'snd_soc_u8500'
>>> which doesn't actually exist. Here we change the reference to the
>>> correct one 'snd_soc_mop500'.
>>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>
> Actually, it does make sense?
>
> As a sentence: "This patch fixes merge error: so such structure 'struct snd_soc_u8500'.

That's interesting. Not only when I read it did it seem correct, I also 
wrote it wrong "as a sentence". Of course "so" should be "no".


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

Re: [PATCH 3/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

On 01/08/12 08:37, Lee Jones wrote:
> On 31/07/12 17:46, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 07/31/2012 05:31 PM, Lee Jones wrote:
>>
>>     Subject doesn't parse for me...
>>
>>> The platform attempts to register platform device 'snd_soc_u8500'
>>> which doesn't actually exist. Here we change the reference to the
>>> correct one 'snd_soc_mop500'.
>>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>
> Actually, it does make sense?
>
> As a sentence: "This patch fixes merge error: so such structure 'struct snd_soc_u8500'.

That's interesting. Not only when I read it did it seem correct, I also 
wrote it wrong "as a sentence". Of course "so" should be "no".


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/6 v2] ARM: ux500: Fix merge error, no matching driver name for, 'snd_soc_u8500'

[Lee Jones]

From: Lee Jones <lee.jones@linaro.org>
Date: Fri, 27 Jul 2012 13:10:52 +0100
Subject: [PATCH 1/1] ARM: ux500: Fix merge error, no matching driver name for
 'snd_soc_u8500'

The platform attempts to register platform device 'snd_soc_u8500'
which doesn't actually exist. Here we change the reference to the
correct one 'snd_soc_mop500'.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-msp.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
index 9960480..df15646 100644
--- a/arch/arm/mach-ux500/board-mop500-msp.c
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -191,9 +191,9 @@ static struct platform_device *db8500_add_msp_i2s(struct device *parent,
 	return pdev;
 }
 
-/* Platform device for ASoC U8500 machine */
-static struct platform_device snd_soc_u8500 = {
-		.name = "snd-soc-u8500",
+/* Platform device for ASoC MOP500 machine */
+static struct platform_device snd_soc_mop500 = {
+		.name = "snd-soc-mop500",
 		.id = 0,
 		.dev = {
 			.platform_data = NULL,
@@ -227,8 +227,8 @@ int mop500_msp_init(struct device *parent)
 {
 	struct platform_device *msp1;
 
-	pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
-	platform_device_register(&snd_soc_u8500);
+	pr_info("%s: Register platform-device 'snd-soc-mop500'.\n", __func__);
+	platform_device_register(&snd_soc_mop500);
 
 	pr_info("Initialize MSP I2S-devices.\n");
 	db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
-- 
1.7.9.5

[PATCH 3/6 v2] ARM: ux500: Fix merge error, no matching driver name for, 'snd_soc_u8500'

[Lee Jones]

From: Lee Jones <lee.jones@linaro.org>
Date: Fri, 27 Jul 2012 13:10:52 +0100
Subject: [PATCH 1/1] ARM: ux500: Fix merge error, no matching driver name for
 'snd_soc_u8500'

The platform attempts to register platform device 'snd_soc_u8500'
which doesn't actually exist. Here we change the reference to the
correct one 'snd_soc_mop500'.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-msp.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
index 9960480..df15646 100644
--- a/arch/arm/mach-ux500/board-mop500-msp.c
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -191,9 +191,9 @@ static struct platform_device *db8500_add_msp_i2s(struct device *parent,
 	return pdev;
 }
 
-/* Platform device for ASoC U8500 machine */
-static struct platform_device snd_soc_u8500 = {
-		.name = "snd-soc-u8500",
+/* Platform device for ASoC MOP500 machine */
+static struct platform_device snd_soc_mop500 = {
+		.name = "snd-soc-mop500",
 		.id = 0,
 		.dev = {
 			.platform_data = NULL,
@@ -227,8 +227,8 @@ int mop500_msp_init(struct device *parent)
 {
 	struct platform_device *msp1;
 
-	pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
-	platform_device_register(&snd_soc_u8500);
+	pr_info("%s: Register platform-device 'snd-soc-mop500'.\n", __func__);
+	platform_device_register(&snd_soc_mop500);
 
 	pr_info("Initialize MSP I2S-devices.\n");
 	db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
-- 
1.7.9.5

Re: [PATCH 3/6 v2] ARM: ux500: Fix merge error, no matching driver name for, 'snd_soc_u8500'

[Lee Jones]

From: Lee Jones <lee.jones@linaro.org>
Date: Fri, 27 Jul 2012 13:10:52 +0100
Subject: [PATCH 1/1] ARM: ux500: Fix merge error, no matching driver name for
 'snd_soc_u8500'

The platform attempts to register platform device 'snd_soc_u8500'
which doesn't actually exist. Here we change the reference to the
correct one 'snd_soc_mop500'.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-msp.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
index 9960480..df15646 100644
--- a/arch/arm/mach-ux500/board-mop500-msp.c
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -191,9 +191,9 @@ static struct platform_device *db8500_add_msp_i2s(struct device *parent,
 	return pdev;
 }
 
-/* Platform device for ASoC U8500 machine */
-static struct platform_device snd_soc_u8500 = {
-		.name = "snd-soc-u8500",
+/* Platform device for ASoC MOP500 machine */
+static struct platform_device snd_soc_mop500 = {
+		.name = "snd-soc-mop500",
 		.id = 0,
 		.dev = {
 			.platform_data = NULL,
@@ -227,8 +227,8 @@ int mop500_msp_init(struct device *parent)
 {
 	struct platform_device *msp1;
 
-	pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
-	platform_device_register(&snd_soc_u8500);
+	pr_info("%s: Register platform-device 'snd-soc-mop500'.\n", __func__);
+	platform_device_register(&snd_soc_mop500);
 
 	pr_info("Initialize MSP I2S-devices.\n");
 	db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
-- 
1.7.9.5

[PATCH 3/6] ARM: ux500: Remove unused snowball_of_platform_devs struct

[Lee Jones]

This was left over during a recent clean-up which removed Device Tree
helper structs. There is no longer a requirement for it, so we can just
remove it.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 80b4f0e..e641003 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -726,11 +726,6 @@ MACHINE_END
 
 #ifdef CONFIG_MACH_UX500_DT
 
-static struct platform_device *snowball_of_platform_devs[] __initdata = {
-	&snowball_led_dev,
-	&snowball_key_dev,
-};
-
 struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = {
 	/* Requires call-back bindings. */
 	OF_DEV_AUXDATA("arm,cortex-a9-pmu", 0, "arm-pmu", &db8500_pmu_platdata),
-- 
1.7.9.5

[PATCH 3/6] ARM: ux500: Remove unused snowball_of_platform_devs struct

[Lee Jones]

This was left over during a recent clean-up which removed Device Tree
helper structs. There is no longer a requirement for it, so we can just
remove it.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 80b4f0e..e641003 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -726,11 +726,6 @@ MACHINE_END
 
 #ifdef CONFIG_MACH_UX500_DT
 
-static struct platform_device *snowball_of_platform_devs[] __initdata = {
-	&snowball_led_dev,
-	&snowball_key_dev,
-};
-
 struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = {
 	/* Requires call-back bindings. */
 	OF_DEV_AUXDATA("arm,cortex-a9-pmu", 0, "arm-pmu", &db8500_pmu_platdata),
-- 
1.7.9.5

[PATCH 3/6] ARM: ux500: Remove unused snowball_of_platform_devs struct

[Lee Jones]

This was left over during a recent clean-up which removed Device Tree
helper structs. There is no longer a requirement for it, so we can just
remove it.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 80b4f0e..e641003 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -726,11 +726,6 @@ MACHINE_END
 
 #ifdef CONFIG_MACH_UX500_DT
 
-static struct platform_device *snowball_of_platform_devs[] __initdata = {
-	&snowball_led_dev,
-	&snowball_key_dev,
-};
-
 struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = {
 	/* Requires call-back bindings. */
 	OF_DEV_AUXDATA("arm,cortex-a9-pmu", 0, "arm-pmu", &db8500_pmu_platdata),
-- 
1.7.9.5

Re: [PATCH 3/6] ARM: ux500: Remove unused snowball_of_platform_devs struct

[Arnd Bergmann]

On Tuesday 31 July 2012, Lee Jones wrote:
> This was left over during a recent clean-up which removed Device Tree
> helper structs. There is no longer a requirement for it, so we can just
> remove it.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 3/6] ARM: ux500: Remove unused snowball_of_platform_devs struct

[Arnd Bergmann]

On Tuesday 31 July 2012, Lee Jones wrote:
> This was left over during a recent clean-up which removed Device Tree
> helper structs. There is no longer a requirement for it, so we can just
> remove it.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 3/6] ARM: ux500: Remove unused snowball_of_platform_devs struct

[Arnd Bergmann]

On Tuesday 31 July 2012, Lee Jones wrote:
> This was left over during a recent clean-up which removed Device Tree
> helper structs. There is no longer a requirement for it, so we can just
> remove it.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 4/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Lee Jones]

Previous attempts to add platform probing of the Audio related devices
only call from non-DT initialisation functions. This patch extends that
functionality to the Device Tree related ones too.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index e641003..87a5cd7 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -794,6 +794,7 @@ static void __init u8500_init_machine(void)
 				ARRAY_SIZE(mop500_platform_devs));
 
 		mop500_sdi_init(parent);
+		mop500_msp_init(parent);
 		i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 		i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
 		i2c_register_board_info(2, mop500_i2c2_devices,
@@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
 
 		mop500_uib_init();
 
+	} else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {
+		mop500_of_msp_init(parent);
 	} else if (of_machine_is_compatible("st-ericsson,hrefv60+")) {
 		/*
 		 * The HREFv60 board removed a GPIO expander and routed
@@ -812,6 +815,7 @@ static void __init u8500_init_machine(void)
 				ARRAY_SIZE(mop500_platform_devs));
 
 		hrefv60_sdi_init(parent);
+		mop500_msp_init(parent);
 
 		i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 		i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;
-- 
1.7.9.5

[PATCH 4/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Lee Jones]

Previous attempts to add platform probing of the Audio related devices
only call from non-DT initialisation functions. This patch extends that
functionality to the Device Tree related ones too.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index e641003..87a5cd7 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -794,6 +794,7 @@ static void __init u8500_init_machine(void)
 				ARRAY_SIZE(mop500_platform_devs));
 
 		mop500_sdi_init(parent);
+		mop500_msp_init(parent);
 		i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 		i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
 		i2c_register_board_info(2, mop500_i2c2_devices,
@@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
 
 		mop500_uib_init();
 
+	} else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {
+		mop500_of_msp_init(parent);
 	} else if (of_machine_is_compatible("st-ericsson,hrefv60+")) {
 		/*
 		 * The HREFv60 board removed a GPIO expander and routed
@@ -812,6 +815,7 @@ static void __init u8500_init_machine(void)
 				ARRAY_SIZE(mop500_platform_devs));
 
 		hrefv60_sdi_init(parent);
+		mop500_msp_init(parent);
 
 		i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 		i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;
-- 
1.7.9.5

[PATCH 4/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

The platform attempts to register platform device 'snd_soc_u8500'
which doesn't actually exist. Here we change the reference to the
correct one 'snd_soc_mop500'.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
index 9960480..1b6a193 100644
--- a/arch/arm/mach-ux500/board-mop500-msp.c
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent)
 	struct platform_device *msp1;
 
 	pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
-	platform_device_register(&snd_soc_u8500);
+	platform_device_register(&snd_soc_mop500);
 
 	pr_info("Initialize MSP I2S-devices.\n");
 	db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
-- 
1.7.9.5

[PATCH 4/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

The platform attempts to register platform device 'snd_soc_u8500'
which doesn't actually exist. Here we change the reference to the
correct one 'snd_soc_mop500'.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
index 9960480..1b6a193 100644
--- a/arch/arm/mach-ux500/board-mop500-msp.c
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent)
 	struct platform_device *msp1;
 
 	pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
-	platform_device_register(&snd_soc_u8500);
+	platform_device_register(&snd_soc_mop500);
 
 	pr_info("Initialize MSP I2S-devices.\n");
 	db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
-- 
1.7.9.5

[PATCH 4/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Lee Jones]

The platform attempts to register platform device 'snd_soc_u8500'
which doesn't actually exist. Here we change the reference to the
correct one 'snd_soc_mop500'.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
index 9960480..1b6a193 100644
--- a/arch/arm/mach-ux500/board-mop500-msp.c
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent)
 	struct platform_device *msp1;
 
 	pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
-	platform_device_register(&snd_soc_u8500);
+	platform_device_register(&snd_soc_mop500);
 
 	pr_info("Initialize MSP I2S-devices.\n");
 	db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
-- 
1.7.9.5

Re: [PATCH 4/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Arnd Bergmann]

On Tuesday 31 July 2012, Lee Jones wrote:
>  arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
> index 9960480..1b6a193 100644
> --- a/arch/arm/mach-ux500/board-mop500-msp.c
> +++ b/arch/arm/mach-ux500/board-mop500-msp.c
> @@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent)
>         struct platform_device *msp1;
>  
>         pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
> -       platform_device_register(&snd_soc_u8500);
> +       platform_device_register(&snd_soc_mop500);
>  
>         pr_info("Initialize MSP I2S-devices.\n");
>         db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,

Against which tree is this? In upstream I only see the snd_soc_u8500 device,
not snd_soc_mop500.

	Arnd

[PATCH 4/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Arnd Bergmann]

On Tuesday 31 July 2012, Lee Jones wrote:
>  arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
> index 9960480..1b6a193 100644
> --- a/arch/arm/mach-ux500/board-mop500-msp.c
> +++ b/arch/arm/mach-ux500/board-mop500-msp.c
> @@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent)
>         struct platform_device *msp1;
>  
>         pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
> -       platform_device_register(&snd_soc_u8500);
> +       platform_device_register(&snd_soc_mop500);
>  
>         pr_info("Initialize MSP I2S-devices.\n");
>         db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,

Against which tree is this? In upstream I only see the snd_soc_u8500 device,
not snd_soc_mop500.

	Arnd

Re: [PATCH 4/6] ARM: ux500: Fix merge error, so such struct 'snd_soc_u8500'

[Arnd Bergmann]

On Tuesday 31 July 2012, Lee Jones wrote:
>  arch/arm/mach-ux500/board-mop500-msp.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
> index 9960480..1b6a193 100644
> --- a/arch/arm/mach-ux500/board-mop500-msp.c
> +++ b/arch/arm/mach-ux500/board-mop500-msp.c
> @@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent)
>         struct platform_device *msp1;
>  
>         pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
> -       platform_device_register(&snd_soc_u8500);
> +       platform_device_register(&snd_soc_mop500);
>  
>         pr_info("Initialize MSP I2S-devices.\n");
>         db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,

Against which tree is this? In upstream I only see the snd_soc_u8500 device,
not snd_soc_mop500.

	Arnd

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

The current kernel commandline for ux500 based devices includes
hard-coded allocations for things like mali and hwmem, which
actually run over lowmem. Here we enable highmem in order to
avoid memory corruption errors.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index c013bbf..f51c351 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -28,6 +28,7 @@ config MACH_MOP500
 	select I2C
 	select I2C_NOMADIK
 	select SOC_BUS
+	select HIGHMEM
 	help
 	  Include support for the MOP500 development platform.
 
-- 
1.7.9.5

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

The current kernel commandline for ux500 based devices includes
hard-coded allocations for things like mali and hwmem, which
actually run over lowmem. Here we enable highmem in order to
avoid memory corruption errors.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index c013bbf..f51c351 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -28,6 +28,7 @@ config MACH_MOP500
 	select I2C
 	select I2C_NOMADIK
 	select SOC_BUS
+	select HIGHMEM
 	help
 	  Include support for the MOP500 development platform.
 
-- 
1.7.9.5

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

The current kernel commandline for ux500 based devices includes
hard-coded allocations for things like mali and hwmem, which
actually run over lowmem. Here we enable highmem in order to
avoid memory corruption errors.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index c013bbf..f51c351 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -28,6 +28,7 @@ config MACH_MOP500
 	select I2C
 	select I2C_NOMADIK
 	select SOC_BUS
+	select HIGHMEM
 	help
 	  Include support for the MOP500 development platform.
 
-- 
1.7.9.5

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Russell King - ARM Linux]

On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
> The current kernel commandline for ux500 based devices includes
> hard-coded allocations for things like mali and hwmem, which
> actually run over lowmem. Here we enable highmem in order to
> avoid memory corruption errors.

Err what?  highmem should make no difference if things are being done
correctly.

In other words, your patch description is too vague to understand what
the problem is and why you have to force highmem on - and nothing should
_require_ highmem.

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Russell King - ARM Linux]

On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
> The current kernel commandline for ux500 based devices includes
> hard-coded allocations for things like mali and hwmem, which
> actually run over lowmem. Here we enable highmem in order to
> avoid memory corruption errors.

Err what?  highmem should make no difference if things are being done
correctly.

In other words, your patch description is too vague to understand what
the problem is and why you have to force highmem on - and nothing should
_require_ highmem.

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Russell King - ARM Linux]

On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
> The current kernel commandline for ux500 based devices includes
> hard-coded allocations for things like mali and hwmem, which
> actually run over lowmem. Here we enable highmem in order to
> avoid memory corruption errors.

Err what?  highmem should make no difference if things are being done
correctly.

In other words, your patch description is too vague to understand what
the problem is and why you have to force highmem on - and nothing should
_require_ highmem.

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

On 31/07/12 14:56, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
>> The current kernel commandline for ux500 based devices includes
>> hard-coded allocations for things like mali and hwmem, which
>> actually run over lowmem. Here we enable highmem in order to
>> avoid memory corruption errors.
>
> Err what?  highmem should make no difference if things are being done
> correctly.
>
> In other words, your patch description is too vague to understand what
> the problem is and why you have to force highmem on - and nothing should
> _require_ highmem.

I think I must have misunderstood some advice which was recently given 
to me. I'll drop the patch.

Thanks for reviewing.

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

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

On 31/07/12 14:56, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
>> The current kernel commandline for ux500 based devices includes
>> hard-coded allocations for things like mali and hwmem, which
>> actually run over lowmem. Here we enable highmem in order to
>> avoid memory corruption errors.
>
> Err what?  highmem should make no difference if things are being done
> correctly.
>
> In other words, your patch description is too vague to understand what
> the problem is and why you have to force highmem on - and nothing should
> _require_ highmem.

I think I must have misunderstood some advice which was recently given 
to me. I'll drop the patch.

Thanks for reviewing.

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

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

On 31/07/12 14:56, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
>> The current kernel commandline for ux500 based devices includes
>> hard-coded allocations for things like mali and hwmem, which
>> actually run over lowmem. Here we enable highmem in order to
>> avoid memory corruption errors.
>
> Err what?  highmem should make no difference if things are being done
> correctly.
>
> In other words, your patch description is too vague to understand what
> the problem is and why you have to force highmem on - and nothing should
> _require_ highmem.

I think I must have misunderstood some advice which was recently given 
to me. I'll drop the patch.

Thanks for reviewing.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Russell King - ARM Linux]

On Tue, Jul 31, 2012 at 03:29:48PM +0100, Lee Jones wrote:
> On 31/07/12 14:56, Russell King - ARM Linux wrote:
>> On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
>>> The current kernel commandline for ux500 based devices includes
>>> hard-coded allocations for things like mali and hwmem, which
>>> actually run over lowmem. Here we enable highmem in order to
>>> avoid memory corruption errors.
>>
>> Err what?  highmem should make no difference if things are being done
>> correctly.
>>
>> In other words, your patch description is too vague to understand what
>> the problem is and why you have to force highmem on - and nothing should
>> _require_ highmem.
>
> I think I must have misunderstood some advice which was recently given  
> to me. I'll drop the patch.
>
> Thanks for reviewing.

There are two reasons for not doing this:

1. it is wrong to force core optional features on by using 'select'
2. it is wrong to use 'select' on user visible symbols because it creates
   confusion, and prevents the user from disabling the option.

Note that highmem is just a hack to make larger memory systems work before
64-bit systems happen - it is _known_ to make systems unstable with large
amounts of highmem (even on x86) due to the requirement to have a certain
amount of metadata stored in lowmem.

Nothing should ever force anyone to have highmem enabled.

I still fail to see how not having highmem enabled would ever cause memory
corruption errors (unless something dealing with memory in a very very
wrong way - iow, not using one of the reservation or memory allocation
methods provided by the kernel.)

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Russell King - ARM Linux]

On Tue, Jul 31, 2012 at 03:29:48PM +0100, Lee Jones wrote:
> On 31/07/12 14:56, Russell King - ARM Linux wrote:
>> On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
>>> The current kernel commandline for ux500 based devices includes
>>> hard-coded allocations for things like mali and hwmem, which
>>> actually run over lowmem. Here we enable highmem in order to
>>> avoid memory corruption errors.
>>
>> Err what?  highmem should make no difference if things are being done
>> correctly.
>>
>> In other words, your patch description is too vague to understand what
>> the problem is and why you have to force highmem on - and nothing should
>> _require_ highmem.
>
> I think I must have misunderstood some advice which was recently given  
> to me. I'll drop the patch.
>
> Thanks for reviewing.

There are two reasons for not doing this:

1. it is wrong to force core optional features on by using 'select'
2. it is wrong to use 'select' on user visible symbols because it creates
   confusion, and prevents the user from disabling the option.

Note that highmem is just a hack to make larger memory systems work before
64-bit systems happen - it is _known_ to make systems unstable with large
amounts of highmem (even on x86) due to the requirement to have a certain
amount of metadata stored in lowmem.

Nothing should ever force anyone to have highmem enabled.

I still fail to see how not having highmem enabled would ever cause memory
corruption errors (unless something dealing with memory in a very very
wrong way - iow, not using one of the reservation or memory allocation
methods provided by the kernel.)

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Russell King - ARM Linux]

On Tue, Jul 31, 2012 at 03:29:48PM +0100, Lee Jones wrote:
> On 31/07/12 14:56, Russell King - ARM Linux wrote:
>> On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
>>> The current kernel commandline for ux500 based devices includes
>>> hard-coded allocations for things like mali and hwmem, which
>>> actually run over lowmem. Here we enable highmem in order to
>>> avoid memory corruption errors.
>>
>> Err what?  highmem should make no difference if things are being done
>> correctly.
>>
>> In other words, your patch description is too vague to understand what
>> the problem is and why you have to force highmem on - and nothing should
>> _require_ highmem.
>
> I think I must have misunderstood some advice which was recently given  
> to me. I'll drop the patch.
>
> Thanks for reviewing.

There are two reasons for not doing this:

1. it is wrong to force core optional features on by using 'select'
2. it is wrong to use 'select' on user visible symbols because it creates
   confusion, and prevents the user from disabling the option.

Note that highmem is just a hack to make larger memory systems work before
64-bit systems happen - it is _known_ to make systems unstable with large
amounts of highmem (even on x86) due to the requirement to have a certain
amount of metadata stored in lowmem.

Nothing should ever force anyone to have highmem enabled.

I still fail to see how not having highmem enabled would ever cause memory
corruption errors (unless something dealing with memory in a very very
wrong way - iow, not using one of the reservation or memory allocation
methods provided by the kernel.)

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Arnd Bergmann]

On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
> I still fail to see how not having highmem enabled would ever cause memory
> corruption errors (unless something dealing with memory in a very very
> wrong way - iow, not using one of the reservation or memory allocation
> methods provided by the kernel.)

The problem is that all users of ux500 systems pass a command line like

vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M

This is of course totally bogus and should not be done. If I understand
Lee correctly, one of the issues resulting from passing a command
line like this without enabling highmem is memory corruption.

"Doctor it hurts when I do this ..."

	Arnd

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Arnd Bergmann]

On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
> I still fail to see how not having highmem enabled would ever cause memory
> corruption errors (unless something dealing with memory in a very very
> wrong way - iow, not using one of the reservation or memory allocation
> methods provided by the kernel.)

The problem is that all users of ux500 systems pass a command line like

vmalloc=256M mem=128M at 0 mali.mali_mem=32M at 128M hwmem=168M at 160M mem=48M at 328M mem_issw=1M at 383M mem=640M at 384M

This is of course totally bogus and should not be done. If I understand
Lee correctly, one of the issues resulting from passing a command
line like this without enabling highmem is memory corruption.

"Doctor it hurts when I do this ..."

	Arnd

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Arnd Bergmann]

On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
> I still fail to see how not having highmem enabled would ever cause memory
> corruption errors (unless something dealing with memory in a very very
> wrong way - iow, not using one of the reservation or memory allocation
> methods provided by the kernel.)

The problem is that all users of ux500 systems pass a command line like

vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M

This is of course totally bogus and should not be done. If I understand
Lee correctly, one of the issues resulting from passing a command
line like this without enabling highmem is memory corruption.

"Doctor it hurts when I do this ..."

	Arnd

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Russell King - ARM Linux]

On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
> On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
> > I still fail to see how not having highmem enabled would ever cause memory
> > corruption errors (unless something dealing with memory in a very very
> > wrong way - iow, not using one of the reservation or memory allocation
> > methods provided by the kernel.)
> 
> The problem is that all users of ux500 systems pass a command line like
> 
> vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M
> 
> This is of course totally bogus and should not be done. If I understand
> Lee correctly, one of the issues resulting from passing a command
> line like this without enabling highmem is memory corruption.

But the question is _why_ does that corruption happen.

>From the above, we will end up with the kernel getting:

0x00000000 - 0x07ffffff (128M @ 0)
0x14800000 - 0x177fffff (48M  @ 328M)
0x18000000 - 0x3fffffff (640M @ 384M)

with:

0x08000000 - 0x081fffff used for mali
0x0a000000 - 0x147fffff used for hwmem
0x17f00000 - 0x17ffffff used for mem_issw

Now, with highmem disabled, the kernel should still map exactly the
regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the
direct mapped region, and truncate the 0x18000000 - 0x3fffffff
region appropriately, reducing the amount of memory available such
that it won't overlap the vmalloc area (which you've specified to be
a minimum of 256M.)

This should _NOT_ cause any memory corruption.

So, come on guys.  Debugging is *mandatory* for this kind of problem.
Papering over it is obscene.

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Russell King - ARM Linux]

On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
> On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
> > I still fail to see how not having highmem enabled would ever cause memory
> > corruption errors (unless something dealing with memory in a very very
> > wrong way - iow, not using one of the reservation or memory allocation
> > methods provided by the kernel.)
> 
> The problem is that all users of ux500 systems pass a command line like
> 
> vmalloc=256M mem=128M at 0 mali.mali_mem=32M at 128M hwmem=168M at 160M mem=48M at 328M mem_issw=1M at 383M mem=640M at 384M
> 
> This is of course totally bogus and should not be done. If I understand
> Lee correctly, one of the issues resulting from passing a command
> line like this without enabling highmem is memory corruption.

But the question is _why_ does that corruption happen.

>From the above, we will end up with the kernel getting:

0x00000000 - 0x07ffffff (128M @ 0)
0x14800000 - 0x177fffff (48M  @ 328M)
0x18000000 - 0x3fffffff (640M @ 384M)

with:

0x08000000 - 0x081fffff used for mali
0x0a000000 - 0x147fffff used for hwmem
0x17f00000 - 0x17ffffff used for mem_issw

Now, with highmem disabled, the kernel should still map exactly the
regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the
direct mapped region, and truncate the 0x18000000 - 0x3fffffff
region appropriately, reducing the amount of memory available such
that it won't overlap the vmalloc area (which you've specified to be
a minimum of 256M.)

This should _NOT_ cause any memory corruption.

So, come on guys.  Debugging is *mandatory* for this kind of problem.
Papering over it is obscene.

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

On 31/07/12 23:01, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
>> On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
>>> I still fail to see how not having highmem enabled would ever cause memory
>>> corruption errors (unless something dealing with memory in a very very
>>> wrong way - iow, not using one of the reservation or memory allocation
>>> methods provided by the kernel.)
>>
>> The problem is that all users of ux500 systems pass a command line like
>>
>> vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M
>>
>> This is of course totally bogus and should not be done. If I understand
>> Lee correctly, one of the issues resulting from passing a command
>> line like this without enabling highmem is memory corruption.
>
> But the question is _why_ does that corruption happen.
>
>  From the above, we will end up with the kernel getting:
>
> 0x00000000 - 0x07ffffff (128M @ 0)
> 0x14800000 - 0x177fffff (48M  @ 328M)
> 0x18000000 - 0x3fffffff (640M @ 384M)
>
> with:
>
> 0x08000000 - 0x081fffff used for mali
> 0x0a000000 - 0x147fffff used for hwmem
> 0x17f00000 - 0x17ffffff used for mem_issw
>
> Now, with highmem disabled, the kernel should still map exactly the
> regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the
> direct mapped region, and truncate the 0x18000000 - 0x3fffffff
> region appropriately, reducing the amount of memory available such
> that it won't overlap the vmalloc area (which you've specified to be
> a minimum of 256M.)
>
> This should _NOT_ cause any memory corruption.
>
> So, come on guys.  Debugging is *mandatory* for this kind of problem.
> Papering over it is obscene.

Actually I didn't go any further with it, as I changed to another 
identical piece of hardware and couldn't reproduce the issue.

FYI, here's the boot log from the broken board:

http://paste.ubuntu.com/1102017/

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

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

On 31/07/12 23:01, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
>> On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
>>> I still fail to see how not having highmem enabled would ever cause memory
>>> corruption errors (unless something dealing with memory in a very very
>>> wrong way - iow, not using one of the reservation or memory allocation
>>> methods provided by the kernel.)
>>
>> The problem is that all users of ux500 systems pass a command line like
>>
>> vmalloc=256M mem=128M at 0 mali.mali_mem=32M at 128M hwmem=168M at 160M mem=48M at 328M mem_issw=1M at 383M mem=640M at 384M
>>
>> This is of course totally bogus and should not be done. If I understand
>> Lee correctly, one of the issues resulting from passing a command
>> line like this without enabling highmem is memory corruption.
>
> But the question is _why_ does that corruption happen.
>
>  From the above, we will end up with the kernel getting:
>
> 0x00000000 - 0x07ffffff (128M @ 0)
> 0x14800000 - 0x177fffff (48M  @ 328M)
> 0x18000000 - 0x3fffffff (640M @ 384M)
>
> with:
>
> 0x08000000 - 0x081fffff used for mali
> 0x0a000000 - 0x147fffff used for hwmem
> 0x17f00000 - 0x17ffffff used for mem_issw
>
> Now, with highmem disabled, the kernel should still map exactly the
> regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the
> direct mapped region, and truncate the 0x18000000 - 0x3fffffff
> region appropriately, reducing the amount of memory available such
> that it won't overlap the vmalloc area (which you've specified to be
> a minimum of 256M.)
>
> This should _NOT_ cause any memory corruption.
>
> So, come on guys.  Debugging is *mandatory* for this kind of problem.
> Papering over it is obscene.

Actually I didn't go any further with it, as I changed to another 
identical piece of hardware and couldn't reproduce the issue.

FYI, here's the boot log from the broken board:

http://paste.ubuntu.com/1102017/

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

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

On 31/07/12 23:01, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
>> On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
>>> I still fail to see how not having highmem enabled would ever cause memory
>>> corruption errors (unless something dealing with memory in a very very
>>> wrong way - iow, not using one of the reservation or memory allocation
>>> methods provided by the kernel.)
>>
>> The problem is that all users of ux500 systems pass a command line like
>>
>> vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M
>>
>> This is of course totally bogus and should not be done. If I understand
>> Lee correctly, one of the issues resulting from passing a command
>> line like this without enabling highmem is memory corruption.
>
> But the question is _why_ does that corruption happen.
>
>  From the above, we will end up with the kernel getting:
>
> 0x00000000 - 0x07ffffff (128M @ 0)
> 0x14800000 - 0x177fffff (48M  @ 328M)
> 0x18000000 - 0x3fffffff (640M @ 384M)
>
> with:
>
> 0x08000000 - 0x081fffff used for mali
> 0x0a000000 - 0x147fffff used for hwmem
> 0x17f00000 - 0x17ffffff used for mem_issw
>
> Now, with highmem disabled, the kernel should still map exactly the
> regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the
> direct mapped region, and truncate the 0x18000000 - 0x3fffffff
> region appropriately, reducing the amount of memory available such
> that it won't overlap the vmalloc area (which you've specified to be
> a minimum of 256M.)
>
> This should _NOT_ cause any memory corruption.
>
> So, come on guys.  Debugging is *mandatory* for this kind of problem.
> Papering over it is obscene.

Actually I didn't go any further with it, as I changed to another 
identical piece of hardware and couldn't reproduce the issue.

FYI, here's the boot log from the broken board:

http://paste.ubuntu.com/1102017/

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Russell King - ARM Linux]

On Wed, Aug 01, 2012 at 08:56:14AM +0100, Lee Jones wrote:
> On 31/07/12 23:01, Russell King - ARM Linux wrote:
>> On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
>>> On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
>>>> I still fail to see how not having highmem enabled would ever cause memory
>>>> corruption errors (unless something dealing with memory in a very very
>>>> wrong way - iow, not using one of the reservation or memory allocation
>>>> methods provided by the kernel.)
>>>
>>> The problem is that all users of ux500 systems pass a command line like
>>>
>>> vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M
>>>
>>> This is of course totally bogus and should not be done. If I understand
>>> Lee correctly, one of the issues resulting from passing a command
>>> line like this without enabling highmem is memory corruption.
>>
>> But the question is _why_ does that corruption happen.
>>
>>  From the above, we will end up with the kernel getting:
>>
>> 0x00000000 - 0x07ffffff (128M @ 0)
>> 0x14800000 - 0x177fffff (48M  @ 328M)
>> 0x18000000 - 0x3fffffff (640M @ 384M)
>>
>> with:
>>
>> 0x08000000 - 0x081fffff used for mali
>> 0x0a000000 - 0x147fffff used for hwmem
>> 0x17f00000 - 0x17ffffff used for mem_issw
>>
>> Now, with highmem disabled, the kernel should still map exactly the
>> regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the
>> direct mapped region, and truncate the 0x18000000 - 0x3fffffff
>> region appropriately, reducing the amount of memory available such
>> that it won't overlap the vmalloc area (which you've specified to be
>> a minimum of 256M.)
>>
>> This should _NOT_ cause any memory corruption.
>>
>> So, come on guys.  Debugging is *mandatory* for this kind of problem.
>> Papering over it is obscene.
>
> Actually I didn't go any further with it, as I changed to another  
> identical piece of hardware and couldn't reproduce the issue.
>
> FYI, here's the boot log from the broken board:
>
> http://paste.ubuntu.com/1102017/

Well, the good thing is this:

   8 Truncating RAM at 18000000-3fffffff to -2c3fffff (vmalloc region overlap).

which means the RAM was properly truncated before it is passed to
memblock, etc.

That oops dump looks very much like an ASoC problem, where
dapm_widget_power_check() recurses into dapm_supply_check_power()
which then recurses back into dapm_widget_power_check(), and it
eventually overflows the kernel stack, corrupting the thread_info
and the pages below.

Given the address of the stack pointer (ebc480a8) I don't think
we can be too sure where it was supposed to be, and where the top
of stack should have been, so we don't know how many pages have
been stomped on and corrupted.

Stopping that recursion is the first thing that needs to be done
so that the cause of it can then be properly debugged without the
kernel itself corrupting memory below the kernel stack.

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Russell King - ARM Linux]

On Wed, Aug 01, 2012 at 08:56:14AM +0100, Lee Jones wrote:
> On 31/07/12 23:01, Russell King - ARM Linux wrote:
>> On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
>>> On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
>>>> I still fail to see how not having highmem enabled would ever cause memory
>>>> corruption errors (unless something dealing with memory in a very very
>>>> wrong way - iow, not using one of the reservation or memory allocation
>>>> methods provided by the kernel.)
>>>
>>> The problem is that all users of ux500 systems pass a command line like
>>>
>>> vmalloc=256M mem=128M at 0 mali.mali_mem=32M at 128M hwmem=168M at 160M mem=48M at 328M mem_issw=1M at 383M mem=640M at 384M
>>>
>>> This is of course totally bogus and should not be done. If I understand
>>> Lee correctly, one of the issues resulting from passing a command
>>> line like this without enabling highmem is memory corruption.
>>
>> But the question is _why_ does that corruption happen.
>>
>>  From the above, we will end up with the kernel getting:
>>
>> 0x00000000 - 0x07ffffff (128M @ 0)
>> 0x14800000 - 0x177fffff (48M  @ 328M)
>> 0x18000000 - 0x3fffffff (640M @ 384M)
>>
>> with:
>>
>> 0x08000000 - 0x081fffff used for mali
>> 0x0a000000 - 0x147fffff used for hwmem
>> 0x17f00000 - 0x17ffffff used for mem_issw
>>
>> Now, with highmem disabled, the kernel should still map exactly the
>> regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the
>> direct mapped region, and truncate the 0x18000000 - 0x3fffffff
>> region appropriately, reducing the amount of memory available such
>> that it won't overlap the vmalloc area (which you've specified to be
>> a minimum of 256M.)
>>
>> This should _NOT_ cause any memory corruption.
>>
>> So, come on guys.  Debugging is *mandatory* for this kind of problem.
>> Papering over it is obscene.
>
> Actually I didn't go any further with it, as I changed to another  
> identical piece of hardware and couldn't reproduce the issue.
>
> FYI, here's the boot log from the broken board:
>
> http://paste.ubuntu.com/1102017/

Well, the good thing is this:

   8 Truncating RAM at 18000000-3fffffff to -2c3fffff (vmalloc region overlap).

which means the RAM was properly truncated before it is passed to
memblock, etc.

That oops dump looks very much like an ASoC problem, where
dapm_widget_power_check() recurses into dapm_supply_check_power()
which then recurses back into dapm_widget_power_check(), and it
eventually overflows the kernel stack, corrupting the thread_info
and the pages below.

Given the address of the stack pointer (ebc480a8) I don't think
we can be too sure where it was supposed to be, and where the top
of stack should have been, so we don't know how many pages have
been stomped on and corrupted.

Stopping that recursion is the first thing that needs to be done
so that the cause of it can then be properly debugged without the
kernel itself corrupting memory below the kernel stack.

Re: [PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

On 01/08/12 09:41, Russell King - ARM Linux wrote:
> On Wed, Aug 01, 2012 at 08:56:14AM +0100, Lee Jones wrote:
>> On 31/07/12 23:01, Russell King - ARM Linux wrote:
>>> On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
>>>> On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
>>>>> I still fail to see how not having highmem enabled would ever cause memory
>>>>> corruption errors (unless something dealing with memory in a very very
>>>>> wrong way - iow, not using one of the reservation or memory allocation
>>>>> methods provided by the kernel.)
>>>>
>>>> The problem is that all users of ux500 systems pass a command line like
>>>>
>>>> vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M
>>>>
>>>> This is of course totally bogus and should not be done. If I understand
>>>> Lee correctly, one of the issues resulting from passing a command
>>>> line like this without enabling highmem is memory corruption.
>>>
>>> But the question is _why_ does that corruption happen.
>>>
>>>   From the above, we will end up with the kernel getting:
>>>
>>> 0x00000000 - 0x07ffffff (128M @ 0)
>>> 0x14800000 - 0x177fffff (48M  @ 328M)
>>> 0x18000000 - 0x3fffffff (640M @ 384M)
>>>
>>> with:
>>>
>>> 0x08000000 - 0x081fffff used for mali
>>> 0x0a000000 - 0x147fffff used for hwmem
>>> 0x17f00000 - 0x17ffffff used for mem_issw
>>>
>>> Now, with highmem disabled, the kernel should still map exactly the
>>> regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the
>>> direct mapped region, and truncate the 0x18000000 - 0x3fffffff
>>> region appropriately, reducing the amount of memory available such
>>> that it won't overlap the vmalloc area (which you've specified to be
>>> a minimum of 256M.)
>>>
>>> This should _NOT_ cause any memory corruption.
>>>
>>> So, come on guys.  Debugging is *mandatory* for this kind of problem.
>>> Papering over it is obscene.
>>
>> Actually I didn't go any further with it, as I changed to another
>> identical piece of hardware and couldn't reproduce the issue.
>>
>> FYI, here's the boot log from the broken board:
>>
>> http://paste.ubuntu.com/1102017/
>
> Well, the good thing is this:
>
>     8 Truncating RAM at 18000000-3fffffff to -2c3fffff (vmalloc region overlap).
>
> which means the RAM was properly truncated before it is passed to
> memblock, etc.
>
> That oops dump looks very much like an ASoC problem, where
> dapm_widget_power_check() recurses into dapm_supply_check_power()
> which then recurses back into dapm_widget_power_check(), and it
> eventually overflows the kernel stack, corrupting the thread_info
> and the pages below.
>
> Given the address of the stack pointer (ebc480a8) I don't think
> we can be too sure where it was supposed to be, and where the top
> of stack should have been, so we don't know how many pages have
> been stomped on and corrupted.
>
> Stopping that recursion is the first thing that needs to be done
> so that the cause of it can then be properly debugged without the
> kernel itself corrupting memory below the kernel stack.

Those were my thoughts.

Here was my cry for help: https://lkml.org/lkml/2012/7/23/181

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

[PATCH 5/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

On 01/08/12 09:41, Russell King - ARM Linux wrote:
> On Wed, Aug 01, 2012 at 08:56:14AM +0100, Lee Jones wrote:
>> On 31/07/12 23:01, Russell King - ARM Linux wrote:
>>> On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
>>>> On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
>>>>> I still fail to see how not having highmem enabled would ever cause memory
>>>>> corruption errors (unless something dealing with memory in a very very
>>>>> wrong way - iow, not using one of the reservation or memory allocation
>>>>> methods provided by the kernel.)
>>>>
>>>> The problem is that all users of ux500 systems pass a command line like
>>>>
>>>> vmalloc=256M mem=128M at 0 mali.mali_mem=32M at 128M hwmem=168M at 160M mem=48M at 328M mem_issw=1M at 383M mem=640M at 384M
>>>>
>>>> This is of course totally bogus and should not be done. If I understand
>>>> Lee correctly, one of the issues resulting from passing a command
>>>> line like this without enabling highmem is memory corruption.
>>>
>>> But the question is _why_ does that corruption happen.
>>>
>>>   From the above, we will end up with the kernel getting:
>>>
>>> 0x00000000 - 0x07ffffff (128M @ 0)
>>> 0x14800000 - 0x177fffff (48M  @ 328M)
>>> 0x18000000 - 0x3fffffff (640M @ 384M)
>>>
>>> with:
>>>
>>> 0x08000000 - 0x081fffff used for mali
>>> 0x0a000000 - 0x147fffff used for hwmem
>>> 0x17f00000 - 0x17ffffff used for mem_issw
>>>
>>> Now, with highmem disabled, the kernel should still map exactly the
>>> regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the
>>> direct mapped region, and truncate the 0x18000000 - 0x3fffffff
>>> region appropriately, reducing the amount of memory available such
>>> that it won't overlap the vmalloc area (which you've specified to be
>>> a minimum of 256M.)
>>>
>>> This should _NOT_ cause any memory corruption.
>>>
>>> So, come on guys.  Debugging is *mandatory* for this kind of problem.
>>> Papering over it is obscene.
>>
>> Actually I didn't go any further with it, as I changed to another
>> identical piece of hardware and couldn't reproduce the issue.
>>
>> FYI, here's the boot log from the broken board:
>>
>> http://paste.ubuntu.com/1102017/
>
> Well, the good thing is this:
>
>     8 Truncating RAM at 18000000-3fffffff to -2c3fffff (vmalloc region overlap).
>
> which means the RAM was properly truncated before it is passed to
> memblock, etc.
>
> That oops dump looks very much like an ASoC problem, where
> dapm_widget_power_check() recurses into dapm_supply_check_power()
> which then recurses back into dapm_widget_power_check(), and it
> eventually overflows the kernel stack, corrupting the thread_info
> and the pages below.
>
> Given the address of the stack pointer (ebc480a8) I don't think
> we can be too sure where it was supposed to be, and where the top
> of stack should have been, so we don't know how many pages have
> been stomped on and corrupted.
>
> Stopping that recursion is the first thing that needs to be done
> so that the cause of it can then be properly debugged without the
> kernel itself corrupting memory below the kernel stack.

Those were my thoughts.

Here was my cry for help: https://lkml.org/lkml/2012/7/23/181

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

[PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Lee Jones]

Previous attempts to add platform probing of the Audio related devices
only call from non-DT initialisation functions. This patch extends that
functionality to the Device Tree related ones too.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index e641003..87a5cd7 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -794,6 +794,7 @@ static void __init u8500_init_machine(void)
 				ARRAY_SIZE(mop500_platform_devs));
 
 		mop500_sdi_init(parent);
+		mop500_msp_init(parent);
 		i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 		i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
 		i2c_register_board_info(2, mop500_i2c2_devices,
@@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
 
 		mop500_uib_init();
 
+	} else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {
+		mop500_of_msp_init(parent);
 	} else if (of_machine_is_compatible("st-ericsson,hrefv60+")) {
 		/*
 		 * The HREFv60 board removed a GPIO expander and routed
@@ -812,6 +815,7 @@ static void __init u8500_init_machine(void)
 				ARRAY_SIZE(mop500_platform_devs));
 
 		hrefv60_sdi_init(parent);
+		mop500_msp_init(parent);
 
 		i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 		i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;
-- 
1.7.9.5

[PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Lee Jones]

Previous attempts to add platform probing of the Audio related devices
only call from non-DT initialisation functions. This patch extends that
functionality to the Device Tree related ones too.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index e641003..87a5cd7 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -794,6 +794,7 @@ static void __init u8500_init_machine(void)
 				ARRAY_SIZE(mop500_platform_devs));
 
 		mop500_sdi_init(parent);
+		mop500_msp_init(parent);
 		i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 		i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
 		i2c_register_board_info(2, mop500_i2c2_devices,
@@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
 
 		mop500_uib_init();
 
+	} else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {
+		mop500_of_msp_init(parent);
 	} else if (of_machine_is_compatible("st-ericsson,hrefv60+")) {
 		/*
 		 * The HREFv60 board removed a GPIO expander and routed
@@ -812,6 +815,7 @@ static void __init u8500_init_machine(void)
 				ARRAY_SIZE(mop500_platform_devs));
 
 		hrefv60_sdi_init(parent);
+		mop500_msp_init(parent);
 
 		i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 		i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;
-- 
1.7.9.5

Re: [PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Arnd Bergmann]

On Tuesday 31 July 2012, Lee Jones wrote:
> diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
> index e641003..87a5cd7 100644
> --- a/arch/arm/mach-ux500/board-mop500.c
> +++ b/arch/arm/mach-ux500/board-mop500.c
> @@ -794,6 +794,7 @@ static void __init u8500_init_machine(void)
>                                 ARRAY_SIZE(mop500_platform_devs));
>  
>                 mop500_sdi_init(parent);
> +               mop500_msp_init(parent);
>                 i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
>                 i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
>                 i2c_register_board_info(2, mop500_i2c2_devices,
> @@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
>  
>                 mop500_uib_init();
>  
> +       } else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {
> +               mop500_of_msp_init(parent);
>         } else if (of_machine_is_compatible("st-ericsson,hrefv60+")) {
>                 /*
>                  * The HREFv60 board removed a GPIO expander and routed
> @@ -812,6 +815,7 @@ static void __init u8500_init_machine(void)
>                                 ARRAY_SIZE(mop500_platform_devs));
>  
>                 hrefv60_sdi_init(parent);
> +               mop500_msp_init(parent);
>  
>                 i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
>                 i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;


Looks like you're adding the same call to each of the three cases:
mop500, snowball and hrefv60+. How about moving it before or after
the if/elseif block?

	Arnd

[PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Arnd Bergmann]

On Tuesday 31 July 2012, Lee Jones wrote:
> diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
> index e641003..87a5cd7 100644
> --- a/arch/arm/mach-ux500/board-mop500.c
> +++ b/arch/arm/mach-ux500/board-mop500.c
> @@ -794,6 +794,7 @@ static void __init u8500_init_machine(void)
>                                 ARRAY_SIZE(mop500_platform_devs));
>  
>                 mop500_sdi_init(parent);
> +               mop500_msp_init(parent);
>                 i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
>                 i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
>                 i2c_register_board_info(2, mop500_i2c2_devices,
> @@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
>  
>                 mop500_uib_init();
>  
> +       } else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {
> +               mop500_of_msp_init(parent);
>         } else if (of_machine_is_compatible("st-ericsson,hrefv60+")) {
>                 /*
>                  * The HREFv60 board removed a GPIO expander and routed
> @@ -812,6 +815,7 @@ static void __init u8500_init_machine(void)
>                                 ARRAY_SIZE(mop500_platform_devs));
>  
>                 hrefv60_sdi_init(parent);
> +               mop500_msp_init(parent);
>  
>                 i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
>                 i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;


Looks like you're adding the same call to each of the three cases:
mop500, snowball and hrefv60+. How about moving it before or after
the if/elseif block?

	Arnd

Re: [PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Lee Jones]

On 31/07/12 21:54, Arnd Bergmann wrote:
> On Tuesday 31 July 2012, Lee Jones wrote:
>> diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
>> index e641003..87a5cd7 100644
>> --- a/arch/arm/mach-ux500/board-mop500.c
>> +++ b/arch/arm/mach-ux500/board-mop500.c
>> @@ -794,6 +794,7 @@ static void __init u8500_init_machine(void)
>>                                  ARRAY_SIZE(mop500_platform_devs));
>>
>>                  mop500_sdi_init(parent);
>> +               mop500_msp_init(parent);
>>                  i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
>>                  i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
>>                  i2c_register_board_info(2, mop500_i2c2_devices,
>> @@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
>>
>>                  mop500_uib_init();
>>
>> +       } else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {
>> +               mop500_of_msp_init(parent);
>>          } else if (of_machine_is_compatible("st-ericsson,hrefv60+")) {
>>                  /*
>>                   * The HREFv60 board removed a GPIO expander and routed
>> @@ -812,6 +815,7 @@ static void __init u8500_init_machine(void)
>>                                  ARRAY_SIZE(mop500_platform_devs));
>>
>>                  hrefv60_sdi_init(parent);
>> +               mop500_msp_init(parent);
>>
>>                  i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
>>                  i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;
>
>
> Looks like you're adding the same call to each of the three cases:
> mop500, snowball and hrefv60+. How about moving it before or after
> the if/elseif block?

The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.

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

[PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Lee Jones]

On 31/07/12 21:54, Arnd Bergmann wrote:
> On Tuesday 31 July 2012, Lee Jones wrote:
>> diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
>> index e641003..87a5cd7 100644
>> --- a/arch/arm/mach-ux500/board-mop500.c
>> +++ b/arch/arm/mach-ux500/board-mop500.c
>> @@ -794,6 +794,7 @@ static void __init u8500_init_machine(void)
>>                                  ARRAY_SIZE(mop500_platform_devs));
>>
>>                  mop500_sdi_init(parent);
>> +               mop500_msp_init(parent);
>>                  i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
>>                  i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
>>                  i2c_register_board_info(2, mop500_i2c2_devices,
>> @@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
>>
>>                  mop500_uib_init();
>>
>> +       } else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {
>> +               mop500_of_msp_init(parent);
>>          } else if (of_machine_is_compatible("st-ericsson,hrefv60+")) {
>>                  /*
>>                   * The HREFv60 board removed a GPIO expander and routed
>> @@ -812,6 +815,7 @@ static void __init u8500_init_machine(void)
>>                                  ARRAY_SIZE(mop500_platform_devs));
>>
>>                  hrefv60_sdi_init(parent);
>> +               mop500_msp_init(parent);
>>
>>                  i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
>>                  i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;
>
>
> Looks like you're adding the same call to each of the three cases:
> mop500, snowball and hrefv60+. How about moving it before or after
> the if/elseif block?

The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.

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

Re: [PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Lee Jones]

On 31/07/12 21:54, Arnd Bergmann wrote:
> On Tuesday 31 July 2012, Lee Jones wrote:
>> diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
>> index e641003..87a5cd7 100644
>> --- a/arch/arm/mach-ux500/board-mop500.c
>> +++ b/arch/arm/mach-ux500/board-mop500.c
>> @@ -794,6 +794,7 @@ static void __init u8500_init_machine(void)
>>                                  ARRAY_SIZE(mop500_platform_devs));
>>
>>                  mop500_sdi_init(parent);
>> +               mop500_msp_init(parent);
>>                  i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
>>                  i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
>>                  i2c_register_board_info(2, mop500_i2c2_devices,
>> @@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
>>
>>                  mop500_uib_init();
>>
>> +       } else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {
>> +               mop500_of_msp_init(parent);
>>          } else if (of_machine_is_compatible("st-ericsson,hrefv60+")) {
>>                  /*
>>                   * The HREFv60 board removed a GPIO expander and routed
>> @@ -812,6 +815,7 @@ static void __init u8500_init_machine(void)
>>                                  ARRAY_SIZE(mop500_platform_devs));
>>
>>                  hrefv60_sdi_init(parent);
>> +               mop500_msp_init(parent);
>>
>>                  i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
>>                  i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;
>
>
> Looks like you're adding the same call to each of the three cases:
> mop500, snowball and hrefv60+. How about moving it before or after
> the if/elseif block?

The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Arnd Bergmann]

On Wednesday 01 August 2012, Lee Jones wrote:
> > Looks like you're adding the same call to each of the three cases:
> > mop500, snowball and hrefv60+. How about moving it before or after
> > the if/elseif block?
> 
> The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.

Ah, I see. Is there a problem in always using mop500_of_msp_init then?
I would guess that this just means you'd have to put the msp into the
device tree files, which don't yet exist for mop500 and hrefv60.

	Arnd

[PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Arnd Bergmann]

On Wednesday 01 August 2012, Lee Jones wrote:
> > Looks like you're adding the same call to each of the three cases:
> > mop500, snowball and hrefv60+. How about moving it before or after
> > the if/elseif block?
> 
> The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.

Ah, I see. Is there a problem in always using mop500_of_msp_init then?
I would guess that this just means you'd have to put the msp into the
device tree files, which don't yet exist for mop500 and hrefv60.

	Arnd

Re: [PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Arnd Bergmann]

On Wednesday 01 August 2012, Lee Jones wrote:
> > Looks like you're adding the same call to each of the three cases:
> > mop500, snowball and hrefv60+. How about moving it before or after
> > the if/elseif block?
> 
> The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.

Ah, I see. Is there a problem in always using mop500_of_msp_init then?
I would guess that this just means you'd have to put the msp into the
device tree files, which don't yet exist for mop500 and hrefv60.

	Arnd

Re: [PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Lee Jones]

On 01/08/12 14:32, Arnd Bergmann wrote:
> On Wednesday 01 August 2012, Lee Jones wrote:
>>> Looks like you're adding the same call to each of the three cases:
>>> mop500, snowball and hrefv60+. How about moving it before or after
>>> the if/elseif block?
>>
>> The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.
>
> Ah, I see. Is there a problem in always using mop500_of_msp_init then?
> I would guess that this just means you'd have to put the msp into the
> device tree files, which don't yet exist for mop500 and hrefv60.

Although that is true, it's only part of the issue.

mop500_of_msp_init is only a temporary solution to aid in step-by-step 
enablement of DT for audio. By the end of the patch-set it has been 
removed (along with the call to it if 'of_machine_is_compatible 
("calaosystems,snowball-a9500")'. What I can do it write a patch to 
consolidate the calls _after_ "ARM: ux500: Remove platform registration 
of MSP devices", as a bolt-on.


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

[PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Lee Jones]

On 01/08/12 14:32, Arnd Bergmann wrote:
> On Wednesday 01 August 2012, Lee Jones wrote:
>>> Looks like you're adding the same call to each of the three cases:
>>> mop500, snowball and hrefv60+. How about moving it before or after
>>> the if/elseif block?
>>
>> The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.
>
> Ah, I see. Is there a problem in always using mop500_of_msp_init then?
> I would guess that this just means you'd have to put the msp into the
> device tree files, which don't yet exist for mop500 and hrefv60.

Although that is true, it's only part of the issue.

mop500_of_msp_init is only a temporary solution to aid in step-by-step 
enablement of DT for audio. By the end of the patch-set it has been 
removed (along with the call to it if 'of_machine_is_compatible 
("calaosystems,snowball-a9500")'. What I can do it write a patch to 
consolidate the calls _after_ "ARM: ux500: Remove platform registration 
of MSP devices", as a bolt-on.


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

Re: [PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Lee Jones]

On 01/08/12 14:32, Arnd Bergmann wrote:
> On Wednesday 01 August 2012, Lee Jones wrote:
>>> Looks like you're adding the same call to each of the three cases:
>>> mop500, snowball and hrefv60+. How about moving it before or after
>>> the if/elseif block?
>>
>> The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.
>
> Ah, I see. Is there a problem in always using mop500_of_msp_init then?
> I would guess that this just means you'd have to put the msp into the
> device tree files, which don't yet exist for mop500 and hrefv60.

Although that is true, it's only part of the issue.

mop500_of_msp_init is only a temporary solution to aid in step-by-step 
enablement of DT for audio. By the end of the patch-set it has been 
removed (along with the call to it if 'of_machine_is_compatible 
("calaosystems,snowball-a9500")'. What I can do it write a patch to 
consolidate the calls _after_ "ARM: ux500: Remove platform registration 
of MSP devices", as a bolt-on.


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Arnd Bergmann]

On Wednesday 01 August 2012, Lee Jones wrote:
> 
> On 01/08/12 14:32, Arnd Bergmann wrote:
> > On Wednesday 01 August 2012, Lee Jones wrote:
> >>> Looks like you're adding the same call to each of the three cases:
> >>> mop500, snowball and hrefv60+. How about moving it before or after
> >>> the if/elseif block?
> >>
> >> The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.
> >
> > Ah, I see. Is there a problem in always using mop500_of_msp_init then?
> > I would guess that this just means you'd have to put the msp into the
> > device tree files, which don't yet exist for mop500 and hrefv60.
> 
> Although that is true, it's only part of the issue.
> 
> mop500_of_msp_init is only a temporary solution to aid in step-by-step 
> enablement of DT for audio. By the end of the patch-set it has been 
> removed (along with the call to it if 'of_machine_is_compatible 
> ("calaosystems,snowball-a9500")'. What I can do it write a patch to 
> consolidate the calls after "ARM: ux500: Remove platform registration 
> of MSP devices", as a bolt-on.

I see. Your solution looks fine then.

	Arnd

[PATCH 5/6] ARM: ux500: Ensure probing of Audio devices when Device Tree is enabled

[Arnd Bergmann]

On Wednesday 01 August 2012, Lee Jones wrote:
> 
> On 01/08/12 14:32, Arnd Bergmann wrote:
> > On Wednesday 01 August 2012, Lee Jones wrote:
> >>> Looks like you're adding the same call to each of the three cases:
> >>> mop500, snowball and hrefv60+. How about moving it before or after
> >>> the if/elseif block?
> >>
> >> The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.
> >
> > Ah, I see. Is there a problem in always using mop500_of_msp_init then?
> > I would guess that this just means you'd have to put the msp into the
> > device tree files, which don't yet exist for mop500 and hrefv60.
> 
> Although that is true, it's only part of the issue.
> 
> mop500_of_msp_init is only a temporary solution to aid in step-by-step 
> enablement of DT for audio. By the end of the patch-set it has been 
> removed (along with the call to it if 'of_machine_is_compatible 
> ("calaosystems,snowball-a9500")'. What I can do it write a patch to 
> consolidate the calls after "ARM: ux500: Remove platform registration 
> of MSP devices", as a bolt-on.

I see. Your solution looks fine then.

	Arnd

[PATCH 6/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

The current kernel commandline for ux500 based devices includes
hard-coded allocations for things like mali and hwmem, which
actually run over lowmem. Here we enable highmem in order to
avoid memory corruption errors.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index c013bbf..f51c351 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -28,6 +28,7 @@ config MACH_MOP500
 	select I2C
 	select I2C_NOMADIK
 	select SOC_BUS
+	select HIGHMEM
 	help
 	  Include support for the MOP500 development platform.
 
-- 
1.7.9.5

[PATCH 6/6] ARM: ux500: Enable HIGHMEM on all mop500 platforms

[Lee Jones]

The current kernel commandline for ux500 based devices includes
hard-coded allocations for things like mali and hwmem, which
actually run over lowmem. Here we enable highmem in order to
avoid memory corruption errors.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index c013bbf..f51c351 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -28,6 +28,7 @@ config MACH_MOP500
 	select I2C
 	select I2C_NOMADIK
 	select SOC_BUS
+	select HIGHMEM
 	help
 	  Include support for the MOP500 development platform.
 
-- 
1.7.9.5

[PATCH 6/6] ASoC: Ux500: Move MSP pinctrl setup into the MSP driver

[Lee Jones]

In the initial submission of the MSP driver msp1 and msp3's associated
pinctrl mechanism was passed back to platform code using a plat_init()
call-back routine, but it has no place in platform code. The MSP driver
should set this up for the appropriate ports. Instead we use a use_pinctrl
identifier which is passed from platform_data/Device Tree which indicates
which ports should use pinctrl.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-msp.c |   75 +-------------------------------
 arch/arm/mach-ux500/include/mach/msp.h |    2 -
 sound/soc/ux500/ux500_msp_i2s.c        |   75 +++++++++++++++++++++++++-------
 sound/soc/ux500/ux500_msp_i2s.h        |    2 -
 4 files changed, 60 insertions(+), 94 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
index 1b6a193..af65fb0 100644
--- a/arch/arm/mach-ux500/board-mop500-msp.c
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -23,53 +23,6 @@
 #include "devices-db8500.h"
 #include "pins-db8500.h"
 
-/* MSP1/3 Tx/Rx usage protection */
-static DEFINE_SPINLOCK(msp_rxtx_lock);
-
-/* Reference Count */
-static int msp_rxtx_ref;
-
-/* Pin modes */
-struct pinctrl *msp1_p;
-struct pinctrl_state *msp1_def;
-struct pinctrl_state *msp1_sleep;
-
-int msp13_i2s_init(void)
-{
-	int retval = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&msp_rxtx_lock, flags);
-	if (msp_rxtx_ref == 0 && !(IS_ERR(msp1_p) || IS_ERR(msp1_def))) {
-		retval = pinctrl_select_state(msp1_p, msp1_def);
-		if (retval)
-			pr_err("could not set MSP1 defstate\n");
-	}
-	if (!retval)
-		msp_rxtx_ref++;
-	spin_unlock_irqrestore(&msp_rxtx_lock, flags);
-
-	return retval;
-}
-
-int msp13_i2s_exit(void)
-{
-	int retval = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&msp_rxtx_lock, flags);
-	WARN_ON(!msp_rxtx_ref);
-	msp_rxtx_ref--;
-	if (msp_rxtx_ref == 0 && !(IS_ERR(msp1_p) || IS_ERR(msp1_sleep))) {
-		retval = pinctrl_select_state(msp1_p, msp1_sleep);
-		if (retval)
-			pr_err("could not set MSP1 sleepstate\n");
-	}
-	spin_unlock_irqrestore(&msp_rxtx_lock, flags);
-
-	return retval;
-}
-
 static struct stedma40_chan_cfg msp0_dma_rx = {
 	.high_priority = true,
 	.dir = STEDMA40_PERIPH_TO_MEM,
@@ -132,8 +85,6 @@ static struct msp_i2s_platform_data msp1_platform_data = {
 	.id = MSP_I2S_1,
 	.msp_i2s_dma_rx = NULL,
 	.msp_i2s_dma_tx = &msp1_dma_tx,
-	.msp_i2s_init = msp13_i2s_init,
-	.msp_i2s_exit = msp13_i2s_exit,
 };
 
 static struct stedma40_chan_cfg msp2_dma_rx = {
@@ -219,47 +170,23 @@ static struct msp_i2s_platform_data msp3_platform_data = {
 	.id		= MSP_I2S_3,
 	.msp_i2s_dma_rx	= &msp1_dma_rx,
 	.msp_i2s_dma_tx	= NULL,
-	.msp_i2s_init = msp13_i2s_init,
-	.msp_i2s_exit = msp13_i2s_exit,
 };
 
 int mop500_msp_init(struct device *parent)
 {
-	struct platform_device *msp1;
-
 	pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
 	platform_device_register(&snd_soc_mop500);
 
 	pr_info("Initialize MSP I2S-devices.\n");
 	db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
 			   &msp0_platform_data);
-	msp1 = db8500_add_msp_i2s(parent, 1, U8500_MSP1_BASE, IRQ_DB8500_MSP1,
+	db8500_add_msp_i2s(parent, 1, U8500_MSP1_BASE, IRQ_DB8500_MSP1,
 			   &msp1_platform_data);
 	db8500_add_msp_i2s(parent, 2, U8500_MSP2_BASE, IRQ_DB8500_MSP2,
 			   &msp2_platform_data);
 	db8500_add_msp_i2s(parent, 3, U8500_MSP3_BASE, IRQ_DB8500_MSP1,
 			   &msp3_platform_data);
 
-	/* Get the pinctrl handle for MSP1 */
-	if (msp1) {
-		msp1_p = pinctrl_get(&msp1->dev);
-		if (IS_ERR(msp1_p))
-			dev_err(&msp1->dev, "could not get MSP1 pinctrl\n");
-		else {
-			msp1_def = pinctrl_lookup_state(msp1_p,
-							PINCTRL_STATE_DEFAULT);
-			if (IS_ERR(msp1_def)) {
-				dev_err(&msp1->dev,
-					"could not get MSP1 defstate\n");
-			}
-			msp1_sleep = pinctrl_lookup_state(msp1_p,
-							  PINCTRL_STATE_SLEEP);
-			if (IS_ERR(msp1_sleep))
-				dev_err(&msp1->dev,
-					"could not get MSP1 idlestate\n");
-		}
-	}
-
 	pr_info("%s: Register platform-device 'ux500-pcm'\n", __func__);
 	platform_device_register(&ux500_pcm);
 
diff --git a/arch/arm/mach-ux500/include/mach/msp.h b/arch/arm/mach-ux500/include/mach/msp.h
index 798be19..3cc7142 100644
--- a/arch/arm/mach-ux500/include/mach/msp.h
+++ b/arch/arm/mach-ux500/include/mach/msp.h
@@ -22,8 +22,6 @@ struct msp_i2s_platform_data {
 	enum msp_i2s_id id;
 	struct stedma40_chan_cfg *msp_i2s_dma_rx;
 	struct stedma40_chan_cfg *msp_i2s_dma_tx;
-	int (*msp_i2s_init) (void);
-	int (*msp_i2s_exit) (void);
 };
 
 #endif
diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c
index 36be11e..2cbfc54 100644
--- a/sound/soc/ux500/ux500_msp_i2s.c
+++ b/sound/soc/ux500/ux500_msp_i2s.c
@@ -15,6 +15,7 @@
 
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
 
@@ -25,6 +26,17 @@
 
 #include "ux500_msp_i2s.h"
 
+/* MSP1/3 Tx/Rx usage protection */
+static DEFINE_SPINLOCK(msp_rxtx_lock);
+
+/* Pin modes */
+struct pinctrl *pinctrl_p;
+struct pinctrl_state *pinctrl_def;
+struct pinctrl_state *pinctrl_sleep;
+
+/* Reference Count */
+int pinctrl_rxtx_ref;
+
  /* Protocol desciptors */
 static const struct msp_protdesc prot_descs[] = {
 	{ /* I2S */
@@ -352,17 +364,23 @@ static int configure_multichannel(struct ux500_msp *msp,
 
 static int enable_msp(struct ux500_msp *msp, struct ux500_msp_config *config)
 {
-	int status = 0;
+	int status = 0, retval = 0;
 	u32 reg_val_DMACR, reg_val_GCR;
+	unsigned long flags;
 
 	/* Check msp state whether in RUN or CONFIGURED Mode */
-	if ((msp->msp_state == MSP_STATE_IDLE) && (msp->plat_init)) {
-		status = msp->plat_init();
-		if (status) {
-			dev_err(msp->dev, "%s: ERROR: Failed to init MSP (%d)!\n",
-				__func__, status);
-			return status;
+	if (msp->msp_state == MSP_STATE_IDLE) {
+		spin_lock_irqsave(&msp_rxtx_lock, flags);
+		if (pinctrl_rxtx_ref == 0 &&
+			!(IS_ERR(pinctrl_p) || IS_ERR(pinctrl_def))) {
+			retval = pinctrl_select_state(pinctrl_p,
+						pinctrl_def);
+			if (retval)
+				pr_err("could not set MSP defstate\n");
 		}
+		if (!retval)
+			pinctrl_rxtx_ref++;
+		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
 	}
 
 	/* Configure msp with protocol dependent settings */
@@ -620,7 +638,8 @@ int ux500_msp_i2s_trigger(struct ux500_msp *msp, int cmd, int direction)
 
 int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
 {
-	int status = 0;
+	int status = 0, retval = 0;
+	unsigned long flags;
 
 	dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
 
@@ -631,12 +650,19 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
 		writel((readl(msp->registers + MSP_GCR) &
 			       (~(FRAME_GEN_ENABLE | SRG_ENABLE))),
 			      msp->registers + MSP_GCR);
-		if (msp->plat_exit)
-			status = msp->plat_exit();
-			if (status)
-				dev_warn(msp->dev,
-					"%s: WARN: ux500_msp_i2s_exit failed (%d)!\n",
-					__func__, status);
+
+		spin_lock_irqsave(&msp_rxtx_lock, flags);
+		WARN_ON(!pinctrl_rxtx_ref);
+		pinctrl_rxtx_ref--;
+		if (pinctrl_rxtx_ref == 0 &&
+			!(IS_ERR(pinctrl_p) || IS_ERR(pinctrl_sleep))) {
+			retval = pinctrl_select_state(pinctrl_p,
+						pinctrl_sleep);
+			if (retval)
+				pr_err("could not set MSP sleepstate\n");
+		}
+		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
+
 		writel(0, msp->registers + MSP_GCR);
 		writel(0, msp->registers + MSP_TCF);
 		writel(0, msp->registers + MSP_RCF);
@@ -678,8 +704,6 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev,
 
 	msp->id = platform_data->id;
 	msp->dev = &pdev->dev;
-	msp->plat_init = platform_data->msp_i2s_init;
-	msp->plat_exit = platform_data->msp_i2s_exit;
 	msp->dma_cfg_rx = platform_data->msp_i2s_dma_rx;
 	msp->dma_cfg_tx = platform_data->msp_i2s_dma_tx;
 
@@ -717,6 +741,25 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev,
 	dev_dbg(&pdev->dev, "I2S device-name: '%s'\n", i2s_cont->name);
 	msp->i2s_cont = i2s_cont;
 
+	pinctrl_p = pinctrl_get(msp->dev);
+	if (IS_ERR(pinctrl_p))
+		dev_err(&pdev->dev, "could not get MSP pinctrl\n");
+	else {
+		pinctrl_def = pinctrl_lookup_state(pinctrl_p,
+						PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(pinctrl_def)) {
+			dev_err(&pdev->dev,
+				"could not get MSP defstate (%li)\n",
+				PTR_ERR(pinctrl_def));
+		}
+		pinctrl_sleep = pinctrl_lookup_state(pinctrl_p,
+						PINCTRL_STATE_SLEEP);
+		if (IS_ERR(pinctrl_sleep))
+			dev_err(&pdev->dev,
+				"could not get MSP idlestate (%li)\n",
+				PTR_ERR(pinctrl_def));
+	}
+
 	return 0;
 
 err_i2s_cont:
diff --git a/sound/soc/ux500/ux500_msp_i2s.h b/sound/soc/ux500/ux500_msp_i2s.h
index 2d9136d..1ce336f 100644
--- a/sound/soc/ux500/ux500_msp_i2s.h
+++ b/sound/soc/ux500/ux500_msp_i2s.h
@@ -524,8 +524,6 @@ struct ux500_msp {
 	struct dma_chan *rx_pipeid;
 	enum msp_state msp_state;
 	int (*transfer) (struct ux500_msp *msp, struct i2s_message *message);
-	int (*plat_init) (void);
-	int (*plat_exit) (void);
 	struct timer_list notify_timer;
 	int def_elem_len;
 	unsigned int dir_busy;
-- 
1.7.9.5

[PATCH 6/6] ASoC: Ux500: Move MSP pinctrl setup into the MSP driver

[Lee Jones]

In the initial submission of the MSP driver msp1 and msp3's associated
pinctrl mechanism was passed back to platform code using a plat_init()
call-back routine, but it has no place in platform code. The MSP driver
should set this up for the appropriate ports. Instead we use a use_pinctrl
identifier which is passed from platform_data/Device Tree which indicates
which ports should use pinctrl.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-msp.c |   75 +-------------------------------
 arch/arm/mach-ux500/include/mach/msp.h |    2 -
 sound/soc/ux500/ux500_msp_i2s.c        |   75 +++++++++++++++++++++++++-------
 sound/soc/ux500/ux500_msp_i2s.h        |    2 -
 4 files changed, 60 insertions(+), 94 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c
index 1b6a193..af65fb0 100644
--- a/arch/arm/mach-ux500/board-mop500-msp.c
+++ b/arch/arm/mach-ux500/board-mop500-msp.c
@@ -23,53 +23,6 @@
 #include "devices-db8500.h"
 #include "pins-db8500.h"
 
-/* MSP1/3 Tx/Rx usage protection */
-static DEFINE_SPINLOCK(msp_rxtx_lock);
-
-/* Reference Count */
-static int msp_rxtx_ref;
-
-/* Pin modes */
-struct pinctrl *msp1_p;
-struct pinctrl_state *msp1_def;
-struct pinctrl_state *msp1_sleep;
-
-int msp13_i2s_init(void)
-{
-	int retval = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&msp_rxtx_lock, flags);
-	if (msp_rxtx_ref == 0 && !(IS_ERR(msp1_p) || IS_ERR(msp1_def))) {
-		retval = pinctrl_select_state(msp1_p, msp1_def);
-		if (retval)
-			pr_err("could not set MSP1 defstate\n");
-	}
-	if (!retval)
-		msp_rxtx_ref++;
-	spin_unlock_irqrestore(&msp_rxtx_lock, flags);
-
-	return retval;
-}
-
-int msp13_i2s_exit(void)
-{
-	int retval = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&msp_rxtx_lock, flags);
-	WARN_ON(!msp_rxtx_ref);
-	msp_rxtx_ref--;
-	if (msp_rxtx_ref == 0 && !(IS_ERR(msp1_p) || IS_ERR(msp1_sleep))) {
-		retval = pinctrl_select_state(msp1_p, msp1_sleep);
-		if (retval)
-			pr_err("could not set MSP1 sleepstate\n");
-	}
-	spin_unlock_irqrestore(&msp_rxtx_lock, flags);
-
-	return retval;
-}
-
 static struct stedma40_chan_cfg msp0_dma_rx = {
 	.high_priority = true,
 	.dir = STEDMA40_PERIPH_TO_MEM,
@@ -132,8 +85,6 @@ static struct msp_i2s_platform_data msp1_platform_data = {
 	.id = MSP_I2S_1,
 	.msp_i2s_dma_rx = NULL,
 	.msp_i2s_dma_tx = &msp1_dma_tx,
-	.msp_i2s_init = msp13_i2s_init,
-	.msp_i2s_exit = msp13_i2s_exit,
 };
 
 static struct stedma40_chan_cfg msp2_dma_rx = {
@@ -219,47 +170,23 @@ static struct msp_i2s_platform_data msp3_platform_data = {
 	.id		= MSP_I2S_3,
 	.msp_i2s_dma_rx	= &msp1_dma_rx,
 	.msp_i2s_dma_tx	= NULL,
-	.msp_i2s_init = msp13_i2s_init,
-	.msp_i2s_exit = msp13_i2s_exit,
 };
 
 int mop500_msp_init(struct device *parent)
 {
-	struct platform_device *msp1;
-
 	pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
 	platform_device_register(&snd_soc_mop500);
 
 	pr_info("Initialize MSP I2S-devices.\n");
 	db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
 			   &msp0_platform_data);
-	msp1 = db8500_add_msp_i2s(parent, 1, U8500_MSP1_BASE, IRQ_DB8500_MSP1,
+	db8500_add_msp_i2s(parent, 1, U8500_MSP1_BASE, IRQ_DB8500_MSP1,
 			   &msp1_platform_data);
 	db8500_add_msp_i2s(parent, 2, U8500_MSP2_BASE, IRQ_DB8500_MSP2,
 			   &msp2_platform_data);
 	db8500_add_msp_i2s(parent, 3, U8500_MSP3_BASE, IRQ_DB8500_MSP1,
 			   &msp3_platform_data);
 
-	/* Get the pinctrl handle for MSP1 */
-	if (msp1) {
-		msp1_p = pinctrl_get(&msp1->dev);
-		if (IS_ERR(msp1_p))
-			dev_err(&msp1->dev, "could not get MSP1 pinctrl\n");
-		else {
-			msp1_def = pinctrl_lookup_state(msp1_p,
-							PINCTRL_STATE_DEFAULT);
-			if (IS_ERR(msp1_def)) {
-				dev_err(&msp1->dev,
-					"could not get MSP1 defstate\n");
-			}
-			msp1_sleep = pinctrl_lookup_state(msp1_p,
-							  PINCTRL_STATE_SLEEP);
-			if (IS_ERR(msp1_sleep))
-				dev_err(&msp1->dev,
-					"could not get MSP1 idlestate\n");
-		}
-	}
-
 	pr_info("%s: Register platform-device 'ux500-pcm'\n", __func__);
 	platform_device_register(&ux500_pcm);
 
diff --git a/arch/arm/mach-ux500/include/mach/msp.h b/arch/arm/mach-ux500/include/mach/msp.h
index 798be19..3cc7142 100644
--- a/arch/arm/mach-ux500/include/mach/msp.h
+++ b/arch/arm/mach-ux500/include/mach/msp.h
@@ -22,8 +22,6 @@ struct msp_i2s_platform_data {
 	enum msp_i2s_id id;
 	struct stedma40_chan_cfg *msp_i2s_dma_rx;
 	struct stedma40_chan_cfg *msp_i2s_dma_tx;
-	int (*msp_i2s_init) (void);
-	int (*msp_i2s_exit) (void);
 };
 
 #endif
diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c
index 36be11e..2cbfc54 100644
--- a/sound/soc/ux500/ux500_msp_i2s.c
+++ b/sound/soc/ux500/ux500_msp_i2s.c
@@ -15,6 +15,7 @@
 
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
 
@@ -25,6 +26,17 @@
 
 #include "ux500_msp_i2s.h"
 
+/* MSP1/3 Tx/Rx usage protection */
+static DEFINE_SPINLOCK(msp_rxtx_lock);
+
+/* Pin modes */
+struct pinctrl *pinctrl_p;
+struct pinctrl_state *pinctrl_def;
+struct pinctrl_state *pinctrl_sleep;
+
+/* Reference Count */
+int pinctrl_rxtx_ref;
+
  /* Protocol desciptors */
 static const struct msp_protdesc prot_descs[] = {
 	{ /* I2S */
@@ -352,17 +364,23 @@ static int configure_multichannel(struct ux500_msp *msp,
 
 static int enable_msp(struct ux500_msp *msp, struct ux500_msp_config *config)
 {
-	int status = 0;
+	int status = 0, retval = 0;
 	u32 reg_val_DMACR, reg_val_GCR;
+	unsigned long flags;
 
 	/* Check msp state whether in RUN or CONFIGURED Mode */
-	if ((msp->msp_state == MSP_STATE_IDLE) && (msp->plat_init)) {
-		status = msp->plat_init();
-		if (status) {
-			dev_err(msp->dev, "%s: ERROR: Failed to init MSP (%d)!\n",
-				__func__, status);
-			return status;
+	if (msp->msp_state == MSP_STATE_IDLE) {
+		spin_lock_irqsave(&msp_rxtx_lock, flags);
+		if (pinctrl_rxtx_ref == 0 &&
+			!(IS_ERR(pinctrl_p) || IS_ERR(pinctrl_def))) {
+			retval = pinctrl_select_state(pinctrl_p,
+						pinctrl_def);
+			if (retval)
+				pr_err("could not set MSP defstate\n");
 		}
+		if (!retval)
+			pinctrl_rxtx_ref++;
+		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
 	}
 
 	/* Configure msp with protocol dependent settings */
@@ -620,7 +638,8 @@ int ux500_msp_i2s_trigger(struct ux500_msp *msp, int cmd, int direction)
 
 int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
 {
-	int status = 0;
+	int status = 0, retval = 0;
+	unsigned long flags;
 
 	dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
 
@@ -631,12 +650,19 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
 		writel((readl(msp->registers + MSP_GCR) &
 			       (~(FRAME_GEN_ENABLE | SRG_ENABLE))),
 			      msp->registers + MSP_GCR);
-		if (msp->plat_exit)
-			status = msp->plat_exit();
-			if (status)
-				dev_warn(msp->dev,
-					"%s: WARN: ux500_msp_i2s_exit failed (%d)!\n",
-					__func__, status);
+
+		spin_lock_irqsave(&msp_rxtx_lock, flags);
+		WARN_ON(!pinctrl_rxtx_ref);
+		pinctrl_rxtx_ref--;
+		if (pinctrl_rxtx_ref == 0 &&
+			!(IS_ERR(pinctrl_p) || IS_ERR(pinctrl_sleep))) {
+			retval = pinctrl_select_state(pinctrl_p,
+						pinctrl_sleep);
+			if (retval)
+				pr_err("could not set MSP sleepstate\n");
+		}
+		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
+
 		writel(0, msp->registers + MSP_GCR);
 		writel(0, msp->registers + MSP_TCF);
 		writel(0, msp->registers + MSP_RCF);
@@ -678,8 +704,6 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev,
 
 	msp->id = platform_data->id;
 	msp->dev = &pdev->dev;
-	msp->plat_init = platform_data->msp_i2s_init;
-	msp->plat_exit = platform_data->msp_i2s_exit;
 	msp->dma_cfg_rx = platform_data->msp_i2s_dma_rx;
 	msp->dma_cfg_tx = platform_data->msp_i2s_dma_tx;
 
@@ -717,6 +741,25 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev,
 	dev_dbg(&pdev->dev, "I2S device-name: '%s'\n", i2s_cont->name);
 	msp->i2s_cont = i2s_cont;
 
+	pinctrl_p = pinctrl_get(msp->dev);
+	if (IS_ERR(pinctrl_p))
+		dev_err(&pdev->dev, "could not get MSP pinctrl\n");
+	else {
+		pinctrl_def = pinctrl_lookup_state(pinctrl_p,
+						PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(pinctrl_def)) {
+			dev_err(&pdev->dev,
+				"could not get MSP defstate (%li)\n",
+				PTR_ERR(pinctrl_def));
+		}
+		pinctrl_sleep = pinctrl_lookup_state(pinctrl_p,
+						PINCTRL_STATE_SLEEP);
+		if (IS_ERR(pinctrl_sleep))
+			dev_err(&pdev->dev,
+				"could not get MSP idlestate (%li)\n",
+				PTR_ERR(pinctrl_def));
+	}
+
 	return 0;
 
 err_i2s_cont:
diff --git a/sound/soc/ux500/ux500_msp_i2s.h b/sound/soc/ux500/ux500_msp_i2s.h
index 2d9136d..1ce336f 100644
--- a/sound/soc/ux500/ux500_msp_i2s.h
+++ b/sound/soc/ux500/ux500_msp_i2s.h
@@ -524,8 +524,6 @@ struct ux500_msp {
 	struct dma_chan *rx_pipeid;
 	enum msp_state msp_state;
 	int (*transfer) (struct ux500_msp *msp, struct i2s_message *message);
-	int (*plat_init) (void);
-	int (*plat_exit) (void);
 	struct timer_list notify_timer;
 	int def_elem_len;
 	unsigned int dir_busy;
-- 
1.7.9.5

Re: [PATCH 1/6] Bugfixes and clean-ups bound for the v3.6 RCs

[Mark Brown]

On Tue, Jul 31, 2012 at 02:31:21PM +0100, Lee Jones wrote:
> This patch-set includes some important changes which should make
> their way to the Mainline Release Candidates for the v3.6 release.

You appear to have double sent this or something...

[PATCH 1/6] Bugfixes and clean-ups bound for the v3.6 RCs

[Mark Brown]

On Tue, Jul 31, 2012 at 02:31:21PM +0100, Lee Jones wrote:
> This patch-set includes some important changes which should make
> their way to the Mainline Release Candidates for the v3.6 release.

You appear to have double sent this or something...

Re: [PATCH 1/6] Bugfixes and clean-ups bound for the v3.6 RCs

[Lee Jones]

On 31/07/12 14:40, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 02:31:21PM +0100, Lee Jones wrote:
>> This patch-set includes some important changes which should make
>> their way to the Mainline Release Candidates for the v3.6 release.
>
> You appear to have double sent this or something...

I know *embarrassed smiley*.

I didn't clear out my patches directory before re-sending.

I'll re-send this patch-set, less the HIGHMEM patch.

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

[PATCH 1/6] Bugfixes and clean-ups bound for the v3.6 RCs

[Lee Jones]

On 31/07/12 14:40, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 02:31:21PM +0100, Lee Jones wrote:
>> This patch-set includes some important changes which should make
>> their way to the Mainline Release Candidates for the v3.6 release.
>
> You appear to have double sent this or something...

I know *embarrassed smiley*.

I didn't clear out my patches directory before re-sending.

I'll re-send this patch-set, less the HIGHMEM patch.

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

Re: [PATCH 1/6] Bugfixes and clean-ups bound for the v3.6 RCs

[Lee Jones]

On 31/07/12 14:40, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 02:31:21PM +0100, Lee Jones wrote:
>> This patch-set includes some important changes which should make
>> their way to the Mainline Release Candidates for the v3.6 release.
>
> You appear to have double sent this or something...

I know *embarrassed smiley*.

I didn't clear out my patches directory before re-sending.

I'll re-send this patch-set, less the HIGHMEM patch.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel