On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz <cont...@steffen-goertz.de> wrote: > +/* Trigger */ > +#define NRF51_TRIGGER_TASK 0x01 > + > +/* Events */ > +#define NRF51_EVENT_CLEAR 0x00
NRF51_TRIGGER_TASK and NRF51_EVENT_CLEAR are generic nRF51 SoC constants. Please put them in a header to avoid duplication. > +static void nrf51_timer_reset(DeviceState *dev) > +{ > + Nrf51TimerState *s = NRF51_TIMER(dev); > + > + s->runstate = NRF51_TIMER_STOPPED; This leaves the timer pending if it was scheduled and then reset is called. I think timer_del() is necessary. > diff --git a/include/hw/timer/nrf51_timer.h b/include/hw/timer/nrf51_timer.h > new file mode 100644 > index 0000000000..a1278f17bd > --- /dev/null > +++ b/include/hw/timer/nrf51_timer.h > @@ -0,0 +1,63 @@ > +/* > +* nRF51 System-on-Chip Timer peripheral > + * > + * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf > + * Product Spec: http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf > + * > + * QEMU interface: > + * + sysbus MMIO regions 0: GPIO registers > + * + sysbus irq > + * > + * Accuracy of the peripheral model: > + * + Only TIMER mode is implemented, COUNTER mode is not implemented. This information was in a doc comment above the device state struct in other patches you've sent. Here it's at the top of the file. For consistency please put it in a doc comment. > +typedef enum { > + NRF51_TIMER_STOPPED = 0, > + NRF51_TIMER_RUNNING > +} Nrf51TimerRunstate; This is basically a bool and the Nrf51TimerState->runstate field is uint8_t. The code can be simplified by dropping the enum and changing the field to "bool running". > +typedef enum { > + NRF51_TIMER_TIMER = 0, > + NRF51_TIMER_COUNTER = 1 > +} Nrf51TimerMode; Since Nrf51TimerMode isn't used in this header file (the Nrf51TimerState->mode field is uint32_t), this enum is internal to the device implementation. It can be moved to the .c file.