Re: [PATCH v2 02/12] device: property: find dependencies of a firmware node

["Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>]

On Wednesday, July 01, 2015 11:40:57 AM Tomeu Vizoso wrote:
> Adds API that allows callers to find out what other firmware nodes a
> node depends on.
> 
> Implementors of bindings documentation can register callbacks that
> return the dependencies of a node.
> 
> Dependency information can be used to change the order in which devices
> are probed, or to print a warning when a device node is going to be
> probed without all its dependencies fulfilled.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>

I'd like to see a description of the new API in English in the changelog.

> ---
> 
> Changes in v2:
> - Allow bindings implementations register a function instead of using
>   class callbacks, as not only subsystems implement firmware bindings.
> 
>  drivers/base/property.c  | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fwnode.h   |  5 +++
>  include/linux/property.h | 12 +++++++
>  3 files changed, 108 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 8ead1ba..9d38ede 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -19,7 +19,13 @@
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
>  

Please add a comment describing this structure.  In particular, what it is
supposed to be used for and how it is supposed to be used.

> +struct dependency_parser {
> +	struct list_head parser;

I'd rather call this "list_node" or something like that.

> +	void (*func)(struct fwnode_handle *fwnode, struct list_head *deps);
> +};
> +
>  static bool fwnode_match_enable = false;
> +static LIST_HEAD(dependency_parsers);
>  
>  /**
>   * device_add_property_set - Add a collection of properties to a device object.
> @@ -553,6 +559,27 @@ bool device_dma_is_coherent(struct device *dev)
>  EXPORT_SYMBOL_GPL(device_dma_is_coherent);
>  
>  /**
> + * fwnode_add_dependency - add firmware node to the passed dependency list
> + * @fwnode: Firmware node to add to dependency list
> + * @list: Dependency list to add the fwnode to
> + */
> +void fwnode_add_dependency(struct fwnode_handle *fwnode,
> +			   struct list_head *list)
> +{
> +	struct fwnode_dependency *dep;
> +
> +	dep = kzalloc(sizeof(*dep), GFP_KERNEL);
> +	if (!dep)
> +		return;
> +
> +	INIT_LIST_HEAD(&dep->dependency);
> +	dep->fwnode = fwnode;
> +
> +	list_add_tail(&dep->dependency, list);
> +}
> +EXPORT_SYMBOL_GPL(fwnode_add_dependency);
> +
> +/**
>   * fwnode_get_parent - return the parent node of a device node
>   * @fwnode: Device node to find the parent node of
>   */
> @@ -600,6 +627,70 @@ bool fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compatible)
>  EXPORT_SYMBOL_GPL(fwnode_is_compatible);
>  
>  /**
> + * fwnode_add_dependency_parser - register dependency parser
> + * @func: Function that will be called to find out dependencies of a node
> + *
> + * Registers a callback that will be called when collecting the dependencies
> + * of a firmware node. The callback should inspect the properties of the node
> + * and call fwnode_add_dependency() for each dependency it recognizes, from
> + * the bindings documentation.
> + */
> +void fwnode_add_dependency_parser(
> +	void (*func)(struct fwnode_handle *fwnode, struct list_head *deps))
> +{
> +	struct dependency_parser *parser;
> +
> +	parser = kzalloc(sizeof(*parser), GFP_KERNEL);
> +	if (!parser)
> +		return;
> +
> +	INIT_LIST_HEAD(&parser->parser);
> +	parser->func = func;
> +
> +	list_add_tail(&parser->parser, &dependency_parsers);

We're modifying a global list here.  Any locking needed?  RCU?  Whatever?

> +}
> +EXPORT_SYMBOL_GPL(fwnode_add_dependency_parser);
> +
> +/**
> + * fwnode_remove_dependency_parser - unregister dependency parser
> + * @func: Function that was to be called to find out dependencies of a node
> + */
> +void fwnode_remove_dependency_parser(
> +	void (*func)(struct fwnode_handle *fwnode, struct list_head *deps))
> +{
> +	struct dependency_parser *parser, *tmp;
> +
> +	list_for_each_entry_safe(parser, tmp, &dependency_parsers, parser) {
> +		if (parser->func == func) {
> +			list_del(&parser->parser);
> +			kfree(parser);
> +			return;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(fwnode_remove_dependency_parser);
> +
> +/**
> + * fwnode_get_dependencies - find out what dependencies a firmware node has
> + * @fwnode: firmware node to find its dependencies
> + * @deps: list of struct fwnode_dependency in which dependencies will be placed
> + */
> +void fwnode_get_dependencies(struct fwnode_handle *fwnode,
> +			     struct list_head *deps)
> +{
> +	struct dependency_parser *parser;
> +	struct fwnode_handle *child;
> +
> +	list_for_each_entry(parser, &dependency_parsers, parser)
> +		parser->func(fwnode, deps);
> +
> +	/* Some device nodes will have dependencies in non-device sub-nodes */
> +	fwnode_for_each_child_node(fwnode, child)
> +		if (!fwnode_property_present(child, "compatible"))

This is a blatant OF-ism.  We need to think about a generic way to express that.

> +			fwnode_get_dependencies(child, deps);
> +}
> +
> +/**
>   * fwnode_driver_match_device - Tell if a driver matches a device.
>   * @drv: the device_driver structure to test
>   * @dev: the device structure to match against
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 0408545..68ab558 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -24,4 +24,9 @@ struct fwnode_handle {
>  	struct fwnode_handle *secondary;
>  };
>  
> +struct fwnode_dependency {
> +	struct fwnode_handle *fwnode;
> +	struct list_head dependency;

So this is a node in the list of dependencies, right?

I'd call that field "list_node", then.

> +};

And fwnode is a firmware node that the owner of the list depends on, right?

> +
>  #endif
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 4e453c4..b8b86ea 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -86,6 +86,18 @@ bool fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compatible);
>  bool fwnode_driver_match_device(struct device *dev,
>  				const struct device_driver *drv);
>  
> +void fwnode_add_dependency(struct fwnode_handle *fwnode,
> +			   struct list_head *list);
> +
> +void fwnode_add_dependency_parser(
> +	void (*func)(struct fwnode_handle *fwnode, struct list_head *deps));
> +
> +void fwnode_remove_dependency_parser(
> +	void (*func)(struct fwnode_handle *fwnode, struct list_head *deps));
> +
> +void fwnode_get_dependencies(struct fwnode_handle *fwnode,
> +			     struct list_head *list);
> +
>  unsigned int device_get_child_node_count(struct device *dev);
>  
>  static inline bool device_property_read_bool(struct device *dev,
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html