Re: [Qemu-devel] [RFC 5/8] arm: Add NRF51 random number generator peripheral

[Peter Maydell]

On 27 June 2018 at 08:33, Steffen Görtz <address@hidden> wrote:
> Add a model of the NRF51 random number generator peripheral.
>
> Changes since v2:
>   - Add missing 'qapi/error.h' for error_abort
>
> Changes since v1:
>   - Add implementation access size hints to MemoryRegionOps
>   - Fail on error if qcrypto_random_bytes fails
>   - Add references to Nrf51 datasheets
>
> Signed-off-by: Steffen Görtz <address@hidden>
> ---

Hi -- a couple of general notes about device modelling
which might apply to other devices in this series as well:

> +static void nrf51_rng_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = nrf51_rng_properties;

In general, every device that has any modifiable state needs:
 * a reset function
 * a vmstate struct that defines its state for migration

These get set up in the class init function by setting
dc->vmsd and dc->reset.

For devices that don't have any modifiable state, a comment
in the init function to say so helps to reassure readers
that the issue has been considered :-)

> +}


> +    struct {
> +        uint32_t active:1;
> +        uint32_t event_valrdy:1;
> +        uint32_t shortcut_stop_on_valrdy:1;
> +        uint32_t interrupt_enabled:1;
> +        uint32_t filter_enabled:1;
> +    } state;

Prefer to avoid bitfields.

thanks
-- PMM