Date:   Tue, 6 Nov 2018 16:46:41 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org
Subject: Re: [PATCH 6/6] device property: Remove struct property_set

On Mon, Nov 05, 2018 at 05:59:28PM +0300, Heikki Krogerus wrote:
> Replacing struct property_set with the software nodes that
> were just introduced.
> 
> The API and functionality for adding properties to devices
> remains the same, however, the goal is to convert the
> drivers to use the API for software nodes when the device
> has no real firmware node, and use the old API only when
> "extra" build-in properties are needed.

It might be slightly easier to review if you preserve the ordering of
functions, i.e. device_add_properties() and device_remove_properties().

> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> ---
>  drivers/base/property.c | 346 +++-------------------------------------
>  1 file changed, 24 insertions(+), 322 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index e20642759c67..1f6b8a21bcfe 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -18,198 +18,6 @@
>  #include <linux/etherdevice.h>
>  #include <linux/phy.h>
>  
> -struct property_set {
> -	struct device *dev;
> -	struct fwnode_handle fwnode;
> -	const struct property_entry *properties;
> -};
> -
> -static const struct fwnode_operations pset_fwnode_ops;
> -
> -static inline bool is_pset_node(const struct fwnode_handle *fwnode)
> -{
> -	return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &pset_fwnode_ops;
> -}
> -
> -#define to_pset_node(__fwnode)						\
> -	({								\
> -		typeof(__fwnode) __to_pset_node_fwnode = __fwnode;	\
> -									\
> -		is_pset_node(__to_pset_node_fwnode) ?			\
> -			container_of(__to_pset_node_fwnode,		\
> -				     struct property_set, fwnode) :	\
> -			NULL;						\
> -	})
> -
> -static const struct property_entry *
> -pset_prop_get(const struct property_set *pset, const char *name)
> -{
> -	const struct property_entry *prop;
> -
> -	if (!pset || !pset->properties)
> -		return NULL;
> -
> -	for (prop = pset->properties; prop->name; prop++)
> -		if (!strcmp(name, prop->name))
> -			return prop;
> -
> -	return NULL;
> -}
> -
> -static const void *property_get_pointer(const struct property_entry *prop)
> -{
> -	switch (prop->type) {
> -	case DEV_PROP_U8:
> -		if (prop->is_array)
> -			return prop->pointer.u8_data;
> -		return &prop->value.u8_data;
> -	case DEV_PROP_U16:
> -		if (prop->is_array)
> -			return prop->pointer.u16_data;
> -		return &prop->value.u16_data;
> -	case DEV_PROP_U32:
> -		if (prop->is_array)
> -			return prop->pointer.u32_data;
> -		return &prop->value.u32_data;
> -	case DEV_PROP_U64:
> -		if (prop->is_array)
> -			return prop->pointer.u64_data;
> -		return &prop->value.u64_data;
> -	case DEV_PROP_STRING:
> -		if (prop->is_array)
> -			return prop->pointer.str;
> -		return &prop->value.str;
> -	default:
> -		return NULL;
> -	}
> -}
> -
> -static const void *pset_prop_find(const struct property_set *pset,
> -				  const char *propname, size_t length)
> -{
> -	const struct property_entry *prop;
> -	const void *pointer;
> -
> -	prop = pset_prop_get(pset, propname);
> -	if (!prop)
> -		return ERR_PTR(-EINVAL);
> -	pointer = property_get_pointer(prop);
> -	if (!pointer)
> -		return ERR_PTR(-ENODATA);
> -	if (length > prop->length)
> -		return ERR_PTR(-EOVERFLOW);
> -	return pointer;
> -}
> -
> -static int pset_prop_read_u8_array(const struct property_set *pset,
> -				   const char *propname,
> -				   u8 *values, size_t nval)
> -{
> -	const void *pointer;
> -	size_t length = nval * sizeof(*values);
> -
> -	pointer = pset_prop_find(pset, propname, length);
> -	if (IS_ERR(pointer))
> -		return PTR_ERR(pointer);
> -
> -	memcpy(values, pointer, length);
> -	return 0;
> -}
> -
> -static int pset_prop_read_u16_array(const struct property_set *pset,
> -				    const char *propname,
> -				    u16 *values, size_t nval)
> -{
> -	const void *pointer;
> -	size_t length = nval * sizeof(*values);
> -
> -	pointer = pset_prop_find(pset, propname, length);
> -	if (IS_ERR(pointer))
> -		return PTR_ERR(pointer);
> -
> -	memcpy(values, pointer, length);
> -	return 0;
> -}
> -
> -static int pset_prop_read_u32_array(const struct property_set *pset,
> -				    const char *propname,
> -				    u32 *values, size_t nval)
> -{
> -	const void *pointer;
> -	size_t length = nval * sizeof(*values);
> -
> -	pointer = pset_prop_find(pset, propname, length);
> -	if (IS_ERR(pointer))
> -		return PTR_ERR(pointer);
> -
> -	memcpy(values, pointer, length);
> -	return 0;
> -}
> -
> -static int pset_prop_read_u64_array(const struct property_set *pset,
> -				    const char *propname,
> -				    u64 *values, size_t nval)
> -{
> -	const void *pointer;
> -	size_t length = nval * sizeof(*values);
> -
> -	pointer = pset_prop_find(pset, propname, length);
> -	if (IS_ERR(pointer))
> -		return PTR_ERR(pointer);
> -
> -	memcpy(values, pointer, length);
> -	return 0;
> -}
> -
> -static int pset_prop_count_elems_of_size(const struct property_set *pset,
> -					 const char *propname, size_t length)
> -{
> -	const struct property_entry *prop;
> -
> -	prop = pset_prop_get(pset, propname);
> -	if (!prop)
> -		return -EINVAL;
> -
> -	return prop->length / length;
> -}
> -
> -static int pset_prop_read_string_array(const struct property_set *pset,
> -				       const char *propname,
> -				       const char **strings, size_t nval)
> -{
> -	const struct property_entry *prop;
> -	const void *pointer;
> -	size_t array_len, length;
> -
> -	/* Find out the array length. */
> -	prop = pset_prop_get(pset, propname);
> -	if (!prop)
> -		return -EINVAL;
> -
> -	if (!prop->is_array)
> -		/* The array length for a non-array string property is 1. */
> -		array_len = 1;
> -	else
> -		/* Find the length of an array. */
> -		array_len = pset_prop_count_elems_of_size(pset, propname,
> -							  sizeof(const char *));
> -
> -	/* Return how many there are if strings is NULL. */
> -	if (!strings)
> -		return array_len;
> -
> -	array_len = min(nval, array_len);
> -	length = array_len * sizeof(*strings);
> -
> -	pointer = pset_prop_find(pset, propname, length);
> -	if (IS_ERR(pointer))
> -		return PTR_ERR(pointer);
> -
> -	memcpy(strings, pointer, length);
> -
> -	return array_len;
> -}
> -
>  struct fwnode_handle *dev_fwnode(struct device *dev)
>  {
>  	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> @@ -217,51 +25,6 @@ struct fwnode_handle *dev_fwnode(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dev_fwnode);
>  
> -static bool pset_fwnode_property_present(const struct fwnode_handle *fwnode,
> -					 const char *propname)
> -{
> -	return !!pset_prop_get(to_pset_node(fwnode), propname);
> -}
> -
> -static int pset_fwnode_read_int_array(const struct fwnode_handle *fwnode,
> -				      const char *propname,
> -				      unsigned int elem_size, void *val,
> -				      size_t nval)
> -{
> -	const struct property_set *node = to_pset_node(fwnode);
> -
> -	if (!val)
> -		return pset_prop_count_elems_of_size(node, propname, elem_size);
> -
> -	switch (elem_size) {
> -	case sizeof(u8):
> -		return pset_prop_read_u8_array(node, propname, val, nval);
> -	case sizeof(u16):
> -		return pset_prop_read_u16_array(node, propname, val, nval);
> -	case sizeof(u32):
> -		return pset_prop_read_u32_array(node, propname, val, nval);
> -	case sizeof(u64):
> -		return pset_prop_read_u64_array(node, propname, val, nval);
> -	}
> -
> -	return -ENXIO;
> -}
> -
> -static int
> -pset_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
> -				       const char *propname,
> -				       const char **val, size_t nval)
> -{
> -	return pset_prop_read_string_array(to_pset_node(fwnode), propname,
> -					   val, nval);
> -}
> -
> -static const struct fwnode_operations pset_fwnode_ops = {
> -	.property_present = pset_fwnode_property_present,
> -	.property_read_int_array = pset_fwnode_read_int_array,
> -	.property_read_string_array = pset_fwnode_property_read_string_array,
> -};
> -
>  /**
>   * device_property_present - check if a property of a device is present
>   * @dev: Device whose property is being checked
> @@ -722,114 +485,53 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
>  EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
>  
>  /**
> - * pset_free_set - releases memory allocated for copied property set
> - * @pset: Property set to release
> - *
> - * Function takes previously copied property set and releases all the
> - * memory allocated to it.
> - */
> -static void pset_free_set(struct property_set *pset)
> -{
> -	if (!pset)
> -		return;
> -
> -	property_entries_free(pset->properties);
> -	kfree(pset);
> -}
> -
> -/**
> - * pset_copy_set - copies property set
> - * @pset: Property set to copy
> + * device_add_properties - Add a collection of properties to a device object.
> + * @dev: Device to add properties to.
> + * @properties: Collection of properties to add.
>   *
> - * This function takes a deep copy of the given property set and returns
> - * pointer to the copy. Call device_free_property_set() to free resources
> - * allocated in this function.
> + * Associate a collection of device properties represented by @properties with
> + * @dev. The function takes a copy of @properties.
>   *
> - * Return: Pointer to the new property set or error pointer.
> + * WARNING: The callers should not use this function if it is known that there
> + * is no real firmware node associated with @dev! In that case the callers
> + * should create a software node and assign it to @dev directly.
>   */
> -static struct property_set *pset_copy_set(const struct property_set *pset)
> +int device_add_properties(struct device *dev,
> +			  const struct property_entry *properties)
>  {
> -	struct property_entry *properties;
> -	struct property_set *p;
> +	struct fwnode_handle *fwnode;
>  
> -	p = kzalloc(sizeof(*p), GFP_KERNEL);
> -	if (!p)
> -		return ERR_PTR(-ENOMEM);
> +	fwnode = fwnode_create_software_node(properties, NULL);
> +	if (IS_ERR(fwnode))
> +		return PTR_ERR(fwnode);
>  
> -	properties = property_entries_dup(pset->properties);
> -	if (IS_ERR(properties)) {
> -		kfree(p);
> -		return ERR_CAST(properties);
> -	}
> -
> -	p->properties = properties;
> -	return p;
> +	set_secondary_fwnode(dev, fwnode);
> +	return 0;
>  }
> +EXPORT_SYMBOL_GPL(device_add_properties);
>  
>  /**
>   * device_remove_properties - Remove properties from a device object.
>   * @dev: Device whose properties to remove.
>   *
>   * The function removes properties previously associated to the device
> - * secondary firmware node with device_add_properties(). Memory allocated
> - * to the properties will also be released.
> + * firmware node with device_add_properties(). Memory allocated to the
> + * properties will also be released.
>   */
>  void device_remove_properties(struct device *dev)
>  {
> -	struct fwnode_handle *fwnode;
> -	struct property_set *pset;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  
> -	fwnode = dev_fwnode(dev);
>  	if (!fwnode)
>  		return;
> -	/*
> -	 * Pick either primary or secondary node depending which one holds
> -	 * the pset. If there is no real firmware node (ACPI/DT) primary
> -	 * will hold the pset.
> -	 */
> -	pset = to_pset_node(fwnode);
> -	if (pset) {
> -		set_primary_fwnode(dev, NULL);
> -	} else {
> -		pset = to_pset_node(fwnode->secondary);
> -		if (pset && dev == pset->dev)
> -			set_secondary_fwnode(dev, NULL);
> +
> +	if (is_software_node(fwnode->secondary)) {
> +		fwnode_remove_software_node(fwnode->secondary);
> +		set_secondary_fwnode(dev, NULL);
>  	}
> -	if (pset && dev == pset->dev)
> -		pset_free_set(pset);
>  }
>  EXPORT_SYMBOL_GPL(device_remove_properties);
>  
> -/**
> - * device_add_properties - Add a collection of properties to a device object.
> - * @dev: Device to add properties to.
> - * @properties: Collection of properties to add.
> - *
> - * Associate a collection of device properties represented by @properties with
> - * @dev as its secondary firmware node. The function takes a copy of
> - * @properties.
> - */
> -int device_add_properties(struct device *dev,
> -			  const struct property_entry *properties)
> -{
> -	struct property_set *p, pset;
> -
> -	if (!properties)
> -		return -EINVAL;
> -
> -	pset.properties = properties;
> -
> -	p = pset_copy_set(&pset);
> -	if (IS_ERR(p))
> -		return PTR_ERR(p);
> -
> -	p->fwnode.ops = &pset_fwnode_ops;
> -	set_secondary_fwnode(dev, &p->fwnode);
> -	p->dev = dev;
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(device_add_properties);
> -
>  /**
>   * fwnode_get_next_parent - Iterate to the node's parent
>   * @fwnode: Firmware whose parent is retrieved
> -- 
> 2.19.1
> 

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists