[PATCH 0/2] hw/core: Make qdev_pass_gpios/sysbus_pass_irq arguments self-describing

[PATCH 0/2] hw/core: Make qdev_pass_gpios/sysbus_pass_irq arguments self-describing

[Philippe Mathieu-Daudé]

Simply rename qdev_pass_gpios() and sysbus_pass_irq() arguments using
self-describing names, to make the direction the GPIO/IRQ lines are
passed obvious.

Note, currently each function arguments are inverted :)

- qdev_pass_gpios: (from -> to)
- sysbus_pass_irq: (to -> from)

Philippe Mathieu-Daudé (2):
  hw/core/qdev: Make qdev_pass_gpios() arguments self-describing
  hw/core/sysbus: Make sysbus_pass_irq() arguments self-describing

 include/hw/qdev-core.h | 10 +++++-----
 include/hw/sysbus.h    |  7 ++++++-
 hw/core/qdev.c         | 14 +++++++-------
 hw/core/sysbus.c       |  5 ++---
 4 files changed, 20 insertions(+), 16 deletions(-)

-- 
2.26.2

[PATCH 1/2] hw/core/qdev: Make qdev_pass_gpios() arguments self-describing

[Philippe Mathieu-Daudé]

Make the direction qdev_pass_gpios() pass the GPIOs more obvious
by renaming its arguments as 'from_dev' and 'to_container'.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/qdev-core.h | 10 +++++-----
 hw/core/qdev.c         | 14 +++++++-------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index ea3f73a282d..c72d4db2d26 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -643,8 +643,8 @@ static inline void qdev_init_gpio_in_named(DeviceState *dev,
 
 /**
  * qdev_pass_gpios: create GPIO lines on container which pass through to device
- * @dev: Device which has GPIO lines
- * @container: Container device which needs to expose them
+ * @from_dev: Device which has GPIO lines
+ * @to_container: Container device which needs to expose them
  * @name: Name of GPIO array to pass through (NULL for the anonymous GPIO array)
  *
  * In QEMU, complicated devices like SoCs are often modelled with a
@@ -653,14 +653,14 @@ static inline void qdev_init_gpio_in_named(DeviceState *dev,
  * to create GPIO arrays on itself which simply pass through to a GPIO
  * array of one of its internal devices.
  *
- * If @dev has both input and output GPIOs named @name then both will
+ * If @from_dev has both input and output GPIOs named @name then both will
  * be passed through. It is not possible to pass a subset of the array
  * with this function.
  *
- * To users of the container device, the GPIO array created on @container
+ * To users of the container device, the GPIO array created on @to_container
  * behaves exactly like any other.
  */
-void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
+void qdev_pass_gpios(DeviceState *from_dev, DeviceState *to_container,
                      const char *name);
 
 BusState *qdev_get_parent_bus(DeviceState *dev);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 96772a15bd5..79cbd990114 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -576,30 +576,30 @@ void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
     qdev_connect_gpio_out_named(dev, NULL, n, pin);
 }
 
-void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
+void qdev_pass_gpios(DeviceState *from_dev, DeviceState *to_container,
                      const char *name)
 {
     int i;
-    NamedGPIOList *ngl = qdev_get_named_gpio_list(dev, name);
+    NamedGPIOList *ngl = qdev_get_named_gpio_list(from_dev, name);
 
     for (i = 0; i < ngl->num_in; i++) {
         const char *nm = ngl->name ? ngl->name : "unnamed-gpio-in";
         char *propname = g_strdup_printf("%s[%d]", nm, i);
 
-        object_property_add_alias(OBJECT(container), propname,
-                                  OBJECT(dev), propname);
+        object_property_add_alias(OBJECT(to_container), propname,
+                                  OBJECT(from_dev), propname);
         g_free(propname);
     }
     for (i = 0; i < ngl->num_out; i++) {
         const char *nm = ngl->name ? ngl->name : "unnamed-gpio-out";
         char *propname = g_strdup_printf("%s[%d]", nm, i);
 
-        object_property_add_alias(OBJECT(container), propname,
-                                  OBJECT(dev), propname);
+        object_property_add_alias(OBJECT(to_container), propname,
+                                  OBJECT(from_dev), propname);
         g_free(propname);
     }
     QLIST_REMOVE(ngl, node);
-    QLIST_INSERT_HEAD(&container->gpios, ngl, node);
+    QLIST_INSERT_HEAD(&to_container->gpios, ngl, node);
 }
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
-- 
2.26.2

