Patchwork [v2,6/9] ARM: mx31ads: add audmux device

login
register
mail settings
Submitter Richard Zhao
Date Feb. 2, 2012, 2:12 a.m.
Message ID <1328148728-32258-7-git-send-email-richard.zhao@linaro.org>
Download mbox | patch
Permalink /patch/139044/
State New
Headers show

Comments

Richard Zhao - Feb. 2, 2012, 2:12 a.m.
Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
 arch/arm/plat-mxc/include/mach/mx31.h |    1 +
 2 files changed, 11 insertions(+), 0 deletions(-)
Shawn Guo - Feb. 2, 2012, 8:55 a.m.
On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao wrote:
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> ---
>  arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
>  arch/arm/plat-mxc/include/mach/mx31.h |    1 +
>  2 files changed, 11 insertions(+), 0 deletions(-)
> 
Hmm, let's see who are actually using mxc_audmux_v2_configure_port().

$ git grep -n mxc_audmux_v2_configure_port arch/arm/
arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:246:     mxc_audmux_v2_configure_port(0,
arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:254:     mxc_audmux_v2_configure_port(4,
arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:257:     mxc_audmux_v2_configure_port(0,
arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:265:     mxc_audmux_v2_configure_port(3,
arch/arm/mach-imx/mach-pcm043.c:365:    mxc_audmux_v2_configure_port(3,
arch/arm/mach-imx/mach-pcm043.c:371:    mxc_audmux_v2_configure_port(0,

$ git grep -n mxc_audmux_v2_configure_port sound/soc/imx/
sound/soc/imx/wm1133-ev1.c:277: mxc_audmux_v2_configure_port(MX31_AUDMUX_PORT1_SSI0, ptcr, pdcr);
sound/soc/imx/wm1133-ev1.c:281: mxc_audmux_v2_configure_port(MX31_AUDMUX_PORT5_SSI_PINS_5, ptcr, pdcr);

I guess audmux device needs to be added for all these users.  And for
sake of bisect, it should be added as part of patch #5.
Shawn Guo - Feb. 2, 2012, 9:11 a.m.
On Thu, Feb 02, 2012 at 04:55:23PM +0800, Shawn Guo wrote:
> On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao wrote:
> > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > ---
> >  arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
> >  arch/arm/plat-mxc/include/mach/mx31.h |    1 +
> >  2 files changed, 11 insertions(+), 0 deletions(-)
> > 
> Hmm, let's see who are actually using mxc_audmux_v2_configure_port().
> 
> $ git grep -n mxc_audmux_v2_configure_port arch/arm/
> arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:246:     mxc_audmux_v2_configure_port(0,
> arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:254:     mxc_audmux_v2_configure_port(4,
> arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:257:     mxc_audmux_v2_configure_port(0,
> arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:265:     mxc_audmux_v2_configure_port(3,
> arch/arm/mach-imx/mach-pcm043.c:365:    mxc_audmux_v2_configure_port(3,
> arch/arm/mach-imx/mach-pcm043.c:371:    mxc_audmux_v2_configure_port(0,
> 
As we are moving audmux into sound/soc/imx, it makes less sense to
still keep these calls in board files.  Instead, I prefer to call it
from machine driver like what wm1133-ev1 does below.  Or we can simply
make the it a audmux-self call with 3 parameters it needs retrieved
from platform_data or device tree, so that machine driver does not
even bother with the call.  Makes sense?

Regards,
Shawn

> $ git grep -n mxc_audmux_v2_configure_port sound/soc/imx/
> sound/soc/imx/wm1133-ev1.c:277: mxc_audmux_v2_configure_port(MX31_AUDMUX_PORT1_SSI0, ptcr, pdcr);
> sound/soc/imx/wm1133-ev1.c:281: mxc_audmux_v2_configure_port(MX31_AUDMUX_PORT5_SSI_PINS_5, ptcr, pdcr);
> 
> I guess audmux device needs to be added for all these users.  And for
> sake of bisect, it should be added as part of patch #5.
>
Richard Zhao - Feb. 2, 2012, 9:24 a.m.
On Thu, Feb 02, 2012 at 05:11:34PM +0800, Shawn Guo wrote:
> On Thu, Feb 02, 2012 at 04:55:23PM +0800, Shawn Guo wrote:
> > On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao wrote:
> > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > ---
> > >  arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
> > >  arch/arm/plat-mxc/include/mach/mx31.h |    1 +
> > >  2 files changed, 11 insertions(+), 0 deletions(-)
> > > 
> > Hmm, let's see who are actually using mxc_audmux_v2_configure_port().
> > 
> > $ git grep -n mxc_audmux_v2_configure_port arch/arm/
> > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:246:     mxc_audmux_v2_configure_port(0,
> > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:254:     mxc_audmux_v2_configure_port(4,
> > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:257:     mxc_audmux_v2_configure_port(0,
> > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:265:     mxc_audmux_v2_configure_port(3,
> > arch/arm/mach-imx/mach-pcm043.c:365:    mxc_audmux_v2_configure_port(3,
> > arch/arm/mach-imx/mach-pcm043.c:371:    mxc_audmux_v2_configure_port(0,
I didn't grep arch/arm. where is pc043 asoc machine file?
> > 
> As we are moving audmux into sound/soc/imx, it makes less sense to
> still keep these calls in board files.
It don't corrupt git bisect. We don't have to include it in this patch.
>  Instead, I prefer to call it
> from machine driver like what wm1133-ev1 does below.
Maybe pass the info as asoc machine driver pdata.
>  Or we can simply
> make the it a audmux-self call with 3 parameters it needs retrieved
> from platform_data or device tree, so that machine driver does not
> even bother with the call.  Makes sense?
audux configuration may change after initial set. For example, it may use
one configuration for audio playback, but use another when you connect a
BT audio device.
> 
> Regards,
> Shawn
> 
> > $ git grep -n mxc_audmux_v2_configure_port sound/soc/imx/
> > sound/soc/imx/wm1133-ev1.c:277: mxc_audmux_v2_configure_port(MX31_AUDMUX_PORT1_SSI0, ptcr, pdcr);
> > sound/soc/imx/wm1133-ev1.c:281: mxc_audmux_v2_configure_port(MX31_AUDMUX_PORT5_SSI_PINS_5, ptcr, pdcr);
It's imx31ads board.
> > 
> > I guess audmux device needs to be added for all these users.  And for
> > sake of bisect, it should be added as part of patch #5.
Yes.

Thanks
Richard
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Mark Brown - Feb. 2, 2012, 12:09 p.m.
On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao wrote:

>  static void __init mxc_init_audio(void)
>  {
>  	imx31_add_imx_ssi(0, NULL);
>  	mxc_iomux_setup_multiple_pins(ssi_pins, ARRAY_SIZE(ssi_pins), "ssi");
> +	imx_add_platform_device("audmux-v2", 0,
> +				audmux_res, ARRAY_SIZE(audmux_res), NULL, 0);

Since the audmux is a part of the SoC silicon shouldn't the SoC just
register the device without individual boards having to do anything
(possibly conditional on ASoC being selected in Kconfig or something)?
It's going to be connected in exactly the same fashion on any system
using the SoC.
Shawn Guo - Feb. 2, 2012, 1:09 p.m.
On Thu, Feb 02, 2012 at 05:24:28PM +0800, Richard Zhao wrote:
> On Thu, Feb 02, 2012 at 05:11:34PM +0800, Shawn Guo wrote:
> > On Thu, Feb 02, 2012 at 04:55:23PM +0800, Shawn Guo wrote:
> > > On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao wrote:
> > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > ---
> > > >  arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
> > > >  arch/arm/plat-mxc/include/mach/mx31.h |    1 +
> > > >  2 files changed, 11 insertions(+), 0 deletions(-)
> > > > 
> > > Hmm, let's see who are actually using mxc_audmux_v2_configure_port().
> > > 
> > > $ git grep -n mxc_audmux_v2_configure_port arch/arm/
> > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:246:     mxc_audmux_v2_configure_port(0,
> > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:254:     mxc_audmux_v2_configure_port(4,
> > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:257:     mxc_audmux_v2_configure_port(0,
> > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:265:     mxc_audmux_v2_configure_port(3,
> > > arch/arm/mach-imx/mach-pcm043.c:365:    mxc_audmux_v2_configure_port(3,
> > > arch/arm/mach-imx/mach-pcm043.c:371:    mxc_audmux_v2_configure_port(0,
> I didn't grep arch/arm. where is pc043 asoc machine file?

It seems to be sound/soc/imx/phycore-ac97.c.

> > > 
> > As we are moving audmux into sound/soc/imx, it makes less sense to
> > still keep these calls in board files.
> It don't corrupt git bisect. We don't have to include it in this patch.

It's logically part of this series.

> >  Instead, I prefer to call it
> > from machine driver like what wm1133-ev1 does below.
> Maybe pass the info as asoc machine driver pdata.

Sounds good.

> >  Or we can simply
> > make the it a audmux-self call with 3 parameters it needs retrieved
> > from platform_data or device tree, so that machine driver does not
> > even bother with the call.  Makes sense?
> audux configuration may change after initial set. For example, it may use
> one configuration for audio playback, but use another when you connect a
> BT audio device.

Right.  I forgot this point.
Shawn Guo - Feb. 2, 2012, 1:17 p.m.
On Thu, Feb 02, 2012 at 12:09:01PM +0000, Mark Brown wrote:
> On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao wrote:
> 
> >  static void __init mxc_init_audio(void)
> >  {
> >  	imx31_add_imx_ssi(0, NULL);
> >  	mxc_iomux_setup_multiple_pins(ssi_pins, ARRAY_SIZE(ssi_pins), "ssi");
> > +	imx_add_platform_device("audmux-v2", 0,
> > +				audmux_res, ARRAY_SIZE(audmux_res), NULL, 0);
> 
> Since the audmux is a part of the SoC silicon shouldn't the SoC just
> register the device without individual boards having to do anything
> (possibly conditional on ASoC being selected in Kconfig or something)?
> It's going to be connected in exactly the same fashion on any system
> using the SoC.

Hmm, we are trying to save adding the device for those boards which do
not route any audmux pins out at all.
Mark Brown - Feb. 2, 2012, 1:26 p.m.
On Thu, Feb 02, 2012 at 09:17:18PM +0800, Shawn Guo wrote:
> On Thu, Feb 02, 2012 at 12:09:01PM +0000, Mark Brown wrote:

> > Since the audmux is a part of the SoC silicon shouldn't the SoC just
> > register the device without individual boards having to do anything
> > (possibly conditional on ASoC being selected in Kconfig or something)?
> > It's going to be connected in exactly the same fashion on any system
> > using the SoC.

> Hmm, we are trying to save adding the device for those boards which do
> not route any audmux pins out at all.

That's why I'm saying perhaps make it conditional on having ASoC built
(or even on having the AUDMUX driver built).
Richard Zhao - Feb. 2, 2012, 1:58 p.m.
On Thu, Feb 02, 2012 at 09:09:03PM +0800, Shawn Guo wrote:
> On Thu, Feb 02, 2012 at 05:24:28PM +0800, Richard Zhao wrote:
> > On Thu, Feb 02, 2012 at 05:11:34PM +0800, Shawn Guo wrote:
> > > On Thu, Feb 02, 2012 at 04:55:23PM +0800, Shawn Guo wrote:
> > > > On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao wrote:
> > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > > ---
> > > > >  arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
> > > > >  arch/arm/plat-mxc/include/mach/mx31.h |    1 +
> > > > >  2 files changed, 11 insertions(+), 0 deletions(-)
> > > > > 
> > > > Hmm, let's see who are actually using mxc_audmux_v2_configure_port().
> > > > 
> > > > $ git grep -n mxc_audmux_v2_configure_port arch/arm/
> > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:246:     mxc_audmux_v2_configure_port(0,
> > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:254:     mxc_audmux_v2_configure_port(4,
> > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:257:     mxc_audmux_v2_configure_port(0,
> > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:265:     mxc_audmux_v2_configure_port(3,
> > > > arch/arm/mach-imx/mach-pcm043.c:365:    mxc_audmux_v2_configure_port(3,
> > > > arch/arm/mach-imx/mach-pcm043.c:371:    mxc_audmux_v2_configure_port(0,
> > I didn't grep arch/arm. where is pc043 asoc machine file?
> 
> It seems to be sound/soc/imx/phycore-ac97.c.
You see, we're not famaliar with the boards.
> 
> > > > 
> > > As we are moving audmux into sound/soc/imx, it makes less sense to
> > > still keep these calls in board files.
> > It don't corrupt git bisect. We don't have to include it in this patch.
> 
> It's logically part of this series.
I don't know much about the above boards and I can not test either. I think I
have to leave it to other volunteers. I mainly focus on audmux itself.
> 
> > >  Instead, I prefer to call it
> > > from machine driver like what wm1133-ev1 does below.
> > Maybe pass the info as asoc machine driver pdata.
> 
> Sounds good.
> 
> > >  Or we can simply
> > > make the it a audmux-self call with 3 parameters it needs retrieved
> > > from platform_data or device tree, so that machine driver does not
> > > even bother with the call.  Makes sense?
> > audux configuration may change after initial set. For example, it may use
> > one configuration for audio playback, but use another when you connect a
> > BT audio device.
> 
> Right.  I forgot this point.
> 
> -- 
> Regards,
> Shawn
Mark Brown - Feb. 2, 2012, 2:09 p.m.
On Thu, Feb 02, 2012 at 09:58:07PM +0800, Richard Zhao wrote:
> On Thu, Feb 02, 2012 at 09:09:03PM +0800, Shawn Guo wrote:

> > It's logically part of this series.

> I don't know much about the above boards and I can not test either. I think I
> have to leave it to other volunteers. I mainly focus on audmux itself.

For stuff like this a build test and CCing the relevant maintainers is
fine, you don't need to test everything.
Shawn Guo - Feb. 2, 2012, 2:11 p.m.
On Thu, Feb 02, 2012 at 01:26:18PM +0000, Mark Brown wrote:
> On Thu, Feb 02, 2012 at 09:17:18PM +0800, Shawn Guo wrote:
> > On Thu, Feb 02, 2012 at 12:09:01PM +0000, Mark Brown wrote:
> 
> > > Since the audmux is a part of the SoC silicon shouldn't the SoC just
> > > register the device without individual boards having to do anything
> > > (possibly conditional on ASoC being selected in Kconfig or something)?
> > > It's going to be connected in exactly the same fashion on any system
> > > using the SoC.
> 
> > Hmm, we are trying to save adding the device for those boards which do
> > not route any audmux pins out at all.
> 
> That's why I'm saying perhaps make it conditional on having ASoC built
> (or even on having the AUDMUX driver built).

Do you mean by having the below in some place like function
imx31_soc_init()?

#ifdef CONFIG_SND_MXC_SOC_AUDMUXV2
	imx_add_platform_device("audmux-v2", 0,
		audmux_res, ARRAY_SIZE(audmux_res), NULL, 0);
#endif

I do not think it's nice and consistent to the way that imx
sub-architecture adds platform device.

Furthermore, when a DT based board boots here, the code is broken.
Explicitly adding the device by individual board as needed can easily
align with DT based boards.  By default, the audmux node in <soc>.dtsi
file has status = "disabled", and any board that needs audmux device
only need to overwrite status property of audmux node as 'okay' in its
<board>.dts.  Then DT core will add the audmux device when the board
boots.
Mark Brown - Feb. 2, 2012, 2:16 p.m.
On Thu, Feb 02, 2012 at 10:11:26PM +0800, Shawn Guo wrote:
> On Thu, Feb 02, 2012 at 01:26:18PM +0000, Mark Brown wrote:

> > That's why I'm saying perhaps make it conditional on having ASoC built
> > (or even on having the AUDMUX driver built).

> Do you mean by having the below in some place like function
> imx31_soc_init()?

> #ifdef CONFIG_SND_MXC_SOC_AUDMUXV2
> 	imx_add_platform_device("audmux-v2", 0,
> 		audmux_res, ARRAY_SIZE(audmux_res), NULL, 0);
> #endif

Yes (you need to check for module too, there's a macro for that the name
of which escapes me right now).

> I do not think it's nice and consistent to the way that imx
> sub-architecture adds platform device.

Well, the i.MX thus far has had relatively few of these always present
type devices - it makes sense to make things conditional for devices
with external signals but for things entirely within the SoC the above
is less work.

> Furthermore, when a DT based board boots here, the code is broken.
> Explicitly adding the device by individual board as needed can easily
> align with DT based boards.  By default, the audmux node in <soc>.dtsi
> file has status = "disabled", and any board that needs audmux device
> only need to overwrite status property of audmux node as 'okay' in its
> <board>.dts.  Then DT core will add the audmux device when the board
> boots.

That seems like more work than is needed for boards, same issue applies.
Shawn Guo - Feb. 2, 2012, 2:25 p.m.
On Thu, Feb 02, 2012 at 09:58:07PM +0800, Richard Zhao wrote:
> On Thu, Feb 02, 2012 at 09:09:03PM +0800, Shawn Guo wrote:
> > On Thu, Feb 02, 2012 at 05:24:28PM +0800, Richard Zhao wrote:
> > > On Thu, Feb 02, 2012 at 05:11:34PM +0800, Shawn Guo wrote:
> > > > On Thu, Feb 02, 2012 at 04:55:23PM +0800, Shawn Guo wrote:
> > > > > On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao wrote:
> > > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > > > ---
> > > > > >  arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
> > > > > >  arch/arm/plat-mxc/include/mach/mx31.h |    1 +
> > > > > >  2 files changed, 11 insertions(+), 0 deletions(-)
> > > > > > 
> > > > > Hmm, let's see who are actually using mxc_audmux_v2_configure_port().
> > > > > 
> > > > > $ git grep -n mxc_audmux_v2_configure_port arch/arm/
> > > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:246:     mxc_audmux_v2_configure_port(0,
> > > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:254:     mxc_audmux_v2_configure_port(4,
> > > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:257:     mxc_audmux_v2_configure_port(0,
> > > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:265:     mxc_audmux_v2_configure_port(3,
> > > > > arch/arm/mach-imx/mach-pcm043.c:365:    mxc_audmux_v2_configure_port(3,
> > > > > arch/arm/mach-imx/mach-pcm043.c:371:    mxc_audmux_v2_configure_port(0,
> > > I didn't grep arch/arm. where is pc043 asoc machine file?
> > 
> > It seems to be sound/soc/imx/phycore-ac97.c.
> You see, we're not famaliar with the boards.

It does not need to be familiar with the board to find that out.  I'm
not familiar with the board either, but I gave the answer.

> > 
> > > > > 
> > > > As we are moving audmux into sound/soc/imx, it makes less sense to
> > > > still keep these calls in board files.
> > > It don't corrupt git bisect. We don't have to include it in this patch.
> > 
> > It's logically part of this series.
> I don't know much about the above boards and I can not test either. I think I
> have to leave it to other volunteers. I mainly focus on audmux itself.

Since you get there, you should be the one cleaning that up.  You will
need to touch those board files anyway, since you need to add audmux
device for those boards.  So no hardware for testing is not an excuse.
For those boards, all you need to do are:

 * Change and compile-test the code
 * Cc board maintainers when submitting the patch

We will wait for board maintainers to respond for a reasonable period
of time.  If we do not get any response during the time, we will send
patch upstream anyway.
Richard Zhao - Feb. 3, 2012, 2:15 a.m.
adding Eric Bénard.

On Thu, Feb 02, 2012 at 09:58:07PM +0800, Richard Zhao wrote:
> On Thu, Feb 02, 2012 at 09:09:03PM +0800, Shawn Guo wrote:
> > On Thu, Feb 02, 2012 at 05:24:28PM +0800, Richard Zhao wrote:
> > > On Thu, Feb 02, 2012 at 05:11:34PM +0800, Shawn Guo wrote:
> > > > On Thu, Feb 02, 2012 at 04:55:23PM +0800, Shawn Guo wrote:
> > > > > On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao wrote:
> > > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > > > ---
> > > > > >  arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
> > > > > >  arch/arm/plat-mxc/include/mach/mx31.h |    1 +
> > > > > >  2 files changed, 11 insertions(+), 0 deletions(-)
> > > > > > 
> > > > > Hmm, let's see who are actually using mxc_audmux_v2_configure_port().
> > > > > 
> > > > > $ git grep -n mxc_audmux_v2_configure_port arch/arm/
> > > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:246:     mxc_audmux_v2_configure_port(0,
> > > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:254:     mxc_audmux_v2_configure_port(4,
> > > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:257:     mxc_audmux_v2_configure_port(0,
> > > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:265:     mxc_audmux_v2_configure_port(3,
> > > > > arch/arm/mach-imx/mach-pcm043.c:365:    mxc_audmux_v2_configure_port(3,
> > > > > arch/arm/mach-imx/mach-pcm043.c:371:    mxc_audmux_v2_configure_port(0,
It's machine specific code, though audmux driver is common. Might be ok?
The asoc machine file for the above 3 boards is not platform driver, and
don't get any plat info.

Sascha, Any suggestion? I saw pcm043 board was added by you. 
> > > I didn't grep arch/arm. where is pc043 asoc machine file?
> > 
> > It seems to be sound/soc/imx/phycore-ac97.c.
> You see, we're not famaliar with the boards.
> > 
> > > > > 
> > > > As we are moving audmux into sound/soc/imx, it makes less sense to
> > > > still keep these calls in board files.
> > > It don't corrupt git bisect. We don't have to include it in this patch.
> > 
> > It's logically part of this series.
> I don't know much about the above boards and I can not test either. I think I
> have to leave it to other volunteers. I mainly focus on audmux itself.
> > 
> > > >  Instead, I prefer to call it
> > > > from machine driver like what wm1133-ev1 does below.
> > > Maybe pass the info as asoc machine driver pdata.
Sorry, they're not platform drivers.
> > 
> > Sounds good.
> > 
> > > >  Or we can simply
> > > > make the it a audmux-self call with 3 parameters it needs retrieved
> > > > from platform_data or device tree, so that machine driver does not
> > > > even bother with the call.  Makes sense?
> > > audux configuration may change after initial set. For example, it may use
> > > one configuration for audio playback, but use another when you connect a
> > > BT audio device.
> > 
> > Right.  I forgot this point.
> > 
> > -- 
> > Regards,
> > Shawn
>
Shawn Guo - Feb. 3, 2012, 1:27 p.m.
On Thu, Feb 02, 2012 at 09:09:03PM +0800, Shawn Guo wrote:
> On Thu, Feb 02, 2012 at 05:24:28PM +0800, Richard Zhao wrote:
> > On Thu, Feb 02, 2012 at 05:11:34PM +0800, Shawn Guo wrote:
...
> > >  Or we can simply
> > > make the it a audmux-self call with 3 parameters it needs retrieved
> > > from platform_data or device tree, so that machine driver does not
> > > even bother with the call.  Makes sense?
> > audux configuration may change after initial set. For example, it may use
> > one configuration for audio playback, but use another when you connect a
> > BT audio device.
> 
> Right.  I forgot this point.
> 
With a second thought on this, we can still do this as long as we have
mxc_audmux_v2_configure_port() exported.  For init-time setup, we can
just do it in audmux driver probe function.
Shawn Guo - Feb. 3, 2012, 1:32 p.m.
On Fri, Feb 03, 2012 at 10:15:54AM +0800, Richard Zhao wrote:
> adding Eric Bénard.
> 
> On Thu, Feb 02, 2012 at 09:58:07PM +0800, Richard Zhao wrote:
> > On Thu, Feb 02, 2012 at 09:09:03PM +0800, Shawn Guo wrote:
> > > On Thu, Feb 02, 2012 at 05:24:28PM +0800, Richard Zhao wrote:
> > > > On Thu, Feb 02, 2012 at 05:11:34PM +0800, Shawn Guo wrote:
> > > > > On Thu, Feb 02, 2012 at 04:55:23PM +0800, Shawn Guo wrote:
> > > > > > On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao wrote:
> > > > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > > > > ---
> > > > > > >  arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
> > > > > > >  arch/arm/plat-mxc/include/mach/mx31.h |    1 +
> > > > > > >  2 files changed, 11 insertions(+), 0 deletions(-)
> > > > > > > 
> > > > > > Hmm, let's see who are actually using mxc_audmux_v2_configure_port().
> > > > > > 
> > > > > > $ git grep -n mxc_audmux_v2_configure_port arch/arm/
> > > > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:246:     mxc_audmux_v2_configure_port(0,
> > > > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:254:     mxc_audmux_v2_configure_port(4,
> > > > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:257:     mxc_audmux_v2_configure_port(0,
> > > > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:265:     mxc_audmux_v2_configure_port(3,
> > > > > > arch/arm/mach-imx/mach-pcm043.c:365:    mxc_audmux_v2_configure_port(3,
> > > > > > arch/arm/mach-imx/mach-pcm043.c:371:    mxc_audmux_v2_configure_port(0,
> It's machine specific code, though audmux driver is common. Might be ok?

With audmux driver moved out, it's not ok to me to have board file
call this function.

> The asoc machine file for the above 3 boards is not platform driver, and
> don't get any plat info.

It can be nicely solved if we do these initial setup in audmux driver
probe function with the configuration data retrieved from audmux
platform data or device tree.
Richard Zhao - Feb. 5, 2012, 4:50 a.m.
Shawn Guo <shawn.guo@linaro.org> wrote:

>On Fri, Feb 03, 2012 at 10:15:54AM +0800, Richard Zhao wrote:
>> adding Eric Bénard.
>> 
>> On Thu, Feb 02, 2012 at 09:58:07PM +0800, Richard Zhao wrote:
>> > On Thu, Feb 02, 2012 at 09:09:03PM +0800, Shawn Guo wrote:
>> > > On Thu, Feb 02, 2012 at 05:24:28PM +0800, Richard Zhao wrote:
>> > > > On Thu, Feb 02, 2012 at 05:11:34PM +0800, Shawn Guo wrote:
>> > > > > On Thu, Feb 02, 2012 at 04:55:23PM +0800, Shawn Guo wrote:
>> > > > > > On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao
>wrote:
>> > > > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
>> > > > > > > ---
>> > > > > > >  arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
>> > > > > > >  arch/arm/plat-mxc/include/mach/mx31.h |    1 +
>> > > > > > >  2 files changed, 11 insertions(+), 0 deletions(-)
>> > > > > > > 
>> > > > > > Hmm, let's see who are actually using
>mxc_audmux_v2_configure_port().
>> > > > > > 
>> > > > > > $ git grep -n mxc_audmux_v2_configure_port arch/arm/
>> > > > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:246:    
>mxc_audmux_v2_configure_port(0,
>> > > > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:254:    
>mxc_audmux_v2_configure_port(4,
>> > > > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:257:    
>mxc_audmux_v2_configure_port(0,
>> > > > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:265:    
>mxc_audmux_v2_configure_port(3,
>> > > > > > arch/arm/mach-imx/mach-pcm043.c:365:   
>mxc_audmux_v2_configure_port(3,
>> > > > > > arch/arm/mach-imx/mach-pcm043.c:371:   
>mxc_audmux_v2_configure_port(0,
>> It's machine specific code, though audmux driver is common. Might be
>ok?
>
>With audmux driver moved out, it's not ok to me to have board file
>call this function.
>
>> The asoc machine file for the above 3 boards is not platform driver,
>and
>> don't get any plat info.
>
>It can be nicely solved if we do these initial setup in audmux driver
>probe function with the configuration data retrieved from audmux
>platform data or device tree.
no, it is asoc machine driver to have machine specific code. 
the machine driver do not correspond to any hw device, which cause hard to bind dt or create platform device.
>
>-- 
>Regards,
>Shawn
>
>_______________________________________________
>linux-arm-kernel mailing list
>linux-arm-kernel@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Richard Zhao - Feb. 14, 2012, 1:35 a.m.
On Sun, Feb 05, 2012 at 12:50:15PM +0800, Richard Zhao wrote:
> 
> 
> Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> >On Fri, Feb 03, 2012 at 10:15:54AM +0800, Richard Zhao wrote:
> >> adding Eric Bénard.
> >> 
> >> On Thu, Feb 02, 2012 at 09:58:07PM +0800, Richard Zhao wrote:
> >> > On Thu, Feb 02, 2012 at 09:09:03PM +0800, Shawn Guo wrote:
> >> > > On Thu, Feb 02, 2012 at 05:24:28PM +0800, Richard Zhao wrote:
> >> > > > On Thu, Feb 02, 2012 at 05:11:34PM +0800, Shawn Guo wrote:
> >> > > > > On Thu, Feb 02, 2012 at 04:55:23PM +0800, Shawn Guo wrote:
> >> > > > > > On Thu, Feb 02, 2012 at 10:12:05AM +0800, Richard Zhao
> >wrote:
> >> > > > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> >> > > > > > > ---
> >> > > > > > >  arch/arm/mach-imx/mach-mx31ads.c      |   10 ++++++++++
> >> > > > > > >  arch/arm/plat-mxc/include/mach/mx31.h |    1 +
> >> > > > > > >  2 files changed, 11 insertions(+), 0 deletions(-)
> >> > > > > > > 
> >> > > > > > Hmm, let's see who are actually using
> >mxc_audmux_v2_configure_port().
> >> > > > > > 
> >> > > > > > $ git grep -n mxc_audmux_v2_configure_port arch/arm/
> >> > > > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:246:    
> >mxc_audmux_v2_configure_port(0,
> >> > > > > > arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c:254:    
> >mxc_audmux_v2_configure_port(4,
> >> > > > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:257:    
> >mxc_audmux_v2_configure_port(0,
> >> > > > > > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c:265:    
> >mxc_audmux_v2_configure_port(3,
> >> > > > > > arch/arm/mach-imx/mach-pcm043.c:365:   
> >mxc_audmux_v2_configure_port(3,
> >> > > > > > arch/arm/mach-imx/mach-pcm043.c:371:   
> >mxc_audmux_v2_configure_port(0,
> >> It's machine specific code, though audmux driver is common. Might be
> >ok?
> >
> >With audmux driver moved out, it's not ok to me to have board file
> >call this function.
> >
> >> The asoc machine file for the above 3 boards is not platform driver,
> >and
> >> don't get any plat info.
> >
> >It can be nicely solved if we do these initial setup in audmux driver
> >probe function with the configuration data retrieved from audmux
> >platform data or device tree.
> no, it is asoc machine driver to have machine specific code. 
> the machine driver do not correspond to any hw device, which cause hard
> to bind dt or create platform device.
I'll have to keep audmux driver in arch/, till ASOC machine driver
has a way to get platfrom parameters.

Thanks
Richard
> >
> >-- 
> >Regards,
> >Shawn
> >
> >_______________________________________________
> >linux-arm-kernel mailing list
> >linux-arm-kernel@lists.infradead.org
> >http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
>
Mark Brown - Feb. 14, 2012, 6:06 a.m.
On Tue, Feb 14, 2012 at 09:35:07AM +0800, Richard Zhao wrote:
> On Sun, Feb 05, 2012 at 12:50:15PM +0800, Richard Zhao wrote:

> > no, it is asoc machine driver to have machine specific code. 
> > the machine driver do not correspond to any hw device, which cause hard
> > to bind dt or create platform device.

> I'll have to keep audmux driver in arch/, till ASOC machine driver
> has a way to get platfrom parameters.

Machine drivers can easily get platform data, they're just regular
drivers of whatever type so can get platform data in the same way that
any other driver for their bus can.
Richard Zhao - Feb. 14, 2012, 7:34 a.m.
On Mon, Feb 13, 2012 at 10:06:28PM -0800, Mark Brown wrote:
> On Tue, Feb 14, 2012 at 09:35:07AM +0800, Richard Zhao wrote:
> > On Sun, Feb 05, 2012 at 12:50:15PM +0800, Richard Zhao wrote:
> 
> > > no, it is asoc machine driver to have machine specific code. 
> > > the machine driver do not correspond to any hw device, which cause hard
> > > to bind dt or create platform device.
> 
> > I'll have to keep audmux driver in arch/, till ASOC machine driver
> > has a way to get platfrom parameters.
> 
> Machine drivers can easily get platform data, they're just regular
> drivers of whatever type so can get platform data in the same way that
> any other driver for their bus can.
Machine drivers don't correspond to any hw devices. If we create a
virtual platform device, it'll be hard for DT binding.

Thanks
Richard
Mark Brown - Feb. 14, 2012, 5:23 p.m.
On Tue, Feb 14, 2012 at 03:34:15PM +0800, Richard Zhao wrote:
> On Mon, Feb 13, 2012 at 10:06:28PM -0800, Mark Brown wrote:

> > Machine drivers can easily get platform data, they're just regular
> > drivers of whatever type so can get platform data in the same way that
> > any other driver for their bus can.

> Machine drivers don't correspond to any hw devices. If we create a
> virtual platform device, it'll be hard for DT binding.

As has been discussed repeatedly and at some considerable length the
board design for audio is considered sufficiently interesting to be
worth representing in the device tree directly, there are plenty of
choices made during board design.  There is absolutely no technical
problem from doing this on the device tree side.

People working on device tree really need to talk to each other more...

Patch

diff --git a/arch/arm/mach-imx/mach-mx31ads.c b/arch/arm/mach-imx/mach-mx31ads.c
index 4917aab..bb69e71 100644
--- a/arch/arm/mach-imx/mach-mx31ads.c
+++ b/arch/arm/mach-imx/mach-mx31ads.c
@@ -486,10 +486,20 @@  static unsigned int ssi_pins[] = {
 	MX31_PIN_STXD5__STXD5,
 };
 
+static const struct resource audmux_res[] __initconst = {
+	{
+		.start = MX31_AUDMUX_BASE_ADDR,
+		.end = MX31_AUDMUX_SIZE,
+		.flags = IORESOURCE_MEM,
+	},
+};
+
 static void __init mxc_init_audio(void)
 {
 	imx31_add_imx_ssi(0, NULL);
 	mxc_iomux_setup_multiple_pins(ssi_pins, ARRAY_SIZE(ssi_pins), "ssi");
+	imx_add_platform_device("audmux-v2", 0,
+				audmux_res, ARRAY_SIZE(audmux_res), NULL, 0);
 }
 
 /* static mappings */
diff --git a/arch/arm/plat-mxc/include/mach/mx31.h b/arch/arm/plat-mxc/include/mach/mx31.h
index e27619e..8a3d5ef 100644
--- a/arch/arm/plat-mxc/include/mach/mx31.h
+++ b/arch/arm/plat-mxc/include/mach/mx31.h
@@ -66,6 +66,7 @@ 
 #define MX31_RNGA_BASE_ADDR			(MX31_AIPS2_BASE_ADDR + 0xb0000)
 #define MX31_IPU_CTRL_BASE_ADDR			(MX31_AIPS2_BASE_ADDR + 0xc0000)
 #define MX31_AUDMUX_BASE_ADDR			(MX31_AIPS2_BASE_ADDR + 0xc4000)
+#define MX31_AUDMUX_SIZE			(SZ_16K)
 #define MX31_MPEG4_ENC_BASE_ADDR		(MX31_AIPS2_BASE_ADDR + 0xc8000)
 #define MX31_GPIO1_BASE_ADDR			(MX31_AIPS2_BASE_ADDR + 0xcc000)
 #define MX31_GPIO2_BASE_ADDR			(MX31_AIPS2_BASE_ADDR + 0xd0000)