Re: [PATCH 18/24] display/sm501 display/ati: Fix to realize "i2c-ddc"

[BALATON Zoltan]

On Mon, 18 May 2020, Markus Armbruster wrote:
> sm501_init() and ati_vga_realize() create an "i2c-ddc" device, but
> neglect to realize it.  Affects machines sam460ex, shix, r2d, and
> fulong2e.
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing the
> device, yet nobody noticed.

No idea why it worked but guests can read EDID info fine with or without 
this patch, so

Tested-by: BALATON Zoltan <address@hidden>

Maybe device is created and working after init as it has nothing special 
to do at realize (it doesn't even have a realize method) so all realize 
would do is to link it in qtree?

Regards,
BALATON Zoltan

> Fix by realizing it right away.  Visible in "info qom-tree"; here's
> the change for sam460ex:
>
>     /machine (sam460ex-machine)
>       [...]
>       /unattached (container)
>         [...]
>    -    /device[14] (sii3112)
>    +    /device[14] (i2c-ddc)
>    +    /device[15] (sii3112)
>         [rest of device[*] renumbered...]
>
> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
> Cc: BALATON Zoltan <address@hidden>
> Cc: address@hidden
> Cc: Magnus Damm <address@hidden>
> Cc: Philippe Mathieu-Daudé <address@hidden>
> Cc: Aleksandar Markovic <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
> hw/display/ati.c   | 1 +
> hw/display/sm501.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 58ec8291d4..7c2177d7b3 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -929,6 +929,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
>     bitbang_i2c_init(&s->bbi2c, i2cbus);
>     I2CSlave *i2cddc = I2C_SLAVE(qdev_create(BUS(i2cbus), TYPE_I2CDDC));
>     i2c_set_slave_address(i2cddc, 0x50);
> +    qdev_init_nofail(DEVICE(i2cddc));
>
>     /* mmio register space */
>     memory_region_init_io(&s->mm, OBJECT(s), &ati_mm_ops, s,
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index acc692531a..132e75b641 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1816,6 +1816,7 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>     /* ddc */
>     I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
>     i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
> +    qdev_init_nofail(DEVICE(ddc));
>
>     /* mmio */
>     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);