Re: [PATCH 1/2] hw/core/qdev: Make qdev_pass_gpios() arguments self-describing

[Peter Maydell]

On Tue, 18 Aug 2020 at 15:38, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Make the direction qdev_pass_gpios() pass the GPIOs more obvious
> by renaming its arguments as 'from_dev' and 'to_container'.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/qdev-core.h | 10 +++++-----
>  hw/core/qdev.c         | 14 +++++++-------
>  2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index ea3f73a282d..c72d4db2d26 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -643,8 +643,8 @@ static inline void qdev_init_gpio_in_named(DeviceState *dev,
>
>  /**
>   * qdev_pass_gpios: create GPIO lines on container which pass through to device

In the line above we say that the pass through is from the container
to the device...

> - * @dev: Device which has GPIO lines
> - * @container: Container device which needs to expose them
> + * @from_dev: Device which has GPIO lines
> + * @to_container: Container device which needs to expose them

...but here we say that it is from the device to the container.

You could make an argument for either convention of from/to,
but we should be consistent. The most useful part of the name
is the part we already have, because 'dev' vs 'container'
is unambiguous, whereas 'to' vs 'from' depends on your
point of view and doesn't really help in working out which
way round the arguments should go.

thanks
-- PMM

[PATCH 2/2] hw/core/sysbus: Make sysbus_pass_irq() arguments self-describing

[Philippe Mathieu-Daudé]

Make the direction sysbus_pass_irq() pass the GPIOs more obvious
by renaming its arguments as 'from_dev' and 'to_dev'.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sysbus.h | 7 ++++++-
 hw/core/sysbus.c    | 5 ++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index da9f85c58ce..65de6f6b062 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -73,7 +73,12 @@ typedef void FindSysbusDeviceFunc(SysBusDevice *sbdev, void *opaque);
 void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
 MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n);
 void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p);
-void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target);
+/**
+ * sysbus_pass_irq: Pass through IRQ/GPIO lines from one to another device
+ * @to_dev: Device which needs to expose IRQ/GPIO lines
+ * @from_dev: Device which has the IRQ/GPIO lines
+ */
+void sysbus_pass_irq(SysBusDevice *to_dev, SysBusDevice *from_dev);
 void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size);
 
 
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 70239b7e7d0..bc23a66b5c5 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -181,10 +181,9 @@ void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
     qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1);
 }
 
-/* Pass IRQs from a target device.  */
-void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target)
+void sysbus_pass_irq(SysBusDevice *to_dev, SysBusDevice *from_dev)
 {
-    qdev_pass_gpios(DEVICE(target), DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ);
+    qdev_pass_gpios(DEVICE(from_dev), DEVICE(to_dev), SYSBUS_DEVICE_GPIO_IRQ);
 }
 
 void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
-- 
2.26.2

Re: [PATCH 2/2] hw/core/sysbus: Make sysbus_pass_irq() arguments self-describing

[Peter Maydell]

On Tue, 18 Aug 2020 at 15:38, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Make the direction sysbus_pass_irq() pass the GPIOs more obvious
> by renaming its arguments as 'from_dev' and 'to_dev'.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/sysbus.h | 7 ++++++-
>  hw/core/sysbus.c    | 5 ++---
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index da9f85c58ce..65de6f6b062 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -73,7 +73,12 @@ typedef void FindSysbusDeviceFunc(SysBusDevice *sbdev, void *opaque);
>  void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
>  MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n);
>  void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p);
> -void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target);
> +/**
> + * sysbus_pass_irq: Pass through IRQ/GPIO lines from one to another device
> + * @to_dev: Device which needs to expose IRQ/GPIO lines
> + * @from_dev: Device which has the IRQ/GPIO lines
> + */
> +void sysbus_pass_irq(SysBusDevice *to_dev, SysBusDevice *from_dev);

I think this is less good than what we had before, because (as
per comments on patch 1) "to" vs "from" is ambiguous.
The 'before' here is not as good as the qdev_pass_gpios()
choice of "dev" vs "container", though, so I guess we should
make this function match that one.

> -/* Pass IRQs from a target device.  */
> -void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target)
> +void sysbus_pass_irq(SysBusDevice *to_dev, SysBusDevice *from_dev)
>  {
> -    qdev_pass_gpios(DEVICE(target), DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ);
> +    qdev_pass_gpios(DEVICE(from_dev), DEVICE(to_dev), SYSBUS_DEVICE_GPIO_IRQ);
>  }

Having these two functions pass the container and target in
opposite orders seems unnecessarily confusing. We should
make them use the same order, at least...

thanks
-- PMM