Re: [PATCH v3 2/5] intc: add goldfish-pic

[Philippe Mathieu-Daudé]

On 3/4/21 11:01 PM, Laurent Vivier wrote:
> Implement the goldfish pic device as defined in
> 
> https://android.googlesource.com/platform/external/qemu/+/master/docs/GOLDFISH-VIRTUAL-HARDWARE.TXT
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  include/hw/intc/goldfish_pic.h |  33 +++++
>  hw/intc/goldfish_pic.c         | 214 +++++++++++++++++++++++++++++++++
>  hw/intc/Kconfig                |   3 +
>  hw/intc/meson.build            |   1 +
>  hw/intc/trace-events           |   8 ++
>  5 files changed, 259 insertions(+)
>  create mode 100644 include/hw/intc/goldfish_pic.h
>  create mode 100644 hw/intc/goldfish_pic.c

> +static const MemoryRegionOps goldfish_pic_ops = {
> +    .read = goldfish_pic_read,
> +    .write = goldfish_pic_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid.max_access_size = 4,

Missing:

       .impl.min_access_size = 4,

> +    .impl.max_access_size = 4,
> +};
> +
> +static void goldfish_pic_reset(DeviceState *dev)
> +{
> +    GoldfishPICState *s = GOLDFISH_PIC(dev);
> +    int i;
> +
> +    trace_goldfish_pic_reset(s, s->idx);
> +    s->pending = 0;
> +    s->enabled = 0;
> +
> +    for (i = 0; i < ARRAY_SIZE(s->stats_irq_count); i++) {
> +        s->stats_irq_count[i] = 0;
> +    }
> +}
> +
> +static void goldfish_pic_realize(DeviceState *dev, Error **errp)
> +{
> +    GoldfishPICState *s = GOLDFISH_PIC(dev);
> +    static int counter;
> +
> +    s->idx = counter++;

Isn't it a bit fragile? Aren't we better using DEFINE_PROP_UINT8()?

> +    trace_goldfish_pic_realize(s, s->idx);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &goldfish_pic_ops, s,
> +                          "goldfish_pic", 0x24);
> +}

Adding .impl.min_access_size:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>