| Message ID | 20230216205708.13453-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v1,1/1] device property: Clarify description on returned value in some functions | expand |
Hi Andy, On Thu, Feb 16, 2023 at 10:57:08PM +0200, Andy Shevchenko wrote: > Some of the functions do not provide a Return: section on absence of which > kernel-doc complains. Besides that several functions return the fwnode > handle with bumped reference count. Add a respective note to make sure > that the caller drops it when it's not needed anymore. > > While at it, unify the style of the Return: sections. > > Reported-by: Daniel Kaehn <kaehndan@gmail.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/base/property.c | 117 ++++++++++++++++++++++++++++------------ > 1 file changed, 82 insertions(+), 35 deletions(-) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 083a95791d3b..b75e63984702 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -37,6 +37,8 @@ EXPORT_SYMBOL_GPL(__dev_fwnode_const); > * @propname: Name of the property > * > * Check if property @propname is present in the device firmware description. > + * > + * Return: true if property @propname is present. Otherwise, returns false. > */ > bool device_property_present(struct device *dev, const char *propname) > { > @@ -48,6 +50,8 @@ EXPORT_SYMBOL_GPL(device_property_present); > * fwnode_property_present - check if a property of a firmware node is present > * @fwnode: Firmware node whose property to check > * @propname: Name of the property > + * > + * Return: true if property @propname is present. Otherwise, returns false. > */ > bool fwnode_property_present(const struct fwnode_handle *fwnode, > const char *propname) > @@ -511,7 +515,7 @@ EXPORT_SYMBOL_GPL(fwnode_property_match_string); > * Caller is responsible to call fwnode_handle_put() on the returned > * args->fwnode pointer. > * > - * Returns: %0 on success > + * Return: %0 on success > * %-ENOENT when the index is out of bounds, the index has an empty > * reference or the property was not found > * %-EINVAL on parse error > @@ -547,8 +551,10 @@ EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args); > * > * @index can be used when the named reference holds a table of references. > * > - * Returns pointer to the reference fwnode, or ERR_PTR. Caller is responsible to > - * call fwnode_handle_put() on the returned fwnode pointer. > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > + * > + * Return: a pointer to the reference fwnode, or an error pointer. > */ > struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode, > const char *name, > @@ -567,7 +573,7 @@ EXPORT_SYMBOL_GPL(fwnode_find_reference); > * fwnode_get_name - Return the name of a node > * @fwnode: The firmware node > * > - * Returns a pointer to the node name. > + * Return: a pointer to the node name, or %NULL. > */ > const char *fwnode_get_name(const struct fwnode_handle *fwnode) > { > @@ -579,7 +585,7 @@ EXPORT_SYMBOL_GPL(fwnode_get_name); > * fwnode_get_name_prefix - Return the prefix of node for printing purposes > * @fwnode: The firmware node > * > - * Returns the prefix of a node, intended to be printed right before the node. > + * Return: the prefix of a node, intended to be printed right before the node. > * The prefix works also as a separator between the nodes. > */ > const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode) > @@ -591,7 +597,10 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode) > * fwnode_get_parent - Return parent firwmare node > * @fwnode: Firmware whose parent is retrieved > * > - * Return parent firmware node of the given node if possible or %NULL if no > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > + * > + * Return: parent firmware node of the given node if possible or %NULL if no > * parent was available. > */ > struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode) > @@ -608,8 +617,11 @@ EXPORT_SYMBOL_GPL(fwnode_get_parent); > * on the passed node, making it suitable for iterating through a > * node's parents. > * > - * Returns a node pointer with refcount incremented, use > - * fwnode_handle_put() on it when done. > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > + * > + * Return: parent firmware node of the given node if possible or %NULL if no > + * parent was available. > */ > struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode) > { > @@ -629,8 +641,7 @@ EXPORT_SYMBOL_GPL(fwnode_get_next_parent); > * firmware node that has a corresponding struct device and returns that struct > * device. > * > - * The caller of this function is expected to call put_device() on the returned > - * device when they are done. > + * Caller is responsible to call put_device() on the returned device pointer. > */ > struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode) > { > @@ -651,7 +662,7 @@ struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode) > * fwnode_count_parents - Return the number of parents a node has > * @fwnode: The node the parents of which are to be counted > * > - * Returns the number of parents a node has. > + * Return: the number of parents a node has. > */ > unsigned int fwnode_count_parents(const struct fwnode_handle *fwnode) > { > @@ -670,12 +681,12 @@ EXPORT_SYMBOL_GPL(fwnode_count_parents); > * @fwnode: The node the parent of which is requested > * @depth: Distance of the parent from the node > * > - * Returns the nth parent of a node. If there is no parent at the requested > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > + * > + * Return: the nth parent of a node. If there is no parent at the requested > * @depth, %NULL is returned. If @depth is 0, the functionality is equivalent to > * fwnode_handle_get(). For @depth == 1, it is fwnode_get_parent() and so on. > - * > - * The caller is responsible for calling fwnode_handle_put() for the returned > - * node. > */ > struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode, > unsigned int depth) > @@ -700,7 +711,7 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent); > * > * A node is considered an ancestor of itself too. > * > - * Returns true if @ancestor is an ancestor of @child. Otherwise, returns false. > + * Return: true if @ancestor is an ancestor of @child. Otherwise, returns false. > */ > bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child) > { > @@ -725,6 +736,9 @@ bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle > * fwnode_get_next_child_node - Return the next child node handle for a node > * @fwnode: Firmware node to find the next child node for. > * @child: Handle to one of the node's child nodes or a %NULL handle. > + * > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. The loop itself will also put the child node, so this is only relevant outside the loop. > */ > struct fwnode_handle * > fwnode_get_next_child_node(const struct fwnode_handle *fwnode, > @@ -735,10 +749,12 @@ fwnode_get_next_child_node(const struct fwnode_handle *fwnode, > EXPORT_SYMBOL_GPL(fwnode_get_next_child_node); > > /** > - * fwnode_get_next_available_child_node - Return the next > - * available child node handle for a node > + * fwnode_get_next_available_child_node - Return the next available child node handle for a node > * @fwnode: Firmware node to find the next child node for. > * @child: Handle to one of the node's child nodes or a %NULL handle. > + * > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. Same here. And the two loops below. > */ > struct fwnode_handle * > fwnode_get_next_available_child_node(const struct fwnode_handle *fwnode, > @@ -762,7 +778,10 @@ EXPORT_SYMBOL_GPL(fwnode_get_next_available_child_node); > /** > * device_get_next_child_node - Return the next child node handle for a device > * @dev: Device to find the next child node for. > - * @child: Handle to one of the device's child nodes or a null handle. > + * @child: Handle to one of the device's child nodes or a %NULL handle. > + * > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > */ > struct fwnode_handle *device_get_next_child_node(const struct device *dev, > struct fwnode_handle *child) > @@ -787,6 +806,9 @@ EXPORT_SYMBOL_GPL(device_get_next_child_node); > * fwnode_get_named_child_node - Return first matching named child node handle > * @fwnode: Firmware node to find the named child node for. > * @childname: String to match child node name against. > + * > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > */ > struct fwnode_handle * > fwnode_get_named_child_node(const struct fwnode_handle *fwnode, > @@ -800,6 +822,9 @@ EXPORT_SYMBOL_GPL(fwnode_get_named_child_node); > * device_get_named_child_node - Return first matching named child node handle > * @dev: Device to find the named child node for. > * @childname: String to match child node name against. > + * > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > */ > struct fwnode_handle *device_get_named_child_node(const struct device *dev, > const char *childname) > @@ -812,7 +837,10 @@ EXPORT_SYMBOL_GPL(device_get_named_child_node); > * fwnode_handle_get - Obtain a reference to a device node > * @fwnode: Pointer to the device node to obtain the reference to. > * > - * Returns the fwnode handle. > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > + * > + * Return: the fwnode handle. > */ > struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode) > { > @@ -827,9 +855,9 @@ EXPORT_SYMBOL_GPL(fwnode_handle_get); > * fwnode_handle_put - Drop reference to a device node > * @fwnode: Pointer to the device node to drop the reference to. > * > - * This has to be used when terminating device_for_each_child_node() iteration > - * with break or return to prevent stale device node references from being left > - * behind. > + * Among other cases this has to be used when terminating device_for_each_child_node() I don't think device_for_each_child_node() should be mentioned here. This isn't really related to that in particular. With these: Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > + * iteration with break or return to prevent stale device node references from > + * being left behind. > */ > void fwnode_handle_put(struct fwnode_handle *fwnode) > { > @@ -841,6 +869,8 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); > * fwnode_device_is_available - check if a device is available for use > * @fwnode: Pointer to the fwnode of the device. > * > + * Return: true if device is available for use. Otherwise, returns false. > + * > * For fwnode node types that don't implement the .device_is_available() > * operation, this function returns true. > */ > @@ -859,6 +889,8 @@ EXPORT_SYMBOL_GPL(fwnode_device_is_available); > /** > * device_get_child_node_count - return the number of child nodes for device > * @dev: Device to cound the child nodes for > + * > + * Return: the number of child nodes for a given device. > */ > unsigned int device_get_child_node_count(const struct device *dev) > { > @@ -934,7 +966,7 @@ EXPORT_SYMBOL_GPL(device_get_phy_mode); > * @fwnode: Pointer to the firmware node > * @index: Index of the IO range > * > - * Returns a pointer to the mapped memory. > + * Return: a pointer to the mapped memory. > */ > void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index) > { > @@ -947,8 +979,8 @@ EXPORT_SYMBOL(fwnode_iomap); > * @fwnode: Pointer to the firmware node > * @index: Zero-based index of the IRQ > * > - * Returns Linux IRQ number on success. Other values are determined > - * accordingly to acpi_/of_ irq_get() operation. > + * Return: Linux IRQ number on success. Other values are determined > + * according to acpi_irq_get() or of_irq_get() operation. > */ > int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index) > { > @@ -967,8 +999,7 @@ EXPORT_SYMBOL(fwnode_irq_get); > * number of the IRQ resource corresponding to the index of the matched > * string. > * > - * Return: > - * Linux IRQ number on success, or negative errno otherwise. > + * Return: Linux IRQ number on success, or negative errno otherwise. > */ > int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name) > { > @@ -990,7 +1021,10 @@ EXPORT_SYMBOL(fwnode_irq_get_byname); > * @fwnode: Pointer to the parent firmware node > * @prev: Previous endpoint node or %NULL to get the first > * > - * Returns an endpoint firmware node pointer or %NULL if no more endpoints > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > + * > + * Return: an endpoint firmware node pointer or %NULL if no more endpoints > * are available. > */ > struct fwnode_handle * > @@ -1030,6 +1064,9 @@ EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint); > * fwnode_graph_get_port_parent - Return the device fwnode of a port endpoint > * @endpoint: Endpoint firmware node of the port > * > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > + * > * Return: the firmware node of the device the @endpoint belongs to. > */ > struct fwnode_handle * > @@ -1051,6 +1088,9 @@ EXPORT_SYMBOL_GPL(fwnode_graph_get_port_parent); > * @fwnode: Endpoint firmware node pointing to the remote endpoint > * > * Extracts firmware node of a remote device the @fwnode points to. > + * > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > */ > struct fwnode_handle * > fwnode_graph_get_remote_port_parent(const struct fwnode_handle *fwnode) > @@ -1071,6 +1111,9 @@ EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_port_parent); > * @fwnode: Endpoint firmware node pointing to the remote endpoint > * > * Extracts firmware node of a remote port the @fwnode points to. > + * > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > */ > struct fwnode_handle * > fwnode_graph_get_remote_port(const struct fwnode_handle *fwnode) > @@ -1084,6 +1127,9 @@ EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_port); > * @fwnode: Endpoint firmware node pointing to the remote endpoint > * > * Extracts firmware node of a remote endpoint the @fwnode points to. > + * > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > */ > struct fwnode_handle * > fwnode_graph_get_remote_endpoint(const struct fwnode_handle *fwnode) > @@ -1111,8 +1157,11 @@ static bool fwnode_graph_remote_available(struct fwnode_handle *ep) > * @endpoint: identifier of the endpoint node under the port node > * @flags: fwnode lookup flags > * > - * Return the fwnode handle of the local endpoint corresponding the port and > - * endpoint IDs or NULL if not found. > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > + * pointer. > + * > + * Return: the fwnode handle of the local endpoint corresponding the port and > + * endpoint IDs or %NULL if not found. > * > * If FWNODE_GRAPH_ENDPOINT_NEXT is passed in @flags and the specified endpoint > * has not been found, look for the closest endpoint ID greater than the > @@ -1120,9 +1169,6 @@ static bool fwnode_graph_remote_available(struct fwnode_handle *ep) > * > * Does not return endpoints that belong to disabled devices or endpoints that > * are unconnected, unless FWNODE_GRAPH_DEVICE_DISABLED is passed in @flags. > - * > - * The returned endpoint needs to be released by calling fwnode_handle_put() on > - * it when it is not needed any more. > */ > struct fwnode_handle * > fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode, > @@ -1328,7 +1374,8 @@ EXPORT_SYMBOL_GPL(fwnode_connection_find_match); > * @fwnode and other device nodes. @match will be used to convert the > * connection description to data the caller is expecting to be returned > * through the @matches array. > - * If @matches is NULL @matches_len is ignored and the total number of resolved > + * > + * If @matches is %NULL @matches_len is ignored and the total number of resolved > * matches is returned. > * > * Return: Number of matches resolved, or negative errno.
On Fri, Feb 17, 2023 at 12:27:53PM +0200, Sakari Ailus wrote: > On Thu, Feb 16, 2023 at 10:57:08PM +0200, Andy Shevchenko wrote: ... > > * fwnode_get_next_child_node - Return the next child node handle for a node > > * @fwnode: Firmware node to find the next child node for. > > * @child: Handle to one of the node's child nodes or a %NULL handle. > > + * > > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > > + * pointer. > > The loop itself will also put the child node, so this is only relevant > outside the loop. Yes and this is exactly what people stumbled over. Hence this note. This call per se doesn't loop, so I didn't get how your comment can be transformed to anything here. Care to elaborate a bit more on what I have to add here or reword? ... > > + * Among other cases this has to be used when terminating device_for_each_child_node() > > I don't think device_for_each_child_node() should be mentioned here. This > isn't really related to that in particular. Okay, I will drop this change. It's anyway out of the scope of this patch. > With these: > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> Thanks, but due to above I can't apply it right away.
Hi Andy, On Fri, Feb 17, 2023 at 01:01:39PM +0200, Andy Shevchenko wrote: > On Fri, Feb 17, 2023 at 12:27:53PM +0200, Sakari Ailus wrote: > > On Thu, Feb 16, 2023 at 10:57:08PM +0200, Andy Shevchenko wrote: > > ... > > > > * fwnode_get_next_child_node - Return the next child node handle for a node > > > * @fwnode: Firmware node to find the next child node for. > > > * @child: Handle to one of the node's child nodes or a %NULL handle. > > > + * > > > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > > > + * pointer. > > > > The loop itself will also put the child node, so this is only relevant > > outside the loop. > > Yes and this is exactly what people stumbled over. Hence this note. > This call per se doesn't loop, so I didn't get how your comment can > be transformed to anything here. Care to elaborate a bit more on > what I have to add here or reword? Ah, indeed. This is achieved by putting the previous child. Generally this function is used via the loop helper macro and not called directly, hence the documentation there matters the most. Those functions appear to be without any documentation though.
On Fri, Feb 17, 2023 at 01:18:31PM +0200, Sakari Ailus wrote: > On Fri, Feb 17, 2023 at 01:01:39PM +0200, Andy Shevchenko wrote: > > On Fri, Feb 17, 2023 at 12:27:53PM +0200, Sakari Ailus wrote: > > > On Thu, Feb 16, 2023 at 10:57:08PM +0200, Andy Shevchenko wrote: ... > > > > * fwnode_get_next_child_node - Return the next child node handle for a node > > > > * @fwnode: Firmware node to find the next child node for. > > > > * @child: Handle to one of the node's child nodes or a %NULL handle. > > > > + * > > > > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > > > > + * pointer. > > > > > > The loop itself will also put the child node, so this is only relevant > > > outside the loop. > > > > Yes and this is exactly what people stumbled over. Hence this note. > > This call per se doesn't loop, so I didn't get how your comment can > > be transformed to anything here. Care to elaborate a bit more on > > what I have to add here or reword? > > Ah, indeed. This is achieved by putting the previous child. Generally this > function is used via the loop helper macro and not called directly, hence > the documentation there matters the most. Those functions appear to be > without any documentation though. So, what should I do?
On Fri, Feb 17, 2023 at 01:28:55PM +0200, Andy Shevchenko wrote: > On Fri, Feb 17, 2023 at 01:18:31PM +0200, Sakari Ailus wrote: > > On Fri, Feb 17, 2023 at 01:01:39PM +0200, Andy Shevchenko wrote: > > > On Fri, Feb 17, 2023 at 12:27:53PM +0200, Sakari Ailus wrote: > > > > On Thu, Feb 16, 2023 at 10:57:08PM +0200, Andy Shevchenko wrote: > > ... > > > > > > * fwnode_get_next_child_node - Return the next child node handle for a node > > > > > * @fwnode: Firmware node to find the next child node for. > > > > > * @child: Handle to one of the node's child nodes or a %NULL handle. > > > > > + * > > > > > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > > > > > + * pointer. > > > > > > > > The loop itself will also put the child node, so this is only relevant > > > > outside the loop. > > > > > > Yes and this is exactly what people stumbled over. Hence this note. > > > This call per se doesn't loop, so I didn't get how your comment can > > > be transformed to anything here. Care to elaborate a bit more on > > > what I have to add here or reword? > > > > Ah, indeed. This is achieved by putting the previous child. Generally this > > function is used via the loop helper macro and not called directly, hence > > the documentation there matters the most. Those functions appear to be > > without any documentation though. > > So, what should I do? Good question. How about this text: The caller is responsible for calling fwnode_handle_put() put on the returned fwnode. Note that this function also puts a reference to @child unconditionally. This is actually done by the firmware specific implementation, namely on OF and at least should be done on swnode. A second patch to document the fwnode iterator macros would be nice.
On Fri, Feb 17, 2023 at 01:44:25PM +0200, Sakari Ailus wrote: > On Fri, Feb 17, 2023 at 01:28:55PM +0200, Andy Shevchenko wrote: > > On Fri, Feb 17, 2023 at 01:18:31PM +0200, Sakari Ailus wrote: > > > On Fri, Feb 17, 2023 at 01:01:39PM +0200, Andy Shevchenko wrote: > > > > On Fri, Feb 17, 2023 at 12:27:53PM +0200, Sakari Ailus wrote: > > > > > On Thu, Feb 16, 2023 at 10:57:08PM +0200, Andy Shevchenko wrote: ... > > > > > > * fwnode_get_next_child_node - Return the next child node handle for a node > > > > > > * @fwnode: Firmware node to find the next child node for. > > > > > > * @child: Handle to one of the node's child nodes or a %NULL handle. > > > > > > + * > > > > > > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > > > > > > + * pointer. > > > > > > > > > > The loop itself will also put the child node, so this is only relevant > > > > > outside the loop. > > > > > > > > Yes and this is exactly what people stumbled over. Hence this note. > > > > This call per se doesn't loop, so I didn't get how your comment can > > > > be transformed to anything here. Care to elaborate a bit more on > > > > what I have to add here or reword? > > > > > > Ah, indeed. This is achieved by putting the previous child. Generally this > > > function is used via the loop helper macro and not called directly, hence > > > the documentation there matters the most. Those functions appear to be > > > without any documentation though. > > > > So, what should I do? > > Good question. > > How about this text: > > The caller is responsible for calling fwnode_handle_put() put on the > returned fwnode. Note that this function also puts a reference to @child > unconditionally. Fine. Does it mean I have to change existing wording of the first sentence everywhere? > This is actually done by the firmware specific implementation, namely on OF > and at least should be done on swnode. Yes, that's. But it's not needed to be added. > A second patch to document the fwnode iterator macros would be nice. Not the reported problem. Maybe someone else can do the job?
On Fri, Feb 17, 2023 at 01:55:43PM +0200, Andy Shevchenko wrote: > On Fri, Feb 17, 2023 at 01:44:25PM +0200, Sakari Ailus wrote: > > On Fri, Feb 17, 2023 at 01:28:55PM +0200, Andy Shevchenko wrote: > > > On Fri, Feb 17, 2023 at 01:18:31PM +0200, Sakari Ailus wrote: > > > > On Fri, Feb 17, 2023 at 01:01:39PM +0200, Andy Shevchenko wrote: > > > > > On Fri, Feb 17, 2023 at 12:27:53PM +0200, Sakari Ailus wrote: > > > > > > On Thu, Feb 16, 2023 at 10:57:08PM +0200, Andy Shevchenko wrote: > > ... > > > > > > > > * fwnode_get_next_child_node - Return the next child node handle for a node > > > > > > > * @fwnode: Firmware node to find the next child node for. > > > > > > > * @child: Handle to one of the node's child nodes or a %NULL handle. > > > > > > > + * > > > > > > > + * Caller is responsible to call fwnode_handle_put() on the returned fwnode > > > > > > > + * pointer. > > > > > > > > > > > > The loop itself will also put the child node, so this is only relevant > > > > > > outside the loop. > > > > > > > > > > Yes and this is exactly what people stumbled over. Hence this note. > > > > > This call per se doesn't loop, so I didn't get how your comment can > > > > > be transformed to anything here. Care to elaborate a bit more on > > > > > what I have to add here or reword? > > > > > > > > Ah, indeed. This is achieved by putting the previous child. Generally this > > > > function is used via the loop helper macro and not called directly, hence > > > > the documentation there matters the most. Those functions appear to be > > > > without any documentation though. > > > > > > So, what should I do? > > > > Good question. > > > > How about this text: > > > > The caller is responsible for calling fwnode_handle_put() put on the > > returned fwnode. Note that this function also puts a reference to @child > > unconditionally. > > Fine. Does it mean I have to change existing wording of the first sentence > everywhere? Up to you. > > > This is actually done by the firmware specific implementation, namely on OF > > and at least should be done on swnode. > > Yes, that's. But it's not needed to be added. > > > A second patch to document the fwnode iterator macros would be nice. > > Not the reported problem. Maybe someone else can do the job? It's perhaps not the most pressing issue at the moment. Let's see.
diff --git a/drivers/base/property.c b/drivers/base/property.c index 083a95791d3b..b75e63984702 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -37,6 +37,8 @@ EXPORT_SYMBOL_GPL(__dev_fwnode_const); * @propname: Name of the property * * Check if property @propname is present in the device firmware description. + * + * Return: true if property @propname is present. Otherwise, returns false. */ bool device_property_present(struct device *dev, const char *propname) { @@ -48,6 +50,8 @@ EXPORT_SYMBOL_GPL(device_property_present); * fwnode_property_present - check if a property of a firmware node is present * @fwnode: Firmware node whose property to check * @propname: Name of the property + * + * Return: true if property @propname is present. Otherwise, returns false. */ bool fwnode_property_present(const struct fwnode_handle *fwnode, const char *propname) @@ -511,7 +515,7 @@ EXPORT_SYMBOL_GPL(fwnode_property_match_string); * Caller is responsible to call fwnode_handle_put() on the returned * args->fwnode pointer. * - * Returns: %0 on success + * Return: %0 on success * %-ENOENT when the index is out of bounds, the index has an empty * reference or the property was not found * %-EINVAL on parse error @@ -547,8 +551,10 @@ EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args); * * @index can be used when the named reference holds a table of references. * - * Returns pointer to the reference fwnode, or ERR_PTR. Caller is responsible to - * call fwnode_handle_put() on the returned fwnode pointer. + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. + * + * Return: a pointer to the reference fwnode, or an error pointer. */ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode, const char *name, @@ -567,7 +573,7 @@ EXPORT_SYMBOL_GPL(fwnode_find_reference); * fwnode_get_name - Return the name of a node * @fwnode: The firmware node * - * Returns a pointer to the node name. + * Return: a pointer to the node name, or %NULL. */ const char *fwnode_get_name(const struct fwnode_handle *fwnode) { @@ -579,7 +585,7 @@ EXPORT_SYMBOL_GPL(fwnode_get_name); * fwnode_get_name_prefix - Return the prefix of node for printing purposes * @fwnode: The firmware node * - * Returns the prefix of a node, intended to be printed right before the node. + * Return: the prefix of a node, intended to be printed right before the node. * The prefix works also as a separator between the nodes. */ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode) @@ -591,7 +597,10 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode) * fwnode_get_parent - Return parent firwmare node * @fwnode: Firmware whose parent is retrieved * - * Return parent firmware node of the given node if possible or %NULL if no + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. + * + * Return: parent firmware node of the given node if possible or %NULL if no * parent was available. */ struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode) @@ -608,8 +617,11 @@ EXPORT_SYMBOL_GPL(fwnode_get_parent); * on the passed node, making it suitable for iterating through a * node's parents. * - * Returns a node pointer with refcount incremented, use - * fwnode_handle_put() on it when done. + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. + * + * Return: parent firmware node of the given node if possible or %NULL if no + * parent was available. */ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode) { @@ -629,8 +641,7 @@ EXPORT_SYMBOL_GPL(fwnode_get_next_parent); * firmware node that has a corresponding struct device and returns that struct * device. * - * The caller of this function is expected to call put_device() on the returned - * device when they are done. + * Caller is responsible to call put_device() on the returned device pointer. */ struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode) { @@ -651,7 +662,7 @@ struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode) * fwnode_count_parents - Return the number of parents a node has * @fwnode: The node the parents of which are to be counted * - * Returns the number of parents a node has. + * Return: the number of parents a node has. */ unsigned int fwnode_count_parents(const struct fwnode_handle *fwnode) { @@ -670,12 +681,12 @@ EXPORT_SYMBOL_GPL(fwnode_count_parents); * @fwnode: The node the parent of which is requested * @depth: Distance of the parent from the node * - * Returns the nth parent of a node. If there is no parent at the requested + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. + * + * Return: the nth parent of a node. If there is no parent at the requested * @depth, %NULL is returned. If @depth is 0, the functionality is equivalent to * fwnode_handle_get(). For @depth == 1, it is fwnode_get_parent() and so on. - * - * The caller is responsible for calling fwnode_handle_put() for the returned - * node. */ struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode, unsigned int depth) @@ -700,7 +711,7 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent); * * A node is considered an ancestor of itself too. * - * Returns true if @ancestor is an ancestor of @child. Otherwise, returns false. + * Return: true if @ancestor is an ancestor of @child. Otherwise, returns false. */ bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child) { @@ -725,6 +736,9 @@ bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle * fwnode_get_next_child_node - Return the next child node handle for a node * @fwnode: Firmware node to find the next child node for. * @child: Handle to one of the node's child nodes or a %NULL handle. + * + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. */ struct fwnode_handle * fwnode_get_next_child_node(const struct fwnode_handle *fwnode, @@ -735,10 +749,12 @@ fwnode_get_next_child_node(const struct fwnode_handle *fwnode, EXPORT_SYMBOL_GPL(fwnode_get_next_child_node); /** - * fwnode_get_next_available_child_node - Return the next - * available child node handle for a node + * fwnode_get_next_available_child_node - Return the next available child node handle for a node * @fwnode: Firmware node to find the next child node for. * @child: Handle to one of the node's child nodes or a %NULL handle. + * + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. */ struct fwnode_handle * fwnode_get_next_available_child_node(const struct fwnode_handle *fwnode, @@ -762,7 +778,10 @@ EXPORT_SYMBOL_GPL(fwnode_get_next_available_child_node); /** * device_get_next_child_node - Return the next child node handle for a device * @dev: Device to find the next child node for. - * @child: Handle to one of the device's child nodes or a null handle. + * @child: Handle to one of the device's child nodes or a %NULL handle. + * + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. */ struct fwnode_handle *device_get_next_child_node(const struct device *dev, struct fwnode_handle *child) @@ -787,6 +806,9 @@ EXPORT_SYMBOL_GPL(device_get_next_child_node); * fwnode_get_named_child_node - Return first matching named child node handle * @fwnode: Firmware node to find the named child node for. * @childname: String to match child node name against. + * + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. */ struct fwnode_handle * fwnode_get_named_child_node(const struct fwnode_handle *fwnode, @@ -800,6 +822,9 @@ EXPORT_SYMBOL_GPL(fwnode_get_named_child_node); * device_get_named_child_node - Return first matching named child node handle * @dev: Device to find the named child node for. * @childname: String to match child node name against. + * + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. */ struct fwnode_handle *device_get_named_child_node(const struct device *dev, const char *childname) @@ -812,7 +837,10 @@ EXPORT_SYMBOL_GPL(device_get_named_child_node); * fwnode_handle_get - Obtain a reference to a device node * @fwnode: Pointer to the device node to obtain the reference to. * - * Returns the fwnode handle. + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. + * + * Return: the fwnode handle. */ struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode) { @@ -827,9 +855,9 @@ EXPORT_SYMBOL_GPL(fwnode_handle_get); * fwnode_handle_put - Drop reference to a device node * @fwnode: Pointer to the device node to drop the reference to. * - * This has to be used when terminating device_for_each_child_node() iteration - * with break or return to prevent stale device node references from being left - * behind. + * Among other cases this has to be used when terminating device_for_each_child_node() + * iteration with break or return to prevent stale device node references from + * being left behind. */ void fwnode_handle_put(struct fwnode_handle *fwnode) { @@ -841,6 +869,8 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); * fwnode_device_is_available - check if a device is available for use * @fwnode: Pointer to the fwnode of the device. * + * Return: true if device is available for use. Otherwise, returns false. + * * For fwnode node types that don't implement the .device_is_available() * operation, this function returns true. */ @@ -859,6 +889,8 @@ EXPORT_SYMBOL_GPL(fwnode_device_is_available); /** * device_get_child_node_count - return the number of child nodes for device * @dev: Device to cound the child nodes for + * + * Return: the number of child nodes for a given device. */ unsigned int device_get_child_node_count(const struct device *dev) { @@ -934,7 +966,7 @@ EXPORT_SYMBOL_GPL(device_get_phy_mode); * @fwnode: Pointer to the firmware node * @index: Index of the IO range * - * Returns a pointer to the mapped memory. + * Return: a pointer to the mapped memory. */ void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index) { @@ -947,8 +979,8 @@ EXPORT_SYMBOL(fwnode_iomap); * @fwnode: Pointer to the firmware node * @index: Zero-based index of the IRQ * - * Returns Linux IRQ number on success. Other values are determined - * accordingly to acpi_/of_ irq_get() operation. + * Return: Linux IRQ number on success. Other values are determined + * according to acpi_irq_get() or of_irq_get() operation. */ int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index) { @@ -967,8 +999,7 @@ EXPORT_SYMBOL(fwnode_irq_get); * number of the IRQ resource corresponding to the index of the matched * string. * - * Return: - * Linux IRQ number on success, or negative errno otherwise. + * Return: Linux IRQ number on success, or negative errno otherwise. */ int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name) { @@ -990,7 +1021,10 @@ EXPORT_SYMBOL(fwnode_irq_get_byname); * @fwnode: Pointer to the parent firmware node * @prev: Previous endpoint node or %NULL to get the first * - * Returns an endpoint firmware node pointer or %NULL if no more endpoints + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. + * + * Return: an endpoint firmware node pointer or %NULL if no more endpoints * are available. */ struct fwnode_handle * @@ -1030,6 +1064,9 @@ EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint); * fwnode_graph_get_port_parent - Return the device fwnode of a port endpoint * @endpoint: Endpoint firmware node of the port * + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. + * * Return: the firmware node of the device the @endpoint belongs to. */ struct fwnode_handle * @@ -1051,6 +1088,9 @@ EXPORT_SYMBOL_GPL(fwnode_graph_get_port_parent); * @fwnode: Endpoint firmware node pointing to the remote endpoint * * Extracts firmware node of a remote device the @fwnode points to. + * + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. */ struct fwnode_handle * fwnode_graph_get_remote_port_parent(const struct fwnode_handle *fwnode) @@ -1071,6 +1111,9 @@ EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_port_parent); * @fwnode: Endpoint firmware node pointing to the remote endpoint * * Extracts firmware node of a remote port the @fwnode points to. + * + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. */ struct fwnode_handle * fwnode_graph_get_remote_port(const struct fwnode_handle *fwnode) @@ -1084,6 +1127,9 @@ EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_port); * @fwnode: Endpoint firmware node pointing to the remote endpoint * * Extracts firmware node of a remote endpoint the @fwnode points to. + * + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. */ struct fwnode_handle * fwnode_graph_get_remote_endpoint(const struct fwnode_handle *fwnode) @@ -1111,8 +1157,11 @@ static bool fwnode_graph_remote_available(struct fwnode_handle *ep) * @endpoint: identifier of the endpoint node under the port node * @flags: fwnode lookup flags * - * Return the fwnode handle of the local endpoint corresponding the port and - * endpoint IDs or NULL if not found. + * Caller is responsible to call fwnode_handle_put() on the returned fwnode + * pointer. + * + * Return: the fwnode handle of the local endpoint corresponding the port and + * endpoint IDs or %NULL if not found. * * If FWNODE_GRAPH_ENDPOINT_NEXT is passed in @flags and the specified endpoint * has not been found, look for the closest endpoint ID greater than the @@ -1120,9 +1169,6 @@ static bool fwnode_graph_remote_available(struct fwnode_handle *ep) * * Does not return endpoints that belong to disabled devices or endpoints that * are unconnected, unless FWNODE_GRAPH_DEVICE_DISABLED is passed in @flags. - * - * The returned endpoint needs to be released by calling fwnode_handle_put() on - * it when it is not needed any more. */ struct fwnode_handle * fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode, @@ -1328,7 +1374,8 @@ EXPORT_SYMBOL_GPL(fwnode_connection_find_match); * @fwnode and other device nodes. @match will be used to convert the * connection description to data the caller is expecting to be returned * through the @matches array. - * If @matches is NULL @matches_len is ignored and the total number of resolved + * + * If @matches is %NULL @matches_len is ignored and the total number of resolved * matches is returned. * * Return: Number of matches resolved, or negative errno.
Some of the functions do not provide a Return: section on absence of which kernel-doc complains. Besides that several functions return the fwnode handle with bumped reference count. Add a respective note to make sure that the caller drops it when it's not needed anymore. While at it, unify the style of the Return: sections. Reported-by: Daniel Kaehn <kaehndan@gmail.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/base/property.c | 117 ++++++++++++++++++++++++++++------------ 1 file changed, 82 insertions(+), 35 deletions(-)