Message-ID: <20181109151341.GX10650@smile.fi.intel.com>
Date:   Fri, 9 Nov 2018 17:13: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 v2 6/6] device property: Remove struct property_set

On Fri, Nov 09, 2018 at 05:21:38PM +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.
> 

Much better, thanks!
One minor comment below.

> Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> ---
>  drivers/base/property.c | 332 ++--------------------------------------
>  1 file changed, 17 insertions(+), 315 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index e20642759c67..240cd0f40605 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
> @@ -721,82 +484,25 @@ 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
> - *
> - * 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.
> - *
> - * Return: Pointer to the new property set or error pointer.
> - */
> -static struct property_set *pset_copy_set(const struct property_set *pset)
> -{
> -	struct property_entry *properties;
> -	struct property_set *p;
> -
> -	p = kzalloc(sizeof(*p), GFP_KERNEL);
> -	if (!p)
> -		return ERR_PTR(-ENOMEM);
> -
> -	properties = property_entries_dup(pset->properties);
> -	if (IS_ERR(properties)) {
> -		kfree(p);
> -		return ERR_CAST(properties);
> -	}
> -
> -	p->properties = properties;
> -	return p;
> -}
> -
>  /**
>   * 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);

I think you may leave these lines (except pset one) as in the original code.

>  	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);
>  
> @@ -806,26 +512,22 @@ EXPORT_SYMBOL_GPL(device_remove_properties);
>   * @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.
> + * @dev. The function takes a copy of @properties.
> + *
> + * 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.
>   */
>  int device_add_properties(struct device *dev,
>  			  const struct property_entry *properties)
>  {
> -	struct property_set *p, pset;
> -
> -	if (!properties)
> -		return -EINVAL;
> -
> -	pset.properties = properties;
> +	struct fwnode_handle *fwnode;
>  
> -	p = pset_copy_set(&pset);
> -	if (IS_ERR(p))
> -		return PTR_ERR(p);
> +	fwnode = fwnode_create_software_node(properties, NULL);
> +	if (IS_ERR(fwnode))
> +		return PTR_ERR(fwnode);
>  
> -	p->fwnode.ops = &pset_fwnode_ops;
> -	set_secondary_fwnode(dev, &p->fwnode);
> -	p->dev = dev;
> +	set_secondary_fwnode(dev, fwnode);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(device_add_properties);
> -- 
> 2.19.1
> 

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists