[PATCH for-5.1 0/3] Move and improve qdev API doc comments

[PATCH for-5.1 0/3] Move and improve qdev API doc comments

[Peter Maydell]

This patchset adds documentation comments to qdev-core.h (and in a few
cases qdev-properties.h) to functions (but not all functions!) which
were missing them.

The prompt for this was Markus' recent welcome cleanup and correction
of the qdev realize functions to get reference count and bus parenting
handling right. Markus put doc comments for new/changed functions in
the qdev.c file. This is something where I have a fairly strong opinion:
 * the right place for doc comments for functions which expose API
   to the rest of QEMU is in the header file, not the .c file
 * we should be consistent about this, especially within a single
   header file but more generally as a project

So patch 1 moves doc comments from qdev.c to qdev-core.h (or
for a few functions qdev-properties.h) to join the doc comments
that were already in those header files. I have expanded on the
comments in some places where I thought that summaries of the
intended use case for the API was helpful.

Because I didn't want to send out a patch that was moving doc
comments around to fit my asserted preferred style and nothing else,
I have attempted to sweeten the pot a bit with patches 2 and 3, which
provide new doc comments for previously undocumented functions:
qdev_unrealize(), and also all the GPIO creation/connection APIs.

thanks
-- PMM

Peter Maydell (3):
  qdev: Move doc comments from qdev.c to qdev-core.h
  qdev: Document qdev_unrealize()
  qdev: Document GPIO related functions

 include/hw/qdev-core.h       | 267 ++++++++++++++++++++++++++++++++++-
 include/hw/qdev-properties.h |  13 ++
 hw/core/qdev.c               |  33 -----
 3 files changed, 278 insertions(+), 35 deletions(-)

-- 
2.20.1

[PATCH for-5.1 1/3] qdev: Move doc comments from qdev.c to qdev-core.h

[Peter Maydell]

The doc-comments which document the qdev API are split between the
header file and the C source files, because as a project we haven't
been consistent about where we put them.

Move all the doc-comments in qdev.c to the header files, so that
users of the APIs don't have to look at the implementation files for
this information.

In the process, unify them into our doc-comment format and expand on
them in some cases to clarify expected use cases.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/qdev-core.h       | 57 ++++++++++++++++++++++++++++++++++++
 include/hw/qdev-properties.h | 13 ++++++++
 hw/core/qdev.c               | 33 ---------------------
 3 files changed, 70 insertions(+), 33 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index fe78073c708..2d441d1fb2e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -320,9 +320,66 @@ compat_props_add(GPtrArray *arr,
 
 /*** Board API.  This should go away once we have a machine config file.  ***/
 
+/**
+ * qdev_new: Create a device on the heap
+ * @name: device type to create (we assert() that this type exists)
+ *
+ * This only allocates the memory and initializes the device state
+ * structure, ready for the caller to set properties if they wish.
+ * The device still needs to be realized.
+ * The returned object has a reference count of 1.
+ */
 DeviceState *qdev_new(const char *name);
+/**
+ * qdev_try_new: Try to create a device on the heap
+ * @name: device type to create
+ *
+ * This is like qdev_new(), except it returns %NULL when type @name
+ * does not exist, rather than asserting.
+ */
 DeviceState *qdev_try_new(const char *name);
+/**
+ * qdev_realize: Realize @dev.
+ * @dev: device to realize
+ * @bus: bus to plug it into (may be NULL)
+ * @errp: pointer to error object
+ *
+ * "Realize" the device, i.e. perform the second phase of device
+ * initialization.
+ * @dev must not be plugged into a bus already.
+ * If @bus, plug @dev into @bus.  This takes a reference to @dev.
+ * If @dev has no QOM parent, make one up, taking another reference.
+ * On success, return true.
+ * On failure, store an error through @errp and return false.
+ *
+ * If you created @dev using qdev_new(), you probably want to use
+ * qdev_realize_and_unref() instead.
+ */
 bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp);
