Re: [Qemu-devel] [Qemu-arm] [PATCH 2/3] hw/sii9022: Add support for Silicon Image SII9022

[Peter Maydell <peter.maydell@linaro.org>]

On 17 February 2018 at 14:00, Linus Walleij <linus.walleij@linaro.org> wrote:
> This adds support for emulating the Silicon Image SII9022 DVI/HDMI
> bridge. It's not very clever right now, it just acknowledges
> the switch into DDC I2C mode and back. Combining this with the
> existing DDC I2C emulation gives the right behavior on the Versatile
> Express emulation passing through the QEMU EDID to the emulated
> platform.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  hw/display/Makefile.objs |   1 +
>  hw/display/sii9022.c     | 185 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 186 insertions(+)
>  create mode 100644 hw/display/sii9022.c
>
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index d3a4cb396eb9..3c7c75b94da5 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -3,6 +3,7 @@ common-obj-$(CONFIG_VGA_CIRRUS) += cirrus_vga.o
>  common-obj-$(CONFIG_G364FB) += g364fb.o
>  common-obj-$(CONFIG_JAZZ_LED) += jazz_led.o
>  common-obj-$(CONFIG_PL110) += pl110.o
> +common-obj-$(CONFIG_SII9022) += sii9022.o
>  common-obj-$(CONFIG_SSD0303) += ssd0303.o
>  common-obj-$(CONFIG_SSD0323) += ssd0323.o
>  common-obj-$(CONFIG_XEN) += xenfb.o
> diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
> new file mode 100644
> index 000000000000..d6f3cdc04293
> --- /dev/null
> +++ b/hw/display/sii9022.c
> @@ -0,0 +1,185 @@
> +/*
> + * Silicon Image SiI9022
> + *
> + * This is a pretty hollow emulation: all we do is acknowledge that we
> + * exist (chip ID) and confirm that we get switched over into DDC mode
> + * so the emulated host can proceed to read out EDID data. All subsequent
> + * set-up of connectors etc will be acknowledged and ignored.
> + *
> + * Copyright (c) 2018 Linus Walleij

Sanity check: really copyright you and not Linaro ?

> + *
> + * This code is licensed under the GNU GPL v2.
> + *
> + * Contributions after 2012-01-13 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.

If this is copyright 2018 then what parts of it are not covered by
the "contributions after 2012-01-13" condition? Could we just have
this be GPL-v2-or-later, which is what most new code is ?


> +static void sii9022_reset(DeviceState *dev)
> +{
> +    sii9022_state *s = SII9022(dev);
> +
> +    s->ptr = 0;
> +    s->addr_byte = false;

I think we should reset ddc/ddc_skip_finish/ddc_req
here too. I suspect the state machine logic means we
can't get to a place where their values are used once
we reset ptr and addr_byte, but it's easier to reason
about if we just reset all the device's state.

I agree with Philippe that we should use trace events here rather
than the DPRINTF macro. Otherwise the code looks fine.

thanks
-- PMM