diff mbox

[v2,03/14] clocksource: sp804: append CONFIG_OF

Message ID 1363108124-17484-4-git-send-email-haojian.zhuang@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Haojian Zhuang March 12, 2013, 5:08 p.m. UTC
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(+)

Comments

Arnd Bergmann March 12, 2013, 7:17 p.m. UTC | #1
(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
Haojian Zhuang March 13, 2013, 3:25 a.m. UTC | #2
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
Arnd Bergmann March 14, 2013, 1:48 p.m. UTC | #3
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 mbox

Patch

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