Re: [resend PATCH] driver core: property: support for generic property

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Monday, January 26, 2015 03:17:40 PM Heikki Krogerus wrote:
> This extends the unified device property interface by adding
> "Generic Property" to it for cases where device tree or ACPI
> are not being used.
> 
> That makes the unified device property interface cover also
> most of the cases where information is extracted from custom
> platform_data in the drivers. So if before we had to check
> separately is there custom platform_data for a driver:
> 
> 	if (pdata)
> 		bar = pdata->bar;
> 	else
> 		device_property_read_u32(dev, "foo", &bar);
> 
> we can now drop that check and simply always use the unified
> device property interface.
> 
> That makes it possible to drop a lot of boiler plate from
> the drivers, plus quite a few header files under
> include/linux/ describing those driver specific platform
> data structures can be removed.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/base/property.c  | 135 ++++++++++++++++++++++++++++++++++++++++++-----
>  include/linux/device.h   |   3 ++
>  include/linux/property.h |  17 ++++++
>  3 files changed, 143 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c458458..4ea6d27 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -15,6 +15,108 @@
>  #include <linux/acpi.h>
>  #include <linux/of.h>
>  
> +static struct dev_gen_prop *dev_prop_get(struct device *dev, const char *name)
> +{
> +	struct dev_gen_prop *prop;
> +
> +	if (!dev->gen_prop)
> +		return NULL;
> +
> +	for (prop = dev->gen_prop; prop->name; prop++)
> +		if (!strcmp(name, prop->name))
> +			return prop;
> +	return NULL;
> +}
> +
> +static int dev_prop_copy_array_u8(u8 *src, u8 *val, size_t nval)
> +{
> +	int i;
> +
> +	for (i = 0; i < nval; i++)
> +		val[i] = src[i];

Use memcpy() perhaps?  And below too?  And then you may not need these
helpers any more.

> +	return 0;
> +}
> +
> +static int dev_prop_copy_array_u16(u16 *src, u16 *val, size_t nval)
> +{
> +	int i;
> +
> +	for (i = 0; i < nval; i++)
> +		val[i] = src[i];
> +	return 0;
> +}
> +
> +static int dev_prop_copy_array_u32(u32 *src, u32 *val, size_t nval)
> +{
> +	int i;
> +
> +	for (i = 0; i < nval; i++)
> +		val[i] = src[i];
> +	return 0;
> +}
> +
> +static int dev_prop_copy_array_u64(u64 *src, u64 *val, size_t nval)
> +{
> +	int i;
> +
> +	for (i = 0; i < nval; i++)
> +		val[i] = src[i];
> +	return 0;
> +}
> +
> +static int dev_prop_copy_array_string(const char **src, const char **val,
> +				      size_t nval)
> +{
> +	int i;
> +
> +	for (i = 0; i < nval; i++)
> +		val[i] = src[i];
> +	return 0;
> +}
> +
> +static int dev_prop_read_array(struct device *dev, const char *name,
> +			       enum dev_prop_type type, void *val, size_t nval)
> +{
> +	struct dev_gen_prop *prop;
> +	int ret = 0;
> +
> +	prop = dev_prop_get(dev, name);
> +	if (!prop)
> +		return -ENODATA;
> +
> +	if (prop->type != type)
> +		return -EPROTO;
> +
> +	if (prop->nval < nval)
> +		return -EOVERFLOW;
> +
> +	switch (prop->type) {
> +	case DEV_PROP_U8:
> +		ret = dev_prop_copy_array_u8((u8 *)prop->num, (u8 *)val,
> +					     nval);
> +		break;
> +	case DEV_PROP_U16:
> +		ret = dev_prop_copy_array_u16((u16 *)prop->num, (u16 *)val,
> +					      nval);
> +		break;
> +	case DEV_PROP_U32:
> +		ret = dev_prop_copy_array_u32((u32 *)prop->num, (u32 *)val,
> +					      nval);
> +		break;
> +	case DEV_PROP_U64:
> +		ret = dev_prop_copy_array_u64(prop->num, (u64 *)val, nval);
> +		break;
> +	case DEV_PROP_STRING:
> +		ret = dev_prop_copy_array_string(prop->str,
> +						 (const char **)val, nval);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +	return ret;
> +}
> +
>  /**
>   * device_property_present - check if a property of a device is present
>   * @dev: Device whose property is being checked
> @@ -26,8 +128,9 @@ bool device_property_present(struct device *dev, const char *propname)
>  {
>  	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>  		return of_property_read_bool(dev->of_node, propname);
> -
> -	return !acpi_dev_prop_get(ACPI_COMPANION(dev), propname, NULL);
> +	if (ACPI_HANDLE(dev))
> +		return !acpi_dev_prop_get(ACPI_COMPANION(dev), propname, NULL);
> +	return !!dev_prop_get(dev, propname);
>  }
>  EXPORT_SYMBOL_GPL(device_property_present);
>  
> @@ -55,8 +158,11 @@ EXPORT_SYMBOL_GPL(fwnode_property_present);
>  	IS_ENABLED(CONFIG_OF) && _dev_->of_node ? \
>  		(OF_DEV_PROP_READ_ARRAY(_dev_->of_node, _propname_, _type_, \
>  					_val_, _nval_)) : \
> -		acpi_dev_prop_read(ACPI_COMPANION(_dev_), _propname_, \
> -				   _proptype_, _val_, _nval_)
> +		ACPI_HANDLE(_dev_) ? \
> +			acpi_dev_prop_read(ACPI_COMPANION(_dev_), _propname_, \
> +					   _proptype_, _val_, _nval_) : \
> +			dev_prop_read_array(_dev_, _propname_, _proptype_, \
> +					   _val_, _nval_)
>  
>  /**
>   * device_property_read_u8_array - return a u8 array property of a device
> @@ -169,10 +275,13 @@ EXPORT_SYMBOL_GPL(device_property_read_u64_array);
>  int device_property_read_string_array(struct device *dev, const char *propname,
>  				      const char **val, size_t nval)
>  {
> -	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> -		of_property_read_string_array(dev->of_node, propname, val, nval) :
> -		acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
> -				   DEV_PROP_STRING, val, nval);
> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> +		return of_property_read_string_array(dev->of_node, propname,
> +						     val, nval);
> +	if (ACPI_HANDLE(dev))
> +		return acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
> +					  DEV_PROP_STRING, val, nval);
> +	return dev_prop_read_array(dev, propname, DEV_PROP_STRING, val, nval);
>  }
>  EXPORT_SYMBOL_GPL(device_property_read_string_array);
>  
> @@ -193,10 +302,12 @@ EXPORT_SYMBOL_GPL(device_property_read_string_array);
>  int device_property_read_string(struct device *dev, const char *propname,
>  				const char **val)
>  {
> -	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> -		of_property_read_string(dev->of_node, propname, val) :
> -		acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
> -				   DEV_PROP_STRING, val, 1);
> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> +		return of_property_read_string(dev->of_node, propname, val);
> +	if (ACPI_HANDLE(dev))
> +		return acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
> +					  DEV_PROP_STRING, val, 1);
> +	return dev_prop_read_array(dev, propname, DEV_PROP_STRING, val, 1);
>  }
>  EXPORT_SYMBOL_GPL(device_property_read_string);
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index fb50673..738dc25 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -27,6 +27,7 @@
>  #include <linux/ratelimit.h>
>  #include <linux/uidgid.h>
>  #include <linux/gfp.h>
> +#include <linux/property.h>
>  #include <asm/device.h>
>  
>  struct device;
> @@ -704,6 +705,7 @@ struct acpi_dev_node {
>   * @archdata:	For arch-specific additions.
>   * @of_node:	Associated device tree node.
>   * @acpi_node:	Associated ACPI device node.
> + * @gen_prop:	Generic device property
>   * @devt:	For creating the sysfs "dev".
>   * @id:		device instance
>   * @devres_lock: Spinlock to protect the resource of the device.
> @@ -780,6 +782,7 @@ struct device {
>  
>  	struct device_node	*of_node; /* associated device tree node */
>  	struct acpi_dev_node	acpi_node; /* associated ACPI device node */
> +	struct dev_gen_prop	*gen_prop; /* generic device property */

That doesn't seem to go in the right direction to be honest.

Actually, having introduced struct fwnode_handle, we should perhaps try to
replace both of_node and acpi_node with a single struct fwnode_handle pointer
and then add a new fwnode_type for the "pdata" stuff.

If you don't want to deal with of_node, which I can understand easily, it
may be worth trying with acpi_node alone at this point and once you have
the fwnode_handle pointer, you might use it for both ACPI and "pdata"?

Grant, Arnd, I wonder what you think?

>  
>  	dev_t			devt;	/* dev_t, creates the sysfs "dev" */
>  	u32			id;	/* device instance */
> diff --git a/include/linux/property.h b/include/linux/property.h
> index a6a3d98..44e875f 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -26,6 +26,23 @@ enum dev_prop_type {
>  	DEV_PROP_MAX,
>  };
>  
> +/**
> + * struct dev_gen_prop - Generic Device Property
> + * @name: property name
> + * @nval: number of elements in the array
> + * @str: value array of strings
> + * @num: value array of numbers
> + *
> + * Used when of_node and acpi_node are missing.
> + */
> +struct dev_gen_prop {
> +	enum dev_prop_type type;
> +	const char *name;
> +	size_t nval;
> +	const char **str;
> +	u64 *num;
> +};
> +
>  bool device_property_present(struct device *dev, const char *propname);
>  int device_property_read_u8_array(struct device *dev, const char *propname,
>  				  u8 *val, size_t nval);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.