Re: [PATCH v2] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 10/07/2015 03:17 PM, Marc Gonzalez wrote:
> On 07/10/2015 14:31, Daniel Lezcano wrote:
>> On 10/07/2015 01:35 PM, Marc Gonzalez wrote:
>>> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
>>> Use it for clocksource, sched_clock, and delay_timer.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>> AFAICS, clocksource_register_hz does not report failures via its
>>> return value (always 0) but writes warnings to stdout?
>>
>> Yeah, it returns always 0. I suggest you assume it is returning an error
>> code, that will be safer for future changes in the framework (if any).
>
> I suppose I'd also need to check the return value of of_clk_get?
> (Looks like you mention it implicitly below.)
>
>>> Open question: can I call register_current_timer_delay,
>>> sched_clock_register, clocksource_register_hz in any order?
>>> ---
>>
>> Yes, I think so. Thomas ?
>>
>> [ ... ]
>>
>>> +static void __init tango_clocksource_init(struct device_node *np)
>>> +{
>>> +	struct clk *clk = of_clk_get(np, 0);
>>> +	unsigned int xtal_freq = clk_get_rate(clk);
>>> +	xtal_in_cnt = of_iomap(np, 0);
>>> +	if (xtal_in_cnt == NULL)
>>> +		panic("%s: of_iomap failed\n", np->full_name);
>>
>>     ^^^^^^^^^^^
>>
>> That does not comply with the Linux kernel coding style.
>
> <confused>
>
> scripts/checkpatch.pl only complains about a missing blank line after
> the declaration block. (Sorry, I'll fix that.)
>
>> 	xtal_in_cnt = of_iomap(np, 0);
>> 	if (!xtal_in_cnt) {
>> 		pr_err("Argh!");
>> 		return;
>> 	}
>
> I know "!xtal_in_cnt" is equivalent to "xtal_in_cnt == NULL" but I'd
> rather emphasize the fact that xtal_in_cnt is a pointer, not a bool.
> (Documentation/CodingStyle does not mandate this particular idiom.)

Yes, if you prefer you can use != NULL.

> I'm also confused that you've replaced panic() with pr_err/return.
> AFAIU, if I don't have a clocksource/sched_clock, the system is dead
> in the water. Might as well stop there, and wait for the operator to
> fix whatever needs fixing. (Several clksrc drivers do this.)

Hmm, yeah that's true but also we have platforms with different 
clocksources, so we don't want to panic if the next clocksource will 
succeed. That's the logic behind not doing panic. There is some legacy 
code still using panic but that should be fixed.

I don't know if your platform can fall under this category, but it would 
be a good practice to pr_err or pr_warn instead of panic in order to be 
consistent with the current direction in the recent drivers.

>> 	clk = of_clk_get(np, 0);
>> 	if (!clk) {
>
> AFAICT, checking for NULL is not good enough here.
> of_clk_get returns ERR_PTR(rc) style errors.
> Looks like I'd need "if (IS_ERR(clk))"

Yep :)


   -- Daniel



-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog