lkml.org 
[lkml]   [2015]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
    From
    Date
    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.



    \
     
     \ /
      Last update: 2015-10-07 15:41    [W:2.753 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site