| Message ID | 1363108124-17484-4-git-send-email-haojian.zhuang@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tuesday 12 March 2013, Haojian Zhuang wrote: > of_find_matching_node() & of_device_is_available() are depend on CONFIG_OF > macro. If CONFIG_OF isn't defined for non-DT mode, the build error > occurs. > > So add CONFIG_OF for those DT functions. I don't like this patch too much: We should be able to do this without the #ifdef. The problem is really that the declarations are also hidden behind an #ifdef. I think we should change the linux/of.h header file to either provide alternative empty inline functions or at least leave the declarations visible so we can compile the code as long as the call gets discarded. > > +#ifdef CONFIG_OF > static struct device_node *from = NULL; > > static struct of_device_id sp804_timer_match[] __initdata = { > @@ -294,3 +295,4 @@ err: > iounmap(base); > } > CLOCKSOURCE_OF_DECLARE(sp804, "arm,sp804", sp804_dt_init); > +#endif There is actually an empty definition for CLOCKSOURCE_OF_DECLARE(), but I think it would be better to still define it the same way as with CONFIG_OF enabled but attribute((__unused__)), like: #ifdef CONFIG_CLKSRC_OF extern void clocksource_of_init(void); #define CLOCKSOURCE_OF_DECLARE(name, compat, fn) \ static const struct of_device_id __clksrc_of_table_##name \ __used __section(__clksrc_of_table) \ = { .compatible = compat, .data = fn }; #else static inline void clocksource_of_init(void) {} #define CLOCKSOURCE_OF_DECLARE(name, compat, fn) \ static const struct of_device_id __clksrc_of_table_##name \ __unused = { .compatible = compat, .data = fn }; #endif This way, all the code still gets built, but since there is nothing pointing to __clksrc_of_table_##name and it is marked __unused, it gets dropped along with all other symbols that are only referenced from it. Arnd
(resending this email as I got a few rejects from mailing lists when I accidentally had an invalid email address for Mike) On Tuesday 12 March 2013, Haojian Zhuang wrote: > of_find_matching_node() & of_device_is_available() are depend on CONFIG_OF > macro. If CONFIG_OF isn't defined for non-DT mode, the build error > occurs. > > So add CONFIG_OF for those DT functions. I don't like this patch too much: We should be able to do this without the #ifdef. The problem is really that the declarations are also hidden behind an #ifdef. I think we should change the linux/of.h header file to either provide alternative empty inline functions or at least leave the declarations visible so we can compile the code as long as the call gets discarded. > > +#ifdef CONFIG_OF > static struct device_node *from = NULL; > > static struct of_device_id sp804_timer_match[] __initdata = { > @@ -294,3 +295,4 @@ err: > iounmap(base); > } > CLOCKSOURCE_OF_DECLARE(sp804, "arm,sp804", sp804_dt_init); > +#endif There is actually an empty definition for CLOCKSOURCE_OF_DECLARE(), but I think it would be better to still define it the same way as with CONFIG_OF enabled but attribute((__unused__)), like: #ifdef CONFIG_CLKSRC_OF extern void clocksource_of_init(void); #define CLOCKSOURCE_OF_DECLARE(name, compat, fn) \ static const struct of_device_id __clksrc_of_table_##name \ __used __section(__clksrc_of_table) \ = { .compatible = compat, .data = fn }; #else static inline void clocksource_of_init(void) {} #define CLOCKSOURCE_OF_DECLARE(name, compat, fn) \ static const struct of_device_id __clksrc_of_table_##name \ __unused = { .compatible = compat, .data = fn }; #endif This way, all the code still gets built, but since there is nothing pointing to __clksrc_of_table_##name and it is marked __unused, it gets dropped along with all other symbols that are only referenced from it. Arnd
On 13 March 2013 03:17, Arnd Bergmann <arnd@arndb.de> wrote: > (resending this email as I got a few rejects from mailing lists when > I accidentally had an invalid email address for Mike) > > On Tuesday 12 March 2013, Haojian Zhuang wrote: >> of_find_matching_node() & of_device_is_available() are depend on CONFIG_OF >> macro. If CONFIG_OF isn't defined for non-DT mode, the build error >> occurs. >> >> So add CONFIG_OF for those DT functions. > > I don't like this patch too much: > > We should be able to do this without the #ifdef. The problem is really that the > declarations are also hidden behind an #ifdef. I think we should change the > linux/of.h header file to either provide alternative empty inline functions > or at least leave the declarations visible so we can compile the code as > long as the call gets discarded. > >> >> +#ifdef CONFIG_OF >> static struct device_node *from = NULL; >> >> static struct of_device_id sp804_timer_match[] __initdata = { >> @@ -294,3 +295,4 @@ err: >> iounmap(base); >> } >> CLOCKSOURCE_OF_DECLARE(sp804, "arm,sp804", sp804_dt_init); >> +#endif > > There is actually an empty definition for CLOCKSOURCE_OF_DECLARE(), > but I think it would be better to still define it the same way as > with CONFIG_OF enabled but attribute((__unused__)), like: > > #ifdef CONFIG_CLKSRC_OF > extern void clocksource_of_init(void); > #define CLOCKSOURCE_OF_DECLARE(name, compat, fn) \ > static const struct of_device_id __clksrc_of_table_##name \ > __used __section(__clksrc_of_table) \ > = { .compatible = compat, .data = fn }; > #else > static inline void clocksource_of_init(void) {} > #define CLOCKSOURCE_OF_DECLARE(name, compat, fn) \ > static const struct of_device_id __clksrc_of_table_##name \ > __unused = { .compatible = compat, .data = fn }; > #endif > > This way, all the code still gets built, but since there is nothing > pointing to __clksrc_of_table_##name and it is marked __unused, it > gets dropped along with all other symbols that are only referenced > from it. > > Arnd > If CONFIG_CLKSRC_OF is depend on CONFIG_USEOF, I think that we can resolve all these issue. We don't need to define CLOCKSOURCE_OF_DECLARE() for non-DT mode, and we also don't need to define of_device_is_available(), ... in non-DT mode. We only need to add "depends on USE_OF" for CLKSRC_OF configuration. It's simpler. What's your opinion? Regards Haojian
On Wednesday 13 March 2013, Haojian Zhuang wrote: > If CONFIG_CLKSRC_OF is depend on CONFIG_USEOF, I think that > we can resolve all these issue. We don't need to define > CLOCKSOURCE_OF_DECLARE() for non-DT mode, and we also > don't need to define of_device_is_available(), ... in non-DT mode. > > We only need to add "depends on USE_OF" for CLKSRC_OF > configuration. It's simpler. What's your opinion? > I think that is not the right symbol. USE_OF is an ARM specific symbol that should not get selected from common code. Also I think 'depends on' is much better than 'select', because it has fewer side-effects. Right now, CLKSRC_OF is only selected by platforms that also select USE_OF on ARM, which seems appropriate for now, but if we want to make CLKSRC_OF a generally visible option, it should use 'depends on OF'. What I don't understand is how that relates to my comment on your code. My goal was to support drivers that can contain all the code needed for CLKSRC_OF without any #ifdef but that still work if CONFIG_OF and CONFIG_CLKSRC_OF are both disabled. Arnd
diff --git a/drivers/clocksource/timer-sp.c b/drivers/clocksource/timer-sp.c index f8c2c54..5f2d700 100644 --- a/drivers/clocksource/timer-sp.c +++ b/drivers/clocksource/timer-sp.c @@ -197,6 +197,7 @@ void __init sp804_clockevents_init(void __iomem *base, unsigned int irq, clockevents_config_and_register(evt, rate, 0xf, 0xffffffff); } +#ifdef CONFIG_OF static struct device_node *from = NULL; static struct of_device_id sp804_timer_match[] __initdata = { @@ -294,3 +295,4 @@ err: iounmap(base); } CLOCKSOURCE_OF_DECLARE(sp804, "arm,sp804", sp804_dt_init); +#endif
of_find_matching_node() & of_device_is_available() are depend on CONFIG_OF macro. If CONFIG_OF isn't defined for non-DT mode, the build error occurs. So add CONFIG_OF for those DT functions. Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> --- drivers/clocksource/timer-sp.c | 2 ++ 1 file changed, 2 insertions(+)