RE: [PATCH v2 2/3] clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer

Noam Camus
2016-10-31T16:53:34Z

> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> Sent: Monday, October 31, 2016 12:28 PM

>> From: Noam Camus <noamca@mellanox.com>
>>
>> nps_setup_clocksource() should take node as only argument i.e.:
>> replace
>> int __init nps_setup_clocksource(struct device_node *node, struct clk
>> *clk) with int __init nps_setup_clocksource(struct device_node *node)
>>
>> This is also serve as preperation for next patch which adds support
>
>s/preperation/preparation/
Thanks, will fix in V4 of this patch set
...

>> +static int nps_get_timer_clk(struct device_node *node,
>> + unsigned long *timer_freq,
>> + struct clk *clk)
>
>This function prototype does not make sense. A pointer to a clock is passed for nothing here.
Thanks, I passed *clk in order for one to do rollback on error (pass clk to clk_disable_unprepare).
I will change prototype to **clk.

>> +{
>> + int ret;
>> +
>> + clk = of_clk_get(node, 0);
>> + if (IS_ERR(clk)) {
>> + pr_err("timer missing clk");
>> + return PTR_ERR(clk);
>> + }
>> +
>> + ret = clk_prepare_enable(clk);
>> + if (ret) {
>> + pr_err("Couldn't enable parent clk\n");
>> + return ret;
>> + }
>> +
>> + *timer_freq = clk_get_rate(clk);
>> +
>
> timer_freq check.
>
> rollback on error.
Thanks, will fix at V4

...
>> - if (ret) {
>> - pr_err("Couldn't enable parent clock\n");
>> - return ret;
>> - }
>> -
>> - nps_timer_rate = clk_get_rate(clk);
>> + nps_get_timer_clk(node, &nps_timer_rate, clk);

>Return code check ?
Thanks, will fix at V4

-Noam