From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754160AbbJGQD5 (ORCPT ); Wed, 7 Oct 2015 12:03:57 -0400 Received: from mail-wi0-f178.google.com ([209.85.212.178]:36652 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753235AbbJGQDz (ORCPT ); Wed, 7 Oct 2015 12:03:55 -0400 Subject: Re: [PATCH v2] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs To: Marc Gonzalez , Thomas Gleixner References: <5613E45C.5020208@sigmadesigns.com> <5614549F.2070002@linaro.org> <5614D66A.1060402@sigmadesigns.com> <56150369.2050609@sigmadesigns.com> <561510BF.7050207@linaro.org> <56151B5B.90404@sigmadesigns.com> Cc: LKML , Mans Rullgard , Mason From: Daniel Lezcano Message-ID: <56154268.5060700@linaro.org> Date: Wed, 7 Oct 2015 18:03:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <56151B5B.90404@sigmadesigns.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 >>> --- >>> 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. > > > > 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 -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog