Re: [Qemu-devel] [PATCH 6/7] hw/timer/nrf51_timer: Add nRF51 Timer peripheral

[Stefan Hajnoczi]

On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<address@hidden> 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.