[U-Boot] [PATCH v2 01/50] dm: clk: Add support for decoding clocks from the device tree

[Masahiro Yamada]

Hi Simon,


2016-01-21 11:46 GMT+09:00 Simon Glass <sjg at chromium.org>:
> Hi Masahiro,
>
> On 20 January 2016 at 19:26, Masahiro Yamada
> <yamada.masahiro at socionext.com> wrote:
>> Hi Simon,
>>
>>
>> 2016-01-21 0:24 GMT+09:00 Simon Glass <sjg at chromium.org>:
>>> Hi Masahiro,
>>>
>>> On 19 January 2016 at 22:38, Masahiro Yamada
>>> <yamada.masahiro at socionext.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>>
>>>> >>>
>>>> >>> +/**
>>>> >>> + * clk_get_by_index() - look up a clock referenced by a device
>>>> >>> + *
>>>> >>> + * Parse a device's 'clocks' list, returning information on the indexed clock,
>>>> >>> + * ensuring that it is activated.
>>>> >>> + *
>>>> >>> + * @dev:       Device containing the clock reference
>>>> >>> + * @index:     Clock index to return (0 = first)
>>>> >>> + * @clk_devp:  Returns clock device
>>>> >>> + * @return:    Peripheral ID for the device to control. This is the first
>>>> >>> + *             argument after the clock node phandle. If there is no arguemnt,
>>>> >>> + *             returns 0. Return -ve error code on any error
>>>> >>> + */
>>>> >>> +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp);
>>>> >>>  #endif /* _CLK_H_ */
>>>> >>
>>>> >>
>>>> >> I want #ifdef in the header too, like mine
>>>> >> http://patchwork.ozlabs.org/patch/566812/
>>>> >
>>>> > I am not keen on that idea since it clutters up header files and we'll
>>>> > get a link error anyway if something is missing. Anyway, I've added
>>>> > it.
>>>>
>>>>
>>>>
>>>> I am afraid there is misunderstanding here.
>>>>
>>>> Please see my patch carefully.
>>>>
>>>>
>>>> What I mean is like this:
>>>>
>>>>
>>>>   #if  ...
>>>>       declaration of function prototype
>>>>   #else
>>>>       static inline empty function
>>>>   #endif
>>>>
>>>>
>>>> This is a common technique to avoid a link error.
>>>
>>> Do you think this function will be called when device tree is not
>>> enabled? I cannot see how it would have any meaning in that case. What
>>> is the purpose?
>>
>>
>> In order to avoid build errors.
>>
>>
>> I think this coding style is often used.
>>
>>
>> For example, you can see include/linux/clk.h  in Linux.
>>
>>
>> #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>> struct clk *of_clk_get(struct device_node *np, int index);
>> struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
>> struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
>> #else
>> static inline struct clk *of_clk_get(struct device_node *np, int index)
>> {
>>         return ERR_PTR(-ENOENT);
>> }
>> static inline struct clk *of_clk_get_by_name(struct device_node *np,
>>                                              const char *name)
>> {
>>         return ERR_PTR(-ENOENT);
>> }
>> #endif
>>
>>
>>
>>
>> If every driver entry in Kconfig specifies "depends on OF_CONTROL" correctly,
>> no link error would happen for this.
>>
>> So, I leave this decision to you.
>> If you do not like #ifdef in header files, just rip it off.
>
> I'm not a huge fan of this - I feel that a link error might be
> preferable to a run-time error. But I don't really have a strong
> opinion on it. I've sent a new patch.
>


The only useful case I've come up with is
when OF_CONTROL is optional.


  ret = clk_get_by_index()
  if (ret == -ENOSYS) {
           If OF_CONTROL is disabled, try best effort
           to do something ...
   }



-- 
Best Regards
Masahiro Yamada