[PATCH 0/2] OMAP4: Add devices for HDMI audio

[PATCH 0/2] OMAP4: Add devices for HDMI audio

[Ricardo Neri]

The addition of two new audio devices is proposed. These two devices are
needed for the ASoC CPU and machine drivers for HDMI audio on
OMAP4.

Implementation of ASoC HDMI audio drivers can be found here:
 http://mailman.alsa-project.org/pipermail/alsa-devel/2011-May/039758.html

Implementation of DSS patches to support HDMI audio can be found here:
 http://www.mail-archive.com/linux-omap@vger.kernel.org/msg49011.html

This implementation is based on on:
* OMAP4: DSS2: Add dss_dss_clk opt clock for OMAP4, by Sumit Semwal
 (http://gitorious.org/linux-omap-dss2/linux/commit/0da5637569bb9bf93e6930cd1c3b452eede50ba2)
* Kernel 2.6.39-rc7
 (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git v2.6.39-rc7)
 
The complete implementation, with devices, DSS and ASoC parts, is available at:
 git://gitorious.org/omap-audio/linux-audio.git ricardon/topic/hdmi-audio-v2
 
Validation was performed on HDMI TV. Penguins were present on the display
and audio playback was performed with aplay at 32, 44.1 and 48kHz with S16_LE
and S24_LE sample formats. Validation was performed on SDP4430 ES2.1 and ES2.2
and Panda ES2.1 and ES2.0.

Ricardo Neri (2):
  OMAP4: HDMI: Add OMAP device for HDMI audio CPU DAI
  OMAP4: Add device for HDMI OMAP4 audio for ASoC machine driver

 arch/arm/mach-omap2/devices.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

[PATCH 1/2] OMAP4: HDMI: Add OMAP device for HDMI audio CPU DAI

[Ricardo Neri]

Add device for HDMI audio. This device uses hwmod data from
dss_hdmi. This device is to be used by the ASoC HDMI CPU DAI to
obtain base memory address and DMA request number.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 arch/arm/mach-omap2/devices.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 7b85585..56e2e98 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -313,6 +313,22 @@ OMAP_MCBSP_PLATFORM_DEVICE(5);
 
 static void omap_init_audio(void)
 {
+	struct omap_hwmod *oh_hdmi;
+	struct omap_device *od_hdmi;
+	char *oh_hdmi_name = "dss_hdmi";
+	char *dev_hdmi_name = "hdmi-audio-dai";
+
+	if (cpu_is_omap44xx()) {
+		oh_hdmi = omap_hwmod_lookup(oh_hdmi_name);
+		WARN(!oh_hdmi, "%s: could not find omap_hwmod for %s\n",
+			__func__, oh_hdmi_name);
+
+		od_hdmi = omap_device_build(dev_hdmi_name, -1, oh_hdmi, NULL, 0,
+			NULL, 0, false);
+		WARN(IS_ERR(od_hdmi), "%s: could not build omap_device for %s\n",
+			__func__, dev_hdmi_name);
+	}
+
 	platform_device_register(&omap_mcbsp1);
 	platform_device_register(&omap_mcbsp2);
 	if (cpu_is_omap243x() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
-- 
1.7.0.4

Re: [PATCH 1/2] OMAP4: HDMI: Add OMAP device for HDMI audio CPU DAI

[Steve Calfee]

On 05/16/11 20:14, Ricardo Neri wrote:

> +++ b/arch/arm/mach-omap2/devices.c
> @@ -313,6 +313,22 @@ OMAP_MCBSP_PLATFORM_DEVICE(5);
>  
>  static void omap_init_audio(void)
>  {
> +	struct omap_hwmod *oh_hdmi;
> +	struct omap_device *od_hdmi;
> +	char *oh_hdmi_name = "dss_hdmi";
> +	char *dev_hdmi_name = "hdmi-audio-dai";
> +
> +	if (cpu_is_omap44xx()) {
> +		oh_hdmi = omap_hwmod_lookup(oh_hdmi_name);
> +		WARN(!oh_hdmi, "%s: could not find omap_hwmod for %s\n",
> +			__func__, oh_hdmi_name);
> +
> +		od_hdmi = omap_device_build(dev_hdmi_name, -1, oh_hdmi, NULL, 0,
> +			NULL, 0, false);
> +		WARN(IS_ERR(od_hdmi), "%s: could not build omap_device for %s\n",
> +			__func__, dev_hdmi_name);
> +	}
> +
>  	platform_device_register(&omap_mcbsp1);
>  	platform_device_register(&omap_mcbsp2);
>  	if (cpu_is_omap243x() || cpu_is_omap34xx() || cpu_is_omap44xx()) {

I know you did not start this, but this cpu_is stuff is cheating. There
is a rule (maybe a guideline, or desire) in the kernel where they try to
minimize #ifdef in c code. So here we have a runtime ifdef. The code
will never be executed on other omap versions, but it takes up space and
obscures the code flow.

I think the generally accepted method of doing stuff like this is to
have the ifdeffery in a header file where a inline code segment is
defined if it applies to the processor being built. If the code does not
apply to the model being built, a null #define is used, which does not
take any space.

Using a conditional inline enables the only source code change for this
file (device.c) being a line where the inline code is called.

Regards, Steve

Re: [PATCH 1/2] OMAP4: HDMI: Add OMAP device for HDMI audio CPU DAI

[Peter Ujfalusi]

On Tuesday 17 May 2011 22:35:09 Steve Calfee wrote:
> I think the generally accepted method of doing stuff like this is to
> have the ifdeffery in a header file where a inline code segment is
> defined if it applies to the processor being built. If the code does not
> apply to the model being built, a null #define is used, which does not
> take any space.

We have for example omap2plus_defconfig, which include support for wide range 
of OMAP based devices, with different CPUs (OMAP2/3/4).
So we need to have runtime distinction of the CPU we are running (since we run 
the same kernel binary on OMAP2/3/4).

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] OMAP4: HDMI: Add OMAP device for HDMI audio CPU DAI

[Steve Calfee]

On 05/17/11 22:41, Peter Ujfalusi wrote:
> On Tuesday 17 May 2011 22:35:09 Steve Calfee wrote:
>> I think the generally accepted method of doing stuff like this is to
>> have the ifdeffery in a header file where a inline code segment is
>> defined if it applies to the processor being built. If the code does not
>> apply to the model being built, a null #define is used, which does not
>> take any space.
> 
> We have for example omap2plus_defconfig, which include support for wide range 
> of OMAP based devices, with different CPUs (OMAP2/3/4).
> So we need to have runtime distinction of the CPU we are running (since we run 
> the same kernel binary on OMAP2/3/4).
> 

Hi Peter,

I am not trying to start a flame war here, especially not with you
really helpful people.

The preferred header contained ifdeffery does not exclude the
possibility of having multiple options selected, even at run time. It
also can prevent multi cpu code bloat if it is not wanted.
Alternatively, X86 distributions such as Ubuntu already deal with
multiple arches (within the base arch intel/amd), using initrd type
startups.

Also the current "cpu_is" stuff is not very scalable, when TI gets to
Omap42, or even slightly smaller such as Omap5, the untidiness of the
current technique will become even more of a problem.

Regards, Steve

Re: [PATCH 1/2] OMAP4: HDMI: Add OMAP device for HDMI audio CPU DAI

[Ricardo Neri]

Hi Steve,

Le mercredi 18 mai 2011 à 12:07 -0500, Steve Calfee a écrit :
> On 05/17/11 22:41, Peter Ujfalusi wrote:
> > On Tuesday 17 May 2011 22:35:09 Steve Calfee wrote:
> >> I think the generally accepted method of doing stuff like this is to
> >> have the ifdeffery in a header file where a inline code segment is
> >> defined if it applies to the processor being built. If the code does not
> >> apply to the model being built, a null #define is used, which does not
> >> take any space.
> > 
> > We have for example omap2plus_defconfig, which include support for wide range 
> > of OMAP based devices, with different CPUs (OMAP2/3/4).
> > So we need to have runtime distinction of the CPU we are running (since we run 
> > the same kernel binary on OMAP2/3/4).
> > 
> 
> Hi Peter,
> 
> I am not trying to start a flame war here, especially not with you
> really helpful people.
> 
> The preferred header contained ifdeffery does not exclude the
> possibility of having multiple options selected, even at run time. It
> also can prevent multi cpu code bloat if it is not wanted.
> Alternatively, X86 distributions such as Ubuntu already deal with
> multiple arches (within the base arch intel/amd), using initrd type
> startups.
> 
> Also the current "cpu_is" stuff is not very scalable, when TI gets to
> Omap42, or even slightly smaller such as Omap5, the untidiness of the
> current technique will become even more of a problem.
> 
> Regards, Steve

Thanks for your comments. Even though the use of cpu_is_ may not be the
best approach, it is used extensively in OMAP code. Do you think that
this situation needs to be fixed before accepting the patches for these
two devices?

Also, in the case of OMAP, runtime CPU identification is needed, for the
reason Peter mentions. It is hard for me to see how it can be
implemented using #ifdef only. Do you think that runtime CPU
identification should be moved to the header containing the ifdeferry?
Individual functions for different domains (audio, video, etc.) would
still need to check at runtime if their devices are required. Perhaps
having a single omap2plus_defconfig for all OMAP2/3/4/x/y/... devices is
no longer convenient as more and more OMAP families arrive. What do you
think?

Regarding using a initrd, that applies only for loadable modules, but
not for built-in modules, correct?

BR,
Ricardo


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] OMAP4: HDMI: Add OMAP device for HDMI audio CPU DAI

[Steve Calfee]

On 05/23/11 20:39, Ricardo Neri wrote:
> Hi Steve,
> 
> Le mercredi 18 mai 2011 à 12:07 -0500, Steve Calfee a écrit :
>> On 05/17/11 22:41, Peter Ujfalusi wrote:
>>> On Tuesday 17 May 2011 22:35:09 Steve Calfee wrote:
>>>> I think the generally accepted method of doing stuff like this is to
>>>> have the ifdeffery in a header file where a inline code segment is
>>>> defined if it applies to the processor being built. If the code does not
>>>> apply to the model being built, a null #define is used, which does not
>>>> take any space.
>>>
<SNIP>
>>
>> The preferred header contained ifdeffery does not exclude the
>> possibility of having multiple options selected, even at run time. It
>> also can prevent multi cpu code bloat if it is not wanted.
>> Alternatively, X86 distributions such as Ubuntu already deal with
>> multiple arches (within the base arch intel/amd), using initrd type
>> startups.
>>
>> Also the current "cpu_is" stuff is not very scalable, when TI gets to
>> Omap42, or even slightly smaller such as Omap5, the untidiness of the
>> current technique will become even more of a problem.
>>
> 
> Thanks for your comments. Even though the use of cpu_is_ may not be the
> best approach, it is used extensively in OMAP code. Do you think that
> this situation needs to be fixed before accepting the patches for these
> two devices?
>

Hi Ricardo,

I am not the omap maintainer, I am not blocking anything.

I have lately been surfing the Omap code on a project. The cpu_is_*
stuff is common and growing. People are fixing it, see the patch for
gpio that was in today's mail.

I know you just want to get your stuff into the tree, not fix the entire
tree from problems you did not cause. However, someday, someone will
have to fix it up.

For instance as an example of how to remove #ifdefs --
arch/arm/mach-omap2/clock.h uses ifdeffery in the header to remove it
from the c code. It checks a config option and adds or removes code with
no source changes.

It is possible to add a config_all_options so run time checks could be
made for the cases when a single executable is desired for multiple
archs. In this case the macro or static inline could wrap the option
selected code with a runtime cpu_is check, all without modifying the .c
source code.

The optimizing compiler is really friendly to conditionally doing stuff,
if/else clauses that are never used (constants in conditionals) just
disappears.

Sample header file definitions (not even compile tested, in fact
intentionaly incomplete but based on your patch):

static inline  void __omap_init_hdmi_audio(struct your args) {
	oh_hdmi = omap_hwmod_lookup(oh_hdmi_name);
	WARN(!oh_hdmi, "%s: could not find omap_hwmod for %s\n",
			__func__, oh_hdmi_name);

	od_hdmi = omap_device_build(dev_hdmi_name, -1, oh_hdmi, NULL, 0,
			NULL, 0, false);
	WARN(IS_ERR(od_hdmi), "%s: could not build omap_device for %s\n",
	__func__, dev_hdmi_name);
}

#ifdef CONFIG_ALL_OPTIONS
static inline  void omap_init_hdmi_audio(struct your args)
{
	if (cpu_is_omap44xx())
		__omap_init_hdmi_audio(struct your args);
}
#else
#ifdef CONFIG_CPU_OMAP44XX
static inline  void omap_init_hdmi_audio(struct your args)
{
	__omap_init_hdmi_audio(struct your args);
}
#else
static inline  void omap_init_hdmi_audio(struct your args) {}
#endif

And then the .c code would just contain:
omap_init_hdmi_audio(struct your args);

> Also, in the case of OMAP, runtime CPU identification is needed, for the
> reason Peter mentions. It is hard for me to see how it can be
> implemented using #ifdef only. Do you think that runtime CPU
> identification should be moved to the header containing the ifdeferry?

Yes, based on configuration options. An alternative to ifdefs and cpu_is
is like today's omap gpio changes, where data is stuck in some structure
at init time and then used at run time to conditionally init stuff. This
can be fairly obscure, but so are macros, and it is caused by the
fundamentally difficult problems of a maze of arches similar, but not
identical.

> Individual functions for different domains (audio, video, etc.) would
> still need to check at runtime if their devices are required. Perhaps
> having a single omap2plus_defconfig for all OMAP2/3/4/x/y/... devices is
> no longer convenient as more and more OMAP families arrive. What do you
> think?

Such choices that are realistic runtime detected requirements are fine.
In most cases I have seen they are arch specific and board specific, so
really the capabilities should be platform specific and only actually
used on systems that need the capabilities.

> 
> Regarding using a initrd, that applies only for loadable modules, but
> not for built-in modules, correct?
> 

Yes, although I think it is possible for an initrd to select another
kernel to load. In any case that is distribution specific and should not
really be required for most omap kernel use. I merely suggested it was a
possible alternative to an all-in one kernel build for your specific
multiarch board. It is an alternative so my poor older omap does not
have to carry around code for omap4, just because someone, somewhere
might want it on their board. In general I think the header and
structure solutions are more applicable.

Regards, Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] OMAP4: HDMI: Add OMAP device for HDMI audio CPU DAI

[Cousson, Benoit]

On 5/24/2011 8:12 PM, Steve Calfee wrote:
> On 05/23/11 20:39, Ricardo Neri wrote:
>> Hi Steve,
>>
>> Le mercredi 18 mai 2011 à 12:07 -0500, Steve Calfee a écrit :
>>> On 05/17/11 22:41, Peter Ujfalusi wrote:
>>>> On Tuesday 17 May 2011 22:35:09 Steve Calfee wrote:
>>>>> I think the generally accepted method of doing stuff like this is to
>>>>> have the ifdeffery in a header file where a inline code segment is
>>>>> defined if it applies to the processor being built. If the code does not
>>>>> apply to the model being built, a null #define is used, which does not
>>>>> take any space.
>>>>
> <SNIP>
>>>
>>> The preferred header contained ifdeffery does not exclude the
>>> possibility of having multiple options selected, even at run time. It
>>> also can prevent multi cpu code bloat if it is not wanted.
>>> Alternatively, X86 distributions such as Ubuntu already deal with
>>> multiple arches (within the base arch intel/amd), using initrd type
>>> startups.
>>>
>>> Also the current "cpu_is" stuff is not very scalable, when TI gets to
>>> Omap42, or even slightly smaller such as Omap5, the untidiness of the
>>> current technique will become even more of a problem.
>>>
>>
>> Thanks for your comments. Even though the use of cpu_is_ may not be the
>> best approach, it is used extensively in OMAP code. Do you think that
>> this situation needs to be fixed before accepting the patches for these
>> two devices?
>>
>
> Hi Ricardo,
>
> I am not the omap maintainer, I am not blocking anything.
>
> I have lately been surfing the Omap code on a project. The cpu_is_*
> stuff is common and growing. People are fixing it, see the patch for
> gpio that was in today's mail.

It is not really growing. We are trying to get rid of it as much as we 
can. There are still some place where it is relevant, but most of the 
time it is not.

The proper way most is to use some features flag if it is a global HW 
characteristic.
In that case it is not even necessary since the hwmod does exist only if 
the HW is there.
So if the lookup is failing, there is no hdmi to handle.
At some point the devices will be instantiated based on a hwmod 
enumeration. So only the relevant devices will be created. That will 
removed most of these nasty cpu_is_ everywhere.

Regards,
Benoit

>
> I know you just want to get your stuff into the tree, not fix the entire
> tree from problems you did not cause. However, someday, someone will
> have to fix it up.
>
> For instance as an example of how to remove #ifdefs --
> arch/arm/mach-omap2/clock.h uses ifdeffery in the header to remove it
> from the c code. It checks a config option and adds or removes code with
> no source changes.
>
> It is possible to add a config_all_options so run time checks could be
> made for the cases when a single executable is desired for multiple
> archs. In this case the macro or static inline could wrap the option
> selected code with a runtime cpu_is check, all without modifying the .c
> source code.
>
> The optimizing compiler is really friendly to conditionally doing stuff,
> if/else clauses that are never used (constants in conditionals) just
> disappears.
>
> Sample header file definitions (not even compile tested, in fact
> intentionaly incomplete but based on your patch):
>
> static inline  void __omap_init_hdmi_audio(struct your args) {
> 	oh_hdmi = omap_hwmod_lookup(oh_hdmi_name);
> 	WARN(!oh_hdmi, "%s: could not find omap_hwmod for %s\n",
> 			__func__, oh_hdmi_name);
>
> 	od_hdmi = omap_device_build(dev_hdmi_name, -1, oh_hdmi, NULL, 0,
> 			NULL, 0, false);
> 	WARN(IS_ERR(od_hdmi), "%s: could not build omap_device for %s\n",
> 	__func__, dev_hdmi_name);
> }
>
> #ifdef CONFIG_ALL_OPTIONS
> static inline  void omap_init_hdmi_audio(struct your args)
> {
> 	if (cpu_is_omap44xx())
> 		__omap_init_hdmi_audio(struct your args);
> }
> #else
> #ifdef CONFIG_CPU_OMAP44XX
> static inline  void omap_init_hdmi_audio(struct your args)
> {
> 	__omap_init_hdmi_audio(struct your args);
> }
> #else
> static inline  void omap_init_hdmi_audio(struct your args) {}
> #endif
>
> And then the .c code would just contain:
> omap_init_hdmi_audio(struct your args);
>
>> Also, in the case of OMAP, runtime CPU identification is needed, for the
>> reason Peter mentions. It is hard for me to see how it can be
>> implemented using #ifdef only. Do you think that runtime CPU
>> identification should be moved to the header containing the ifdeferry?
>
> Yes, based on configuration options. An alternative to ifdefs and cpu_is
> is like today's omap gpio changes, where data is stuck in some structure
> at init time and then used at run time to conditionally init stuff. This
> can be fairly obscure, but so are macros, and it is caused by the
> fundamentally difficult problems of a maze of arches similar, but not
> identical.
>
>> Individual functions for different domains (audio, video, etc.) would
>> still need to check at runtime if their devices are required. Perhaps
>> having a single omap2plus_defconfig for all OMAP2/3/4/x/y/... devices is
>> no longer convenient as more and more OMAP families arrive. What do you
>> think?
>
> Such choices that are realistic runtime detected requirements are fine.
> In most cases I have seen they are arch specific and board specific, so
> really the capabilities should be platform specific and only actually
> used on systems that need the capabilities.
>
>>
>> Regarding using a initrd, that applies only for loadable modules, but
>> not for built-in modules, correct?
>>
>
> Yes, although I think it is possible for an initrd to select another
> kernel to load. In any case that is distribution specific and should not
> really be required for most omap kernel use. I merely suggested it was a
> possible alternative to an all-in one kernel build for your specific
> multiarch board. It is an alternative so my poor older omap does not
> have to carry around code for omap4, just because someone, somewhere
> might want it on their board. In general I think the header and
> structure solutions are more applicable.
>
> Regards, Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] OMAP4: Add device for HDMI OMAP4 audio for ASoC machine driver

[Ricardo Neri]

This device is used by the OMAP4 HDMI audio machine driver to register the
HDMI sound card.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 arch/arm/mach-omap2/devices.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 56e2e98..6add0d2 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -300,6 +300,14 @@ static struct platform_device omap_pcm = {
 };
 
 /*
+ * Device for the ASoC OMAP4 HDMI machine driver
+ */
+static struct platform_device omap4_hdmi_audio = {
+	.name	= "omap4-hdmi-audio",
+	.id	= -1,
+};
+
+/*
  * OMAP2420 has 2 McBSP ports
  * OMAP2430 has 5 McBSP ports
  * OMAP3 has 5 McBSP ports
@@ -327,6 +335,8 @@ static void omap_init_audio(void)
 			NULL, 0, false);
 		WARN(IS_ERR(od_hdmi), "%s: could not build omap_device for %s\n",
 			__func__, dev_hdmi_name);
+
+		platform_device_register(&omap4_hdmi_audio);
 	}
 
 	platform_device_register(&omap_mcbsp1);
-- 
1.7.0.4