+/**
+ * qdev_realize_and_unref: Realize @dev and drop a reference
+ * @dev: device to realize
+ * @bus: bus to plug it into (may be NULL)
+ * @errp: pointer to error object
+ *
+ * Realize @dev and drop a reference.
+ * This is like qdev_realize(), except the caller must hold a
+ * (private) reference, which is dropped on return regardless of
+ * success or failure.  Intended use::
+ *
+ *     dev = qdev_new();
+ *     [...]
+ *     qdev_realize_and_unref(dev, bus, errp);
+ *
+ * Now @dev can go away without further ado.
+ *
+ * If you are embedding the device into some other QOM device and
+ * initialized it via some variant on object_initialize_child() then
+ * do not use this function, because that family of functions arrange
+ * for the only reference to the child device to be held by the parent
+ * via the child<> property, and so the reference-count-drop done here
+ * would be incorrect. For that use case you want qdev_realize().
+ */
 bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
 void qdev_unrealize(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 587e5b7d317..8f3a98cba69 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -282,6 +282,19 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
  */
 void qdev_property_add_static(DeviceState *dev, Property *prop);
 
+/**
+ * qdev_alias_all_properties: Create aliases on source for all target properties
+ * @target: Device which has properties to be aliased
+ * @source: Object to add alias properties to
+ *
+ * Add alias properties to the @source object for all qdev properties on
+ * the @target DeviceState.
+ *
+ * This is useful when @target is an internal implementation object
+ * owned by @source, and you want to expose all the properties of that
+ * implementation object as properties on the @source object so that users
+ * of @source can set them.
+ */
 void qdev_alias_all_properties(DeviceState *target, Object *source);
 
 /**
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 01796823b41..96772a15bd5 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -128,13 +128,6 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
     }
 }
 
-/*
- * Create a device on the heap.
- * A type @name must exist.
- * This only initializes the device state structure and allows
- * properties to be set.  The device still needs to be realized.  See
- * qdev-core.h.
- */
 DeviceState *qdev_new(const char *name)
 {
     if (!object_class_by_name(name)) {
@@ -143,11 +136,6 @@ DeviceState *qdev_new(const char *name)
     return DEVICE(object_new(name));
 }
 
-/*
- * Try to create a device on the heap.
- * This is like qdev_new(), except it returns %NULL when type @name
- * does not exist.
- */
 DeviceState *qdev_try_new(const char *name)
 {
     if (!module_object_class_by_name(name)) {
@@ -378,14 +366,6 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
     qdev_unrealize(dev);
 }
 
-/*
- * Realize @dev.
- * @dev must not be plugged into a bus.
- * If @bus, plug @dev into @bus.  This takes a reference to @dev.
- * If @dev has no QOM parent, make one up, taking another reference.
- * On success, return true.
- * On failure, store an error through @errp and return false.
- */
 bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
 {
     assert(!dev->realized && !dev->parent_bus);
@@ -399,16 +379,6 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
     return object_property_set_bool(OBJECT(dev), "realized", true, errp);
 }
 
-/*
- * Realize @dev and drop a reference.
- * This is like qdev_realize(), except the caller must hold a
- * (private) reference, which is dropped on return regardless of
- * success or failure.  Intended use:
- *     dev = qdev_new();
- *     [...]
- *     qdev_realize_and_unref(dev, bus, errp);
- * Now @dev can go away without further ado.
- */
 bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp)
 {
     bool ret;
@@ -814,9 +784,6 @@ static void qdev_class_add_property(DeviceClass *klass, Property *prop)
                                           prop->info->description);
 }
 
-/* @qdev_alias_all_properties - Add alias properties to the source object for
- * all qdev properties on the target DeviceState.
- */
 void qdev_alias_all_properties(DeviceState *target, Object *source)
 {
     ObjectClass *class;
-- 
2.20.1

[PATCH for-5.1 2/3] qdev: Document qdev_unrealize()

[Peter Maydell]

Add a doc comment for qdev_unrealize(), to go with the new
documentation for the realize part of the qdev lifecycle.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/qdev-core.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 2d441d1fb2e..1d2bf5f37da 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -381,6 +381,25 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp);
  * would be incorrect. For that use case you want qdev_realize().
  */
 bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
+/**
+ * qdev_unrealize: Unrealize a device
+ * @dev: device to unrealize
+ *
+ * This function will "unrealize" a device, which is the first phase
+ * of correctly destroying a device that has been realized. It will:
+ *
+ *  - unrealize any child buses by calling qbus_unrealize()
+ *    (this will recursively unrealize any devices on those buses)
+ *  - call the the unrealize method of @dev
+ *
+ * The device can then be freed by causing its reference count to go
+ * to zero.
+ *
+ * Warning: most devices in QEMU do not expect to be unrealized.  Only
+ * devices which are hot-unpluggable should be unrealized (as part of
+ * the unplugging process); all other devices are expected to last for
+ * the life of the simulation and should not be unrealized and freed.
+ */
 void qdev_unrealize(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
-- 
2.20.1

[PATCH for-5.1 3/3] qdev: Document GPIO related functions

[Peter Maydell]

Add documentation comments for the various qdev functions
related to creating and connecting GPIO lines.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/qdev-core.h | 191 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 189 insertions(+), 2 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1d2bf5f37da..ea3f73a282d 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -424,13 +424,132 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
 void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
 
+/**
+ * qdev_get_gpio_in: Get one of a device's anonymous input GPIO lines
+ * @dev: Device whose GPIO we want
+ * @n: Number of the anonymous GPIO line (which must be in range)
+ *
+ * Returns the qemu_irq corresponding to an anonymous input GPIO line
+ * (which the device has set up with qdev_init_gpio_in()). The index
+ * @n of the GPIO line must be valid (i.e. be at least 0 and less than
+ * the total number of anonymous input GPIOs the device has); this
+ * function will assert() if passed an invalid index.
+ *
+ * This function is intended to be used by board code or SoC "container"
+ * device models to wire up the GPIO lines; usually the return value
+ * will be passed to qdev_connect_gpio_out() or a similar function to
+ * connect another device's output GPIO line to this input.
+ *
+ * For named input GPIO lines, use qdev_get_gpio_in_named().
+ */
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
+/**
+ * qdev_get_gpio_in_named: Get one of a device's named input GPIO lines
+ * @dev: Device whose GPIO we want
+ * @name: Name of the input GPIO array
+ * @n: Number of the GPIO line in that array (which must be in range)
+ *
+ * Returns the qemu_irq corresponding to a named input GPIO line
+ * (which the device has set up with qdev_init_gpio_in_named()).
+ * The @name string must correspond to an input GPIO array which exists on
+ * the device, and the index @n of the GPIO line must be valid (i.e.
+ * be at least 0 and less than the total number of input GPIOs in that
+ * array); this function will assert() if passed an invalid name or index.
+ *
+ * For anonymous input GPIO lines, use qdev_get_gpio_in().
+ */
 qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n);
 
+/**
+ * qdev_connect_gpio_out: Connect one of a device's anonymous output GPIO lines
+ * @dev: Device whose GPIO to connect
+ * @n: Number of the anonymous output GPIO line (which must be in range)
+ * @pin: qemu_irq to connect the output line to
+ *
+ * This function connects an anonymous output GPIO line on a 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 (i.e. be at least 0 and
+ * less than the total number of anonymous output GPIOs the device has
+ * created with qdev_init_gpio_out()); 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. (Warning: there is no assertion or other guard to
+ * catch this error: the model will just not do the right thing.)
+ * Instead, for fan-out you can use the TYPE_IRQ_SPLIT device: connect
+ * a device's outbound GPIO to the splitter's input, and connect each
+ * of the splitter's outputs to a different device.  For fan-in you
+ * can use the TYPE_OR_IRQ device, which is a model of a logical OR
+ * gate with multiple inputs and one output.
+ *
+ * For named output GPIO lines, use qdev_connect_gpio_out_named().
+ */
 void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
+/**
+ * qdev_connect_gpio_out: Connect one of a device's anonymous output GPIO lines
+ * @dev: Device whose GPIO to connect
+ * @name: Name of the output GPIO array
+ * @n: Number of the anonymous output GPIO line (which must be in range)
+ * @pin: qemu_irq to connect the output line to
+ *
+ * This function connects an anonymous output GPIO line on a 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 @name string must correspond to an output GPIO array which exists on
+ * the device, and the index @n of the GPIO line must be valid (i.e.
+ * be at least 0 and less than the total number of input GPIOs in that
+ * array); this function will assert() if passed an invalid name or index.
+ *
+ * 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.
+ *
+ * For named output GPIO lines, use qdev_connect_gpio_out_named().
+ */
 void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
                                  qemu_irq 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
+ * @icpt: New qemu_irq to connect instead
+ * @name: Name of the output GPIO array
+ * @n: Number of the GPIO line in the array
+ *
+ * This function is provided only for use by the qtest testing framework
+ * and is not suitable for use in non-testing parts of QEMU.
+ *
+ * This function breaks an existing connection of an outbound GPIO
+ * line from @dev, and replaces it with the new qemu_irq @icpt, as if
+ * ``qdev_connect_gpio_out_named(dev, icpt, name, n)`` had been called.
+ * The previously connected qemu_irq is returned, so it can be restored
+ * by a second call to qdev_intercept_gpio_out() if desired.
+ */
 qemu_irq qdev_intercept_gpio_out(DeviceState *dev, qemu_irq icpt,
                                  const char *name, int n);
 
@@ -438,10 +557,59 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
 
 /*** Device API.  ***/
 
-/* Register device properties.  */
-/* GPIO inputs also double as IRQ sinks.  */
+/**
+ * qdev_init_gpio_in: create an array of anonymous input GPIO lines
+ * @dev: Device to create input GPIOs for
+ * @handler: Function to call when GPIO line value is set
+ * @n: Number of GPIO lines to create
+ *
+ * Devices should use functions in the qdev_init_gpio_in* family in
+ * their instance_init or realize methods to create any input GPIO
+ * lines they need. There is no functional difference between
+ * anonymous and named GPIO lines. Stylistically, named GPIOs are
+ * preferable (easier to understand at callsites) unless a device
+ * has exactly one uniform kind of GPIO input whose purpose is obvious.
+ * Note that input GPIO lines can serve as 'sinks' for IRQ lines.
+ *
+ * See qdev_get_gpio_in() for how code that uses such a device can get
+ * hold of an input GPIO line to manipulate it.
+ */
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n);
+/**
+ * qdev_init_gpio_out: create an array of anonymous output GPIO lines
+ * @dev: Device to create output GPIOs for
+ * @pins: Pointer to qemu_irq or qemu_irq array for the GPIO lines
+ * @n: Number of GPIO lines to create
+ *
+ * Devices should use functions in the qdev_init_gpio_out* family
+ * in their instance_init or realize methods to create any output
+ * GPIO lines they need. There is no functional difference between
+ * anonymous and named GPIO lines. Stylistically, named GPIOs are
+ * preferable (easier to understand at callsites) unless a device
+ * has exactly one uniform kind of GPIO output whose purpose is obvious.
+ *
+ * The @pins argument should be a pointer to either a "qemu_irq"
+ * (if @n == 1) or a "qemu_irq []" array (if @n > 1) 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 qdev_connect_gpio_out() for how code that uses such a device
+ * can connect to one of its output GPIO lines.
+ */
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n);
+/**
+ * qdev_init_gpio_out: create an array of named output GPIO lines
+ * @dev: Device to create output GPIOs for
+ * @pins: Pointer to qemu_irq or qemu_irq array for the GPIO lines
+ * @name: Name to give this array of GPIO lines
+ * @n: Number of GPIO lines to create
+ *
+ * Like qdev_init_gpio_out(), but creates an array of GPIO output lines
+ * with a name. Code using the device can then connect these GPIO lines
+ * using qdev_connect_gpio_out_named().
+ */
 void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
                               const char *name, int n);
 /**
@@ -473,6 +641,25 @@ static inline void qdev_init_gpio_in_named(DeviceState *dev,
     qdev_init_gpio_in_named_with_opaque(dev, handler, dev, name, n);
 }
 
+/**
+ * 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
+ * @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
+ * "container" QOM device which itself contains other QOM devices and
+ * which wires them up appropriately. This function allows the container
+ * 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
+ * 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
+ * behaves exactly like any other.
+ */
 void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
                      const char *name);
 
-- 
2.20.1

Re: [PATCH for-5.1 0/3] Move and improve qdev API doc comments

[Richard Henderson]

On 7/11/20 7:24 AM, Peter Maydell wrote:
> Peter Maydell (3):
>   qdev: Move doc comments from qdev.c to qdev-core.h
>   qdev: Document qdev_unrealize()
>   qdev: Document GPIO related functions

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~