[U-Boot] [PATCH v7 01/12] clk: Use dummy clk_get_by_* functions when CONFIG_CLK is disabled

[Masahiro Yamada]

Hi Paul,

2016-09-12 0:14 GMT+09:00 Paul Burton <paul.burton at imgtec.com>:
> On 11/09/16 14:25, Masahiro Yamada wrote:
>> Hi Paul,
>>
>> 2016-09-09 17:01 GMT+09:00 Paul Burton <paul.burton at imgtec.com>:
>>> On 09/09/16 04:15, Masahiro Yamada wrote:
>>>> 2016-09-08 15:47 GMT+09:00 Paul Burton <paul.burton at imgtec.com>:
>>>>> The implementations of clk_get_by_index & clk_get_by_name are only
>>>>> available when CONFIG_CLK is enabled.
>>>>
>>>> Unless I am missing something,
>>>> I think this statement also applies to other clk API functions
>>>> such as clk_request(), clk_free(), clk_get_rate(), etc.
>>>
>>> Hi Masahiro,
>>>
>>> Yes, I agree. To be clear though, this patch doesn't add any new stub
>>> functions it simply makes the conditions for the existing ones being
>>> provided match the conditions for the real implementations not being
>>> provided.
>>>
>>>>> Provide the dummies when this is
>>>>> not the case in order to avoid build failures.
>>>>
>>>> Why are other functions OK without dummy stubs?
>>>
>>> In general, I presume because they aren't used.
>>>
>>> In the specific case I'm using clk_get_by_index for
>>> (drivers/serial/ns16550.c in patch 2 of this series) the fact that the
>>> dummy clk_get_by_index always returns an error will cause the compiler
>>> to optimise out a call to clk_get_rate so any dummy implementation
>>> provided for it wouldn't really get used.
>>
>> I see, but I do not think it is a good idea
>> to rely on the optimization by compiler in this case.
>>
>> Could you add stubs for other APIs, please?
>
> Hi Masahiro,
>
> I don't mind doing that if it's deemed the right approach, but I see it
> as very much a separate change to this patch which fixes the condition
> for providing the existing stubs so would like if we can consider it
> separately.
>
> As to whether providing stubs for the other functions is correct - I'm
> not so sure. Right now, code that makes use of the clock functions but
> can also function without CONFIG_CLK will either:
>
>   - Correctly check for errors from the clk_get_* functions and see
> calls to the other clock functions get optimised out. This is what
> happens in the ns16550 code after my patch 2 of this series, and it is
> also the case for other code already in U-Boot (eg.
> drivers/usb/host/ehci-generic.c).
>
>   - Get it wrong, not check for errors from clk_get_* properly & get a
> fairly easy to track down link error when building U-Boot.
>
> If we add stub functions for the rest of the clk_* functions then that
> second case would become a runtime error rather than a link time one,
> and could be missed or overlooked much more easily. If we were to make
> the stubs do something like the Linux BUILD_BUG_ON macro then that would
> solve that problem, but would require all users of clk_ functions to
> #ifdef on CONFIG_CLK. So I actually think the current situation isn't so
> bad.

OK, thanks for explaining this.
(For the case, perhaps drivers that need clk can have
"depends on CLK" in Kconfig.  Anyway, this should be considered separately.)

So, let's go this series as it is.



-- 
Best Regards
Masahiro Yamada