[PATCH 0/3] hw/sysbus: Document GPIO related functions

[PATCH 0/3] hw/sysbus: Document GPIO related functions

[Philippe Mathieu-Daudé]

Reduce the scope of a pair of qdev/sysbus functions,
then document the sysbus functions related to creating
and connecting GPIO lines.

Based-on: <20211218130437.1516929-1-f4bug@amsat.org>

Philippe Mathieu-Daudé (3):
  hw/qdev: Restrict qdev_get_gpio_out_connector() to qdev-internal.h
  hw/sysbus: Restrict sysbus_get_connected_irq() to sysbus-internal.h
  hw/sysbus: Document GPIO related functions

 hw/core/qdev-internal.h   | 15 +++++++++
 hw/core/sysbus-internal.h | 16 +++++++++
 include/hw/qdev-core.h    | 18 ----------
 include/hw/sysbus.h       | 69 ++++++++++++++++++++++++++++++++++++---
 hw/core/gpio.c            |  1 +
 hw/core/platform-bus.c    |  2 +-
 hw/core/sysbus.c          |  2 ++
 7 files changed, 99 insertions(+), 24 deletions(-)
 create mode 100644 hw/core/qdev-internal.h
 create mode 100644 hw/core/sysbus-internal.h

-- 
2.33.1

[PATCH 1/3] hw/qdev: Restrict qdev_get_gpio_out_connector() to qdev-internal.h

[Philippe Mathieu-Daudé]

qdev_get_gpio_out_connector() is called by sysbus_get_connected_irq()
which is only used by platform-bus.c; restrict it to hw/core/ by
adding a local "qdev-internal.h" header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/qdev-internal.h | 15 +++++++++++++++
 include/hw/qdev-core.h  | 18 ------------------
 hw/core/gpio.c          |  1 +
 hw/core/sysbus.c        |  1 +
 4 files changed, 17 insertions(+), 18 deletions(-)
 create mode 100644 hw/core/qdev-internal.h

diff --git a/hw/core/qdev-internal.h b/hw/core/qdev-internal.h
new file mode 100644
index 00000000000..6ec17d0ea70
--- /dev/null
+++ b/hw/core/qdev-internal.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * qdev internal helpers
+ *
+ * Copyright (c) 2009-2021 QEMU contributors
+ */
+#ifndef HW_CORE_QDEV_INTERNAL_H
+#define HW_CORE_QDEV_INTERNAL_H
+
+#include "hw/qdev-core.h"
+
+/* Following functions are only used by the platform-bus subsystem */
+qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name, int n);
+
+#endif /* HW_CORE_QDEV_INTERNAL_H */
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index d19c9417520..655899654bb 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -532,24 +532,6 @@ void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
 void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
                                  qemu_irq input_pin);
 
-/**
- * qdev_get_gpio_out_connector: Get the qemu_irq connected to an output GPIO
- * @dev: Device whose output GPIO we are interested in
- * @name: Name of the output GPIO array
- * @n: Number of the output GPIO line within that array
- *
- * Returns whatever qemu_irq is currently connected to the specified
- * output GPIO line of @dev. This will be NULL if the output GPIO line
- * has never been wired up to the anything.  Note that the qemu_irq
- * returned does not belong to @dev -- it will be the input GPIO or
- * IRQ of whichever device the board code has connected up to @dev's
- * output GPIO.
- *
- * You probably don't need to use this function -- it is used only
- * by the platform-bus subsystem.
- */
-qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name, int n);
-
 /**
  * qdev_intercept_gpio_out: Intercept an existing GPIO connection
  * @dev: Device to intercept the outbound GPIO line from
diff --git a/hw/core/gpio.c b/hw/core/gpio.c
index 80d07a6ec99..513ccbd1062 100644
--- a/hw/core/gpio.c
+++ b/hw/core/gpio.c
@@ -21,6 +21,7 @@
 #include "hw/qdev-core.h"
 #include "hw/irq.h"
 #include "qapi/error.h"
+#include "qdev-internal.h"
 
 static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
                                                const char *name)
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 05c1da3d311..0e6773c8df7 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -23,6 +23,7 @@
 #include "hw/sysbus.h"
 #include "monitor/monitor.h"
 #include "exec/address-spaces.h"
+#include "qdev-internal.h"
 
 static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *sysbus_get_fw_dev_path(DeviceState *dev);
-- 
2.33.1

Re: [PATCH 1/3] hw/qdev: Restrict qdev_get_gpio_out_connector() to qdev-internal.h

[wangyanan (Y) via]

Hi,

On 2021/12/30 6:52, Philippe Mathieu-Daudé wrote:
> qdev_get_gpio_out_connector() is called by sysbus_get_connected_irq()
> which is only used by platform-bus.c; restrict it to hw/core/ by
> adding a local "qdev-internal.h" header.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/core/qdev-internal.h | 15 +++++++++++++++
>   include/hw/qdev-core.h  | 18 ------------------
>   hw/core/gpio.c          |  1 +
>   hw/core/sysbus.c        |  1 +
>   4 files changed, 17 insertions(+), 18 deletions(-)
>   create mode 100644 hw/core/qdev-internal.h
>
> diff --git a/hw/core/qdev-internal.h b/hw/core/qdev-internal.h
> new file mode 100644
> index 00000000000..6ec17d0ea70
> --- /dev/null
> +++ b/hw/core/qdev-internal.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * qdev internal helpers
> + *
> + * Copyright (c) 2009-2021 QEMU contributors
> + */
> +#ifndef HW_CORE_QDEV_INTERNAL_H
> +#define HW_CORE_QDEV_INTERNAL_H
> +
> +#include "hw/qdev-core.h"
> +
> +/* Following functions are only used by the platform-bus subsystem */
Could it be better to also keep the original function comment here?
> +qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name, int n);
> +
> +#endif /* HW_CORE_QDEV_INTERNAL_H */
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index d19c9417520..655899654bb 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -532,24 +532,6 @@ void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
>   void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
>                                    qemu_irq input_pin);
>   
> -/**
> - * qdev_get_gpio_out_connector: Get the qemu_irq connected to an output GPIO
> - * @dev: Device whose output GPIO we are interested in
> - * @name: Name of the output GPIO array
> - * @n: Number of the output GPIO line within that array
> - *
> - * Returns whatever qemu_irq is currently connected to the specified
> - * output GPIO line of @dev. This will be NULL if the output GPIO line
> - * has never been wired up to the anything.  Note that the qemu_irq
> - * returned does not belong to @dev -- it will be the input GPIO or
> - * IRQ of whichever device the board code has connected up to @dev's
> - * output GPIO.
> - *
> - * You probably don't need to use this function -- it is used only
> - * by the platform-bus subsystem.
> - */
> -qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name, int n);
> -
>   /**
>    * qdev_intercept_gpio_out: Intercept an existing GPIO connection
>    * @dev: Device to intercept the outbound GPIO line from
> diff --git a/hw/core/gpio.c b/hw/core/gpio.c
> index 80d07a6ec99..513ccbd1062 100644
> --- a/hw/core/gpio.c
> +++ b/hw/core/gpio.c
> @@ -21,6 +21,7 @@
>   #include "hw/qdev-core.h"
>   #include "hw/irq.h"
>   #include "qapi/error.h"
> +#include "qdev-internal.h"
>   
>   static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
>                                                  const char *name)
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 05c1da3d311..0e6773c8df7 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -23,6 +23,7 @@
>   #include "hw/sysbus.h"
>   #include "monitor/monitor.h"
>   #include "exec/address-spaces.h"
> +#include "qdev-internal.h"
>   
>   static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>   static char *sysbus_get_fw_dev_path(DeviceState *dev);
Otherwise, the tweak looks reasonable:
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,
Yanan

Re: [PATCH 1/3] hw/qdev: Restrict qdev_get_gpio_out_connector() to qdev-internal.h

[Philippe Mathieu-Daudé]

On 12/31/21 08:30, wangyanan (Y) wrote:
> Hi,
> 
> On 2021/12/30 6:52, Philippe Mathieu-Daudé wrote:
>> qdev_get_gpio_out_connector() is called by sysbus_get_connected_irq()
>> which is only used by platform-bus.c; restrict it to hw/core/ by
>> adding a local "qdev-internal.h" header.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/core/qdev-internal.h | 15 +++++++++++++++
>>   include/hw/qdev-core.h  | 18 ------------------
>>   hw/core/gpio.c          |  1 +
>>   hw/core/sysbus.c        |  1 +
>>   4 files changed, 17 insertions(+), 18 deletions(-)
>>   create mode 100644 hw/core/qdev-internal.h
>>
>> diff --git a/hw/core/qdev-internal.h b/hw/core/qdev-internal.h
>> new file mode 100644
>> index 00000000000..6ec17d0ea70
>> --- /dev/null
>> +++ b/hw/core/qdev-internal.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * qdev internal helpers
>> + *
>> + * Copyright (c) 2009-2021 QEMU contributors
>> + */
>> +#ifndef HW_CORE_QDEV_INTERNAL_H
>> +#define HW_CORE_QDEV_INTERNAL_H
>> +
>> +#include "hw/qdev-core.h"
>> +
>> +/* Following functions are only used by the platform-bus subsystem */
> Could it be better to also keep the original function comment here?

We could, but this include being now internal, it seems superfluous.

Since Peter documented this function, let see if he has an preference.

>> +qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char
>> *name, int n);
>> +
>> +#endif /* HW_CORE_QDEV_INTERNAL_H */
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index d19c9417520..655899654bb 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -532,24 +532,6 @@ void qdev_connect_gpio_out(DeviceState *dev, int
>> n, qemu_irq pin);
>>   void qdev_connect_gpio_out_named(DeviceState *dev, const char *name,
>> int n,
>>                                    qemu_irq input_pin);
>>   -/**
>> - * qdev_get_gpio_out_connector: Get the qemu_irq connected to an
>> output GPIO
>> - * @dev: Device whose output GPIO we are interested in
>> - * @name: Name of the output GPIO array
>> - * @n: Number of the output GPIO line within that array
>> - *
>> - * Returns whatever qemu_irq is currently connected to the specified
>> - * output GPIO line of @dev. This will be NULL if the output GPIO line
>> - * has never been wired up to the anything.  Note that the qemu_irq
>> - * returned does not belong to @dev -- it will be the input GPIO or
>> - * IRQ of whichever device the board code has connected up to @dev's
>> - * output GPIO.
>> - *
>> - * You probably don't need to use this function -- it is used only
>> - * by the platform-bus subsystem.
>> - */
>> -qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char
>> *name, int n);
>> -
>>   /**
>>    * qdev_intercept_gpio_out: Intercept an existing GPIO connection
>>    * @dev: Device to intercept the outbound GPIO line from
>> diff --git a/hw/core/gpio.c b/hw/core/gpio.c
>> index 80d07a6ec99..513ccbd1062 100644
>> --- a/hw/core/gpio.c
>> +++ b/hw/core/gpio.c
>> @@ -21,6 +21,7 @@
>>   #include "hw/qdev-core.h"
>>   #include "hw/irq.h"
>>   #include "qapi/error.h"
>> +#include "qdev-internal.h"
>>     static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
>>                                                  const char *name)
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index 05c1da3d311..0e6773c8df7 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -23,6 +23,7 @@
>>   #include "hw/sysbus.h"
>>   #include "monitor/monitor.h"
>>   #include "exec/address-spaces.h"
>> +#include "qdev-internal.h"
>>     static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int
>> indent);
>>   static char *sysbus_get_fw_dev_path(DeviceState *dev);
> Otherwise, the tweak looks reasonable:
> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,

Phil.

Re: [PATCH 1/3] hw/qdev: Restrict qdev_get_gpio_out_connector() to qdev-internal.h

[wangyanan (Y) via]

On 2021/12/31 20:11, Philippe Mathieu-Daudé wrote:
> On 12/31/21 08:30, wangyanan (Y) wrote:
>> Hi,
>>
>> On 2021/12/30 6:52, Philippe Mathieu-Daudé wrote:
>>> qdev_get_gpio_out_connector() is called by sysbus_get_connected_irq()
>>> which is only used by platform-bus.c; restrict it to hw/core/ by
>>> adding a local "qdev-internal.h" header.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>    hw/core/qdev-internal.h | 15 +++++++++++++++
>>>    include/hw/qdev-core.h  | 18 ------------------
>>>    hw/core/gpio.c          |  1 +
>>>    hw/core/sysbus.c        |  1 +
>>>    4 files changed, 17 insertions(+), 18 deletions(-)
>>>    create mode 100644 hw/core/qdev-internal.h
>>>
>>> diff --git a/hw/core/qdev-internal.h b/hw/core/qdev-internal.h
>>> new file mode 100644
>>> index 00000000000..6ec17d0ea70
>>> --- /dev/null
>>> +++ b/hw/core/qdev-internal.h
>>> @@ -0,0 +1,15 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * qdev internal helpers
>>> + *
>>> + * Copyright (c) 2009-2021 QEMU contributors
>>> + */
>>> +#ifndef HW_CORE_QDEV_INTERNAL_H
>>> +#define HW_CORE_QDEV_INTERNAL_H
>>> +
>>> +#include "hw/qdev-core.h"
>>> +
>>> +/* Following functions are only used by the platform-bus subsystem */
>> Could it be better to also keep the original function comment here?
> We could, but this include being now internal, it seems superfluous.
Ok, makes sense to me. :)

Thanks,
Yanan
> Since Peter documented this function, let see if he has an preference.
>
>>> +qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char
>>> *name, int n);
>>> +
>>> +#endif /* HW_CORE_QDEV_INTERNAL_H */
>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>> index d19c9417520..655899654bb 100644
>>> --- a/include/hw/qdev-core.h
>>> +++ b/include/hw/qdev-core.h
>>> @@ -532,24 +532,6 @@ void qdev_connect_gpio_out(DeviceState *dev, int
>>> n, qemu_irq pin);
>>>    void qdev_connect_gpio_out_named(DeviceState *dev, const char *name,
>>> int n,
>>>                                     qemu_irq input_pin);
>>>    -/**
>>> - * qdev_get_gpio_out_connector: Get the qemu_irq connected to an
>>> output GPIO
>>> - * @dev: Device whose output GPIO we are interested in
>>> - * @name: Name of the output GPIO array
>>> - * @n: Number of the output GPIO line within that array
>>> - *
>>> - * Returns whatever qemu_irq is currently connected to the specified
>>> - * output GPIO line of @dev. This will be NULL if the output GPIO line
>>> - * has never been wired up to the anything.  Note that the qemu_irq
>>> - * returned does not belong to @dev -- it will be the input GPIO or
>>> - * IRQ of whichever device the board code has connected up to @dev's
>>> - * output GPIO.
>>> - *
>>> - * You probably don't need to use this function -- it is used only
>>> - * by the platform-bus subsystem.
>>> - */
>>> -qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char
>>> *name, int n);
>>> -
>>>    /**
>>>     * qdev_intercept_gpio_out: Intercept an existing GPIO connection
>>>     * @dev: Device to intercept the outbound GPIO line from
>>> diff --git a/hw/core/gpio.c b/hw/core/gpio.c
>>> index 80d07a6ec99..513ccbd1062 100644
>>> --- a/hw/core/gpio.c
>>> +++ b/hw/core/gpio.c
>>> @@ -21,6 +21,7 @@
>>>    #include "hw/qdev-core.h"
>>>    #include "hw/irq.h"
>>>    #include "qapi/error.h"
>>> +#include "qdev-internal.h"
>>>      static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
>>>                                                   const char *name)
>>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>>> index 05c1da3d311..0e6773c8df7 100644
>>> --- a/hw/core/sysbus.c
>>> +++ b/hw/core/sysbus.c
>>> @@ -23,6 +23,7 @@
>>>    #include "hw/sysbus.h"
>>>    #include "monitor/monitor.h"
>>>    #include "exec/address-spaces.h"
>>> +#include "qdev-internal.h"
>>>      static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int
>>> indent);
>>>    static char *sysbus_get_fw_dev_path(DeviceState *dev);
>> Otherwise, the tweak looks reasonable:
>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> Thanks,
>
> Phil.
>
> .

[PATCH 2/3] hw/sysbus: Restrict sysbus_get_connected_irq() to sysbus-internal.h

[Philippe Mathieu-Daudé]

sysbus_get_connected_irq() and sysbus_is_irq_connected() are only
used by platform-bus.c; restrict them to hw/core/ by adding a local
"sysbus-internal.h" header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/sysbus-internal.h | 16 ++++++++++++++++
 include/hw/sysbus.h       |  2 --
 hw/core/platform-bus.c    |  2 +-
 hw/core/sysbus.c          |  1 +
 4 files changed, 18 insertions(+), 3 deletions(-)
 create mode 100644 hw/core/sysbus-internal.h

diff --git a/hw/core/sysbus-internal.h b/hw/core/sysbus-internal.h
new file mode 100644
index 00000000000..991b3e3159c
--- /dev/null
+++ b/hw/core/sysbus-internal.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * SysBus internal helpers
+ *
+ * Copyright (c) 2021 QEMU contributors
+ */
+#ifndef HW_CORE_SYSBUS_INTERNAL_H
+#define HW_CORE_SYSBUS_INTERNAL_H
+
+#include "hw/sysbus.h"
+
+/* Following functions are only used by the platform-bus subsystem */
+qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n);
+bool sysbus_is_irq_connected(SysBusDevice *dev, int n);
+
+#endif /* HW_CORE_SYSBUS_INTERNAL_H */
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index 3564b7b6a22..24645ee7996 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -77,8 +77,6 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size);
 bool sysbus_has_irq(SysBusDevice *dev, int n);
 bool sysbus_has_mmio(SysBusDevice *dev, unsigned int n);
 void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq);
-bool sysbus_is_irq_connected(SysBusDevice *dev, int n);
-qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n);
 void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr);
 void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
                              int priority);
diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
index b8487b26b67..016fb71eba1 100644
--- a/hw/core/platform-bus.c
+++ b/hw/core/platform-bus.c
@@ -25,7 +25,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
-
+#include "sysbus-internal.h"
 
 /*
  * Returns the PlatformBus IRQ number for a SysBusDevice irq number or -1 if
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 0e6773c8df7..dcd7beda184 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -24,6 +24,7 @@
 #include "monitor/monitor.h"
 #include "exec/address-spaces.h"
 #include "qdev-internal.h"
+#include "sysbus-internal.h"
 
 static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *sysbus_get_fw_dev_path(DeviceState *dev);
-- 
2.33.1

Re: [PATCH 2/3] hw/sysbus: Restrict sysbus_get_connected_irq() to sysbus-internal.h

[wangyanan (Y) via]

On 2021/12/30 6:52, Philippe Mathieu-Daudé wrote:
> sysbus_get_connected_irq() and sysbus_is_irq_connected() are only
> used by platform-bus.c; restrict them to hw/core/ by adding a local
> "sysbus-internal.h" header.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/core/sysbus-internal.h | 16 ++++++++++++++++
>   include/hw/sysbus.h       |  2 --
>   hw/core/platform-bus.c    |  2 +-
>   hw/core/sysbus.c          |  1 +
>   4 files changed, 18 insertions(+), 3 deletions(-)
>   create mode 100644 hw/core/sysbus-internal.h
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,
Yanan
> diff --git a/hw/core/sysbus-internal.h b/hw/core/sysbus-internal.h
> new file mode 100644
> index 00000000000..991b3e3159c
> --- /dev/null
> +++ b/hw/core/sysbus-internal.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * SysBus internal helpers
> + *
> + * Copyright (c) 2021 QEMU contributors
> + */
> +#ifndef HW_CORE_SYSBUS_INTERNAL_H
> +#define HW_CORE_SYSBUS_INTERNAL_H
> +
> +#include "hw/sysbus.h"
> +
> +/* Following functions are only used by the platform-bus subsystem */
> +qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n);
> +bool sysbus_is_irq_connected(SysBusDevice *dev, int n);
> +
> +#endif /* HW_CORE_SYSBUS_INTERNAL_H */
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index 3564b7b6a22..24645ee7996 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -77,8 +77,6 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size);
>   bool sysbus_has_irq(SysBusDevice *dev, int n);
>   bool sysbus_has_mmio(SysBusDevice *dev, unsigned int n);
>   void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq);
> -bool sysbus_is_irq_connected(SysBusDevice *dev, int n);
> -qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n);
>   void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr);
>   void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
>                                int priority);
> diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
> index b8487b26b67..016fb71eba1 100644
> --- a/hw/core/platform-bus.c
> +++ b/hw/core/platform-bus.c
> @@ -25,7 +25,7 @@
>   #include "qapi/error.h"
>   #include "qemu/error-report.h"
>   #include "qemu/module.h"
> -
> +#include "sysbus-internal.h"
>   
>   /*
>    * Returns the PlatformBus IRQ number for a SysBusDevice irq number or -1 if
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 0e6773c8df7..dcd7beda184 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -24,6 +24,7 @@
>   #include "monitor/monitor.h"
>   #include "exec/address-spaces.h"
>   #include "qdev-internal.h"
> +#include "sysbus-internal.h"
>   
>   static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>   static char *sysbus_get_fw_dev_path(DeviceState *dev);

[PATCH 3/3] hw/sysbus: Document GPIO related functions

[Philippe Mathieu-Daudé]

Similarly to cd07d7f9f51 ("qdev: Document GPIO related functions"),
add documentation comments for the various sysbus functions
related to creating and connecting GPIO lines.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/sysbus.h | 67 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 3 deletions(-)

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index 24645ee7996..7b2b7c7faaa 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -69,14 +69,75 @@ 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);
-void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size);
 
+/**
+ * sysbus_init_irq: Create an output GPIO line
+ * @dev: Sysbus device to create output GPIO for
+ * @irq: Pointer to qemu_irq for the GPIO lines
+ *
+ * Sysbus devices should use this function in their instance_init
+ * or realize methods to create any output GPIO lines they need.
+ *
+ * The @irq argument should be a pointer to either a "qemu_irq" in
+ * the device's state structure. The device implementation can then raise
+ * and lower the GPIO line by calling qemu_set_irq(). (If anything is
+ * connected to the other end of the GPIO this will cause the handler
+ * function for that input GPIO to be called.)
+ *
+ * See sysbus_connect_irq() for how code that uses such a device can
+ * connect to one of its output GPIO lines.
+ *
+ * There is no need to release the @pins allocated array because it
+ * will be automatically released when @dev calls its instance_finalize()
+ * handler.
+ */
+void sysbus_init_irq(SysBusDevice *dev, qemu_irq *irq);
+
+/**
+ * sysbus_pass_irq: Create GPIO lines on container which pass through
+ *                  to a target device
+ * @dev: Device which needs to expose GPIO lines
+ * @target: Device which has GPIO lines
+ *
+ * This function allows a container device to create GPIO arrays on itself
+ * which simply pass through to a GPIO array of another device. It is
+ * useful when modelling complex devices such system-on-chip, where a
+ * sysbus device contains other sysbus devices.
+ *
+ * It is not possible to pass a subset of the GPIO lines with this function.
+ *
+ * To users of the container sysbus device, the GPIO array created on @dev
+ * behaves exactly like any other.
+ */
+void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target);
+
+void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size);
 
 bool sysbus_has_irq(SysBusDevice *dev, int n);
 bool sysbus_has_mmio(SysBusDevice *dev, unsigned int n);
+
+/**
+ * sysbus_connect_irq: Connect a sysbus device output GPIO line
+ * @dev: sysbus device whose GPIO to connect
+ * @n: Number of the output GPIO line (which must be in range)
+ * @pin: qemu_irq to connect the output line to
+ *
+ * This function connects an output GPIO line on a sysbus device
+ * up to an arbitrary qemu_irq, so that when the device asserts that
+ * output GPIO line, the qemu_irq's callback is invoked.
+ * The index @n of the GPIO line must be valid, otherwise this function
+ * will assert().
+ *
+ * Outbound GPIO lines can be connected to any qemu_irq, but the common
+ * case is connecting them to another device's inbound GPIO line, using
+ * the qemu_irq returned by qdev_get_gpio_in() or qdev_get_gpio_in_named().
+ *
+ * It is not valid to try to connect one outbound GPIO to multiple
+ * qemu_irqs at once, or to connect multiple outbound GPIOs to the
+ * same qemu_irq; see qdev_connect_gpio_out() for details.
+ */
 void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq);
+
 void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr);
 void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
                              int priority);
-- 
2.33.1

Re: [PATCH 3/3] hw/sysbus: Document GPIO related functions

[Peter Maydell]

On Wed, 29 Dec 2021 at 22:52, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Similarly to cd07d7f9f51 ("qdev: Document GPIO related functions"),
> add documentation comments for the various sysbus functions
> related to creating and connecting GPIO lines.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/sysbus.h | 67 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index 24645ee7996..7b2b7c7faaa 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -69,14 +69,75 @@ 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);
> -void sysbus_init_ioports(SysBusDevice *dev, uint32_t ioport, uint32_t size);
>
> +/**
> + * sysbus_init_irq: Create an output GPIO line
> + * @dev: Sysbus device to create output GPIO for
> + * @irq: Pointer to qemu_irq for the GPIO lines
> + *
> + * Sysbus devices should use this function in their instance_init
> + * or realize methods to create any output GPIO lines they need.

It's true that this works with a qemu_irq which can be used
as an arbitrary GPIO line. But mostly we use sysbus_init_irq() when
creating things which are actually outbound IRQ lines, and
qdev_init_gpio_out{,_named}() when creating generic output
GPIO lines. So we should phrase the documentation of these
functions to steer readers towards following that convention.

(Looking at the code, I discover that under the hood the
"sysbus irq" code is really using a named output GPIO
array with a specific name (SYSBUS_DEVICE_GPIO_IRQ,
aka "sysbus-irq"). The only functional difference is that
a sysbus device can be notified when one of its IRQs is
connected, which is a nasty hack for the benefit of platform vfio.)

> + *
> + * The @irq argument should be a pointer to either a "qemu_irq" in
> + * the device's state structure.

Missing "or ..." clause, or should "either" be deleted ?

> The device implementation can then raise
> + * and lower the GPIO line by calling qemu_set_irq(). (If anything is
> + * connected to the other end of the GPIO this will cause the handler
> + * function for that input GPIO to be called.)
> + *
> + * See sysbus_connect_irq() for how code that uses such a device can
> + * connect to one of its output GPIO lines.
> + *
> + * There is no need to release the @pins allocated array because it
> + * will be automatically released when @dev calls its instance_finalize()
> + * handler.
> + */

thanks
-- PMM