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

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

[Philippe Mathieu-Daudé]

Trivial patches to use the TYPE_* definitions instead of
hardcoded names.

Philippe Mathieu-Daudé (2):
  hw/arm: Use TYPE_SYSBUS_OHCI instead of hardcoded string
  hw/ppc: Use TYPE_SYSBUS_OHCI instead of hardcoded string

 hw/arm/allwinner-h3.c | 9 +++++----
 hw/arm/pxa2xx.c       | 3 ++-
 hw/display/sm501.c    | 3 ++-
 hw/ppc/sam460ex.c     | 3 ++-
 4 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.21.3

[PATCH 1/2] hw/arm: Use TYPE_SYSBUS_OHCI instead of hardcoded string

[Philippe Mathieu-Daudé]

By using the TYPE_* definitions for devices, we can:
- quickly find where devices are used with 'git-grep'
- easily rename a non-user-creatable device (one-line change).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/allwinner-h3.c | 9 +++++----
 hw/arm/pxa2xx.c       | 3 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index 8e09468e86..231d9ad647 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -28,6 +28,7 @@
 #include "hw/sysbus.h"
 #include "hw/char/serial.h"
 #include "hw/misc/unimp.h"
+#include "hw/usb/hcd-ohci.h"
 #include "hw/usb/hcd-ehci.h"
 #include "hw/loader.h"
 #include "sysemu/sysemu.h"
@@ -381,16 +382,16 @@ static void allwinner_h3_realize(DeviceState *dev, Error **errp)
                          qdev_get_gpio_in(DEVICE(&s->gic),
                                           AW_H3_GIC_SPI_EHCI3));
 
-    sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI0],
+    sysbus_create_simple(TYPE_SYSBUS_OHCI, s->memmap[AW_H3_OHCI0],
                          qdev_get_gpio_in(DEVICE(&s->gic),
                                           AW_H3_GIC_SPI_OHCI0));
-    sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI1],
+    sysbus_create_simple(TYPE_SYSBUS_OHCI, s->memmap[AW_H3_OHCI1],
                          qdev_get_gpio_in(DEVICE(&s->gic),
                                           AW_H3_GIC_SPI_OHCI1));
-    sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI2],
+    sysbus_create_simple(TYPE_SYSBUS_OHCI, s->memmap[AW_H3_OHCI2],
                          qdev_get_gpio_in(DEVICE(&s->gic),
                                           AW_H3_GIC_SPI_OHCI2));
-    sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI3],
+    sysbus_create_simple(TYPE_SYSBUS_OHCI, s->memmap[AW_H3_OHCI3],
                          qdev_get_gpio_in(DEVICE(&s->gic),
                                           AW_H3_GIC_SPI_OHCI3));
 
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index f104a33463..1e8a1cbc24 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -22,6 +22,7 @@
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "hw/ssi/ssi.h"
+#include "hw/usb/hcd-ohci.h"
 #include "chardev/char-fe.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/qtest.h"
@@ -2196,7 +2197,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
         s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
     }
 
-    sysbus_create_simple("sysbus-ohci", 0x4c000000,
+    sysbus_create_simple(TYPE_SYSBUS_OHCI, 0x4c000000,
                          qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
 
     s->pcmcia[0] = pxa2xx_pcmcia_init(address_space, 0x20000000);
-- 
2.21.3

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

[Philippe Mathieu-Daudé]

By using the TYPE_* definitions for devices, we can:
- quickly find where devices are used with 'git-grep'
- easily rename a non-user-creatable device (one-line change).

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);
-- 
2.21.3

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2867 bytes --]

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?

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);
>

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

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 3050 bytes --]

On Fri, Jul 3, 2020, 20:49 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> By using the TYPE_* definitions for devices, we can:
> - quickly find where devices are used with 'git-grep'
> - easily rename a non-user-creatable device (one-line change).
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
Reviewed-By: Niek Linnenbank <nieklinnenbank@gmail.com>

---
>  hw/arm/allwinner-h3.c | 9 +++++----
>  hw/arm/pxa2xx.c       | 3 ++-
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> index 8e09468e86..231d9ad647 100644
> --- a/hw/arm/allwinner-h3.c
> +++ b/hw/arm/allwinner-h3.c
> @@ -28,6 +28,7 @@
>  #include "hw/sysbus.h"
>  #include "hw/char/serial.h"
>  #include "hw/misc/unimp.h"
> +#include "hw/usb/hcd-ohci.h"
>  #include "hw/usb/hcd-ehci.h"
>  #include "hw/loader.h"
>  #include "sysemu/sysemu.h"
> @@ -381,16 +382,16 @@ static void allwinner_h3_realize(DeviceState *dev,
> Error **errp)
>                           qdev_get_gpio_in(DEVICE(&s->gic),
>                                            AW_H3_GIC_SPI_EHCI3));
>
> -    sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI0],
> +    sysbus_create_simple(TYPE_SYSBUS_OHCI, s->memmap[AW_H3_OHCI0],
>                           qdev_get_gpio_in(DEVICE(&s->gic),
>                                            AW_H3_GIC_SPI_OHCI0));
> -    sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI1],
> +    sysbus_create_simple(TYPE_SYSBUS_OHCI, s->memmap[AW_H3_OHCI1],
>                           qdev_get_gpio_in(DEVICE(&s->gic),
>                                            AW_H3_GIC_SPI_OHCI1));
> -    sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI2],
> +    sysbus_create_simple(TYPE_SYSBUS_OHCI, s->memmap[AW_H3_OHCI2],
>                           qdev_get_gpio_in(DEVICE(&s->gic),
>                                            AW_H3_GIC_SPI_OHCI2));
> -    sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI3],
> +    sysbus_create_simple(TYPE_SYSBUS_OHCI, s->memmap[AW_H3_OHCI3],
>                           qdev_get_gpio_in(DEVICE(&s->gic),
>                                            AW_H3_GIC_SPI_OHCI3));
>
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index f104a33463..1e8a1cbc24 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -22,6 +22,7 @@
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/ssi/ssi.h"
> +#include "hw/usb/hcd-ohci.h"
>  #include "chardev/char-fe.h"
>  #include "sysemu/blockdev.h"
>  #include "sysemu/qtest.h"
> @@ -2196,7 +2197,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>          s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>      }
>
> -    sysbus_create_simple("sysbus-ohci", 0x4c000000,
> +    sysbus_create_simple(TYPE_SYSBUS_OHCI, 0x4c000000,
>                           qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>
>      s->pcmcia[0] = pxa2xx_pcmcia_init(address_space, 0x20000000);
> --
> 2.21.3
>
>

[-- Attachment #2: Type: text/html, Size: 4311 bytes --]

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);
>>

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

On Fri, Jul 03, 2020 at 08:49:52PM +0200, Philippe Mathieu-Daudé wrote:
> By using the TYPE_* definitions for devices, we can:
> - quickly find where devices are used with 'git-grep'
> - easily rename a non-user-creatable device (one-line change).
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  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);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]