| Message ID | 1328148728-32258-7-git-send-email-richard.zhao@linaro.org |
|---|---|
| State | New |
| Headers | show |
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.
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. >
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.
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.
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).
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.
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.
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)
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(-)