Re: [PATCH 2/2] hw/ppc: Use TYPE_SYSBUS_OHCI instead of hardcoded string

[Philippe Mathieu-Daudé]

On 7/3/20 9:58 PM, BALATON Zoltan wrote:
> On Fri, 3 Jul 2020, Philippe Mathieu-Daudé wrote:
>> By using the TYPE_* definitions for devices, we can:
>> - quickly find where devices are used with 'git-grep'
> 
> You could just as well grep for the type name but it's true if some
> files use name and others the constant then you need to grep for both.
> 
>> - easily rename a non-user-creatable device (one-line change).
> 
> But most devices are user creatable and thus their name is part of the
> CLI so inlikely to change due to preserving backward compatibility of
> command lines. So usefulness of this change seems limited to me.
> 
> But my problem with it is not the above. It's that hcd-ohci.h is not in
> include but in hw/usb so it's internal to the implementation of the
> device model and defines things that users of the device should not
> need, therefore they should not include this header. So if you want to
> use the defined constant then that should be split off to some public
> header instead of including hw/usb/hcd-ohci.h. Maybe we need a new
> header for these TYPE_* constants similar to qemu/typedefs.h?

Indeed you are right. Thanks for noticing the bad effect of my patch.

> 
> Regards,
> BALATON Zoltan
> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/display/sm501.c | 3 ++-
>> hw/ppc/sam460ex.c  | 3 ++-
>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index 9cccc68c35..c122a4eca5 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -36,6 +36,7 @@
>> #include "hw/qdev-properties.h"
>> #include "hw/i2c/i2c.h"
>> #include "hw/display/i2c-ddc.h"
>> +#include "hw/usb/hcd-ohci.h"
>> #include "qemu/range.h"
>> #include "ui/pixel_ops.h"
>> #include "qemu/bswap.h"
>> @@ -1961,7 +1962,7 @@ static void sm501_realize_sysbus(DeviceState
>> *dev, Error **errp)
>>     sysbus_init_mmio(sbd, &s->state.mmio_region);
>>
>>     /* bridge to usb host emulation module */
>> -    usb_dev = qdev_new("sysbus-ohci");
>> +    usb_dev = qdev_new(TYPE_SYSBUS_OHCI);
>>     qdev_prop_set_uint32(usb_dev, "num-ports", 2);
>>     qdev_prop_set_uint64(usb_dev, "dma-offset", s->base);
>>     sysbus_realize_and_unref(SYS_BUS_DEVICE(usb_dev), &error_fatal);
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index 1a106a68de..593436937a 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -35,6 +35,7 @@
>> #include "hw/char/serial.h"
>> #include "hw/i2c/ppc4xx_i2c.h"
>> #include "hw/i2c/smbus_eeprom.h"
>> +#include "hw/usb/hcd-ohci.h"
>> #include "hw/usb/hcd-ehci.h"
>> #include "hw/ppc/fdt.h"
>> #include "hw/qdev-properties.h"
>> @@ -370,7 +371,7 @@ static void sam460ex_init(MachineState *machine)
>>
>>     /* USB */
>>     sysbus_create_simple(TYPE_PPC4xx_EHCI, 0x4bffd0400, uic[2][29]);
>> -    dev = qdev_new("sysbus-ohci");
>> +    dev = qdev_new(TYPE_SYSBUS_OHCI);
>>     qdev_prop_set_string(dev, "masterbus", "usb-bus.0");
>>     qdev_prop_set_uint32(dev, "num-ports", 6);
>>     sbdev = SYS_BUS_DEVICE(dev);
>>