[PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[Kuninori Morimoto]

Current sound card is using snd-simple-card,
but it requests probe deferral when boot, since the detection
timing of snd-simple-card is earlier than sh.

Because of this behavior, if platform has some sound cards,
the postponed probe order was changed
and it breaks ALSA device list order.

This patch makes it consistent on mackerel/ap4evb

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 arch/arm/mach-shmobile/board-ap4evb.c   |    2 +-
 arch/arm/mach-shmobile/board-mackerel.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-ap4evb.c b/arch/arm/mach-shmobile/board-ap4evb.c
index d7417f3..2a1e9b5 100644
--- a/arch/arm/mach-shmobile/board-ap4evb.c
+++ b/arch/arm/mach-shmobile/board-ap4evb.c
@@ -1064,8 +1064,8 @@ static struct platform_device *ap4evb_devices[] __initdata = {
 	&sdhi1_device,
 	&usb1_host_device,
 	&fsi_device,
-	&fsi_ak4643_device,
 	&fsi_hdmi_device,
+	&fsi_ak4643_device,
 	&sh_mmcif_device,
 	&hdmi_device,
 	&lcdc_device,
diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index 798be2b..2313a6f 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -1300,8 +1300,8 @@ static struct platform_device *mackerel_devices[] __initdata = {
 	&usbhs0_device,
 	&leds_device,
 	&fsi_device,
-	&fsi_ak4643_device,
 	&fsi_hdmi_device,
+	&fsi_ak4643_device,
 	&nand_flash_device,
 	&sdhi0_device,
 #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
-- 
1.7.5.4

Re: [PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[Simon Horman]

On Mon, May 28, 2012 at 11:38:34PM -0700, Kuninori Morimoto wrote:
> Current sound card is using snd-simple-card,
> but it requests probe deferral when boot, since the detection
> timing of snd-simple-card is earlier than sh.
> 
> Because of this behavior, if platform has some sound cards,
> the postponed probe order was changed
> and it breaks ALSA device list order.
> 
> This patch makes it consistent on mackerel/ap4evb

Hi Morimoto-san,

Is the problem that a previous patch altered the order of the
ALSA index of the sound cards? And this patch restores the previous order?

If so, I wonder if this is really necessary as is should
be possible to set the index using a module parameter if one
is relying on it.

> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  arch/arm/mach-shmobile/board-ap4evb.c   |    2 +-
>  arch/arm/mach-shmobile/board-mackerel.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/board-ap4evb.c b/arch/arm/mach-shmobile/board-ap4evb.c
> index d7417f3..2a1e9b5 100644
> --- a/arch/arm/mach-shmobile/board-ap4evb.c
> +++ b/arch/arm/mach-shmobile/board-ap4evb.c
> @@ -1064,8 +1064,8 @@ static struct platform_device *ap4evb_devices[] __initdata = {
>  	&sdhi1_device,
>  	&usb1_host_device,
>  	&fsi_device,
> -	&fsi_ak4643_device,
>  	&fsi_hdmi_device,
> +	&fsi_ak4643_device,
>  	&sh_mmcif_device,
>  	&hdmi_device,
>  	&lcdc_device,
> diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
> index 798be2b..2313a6f 100644
> --- a/arch/arm/mach-shmobile/board-mackerel.c
> +++ b/arch/arm/mach-shmobile/board-mackerel.c
> @@ -1300,8 +1300,8 @@ static struct platform_device *mackerel_devices[] __initdata = {
>  	&usbhs0_device,
>  	&leds_device,
>  	&fsi_device,
> -	&fsi_ak4643_device,
>  	&fsi_hdmi_device,
> +	&fsi_ak4643_device,
>  	&nand_flash_device,
>  	&sdhi0_device,
>  #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[Paul Mundt]

On Tue, May 29, 2012 at 03:46:25PM +0900, Simon Horman wrote:
> On Mon, May 28, 2012 at 11:38:34PM -0700, Kuninori Morimoto wrote:
> > Current sound card is using snd-simple-card,
> > but it requests probe deferral when boot, since the detection
> > timing of snd-simple-card is earlier than sh.
> > 
> > Because of this behavior, if platform has some sound cards,
> > the postponed probe order was changed
> > and it breaks ALSA device list order.
> > 
> > This patch makes it consistent on mackerel/ap4evb
> 
> Hi Morimoto-san,
> 
> Is the problem that a previous patch altered the order of the
> ALSA index of the sound cards? And this patch restores the previous order?
> 
> If so, I wonder if this is really necessary as is should
> be possible to set the index using a module parameter if one
> is relying on it.
> 
One would expect so. What happens if the ALSA drivers are built as
modules and inserted in random order today? If you require persistent
assignment then you will have to deal with it either through module
parameters or udev.

Re: [PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[Simon Horman]

On Tue, May 29, 2012 at 04:04:58PM +0900, Paul Mundt wrote:
> On Tue, May 29, 2012 at 03:46:25PM +0900, Simon Horman wrote:
> > On Mon, May 28, 2012 at 11:38:34PM -0700, Kuninori Morimoto wrote:
> > > Current sound card is using snd-simple-card,
> > > but it requests probe deferral when boot, since the detection
> > > timing of snd-simple-card is earlier than sh.
> > > 
> > > Because of this behavior, if platform has some sound cards,
> > > the postponed probe order was changed
> > > and it breaks ALSA device list order.
> > > 
> > > This patch makes it consistent on mackerel/ap4evb
> > 
> > Hi Morimoto-san,
> > 
> > Is the problem that a previous patch altered the order of the
> > ALSA index of the sound cards? And this patch restores the previous order?
> > 
> > If so, I wonder if this is really necessary as is should
> > be possible to set the index using a module parameter if one
> > is relying on it.
> > 
> One would expect so. What happens if the ALSA drivers are built as
> modules and inserted in random order today? If you require persistent
> assignment then you will have to deal with it either through module
> parameters or udev.

I'm unsure if that was a rhetorical question or not,
but I believe the answer is yes.

Re: [PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[Kuninori Morimoto]

Hi Simon, Paul

Thank you for checking patch

> > > Is the problem that a previous patch altered the order of the
> > > ALSA index of the sound cards? And this patch restores the previous order?
> > > 
> > > If so, I wonder if this is really necessary as is should
> > > be possible to set the index using a module parameter if one
> > > is relying on it.
> > > 
> > One would expect so. What happens if the ALSA drivers are built as
> > modules and inserted in random order today? If you require persistent
> > assignment then you will have to deal with it either through module
> > parameters or udev.
> 
> I'm unsure if that was a rhetorical question or not,
> but I believe the answer is yes.

Yes.
The ALSA card list order was same as inserted alsa-module
order if it was built as module.
This is very OK.
And I'm not sure ALSA already has index specification method.

Sorry for my confusion.
My problem was deferred probe order on built-in.

Current my issue is...
  normal probe of ALSA-card.0  -> requests probe deferral
  normal probe of ALSA-card.1  -> requests probe deferral
  ...
  deferred-probe of ALSA-card.1  <= order changed
  deferred-probe of ALSA-card.0  <= order changed

Now, I re-checked and noticed that
the main issue was additional method for deferred_probe_pending_list on dd.c.

Below patch solved this issue.
What do you think this ?

------------------------------------------
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1b1cbb5..dcb8a6e 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -100,7 +100,7 @@ static void driver_deferred_probe_add(struct device *dev)
        mutex_lock(&deferred_probe_mutex);
        if (list_empty(&dev->p->deferred_probe)) {
                dev_dbg(dev, "Added to deferred list\n");
-               list_add(&dev->p->deferred_probe, &deferred_probe_pending_list);
+               list_add_tail(&dev->p->deferred_probe, &deferred_probe_pending_list);
        }
        mutex_unlock(&deferred_probe_mutex);
 }
------------------------------------------


Best regards
---
Kuninori Morimoto

Re: [PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[Simon Horman]

On Tue, May 29, 2012 at 12:40:08AM -0700, Kuninori Morimoto wrote:
> 
> Hi Simon, Paul
> 
> Thank you for checking patch
> 
> > > > Is the problem that a previous patch altered the order of the
> > > > ALSA index of the sound cards? And this patch restores the previous order?
> > > > 
> > > > If so, I wonder if this is really necessary as is should
> > > > be possible to set the index using a module parameter if one
> > > > is relying on it.
> > > > 
> > > One would expect so. What happens if the ALSA drivers are built as
> > > modules and inserted in random order today? If you require persistent
> > > assignment then you will have to deal with it either through module
> > > parameters or udev.
> > 
> > I'm unsure if that was a rhetorical question or not,
> > but I believe the answer is yes.
> 
> Yes.
> The ALSA card list order was same as inserted alsa-module
> order if it was built as module.
> This is very OK.
> And I'm not sure ALSA already has index specification method.
> Sorry for my confusion.
> My problem was deferred probe order on built-in.

I was under the impression that the index could be set for a built-in too.
But poking around in /sys on an armadillo board, that seems not to be the
case.

> 
> Current my issue is...
>   normal probe of ALSA-card.0  -> requests probe deferral
>   normal probe of ALSA-card.1  -> requests probe deferral
>   ...
>   deferred-probe of ALSA-card.1  <= order changed
>   deferred-probe of ALSA-card.0  <= order changed
> 
> Now, I re-checked and noticed that
> the main issue was additional method for deferred_probe_pending_list on dd.c.
> 
> Below patch solved this issue.
> What do you think this ?

It seems reasonable to expect things to be appended not prepended,
so I think your patch makes some sense.

I'm not entirely sure it makes any sense to be relying on the
order of cards being initialised. But I guess the order of
initialisation is relied on in other places too.

Re: [PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[Kuninori Morimoto]

Hi Simon

Thank you for your reply

> > And I'm not sure ALSA already has index specification method.
> > Sorry for my confusion.
> > My problem was deferred probe order on built-in.
> 
> I was under the impression that the index could be set for a built-in too.
> But poking around in /sys on an armadillo board, that seems not to be the
> case.

I guess on /sys index is depends on .id ? 
In mackerel case, it is

static struct platform_device fsi_ak4643_device = {
	.name	= "asoc-simple-card",
        ...
}

static struct platform_device fsi_hdmi_device = {
	.name	= "asoc-simple-card",
	.id	= 1,
        ...
}

# fsi_ak4643_device didn't have .id = 0, but it is.

Does these appear on /sys ?

Now, platform_device order are

static struct platform_device *mackerel_devices[] __initdata = {
	...
	&fsi_ak4643_device,  // .id = 0
	&fsi_hdmi_device,    // .id = 1
	...
}

ALSA sound card list order is same as detected/probed order, not these .id order.

> > Current my issue is...
> >   normal probe of ALSA-card.0  -> requests probe deferral
> >   normal probe of ALSA-card.1  -> requests probe deferral
> >   ...
> >   deferred-probe of ALSA-card.1  <= order changed
> >   deferred-probe of ALSA-card.0  <= order changed

So, if this happen, ALSA-card list order become un-expected order.

> I'm not entirely sure it makes any sense to be relying on the
> order of cards being initialised. But I guess the order of
> initialisation is relied on in other places too.

It decides "1st player", "2nd player".
I'm expecting "fsi_ak4643" is 1st player, but...

Best regards
---
Kuninori Morimoto

Re: [PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[Kuninori Morimoto]

Hi

> Hi Simon
> 
> Thank you for your reply
> 
> > > And I'm not sure ALSA already has index specification method.
> > > Sorry for my confusion.
> > > My problem was deferred probe order on built-in.
> > 
> > I was under the impression that the index could be set for a built-in too.
> > But poking around in /sys on an armadillo board, that seems not to be the
> > case.
> 
> I guess on /sys index is depends on .id ? 
> In mackerel case, it is
> 
> static struct platform_device fsi_ak4643_device = {
> 	.name	= "asoc-simple-card",
>         ...
> }
> 
> static struct platform_device fsi_hdmi_device = {
> 	.name	= "asoc-simple-card",
> 	.id	= 1,
>         ...
> }
> 
> # fsi_ak4643_device didn't have .id = 0, but it is.
> 
> Does these appear on /sys ?
> 
> Now, platform_device order are
> 
> static struct platform_device *mackerel_devices[] __initdata = {
> 	...
> 	&fsi_ak4643_device,  // .id = 0
> 	&fsi_hdmi_device,    // .id = 1
> 	...
> }
> 
> ALSA sound card list order is same as detected/probed order, not these .id order.
> 
> > > Current my issue is...
> > >   normal probe of ALSA-card.0  -> requests probe deferral
> > >   normal probe of ALSA-card.1  -> requests probe deferral
> > >   ...
> > >   deferred-probe of ALSA-card.1  <= order changed
> > >   deferred-probe of ALSA-card.0  <= order changed
> 
> So, if this happen, ALSA-card list order become un-expected order.
> 
> > I'm not entirely sure it makes any sense to be relying on the
> > order of cards being initialised. But I guess the order of
> > initialisation is relied on in other places too.
> 
> It decides "1st player", "2nd player".
> I'm expecting "fsi_ak4643" is 1st player, but...

This means if it was "built-in" case

Best regards
---
Kuninori Morimoto

Re: [PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[Simon Horman]

On Tue, May 29, 2012 at 06:09:47PM -0700, Kuninori Morimoto wrote:
> 
> Hi
> 
> > Hi Simon
> > 
> > Thank you for your reply
> > 
> > > > And I'm not sure ALSA already has index specification method.
> > > > Sorry for my confusion.
> > > > My problem was deferred probe order on built-in.
> > > 
> > > I was under the impression that the index could be set for a built-in too.
> > > But poking around in /sys on an armadillo board, that seems not to be the
> > > case.
> > 
> > I guess on /sys index is depends on .id ? 
> > In mackerel case, it is
> > 
> > static struct platform_device fsi_ak4643_device = {
> > 	.name	= "asoc-simple-card",
> >         ...
> > }
> > 
> > static struct platform_device fsi_hdmi_device = {
> > 	.name	= "asoc-simple-card",
> > 	.id	= 1,
> >         ...
> > }
> > 
> > # fsi_ak4643_device didn't have .id = 0, but it is.
> > 
> > Does these appear on /sys ?
> > 
> > Now, platform_device order are
> > 
> > static struct platform_device *mackerel_devices[] __initdata = {
> > 	...
> > 	&fsi_ak4643_device,  // .id = 0
> > 	&fsi_hdmi_device,    // .id = 1
> > 	...
> > }
> > 
> > ALSA sound card list order is same as detected/probed order, not these .id order.
> > 
> > > > Current my issue is...
> > > >   normal probe of ALSA-card.0  -> requests probe deferral
> > > >   normal probe of ALSA-card.1  -> requests probe deferral
> > > >   ...
> > > >   deferred-probe of ALSA-card.1  <= order changed
> > > >   deferred-probe of ALSA-card.0  <= order changed
> > 
> > So, if this happen, ALSA-card list order become un-expected order.
> > 
> > > I'm not entirely sure it makes any sense to be relying on the
> > > order of cards being initialised. But I guess the order of
> > > initialisation is relied on in other places too.
> > 
> > It decides "1st player", "2nd player".
> > I'm expecting "fsi_ak4643" is 1st player, but...
> 
> This means if it was "built-in" case

Hi Morimoto,

thanks for the clarification. Given the circumstances I think that
your original patch is fine.

Re: [PATCH] ARM: mach-shmobile: tidyup: sound card detection order

[Kuninori Morimoto]

Hi Simon

Thank you for checking / replying

> > > It decides "1st player", "2nd player".
> > > I'm expecting "fsi_ak4643" is 1st player, but...
> > 
> > This means if it was "built-in" case
> 
> Hi Morimoto,
> 
> thanks for the clarification. Given the circumstances I think that
> your original patch is fine.

Thanks !
OK. I send the patch

Best regards
---
Kuninori Morimoto

[PATCH] driver core: fixup reversed deferred probe order

[Kuninori Morimoto]

If driver requests probe deferral,
it will be added to deferred_probe_pending_list
by driver_deferred_probe_add(), but, it used list_add().
Because of that, deferred probe will be run as reversed order.
This patch uses list_add_tail(), and solved this issue.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/base/dd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1b1cbb5..dcb8a6e 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -100,7 +100,7 @@ static void driver_deferred_probe_add(struct device *dev)
 	mutex_lock(&deferred_probe_mutex);
 	if (list_empty(&dev->p->deferred_probe)) {
 		dev_dbg(dev, "Added to deferred list\n");
-		list_add(&dev->p->deferred_probe, &deferred_probe_pending_list);
+		list_add_tail(&dev->p->deferred_probe, &deferred_probe_pending_list);
 	}
 	mutex_unlock(&deferred_probe_mutex);
 }
-- 
1.7.5.4

Re: [PATCH] driver core: fixup reversed deferred probe order

[Greg Kroah-Hartman]

On Tue, May 29, 2012 at 06:46:06PM -0700, Kuninori Morimoto wrote:
> If driver requests probe deferral,
> it will be added to deferred_probe_pending_list
> by driver_deferred_probe_add(), but, it used list_add().
> Because of that, deferred probe will be run as reversed order.
> This patch uses list_add_tail(), and solved this issue.

Is this causing a problem with 3.5-rc2 on some systems, so it needs to
get into 3.5-final, or can it wait for 3.6?

thanks,

greg k-h

Re: [PATCH] driver core: fixup reversed deferred probe order

[Kuninori Morimoto]

Hi Greg

Thank you for checking this patch

> > If driver requests probe deferral,
> > it will be added to deferred_probe_pending_list
> > by driver_deferred_probe_add(), but, it used list_add().
> > Because of that, deferred probe will be run as reversed order.
> > This patch uses list_add_tail(), and solved this issue.
> 
> Is this causing a problem with 3.5-rc2 on some systems, so it needs to
> get into 3.5-final, or can it wait for 3.6?

ALSA SoC sound card detection order was broken by this issue in v3.5-rcX
(on some board).
So, I'm happy if it was included in v3.5-final.

Best regards
---
Kuninori Morimoto

Re: [PATCH] driver core: fixup reversed deferred probe order

[Greg Kroah-Hartman]

On Tue, Jun 12, 2012 at 05:03:38PM -0700, Kuninori Morimoto wrote:
> 
> Hi Greg
> 
> Thank you for checking this patch
> 
> > > If driver requests probe deferral,
> > > it will be added to deferred_probe_pending_list
> > > by driver_deferred_probe_add(), but, it used list_add().
> > > Because of that, deferred probe will be run as reversed order.
> > > This patch uses list_add_tail(), and solved this issue.
> > 
> > Is this causing a problem with 3.5-rc2 on some systems, so it needs to
> > get into 3.5-final, or can it wait for 3.6?
> 
> ALSA SoC sound card detection order was broken by this issue in v3.5-rcX
> (on some board).
> So, I'm happy if it was included in v3.5-final.

Ok, thanks for letting me know, will queue it up for that release.

greg k-h

Re: [PATCH] driver core: fixup reversed deferred probe order

[Grant Likely]

On Wed, May 30, 2012 at 2:46 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> If driver requests probe deferral,
> it will be added to deferred_probe_pending_list
> by driver_deferred_probe_add(), but, it used list_add().
> Because of that, deferred probe will be run as reversed order.
> This patch uses list_add_tail(), and solved this issue.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

(But not tested; I'm not in a position to test anything at the moment)  :-(

g.

> ---
>  drivers/base/dd.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 1b1cbb5..dcb8a6e 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -100,7 +100,7 @@ static void driver_deferred_probe_add(struct device *dev)
>         mutex_lock(&deferred_probe_mutex);
>         if (list_empty(&dev->p->deferred_probe)) {
>                 dev_dbg(dev, "Added to deferred list\n");
> -               list_add(&dev->p->deferred_probe, &deferred_probe_pending_list);
> +               list_add_tail(&dev->p->deferred_probe, &deferred_probe_pending_list);
>         }
>         mutex_unlock(&deferred_probe_mutex);
>  }
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.