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

["Cousson, Benoit" <b-cousson@ti.com>]

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