On 25/11/15 15:10, Daniel Lezcano wrote: > On 11/25/2015 03:51 PM, Vladimir Murzin wrote: >> Hi Daniel, >> >> Thanks for you review, I agree on all concerns raised and address them >> in the next version. Just some points to confirm below (I left only >> relevant parts). >> >>>> +static irqreturn_t mps2_timer_interrupt(int irq, void *dev_id) >>>> +{ >>>> + struct clockevent_mps2 *ce = dev_id; >>>> + u32 status = readl(ce->reg + TIMER_INT); >>>> + >>>> + if (!status) >>>> + return IRQ_NONE; >>> >>> Why that could happen ? Add a comment. >>> >> >> Sort of defensive programming, I never seen it happens, but just in case >> of spurious interrupts... Do you prefer to get rid of this check or >> >> /* spurious interrupt? */ >> if (!status) >> return IRQ_NONE; >> >> would be fine to you? > > Yes, that would be fine, but if it does never happen, we don't really > need this check. If you prefer to make sure there isn't a spurious > interrupt and considering it shouldn't happen, a pr_info trace may help > to spot this misbehavior. > Good, I'll put pr_warn("spurious interrupt\n") then. >>>> +} >>>> + >>>> +static void __init mps2_timer_init(struct device_node *np) >>>> +{ >>>> + static int clksrc; >>>> + >>>> + if (!clksrc && !mps2_clocksource_init(np)) >>>> + clksrc = 1; >>>> + else >>>> + mps2_clockevents_init(np); >>> >>> That assumes the clocksource is defined before the clockevents in the >>> DT. If it is not the case, the mps2_clocksource_init will fail (and spit >>> errors) and mps2_clockevents_init() won't be called. >>> >> >> Does following (stolen from efm32) look better to you? >> >> static void __init mps2_timer_init(struct device_node *np) >> { >> static int has_clocksource, has_clockevent; >> int ret; >> >> if (!has_clocksource) { >> ret = mps2_clocksource_init(np); >> if (!ret) { >> has_clocksource = 1; >> return; >> } >> } >> >> if (!has_clockevent) { >> ret = mps2_clockevent_init(np); >> if (!ret) { >> has_clockevent = 1; >> return; >> } >> } >> } > > Yes. > > I don't like to have a "CLOCKSOURCE_OF_DECLARE" to initialize a > clockevent but I can't blame you, this is what is done in the other > drivers. Thanks for your time! Vladimir > >>>> +} >>>> + >>>> +CLOCKSOURCE_OF_DECLARE(mps2_timer, "arm,mps2-timer", mps2_timer_init); >>>> >>> >>> >> > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html