From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46915) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uh6y5-0006Hl-74 for qemu-devel@nongnu.org; Mon, 27 May 2013 19:36:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uh6xv-0006Ul-Rh for qemu-devel@nongnu.org; Mon, 27 May 2013 19:36:45 -0400 Received: from lemon.ertos.nicta.com.au ([203.143.174.143]:51806) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uh6xv-0006UD-Go for qemu-devel@nongnu.org; Mon, 27 May 2013 19:36:35 -0400 Date: Tue, 28 May 2013 09:36:22 +1000 Message-ID: <84d2sct2q1.wl%peter@chubb.wattle.id.au> From: Peter Chubb In-Reply-To: <1369691927-27832-1-git-send-email-jcd@tribudubois.net> References: <1369691927-27832-1-git-send-email-jcd@tribudubois.net> MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Subject: Re: [Qemu-devel] [PATCH] i.MX: Improve EPIT timer code. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jean-Christophe DUBOIS Cc: peter.maydell@linaro.org, peter.chubb@nicta.com.au, qemu-devel@nongnu.org Hi Jean, Thanks for this. Most of it appears cosmetic and an improvement. Comments in-line below. > > /* > * Update interrupt status > */ > -static void imx_timerp_update(IMXTimerPState *s) > +static void imx_timer_epit_update(IMXTimerEPITState *s) > { > - if (s->sr && (s->cr & CR_OCIEN)) { > + if (s->sr && (s->cr & CR_OCIEN) && (s->cr & CR_EN)) { Why not if (s->sr && (s->cr & (CR_OCIEN|CR_EN) == (CR_OCIEN|CR_EN))) > qemu_irq_raise(s->irq); > } else { > qemu_irq_lower(s->irq); > } > } > > -static void set_timerp_freq(IMXTimerPState *s) > +static void imx_timer_epit_set_freq(IMXTimerEPITState *s) > { > unsigned clksrc; > unsigned prescaler; > - uint32_t freq; getting rid of this variable means a pointer dereference every time below. That's OK at some optimisation levels and for some compilers as they'll cache the variable. But I prefer making this explicit so less competent compilers can avoid the pointer dereference. > > clksrc = extract32(s->cr, CR_CLKSRC_SHIFT, 2); > prescaler = 1 + extract32(s->cr, CR_PRESCALE_SHIFT, 12); > > - freq = imx_clock_frequency(s->ccm, imx_timerp_clocks[clksrc]) / prescaler; > + s->freq = imx_clock_frequency(s->ccm, > + imx_timer_epit_clocks[clksrc]) / prescaler; > > - s->freq = freq; > DPRINTF("Setting ptimer frequency to %u\n", freq); And here it's inconsistent --- s/freq/s->freq/ and as you've renamed, maybe s/ptimer/epit/ The rest looks OK. I'm not that keen on the longer names, though. Maybe s/timer_epit/epit/ throughout. -- Dr Peter Chubb peter.chubb AT nicta.com.au http://www.ssrg.nicta.com.au Software Systems Research Group/NICTA