| Message ID | 1305602079-3852-2-git-send-email-ricardo.neri@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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
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
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
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()) {
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(-)