Hi Simon,
2016-01-21 11:46 GMT+09:00 Simon Glass <s...@chromium.org>: > Hi Masahiro, > > On 20 January 2016 at 19:26, Masahiro Yamada > <yamada.masah...@socionext.com> wrote: >> Hi Simon, >> >> >> 2016-01-21 0:24 GMT+09:00 Simon Glass <s...@chromium.org>: >>> Hi Masahiro, >>> >>> On 19 January 2016 at 22:38, Masahiro Yamada >>> <yamada.masah...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot