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

[Peter Maydell <peter.maydell@linaro.org>]

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