[U-Boot] [PATCH v2 01/50] dm: clk: Add support for decoding clocks from the device tree
Simon Glass
sjg at chromium.org
Thu Jan 21 03:46:20 CET 2016
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.
Regards,
Simon
More information about the U-Boot
mailing list