From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1754002AbbJGNTo (ORCPT <rfc822;w@1wt.eu>);
	Wed, 7 Oct 2015 09:19:44 -0400
Received: from mail1.bemta8.messagelabs.com ([216.82.243.199]:47499 "EHLO
	mail1.bemta8.messagelabs.com" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with ESMTP id S1753804AbbJGNTn (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 7 Oct 2015 09:19:43 -0400
X-Env-Sender: Marc_Gonzalez@sigmadesigns.com
X-Msg-Ref: server-10.tower-220.messagelabs.com!1444223837!14041052!1
X-Originating-IP: [195.215.56.170]
X-StarScan-Received: 
X-StarScan-Version: 6.13.16; banners=-,-,-
X-VirusChecked: Checked
Subject: Re: [PATCH v2] clocksource/drivers/tango_xtal: Add new timer for
 Tango SoCs
To: Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>
CC: LKML <linux-kernel@vger.kernel.org>, Mans Rullgard <mans@mansr.com>,
        Mason <slash.tmp@free.fr>
References: <5613E45C.5020208@sigmadesigns.com> <5614549F.2070002@linaro.org>
 <5614D66A.1060402@sigmadesigns.com> <56150369.2050609@sigmadesigns.com>
 <561510BF.7050207@linaro.org>
From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Message-ID: <56151B5B.90404@sigmadesigns.com>
Date: Wed, 7 Oct 2015 15:17:15 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Firefox/38.0 SeaMonkey/2.35
MIME-Version: 1.0
In-Reply-To: <561510BF.7050207@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Originating-IP: [172.27.0.114]
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.