Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754445Ab1BYIOg (ORCPT ); Fri, 25 Feb 2011 03:14:36 -0500 Received: from moutng.kundenserver.de ([212.227.126.187]:58235 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752774Ab1BYIOd (ORCPT ); Fri, 25 Feb 2011 03:14:33 -0500 From: Arnd Bergmann To: Peppe CAVALLARO Subject: Re: [PATCH (sh-2.6) 1/4] clksource: Generic timer infrastructure Date: Thu, 24 Feb 2011 18:20:55 +0100 User-Agent: KMail/1.12.2 (Linux/2.6.31-22-generic; KDE/4.3.2; x86_64; ; ) Cc: "linux-sh@vger.kernel.org" , "netdev@vger.kernel.org" , Stuart MENEFY , John Stultz , Thomas Gleixner , linux-kernel@vger.kernel.org References: <1298369864-24429-1-git-send-email-peppe.cavallaro@st.com> <1298369864-24429-2-git-send-email-peppe.cavallaro@st.com> In-Reply-To: <1298369864-24429-2-git-send-email-peppe.cavallaro@st.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201102241820.55873.arnd@arndb.de> X-Provags-ID: V02:K0:an6r35MH/EtBvKSsUfCSNtS3mPsLEYMq89+0CTWktw5 MfP/WmqGHtOa4hv+zbD41Ao84r2NnVPQLiAmTpmkjRpUALCPeV LlOY9SmGM7/AI/QodtAvOix7Lk7MX5xHCj1pIyO0HJ8zwJWodS i67pQ6Jv70KBvlJK67v02psV5B89dkJRdE34K9RxDYsJX3B87g 7BdmDm2XgyUTlJmJhPr4Q== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3725 Lines: 103 On Tuesday 22 February 2011, Peppe CAVALLARO wrote: > From: Stuart Menefy > > Many devices targeted at the embedded market provide a number of > generic timers which are capable of generating interrupts at a > requested rate. These can then be used in the implementation of drivers > for other peripherals which require a timer interrupt, without having > to provide an additional timer as part of that peripheral. > > A code provides a simple abstraction layer which allows a timer to be > registered, and for a driver to request a timer. > > Currently this doesn't provide any of the additional information, such > as precision or position in clock framework which might be required > for a fully featured driver. This code should probably be discussed on a more broader platform than the netdev and linux-sh mailing lists, as the scope is neither sh nor network specific. You should at least add linux-kernel@vger.kernel.org, possibly also linux-arch@vger.kernel.org. Further, John and Thomas are responsible for the timekeeping infrastructure, and they are probably interested in this as well. Why is this code useful to you? In the scenarios I've seen, the board can always assign a timer to a specific device in a fixed way that can be describe in a board file or device tree. Also, what is the difference between this and clkdev? > Signed-off-by: Stuart Menefy > Hacked-by: Giuseppe Cavallaro > --- > drivers/clocksource/Makefile | 1 + > drivers/clocksource/generictimer.c | 60 ++++++++++++++++++++++++++++++++++++ > include/linux/generictimer.h | 41 ++++++++++++++++++++++++ > 3 files changed, 102 insertions(+), 0 deletions(-) > create mode 100644 drivers/clocksource/generictimer.c > create mode 100644 include/linux/generictimer.h I don't think it fits that well into the drivers/clocksource directory, because you don't actually register a struct clock_event_device or struct clocksource. I don't know if this could also be merged with the clocksource infrastructure, but your code currently doesn't do that. > +struct generic_timer *generic_timer_claim(void (*handler) (void *), void *data) > +{ > + struct generic_timer *gt = NULL; > + > + if (!handler) { > + pr_err("%s: invalid handler\n", __func__); > + return NULL; > + } > + > + mutex_lock(>_mutex); > + if (!list_empty(>_list)) { > + struct list_head *list = gt_list.next; > + list_del(list); > + gt = container_of(list, struct generic_timer, list); > + } > + mutex_unlock(>_mutex); > + > + if (!gt) > + return NULL; > + > + /* Prepare the new handler */ > + gt->priv_handler = handler; > + gt->data = data; > + > + return gt; > +} This does not seem very generic. You put timers into the list and take them out again, but don't have any way to deal with timers that match specific purposes. It obviously works for your specific use case where you register exactly one timer, and use that in exactly one driver. If more drivers were converted to generic_timer, which is obviously the intention, then you might have a situation with very different timers (fixed/variable tick, high/low frequencies, accurate/inaccurate), or you might have fewer timers than users. > +static inline void generic_timer_set_rate(struct generic_timer *gt, > + unsigned long rate) > +{ > + gt->set_rate(gt, rate); > +} > + > +#endif /* __STM_GENERIC_TIMER_H */ Shouldn't this one allow a return code? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/