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
