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

[Marc Gonzalez <marc_gonzalez@sigmadesigns.com>]

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.)

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.)

> 	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))"

Thanks for the review.

Regards.