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

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

[Heikki Krogerus]

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];
+	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 */
 
 	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);
-- 
2.1.4

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

[Heikki Krogerus]

On Mon, Jan 26, 2015 at 03:17:40PM +0200, 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.

Gentle ping on this.


-- 
heikki

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

[Rafael J. Wysocki]

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.

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

[Grant Likely]

On Thu, Mar 12, 2015 at 1:41 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 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>
[...]
>> @@ -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.

I agree on this, but it will be a lot of work to convert...

> 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?

That makes sense, and we can populate the fwnode_handle even when
using DT. That will allow a transition period to move everyone over to
the fwnode_handle.

g.

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

[Arnd Bergmann]

On Friday 13 March 2015 15:10:38 Grant Likely wrote:
> On Thu, Mar 12, 2015 at 1:41 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, January 26, 2015 03:17:40 PM Heikki Krogerus wrote:
> > 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.
> 
> I agree on this, but it will be a lot of work to convert...
> 
> > 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?
> 
> That makes sense, and we can populate the fwnode_handle even when
> using DT. That will allow a transition period to move everyone over to
> the fwnode_handle.

Agreed on both as well.

	Arnd

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

[Rafael J. Wysocki]

On Friday, March 13, 2015 04:24:11 PM Arnd Bergmann wrote:
> On Friday 13 March 2015 15:10:38 Grant Likely wrote:
> > On Thu, Mar 12, 2015 at 1:41 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > On Monday, January 26, 2015 03:17:40 PM Heikki Krogerus wrote:
> > > 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.
> > 
> > I agree on this, but it will be a lot of work to convert...
> > 
> > > 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?
> > 
> > That makes sense, and we can populate the fwnode_handle even when
> > using DT. That will allow a transition period to move everyone over to
> > the fwnode_handle.
> 
> Agreed on both as well.

OK

So below is what it takes to use struct fwnode_handle to represent ACPI
companions.

Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: driver core / ACPI: Represent ACPI companions using fwnode_handle

Now that we have struct fwnode_handle, we can use that to point to
ACPI companions from struct device objects instead of pointing to
struct acpi_device directly.

There are two benefits from that.  First, the somewhat ugly and
hackish struct acpi_dev_node can be dropped and, second, the same
struct fwnode_handle pointer can be used in the future to point
to other (non-ACPI) firmware device node types.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_platform.c    |    2 +-
 drivers/acpi/dock.c             |    2 +-
 drivers/base/platform.c         |    2 +-
 drivers/i2c/i2c-core.c          |    4 ++--
 include/acpi/acpi_bus.h         |    3 ++-
 include/linux/acpi.h            |    7 ++++---
 include/linux/device.h          |   13 +++----------
 include/linux/fwnode.h          |   25 +++++++++++++++++++++++++
 include/linux/i2c.h             |    4 ++--
 include/linux/platform_device.h |    2 +-
 include/linux/property.h        |   11 +----------
 11 files changed, 43 insertions(+), 32 deletions(-)

Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -38,6 +38,7 @@ struct class;
 struct subsys_private;
 struct bus_type;
 struct device_node;
+struct fwnode_handle;
 struct iommu_ops;
 struct iommu_group;
 
@@ -650,14 +651,6 @@ struct device_dma_parameters {
 	unsigned long segment_boundary_mask;
 };
 
-struct acpi_device;
-
-struct acpi_dev_node {
-#ifdef CONFIG_ACPI
-	struct acpi_device *companion;
-#endif
-};
-
 /**
  * struct device - The basic device structure
  * @parent:	The device's "parent" device, the device to which it is attached.
@@ -703,7 +696,7 @@ struct acpi_dev_node {
  * @cma_area:	Contiguous memory area for dma allocations
  * @archdata:	For arch-specific additions.
  * @of_node:	Associated device tree node.
- * @acpi_node:	Associated ACPI device node.
+ * @fwnode:	Associated device node supplied by platform firmware.
  * @devt:	For creating the sysfs "dev".
  * @id:		device instance
  * @devres_lock: Spinlock to protect the resource of the device.
@@ -779,7 +772,7 @@ struct device {
 	struct dev_archdata	archdata;
 
 	struct device_node	*of_node; /* associated device tree node */
-	struct acpi_dev_node	acpi_node; /* associated ACPI device node */
+	struct fwnode_handle	*fwnode; /* firmware device node */
 
 	dev_t			devt;	/* dev_t, creates the sysfs "dev" */
 	u32			id;	/* device instance */
Index: linux-pm/include/linux/fwnode.h
===================================================================
--- /dev/null
+++ linux-pm/include/linux/fwnode.h
@@ -0,0 +1,25 @@
+/*
+ * fwnode.h - Firmware device node object handle type definition.
+ *
+ * Copyright (C) 2015, Intel Corporation
+ * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _LINUX_FWNODE_H_
+#define _LINUX_FWNODE_H_
+
+enum fwnode_type {
+	FWNODE_INVALID = 0,
+	FWNODE_OF,
+	FWNODE_ACPI,
+};
+
+struct fwnode_handle {
+	enum fwnode_type type;
+};
+
+#endif
Index: linux-pm/include/linux/property.h
===================================================================
--- linux-pm.orig/include/linux/property.h
+++ linux-pm/include/linux/property.h
@@ -13,6 +13,7 @@
 #ifndef _LINUX_PROPERTY_H_
 #define _LINUX_PROPERTY_H_
 
+#include <linux/fwnode.h>
 #include <linux/types.h>
 
 struct device;
@@ -40,16 +41,6 @@ int device_property_read_string_array(st
 int device_property_read_string(struct device *dev, const char *propname,
 				const char **val);
 
-enum fwnode_type {
-	FWNODE_INVALID = 0,
-	FWNODE_OF,
-	FWNODE_ACPI,
-};
-
-struct fwnode_handle {
-	enum fwnode_type type;
-};
-
 bool fwnode_property_present(struct fwnode_handle *fwnode, const char *propname);
 int fwnode_property_read_u8_array(struct fwnode_handle *fwnode,
 				  const char *propname, u8 *val,
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -386,7 +386,8 @@ static inline bool is_acpi_node(struct f
 
 static inline struct acpi_device *acpi_node(struct fwnode_handle *fwnode)
 {
-	return fwnode ? container_of(fwnode, struct acpi_device, fwnode) : NULL;
+	return is_acpi_node(fwnode) ?
+		container_of(fwnode, struct acpi_device, fwnode) : NULL;
 }
 
 static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device *adev)
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -29,7 +29,7 @@
 #include <linux/ioport.h>	/* for struct resource */
 #include <linux/resource_ext.h>
 #include <linux/device.h>
-#include <linux/property.h>
+#include <linux/fwnode.h>
 
 #ifndef _LINUX
 #define _LINUX
@@ -53,8 +53,9 @@ static inline acpi_handle acpi_device_ha
 	return adev ? adev->handle : NULL;
 }
 
-#define ACPI_COMPANION(dev)		((dev)->acpi_node.companion)
-#define ACPI_COMPANION_SET(dev, adev)	ACPI_COMPANION(dev) = (adev)
+#define ACPI_COMPANION(dev)		acpi_node((dev)->fwnode)
+#define ACPI_COMPANION_SET(dev, adev)	(dev)->fwnode = (adev) ? \
+	acpi_fwnode_handle(adev) : NULL
 #define ACPI_HANDLE(dev)		acpi_device_handle(ACPI_COMPANION(dev))
 
 static inline void acpi_preset_companion(struct device *dev,
Index: linux-pm/include/linux/platform_device.h
===================================================================
--- linux-pm.orig/include/linux/platform_device.h
+++ linux-pm/include/linux/platform_device.h
@@ -59,7 +59,7 @@ extern int platform_add_devices(struct p
 
 struct platform_device_info {
 		struct device *parent;
-		struct acpi_dev_node acpi_node;
+		struct fwnode_handle *fwnode;
 
 		const char *name;
 		int id;
Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -615,7 +615,7 @@ void acpi_dock_add(struct acpi_device *a
 	memset(&pdevinfo, 0, sizeof(pdevinfo));
 	pdevinfo.name = "dock";
 	pdevinfo.id = dock_station_count;
-	pdevinfo.acpi_node.companion = adev;
+	pdevinfo.fwnode = acpi_fwnode_handle(adev);
 	pdevinfo.data = &ds;
 	pdevinfo.size_data = sizeof(ds);
 	dd = platform_device_register_full(&pdevinfo);
Index: linux-pm/drivers/acpi/acpi_platform.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_platform.c
+++ linux-pm/drivers/acpi/acpi_platform.c
@@ -102,7 +102,7 @@ struct platform_device *acpi_create_plat
 	pdevinfo.id = -1;
 	pdevinfo.res = resources;
 	pdevinfo.num_res = count;
-	pdevinfo.acpi_node.companion = adev;
+	pdevinfo.fwnode = acpi_fwnode_handle(adev);
 	pdevinfo.dma_mask = DMA_BIT_MASK(32);
 	pdev = platform_device_register_full(&pdevinfo);
 	if (IS_ERR(pdev))
Index: linux-pm/drivers/base/platform.c
===================================================================
--- linux-pm.orig/drivers/base/platform.c
+++ linux-pm/drivers/base/platform.c
@@ -454,7 +454,7 @@ struct platform_device *platform_device_
 		goto err_alloc;
 
 	pdev->dev.parent = pdevinfo->parent;
-	ACPI_COMPANION_SET(&pdev->dev, pdevinfo->acpi_node.companion);
+	pdev->dev.fwnode = pdevinfo->fwnode;
 
 	if (pdevinfo->dma_mask) {
 		/*
Index: linux-pm/drivers/i2c/i2c-core.c
===================================================================
--- linux-pm.orig/drivers/i2c/i2c-core.c
+++ linux-pm/drivers/i2c/i2c-core.c
@@ -133,7 +133,7 @@ static acpi_status acpi_i2c_add_device(a
 		return AE_OK;
 
 	memset(&info, 0, sizeof(info));
-	info.acpi_node.companion = adev;
+	info.fwnode = acpi_fwnode_handle(adev);
 	info.irq = -1;
 
 	INIT_LIST_HEAD(&resource_list);
@@ -974,7 +974,7 @@ i2c_new_device(struct i2c_adapter *adap,
 	client->dev.bus = &i2c_bus_type;
 	client->dev.type = &i2c_client_type;
 	client->dev.of_node = info->of_node;
-	ACPI_COMPANION_SET(&client->dev, info->acpi_node.companion);
+	client->dev.fwnode = info->fwnode;
 
 	i2c_dev_set_name(adap, client);
 	status = device_register(&client->dev);
Index: linux-pm/include/linux/i2c.h
===================================================================
--- linux-pm.orig/include/linux/i2c.h
+++ linux-pm/include/linux/i2c.h
@@ -278,7 +278,7 @@ static inline int i2c_slave_event(struct
  * @platform_data: stored in i2c_client.dev.platform_data
  * @archdata: copied into i2c_client.dev.archdata
  * @of_node: pointer to OpenFirmware device node
- * @acpi_node: ACPI device node
+ * @fwnode: device node supplied by the platform firmware
  * @irq: stored in i2c_client.irq
  *
  * I2C doesn't actually support hardware probing, although controllers and
@@ -299,7 +299,7 @@ struct i2c_board_info {
 	void		*platform_data;
 	struct dev_archdata	*archdata;
 	struct device_node *of_node;
-	struct acpi_dev_node acpi_node;
+	struct fwnode_handle *fwnode;
 	int		irq;
 };
 

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

[Greg Kroah-Hartman]

On Sat, Mar 14, 2015 at 02:09:06AM +0100, Rafael J. Wysocki wrote:
> On Friday, March 13, 2015 04:24:11 PM Arnd Bergmann wrote:
> > On Friday 13 March 2015 15:10:38 Grant Likely wrote:
> > > On Thu, Mar 12, 2015 at 1:41 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > On Monday, January 26, 2015 03:17:40 PM Heikki Krogerus wrote:
> > > > 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.
> > > 
> > > I agree on this, but it will be a lot of work to convert...
> > > 
> > > > 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?
> > > 
> > > That makes sense, and we can populate the fwnode_handle even when
> > > using DT. That will allow a transition period to move everyone over to
> > > the fwnode_handle.
> > 
> > Agreed on both as well.
> 
> OK
> 
> So below is what it takes to use struct fwnode_handle to represent ACPI
> companions.
> 
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: driver core / ACPI: Represent ACPI companions using fwnode_handle
> 
> Now that we have struct fwnode_handle, we can use that to point to
> ACPI companions from struct device objects instead of pointing to
> struct acpi_device directly.
> 
> There are two benefits from that.  First, the somewhat ugly and
> hackish struct acpi_dev_node can be dropped and, second, the same
> struct fwnode_handle pointer can be used in the future to point
> to other (non-ACPI) firmware device node types.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/acpi_platform.c    |    2 +-
>  drivers/acpi/dock.c             |    2 +-
>  drivers/base/platform.c         |    2 +-
>  drivers/i2c/i2c-core.c          |    4 ++--
>  include/acpi/acpi_bus.h         |    3 ++-
>  include/linux/acpi.h            |    7 ++++---
>  include/linux/device.h          |   13 +++----------
>  include/linux/fwnode.h          |   25 +++++++++++++++++++++++++
>  include/linux/i2c.h             |    4 ++--
>  include/linux/platform_device.h |    2 +-
>  include/linux/property.h        |   11 +----------
>  11 files changed, 43 insertions(+), 32 deletions(-)

Nice, I like the driver core changes:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Grant Likely]

On Sat, Mar 14, 2015 at 9:42 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Mar 14, 2015 at 02:09:06AM +0100, Rafael J. Wysocki wrote:
>> On Friday, March 13, 2015 04:24:11 PM Arnd Bergmann wrote:
>> > On Friday 13 March 2015 15:10:38 Grant Likely wrote:
>> > > On Thu, Mar 12, 2015 at 1:41 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > > > On Monday, January 26, 2015 03:17:40 PM Heikki Krogerus wrote:
>> > > > 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.
>> > >
>> > > I agree on this, but it will be a lot of work to convert...
>> > >
>> > > > 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?
>> > >
>> > > That makes sense, and we can populate the fwnode_handle even when
>> > > using DT. That will allow a transition period to move everyone over to
>> > > the fwnode_handle.
>> >
>> > Agreed on both as well.
>>
>> OK
>>
>> So below is what it takes to use struct fwnode_handle to represent ACPI
>> companions.
>>
>> Rafael
>>
>>
>> ---
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Subject: driver core / ACPI: Represent ACPI companions using fwnode_handle
>>
>> Now that we have struct fwnode_handle, we can use that to point to
>> ACPI companions from struct device objects instead of pointing to
>> struct acpi_device directly.
>>
>> There are two benefits from that.  First, the somewhat ugly and
>> hackish struct acpi_dev_node can be dropped and, second, the same
>> struct fwnode_handle pointer can be used in the future to point
>> to other (non-ACPI) firmware device node types.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>  drivers/acpi/acpi_platform.c    |    2 +-
>>  drivers/acpi/dock.c             |    2 +-
>>  drivers/base/platform.c         |    2 +-
>>  drivers/i2c/i2c-core.c          |    4 ++--
>>  include/acpi/acpi_bus.h         |    3 ++-
>>  include/linux/acpi.h            |    7 ++++---
>>  include/linux/device.h          |   13 +++----------
>>  include/linux/fwnode.h          |   25 +++++++++++++++++++++++++
>>  include/linux/i2c.h             |    4 ++--
>>  include/linux/platform_device.h |    2 +-
>>  include/linux/property.h        |   11 +----------
>>  11 files changed, 43 insertions(+), 32 deletions(-)
>
> Nice, I like the driver core changes:
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

I like it too. The only comment I have is I would use a static inline
helper for getting/setting the fwnode_handle pointer. The reason being
that there are situations where we're going to want to attach both a
DT node and static data to the same node. Using a helper isolates the
rest of the kernel from any future changes here.

However, despite that comment:

Acked-by: Grant Likely <grant.likely@linaro.org>

My comment isn't significant enough to hold up this change.

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

[Rafael J. Wysocki]

On Monday, March 16, 2015 09:47:22 AM Grant Likely wrote:
> On Sat, Mar 14, 2015 at 9:42 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Mar 14, 2015 at 02:09:06AM +0100, Rafael J. Wysocki wrote:
> >> On Friday, March 13, 2015 04:24:11 PM Arnd Bergmann wrote:
> >> > On Friday 13 March 2015 15:10:38 Grant Likely wrote:
> >> > > On Thu, Mar 12, 2015 at 1:41 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > > > On Monday, January 26, 2015 03:17:40 PM Heikki Krogerus wrote:
> >> > > > 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.
> >> > >
> >> > > I agree on this, but it will be a lot of work to convert...
> >> > >
> >> > > > 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?
> >> > >
> >> > > That makes sense, and we can populate the fwnode_handle even when
> >> > > using DT. That will allow a transition period to move everyone over to
> >> > > the fwnode_handle.
> >> >
> >> > Agreed on both as well.
> >>
> >> OK
> >>
> >> So below is what it takes to use struct fwnode_handle to represent ACPI
> >> companions.
> >>
> >> Rafael
> >>
> >>
> >> ---
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Subject: driver core / ACPI: Represent ACPI companions using fwnode_handle
> >>
> >> Now that we have struct fwnode_handle, we can use that to point to
> >> ACPI companions from struct device objects instead of pointing to
> >> struct acpi_device directly.
> >>
> >> There are two benefits from that.  First, the somewhat ugly and
> >> hackish struct acpi_dev_node can be dropped and, second, the same
> >> struct fwnode_handle pointer can be used in the future to point
> >> to other (non-ACPI) firmware device node types.
> >>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> ---
> >>  drivers/acpi/acpi_platform.c    |    2 +-
> >>  drivers/acpi/dock.c             |    2 +-
> >>  drivers/base/platform.c         |    2 +-
> >>  drivers/i2c/i2c-core.c          |    4 ++--
> >>  include/acpi/acpi_bus.h         |    3 ++-
> >>  include/linux/acpi.h            |    7 ++++---
> >>  include/linux/device.h          |   13 +++----------
> >>  include/linux/fwnode.h          |   25 +++++++++++++++++++++++++
> >>  include/linux/i2c.h             |    4 ++--
> >>  include/linux/platform_device.h |    2 +-
> >>  include/linux/property.h        |   11 +----------
> >>  11 files changed, 43 insertions(+), 32 deletions(-)
> >
> > Nice, I like the driver core changes:
> >
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> I like it too. The only comment I have is I would use a static inline
> helper for getting/setting the fwnode_handle pointer.

Yes, that's the next step. :-)

> The reason being
> that there are situations where we're going to want to attach both a
> DT node and static data to the same node. Using a helper isolates the
> rest of the kernel from any future changes here.

Right.

I'm going to send a couple more patches on top of this one going in that
direction.

> However, despite that comment:
> 
> Acked-by: Grant Likely <grant.likely@linaro.org>
> 
> My comment isn't significant enough to hold up this change.

OK then, I'm queuing this up for 4.1.  Thanks!


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

[PATCH] ACPI: Introduce has_acpi_companion()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Now that the ACPI companions of devices are represented by pointers
to struct fwnode_handle, it is not quite efficient to check whether
or not an ACPI companion of a device is present by evaluating the
ACPI_COMPANION() macro.

For this reason, introduce a special static inline routine for that,
has_acpi_companion(), and update the code to use it where applicable.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

On top of https://patchwork.kernel.org/patch/6010351/

---
 drivers/acpi/glue.c                         |    4 ++--
 drivers/i2c/busses/i2c-designware-platdrv.c |    4 ++--
 drivers/iommu/intel-iommu.c                 |    2 +-
 include/linux/acpi.h                        |   10 ++++++++++
 4 files changed, 15 insertions(+), 5 deletions(-)

Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -58,6 +58,11 @@ static inline acpi_handle acpi_device_ha
 	acpi_fwnode_handle(adev) : NULL
 #define ACPI_HANDLE(dev)		acpi_device_handle(ACPI_COMPANION(dev))
 
+static inline bool has_acpi_companion(struct device *dev)
+{
+	return is_acpi_node(dev->fwnode);
+}
+
 static inline void acpi_preset_companion(struct device *dev,
 					 struct acpi_device *parent, u64 addr)
 {
@@ -472,6 +477,11 @@ static inline struct fwnode_handle *acpi
 	return NULL;
 }
 
+static inline bool has_acpi_companion(struct device *dev)
+{
+	return false;
+}
+
 static inline const char *acpi_dev_name(struct acpi_device *adev)
 {
 	return NULL;
Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
===================================================================
--- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c
+++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -166,7 +166,7 @@ static int dw_i2c_probe(struct platform_
 	/* fast mode by default because of legacy reasons */
 	clk_freq = 400000;
 
-	if (ACPI_COMPANION(&pdev->dev)) {
+	if (has_acpi_companion(&pdev->dev)) {
 		dw_i2c_acpi_configure(pdev);
 	} else if (pdev->dev.of_node) {
 		of_property_read_u32(pdev->dev.of_node,
@@ -286,7 +286,7 @@ static int dw_i2c_remove(struct platform
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
-	if (ACPI_COMPANION(&pdev->dev))
+	if (has_acpi_companion(&pdev->dev))
 		dw_i2c_acpi_unconfigure(pdev);
 
 	return 0;
Index: linux-pm/drivers/iommu/intel-iommu.c
===================================================================
--- linux-pm.orig/drivers/iommu/intel-iommu.c
+++ linux-pm/drivers/iommu/intel-iommu.c
@@ -684,7 +684,7 @@ static struct intel_iommu *device_to_iom
 	if (dev_is_pci(dev)) {
 		pdev = to_pci_dev(dev);
 		segment = pci_domain_nr(pdev->bus);
-	} else if (ACPI_COMPANION(dev))
+	} else if (has_acpi_companion(dev))
 		dev = &ACPI_COMPANION(dev)->dev;
 
 	rcu_read_lock();
Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -168,7 +168,7 @@ int acpi_bind_one(struct device *dev, st
 	unsigned int node_id;
 	int retval = -EINVAL;
 
-	if (ACPI_COMPANION(dev)) {
+	if (has_acpi_companion(dev)) {
 		if (acpi_dev) {
 			dev_warn(dev, "ACPI companion already set\n");
 			return -EINVAL;
@@ -220,7 +220,7 @@ int acpi_bind_one(struct device *dev, st
 	list_add(&physical_node->node, physnode_list);
 	acpi_dev->physical_node_count++;
 
-	if (!ACPI_COMPANION(dev))
+	if (!has_acpi_companion(dev))
 		ACPI_COMPANION_SET(dev, acpi_dev);
 
 	acpi_physnode_link_name(physical_node_name, node_id);

[PATCH] driver core: Implement device property accessors through fwnode ones

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Now that the ACPI companions of devices are pointed to by the fwnode
field in struct device, the device_property_*() accessor functions
can be modified to use their fwnode_property_*() counterparts
internally with minimum extra overhead in the IS_ENABLED(CONFIG_OF)
case, so make those changes.

This allows us to get rid of the rather ugly DEV_PROP_READ_ARRAY()
macro among other things.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

On top of

http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=ce793486e23e0162a732c605189c8028e0910e86

---
 drivers/base/property.c |   44 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

Index: linux-pm/drivers/base/property.c
===================================================================
--- linux-pm.orig/drivers/base/property.c
+++ linux-pm/drivers/base/property.c
@@ -15,6 +15,12 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 
+static inline struct fwnode_handle *dev_fwnode(struct device *dev)
+{
+	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
+		&dev->of_node->fwnode : dev->fwnode;
+}
+
 /**
  * device_property_present - check if a property of a device is present
  * @dev: Device whose property is being checked
@@ -24,10 +30,7 @@
  */
 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);
+	return fwnode_property_present(dev_fwnode(dev), propname);
 }
 EXPORT_SYMBOL_GPL(device_property_present);
 
@@ -47,17 +50,6 @@ bool fwnode_property_present(struct fwno
 }
 EXPORT_SYMBOL_GPL(fwnode_property_present);
 
-#define OF_DEV_PROP_READ_ARRAY(node, propname, type, val, nval) \
-	(val) ? of_property_read_##type##_array((node), (propname), (val), (nval)) \
-	      : of_property_count_elems_of_size((node), (propname), sizeof(type))
-
-#define DEV_PROP_READ_ARRAY(_dev_, _propname_, _type_, _proptype_, _val_, _nval_) \
-	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_)
-
 /**
  * device_property_read_u8_array - return a u8 array property of a device
  * @dev: Device to get the property of
@@ -77,7 +69,7 @@ EXPORT_SYMBOL_GPL(fwnode_property_presen
 int device_property_read_u8_array(struct device *dev, const char *propname,
 				  u8 *val, size_t nval)
 {
-	return DEV_PROP_READ_ARRAY(dev, propname, u8, DEV_PROP_U8, val, nval);
+	return fwnode_property_read_u8_array(dev_fwnode(dev), propname, val, nval);
 }
 EXPORT_SYMBOL_GPL(device_property_read_u8_array);
 
@@ -100,7 +92,7 @@ EXPORT_SYMBOL_GPL(device_property_read_u
 int device_property_read_u16_array(struct device *dev, const char *propname,
 				   u16 *val, size_t nval)
 {
-	return DEV_PROP_READ_ARRAY(dev, propname, u16, DEV_PROP_U16, val, nval);
+	return fwnode_property_read_u16_array(dev_fwnode(dev), propname, val, nval);
 }
 EXPORT_SYMBOL_GPL(device_property_read_u16_array);
 
@@ -123,7 +115,7 @@ EXPORT_SYMBOL_GPL(device_property_read_u
 int device_property_read_u32_array(struct device *dev, const char *propname,
 				   u32 *val, size_t nval)
 {
-	return DEV_PROP_READ_ARRAY(dev, propname, u32, DEV_PROP_U32, val, nval);
+	return fwnode_property_read_u32_array(dev_fwnode(dev), propname, val, nval);
 }
 EXPORT_SYMBOL_GPL(device_property_read_u32_array);
 
@@ -146,7 +138,7 @@ EXPORT_SYMBOL_GPL(device_property_read_u
 int device_property_read_u64_array(struct device *dev, const char *propname,
 				   u64 *val, size_t nval)
 {
-	return DEV_PROP_READ_ARRAY(dev, propname, u64, DEV_PROP_U64, val, nval);
+	return fwnode_property_read_u64_array(dev_fwnode(dev), propname, val, nval);
 }
 EXPORT_SYMBOL_GPL(device_property_read_u64_array);
 
@@ -169,10 +161,7 @@ EXPORT_SYMBOL_GPL(device_property_read_u
 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);
+	return fwnode_property_read_string_array(dev_fwnode(dev), propname, val, nval);
 }
 EXPORT_SYMBOL_GPL(device_property_read_string_array);
 
@@ -193,13 +182,14 @@ EXPORT_SYMBOL_GPL(device_property_read_s
 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);
+	return fwnode_property_read_string(dev_fwnode(dev), propname, val);
 }
 EXPORT_SYMBOL_GPL(device_property_read_string);
 
+#define OF_DEV_PROP_READ_ARRAY(node, propname, type, val, nval) \
+	(val) ? of_property_read_##type##_array((node), (propname), (val), (nval)) \
+	      : of_property_count_elems_of_size((node), (propname), sizeof(type))
+
 #define FWNODE_PROP_READ_ARRAY(_fwnode_, _propname_, _type_, _proptype_, _val_, _nval_) \
 ({ \
 	int _ret_; \

Re: [PATCH] driver core: Implement device property accessors through fwnode ones

[Greg Kroah-Hartman]

On Mon, Mar 23, 2015 at 12:10:30AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Now that the ACPI companions of devices are pointed to by the fwnode
> field in struct device, the device_property_*() accessor functions
> can be modified to use their fwnode_property_*() counterparts
> internally with minimum extra overhead in the IS_ENABLED(CONFIG_OF)
> case, so make those changes.
> 
> This allows us to get rid of the rather ugly DEV_PROP_READ_ARRAY()
> macro among other things.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> On top of
> 
> http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=ce793486e23e0162a732c605189c8028e0910e86

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 0/3] device property: support for "built-in" properties

[Rafael J. Wysocki]

Hi,

Patches [1-2/3] add support for properties provided in a "pdata way"
roughly along the lines of the Heikki's patch at

https://patchwork.kernel.org/patch/5709461/

but using the fwnode field in struct device introduced by one of my
previous patches (currently in linux-next).

Patch [3/3] does one more interesting thing on top of that, but is mostly
for discussion.  Namely, it changes the behavior of the unified device
properties API to fall back to the "built-in" properties when it cannot
find the requested data in the stuff provided by the platform firmware.

Currently, that patch would introduce an artificial difference between ACPI
and DT, because DT uses the of_node pointer in struct device won't use the
set_primary_fwnode() thing introduced buy the [1/3].  However, I have an
idea about how that can be worked around, but more on that in the changelog
of patch [3/3].

Kind regards,
Rafael

[PATCH 1/3] device property: Make it possible to use secondary firmware nodes

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Add a secondary pointer to struct fwnode_handle so as to make it
possible for a device to have two firmware nodes associated with
it at the same time, for example, an ACPI node and a node with
a set of properties provided by platform initialization code.

In the future that will allow device property lookup to fall back
from the primary firmware node to the secondary one if the given
property is not present there to make it easier to provide defaults
for device properties used by device drivers.

Introduce two helper routines, set_primary_fwnode() and
set_secondary_fwnode() allowing callers to add a primary/secondary
firmware node to the given device in such a way that

 (1) If there's only one firmware node for that device, it will be
     pointed to by the device's firmware node pointer.
 (2) If both the primary and secondary firmware nodes are present,
     the primary one will be pointed to by the device's firmware
     node pointer, while the secondary one will be pointed to by the
     primary node's secondary pointer.
 (3) If one of these nodes is removed (by calling one of the new
     nelpers with NULL as the second argument), the other one will
     be preserved.

Make ACPI use set_primary_fwnode() for attaching its firmware nodes
to devices.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/core.c    |   51 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h   |    4 +--
 include/linux/device.h |    3 ++
 include/linux/fwnode.h |    1 
 4 files changed, 57 insertions(+), 2 deletions(-)

Index: linux-pm/include/linux/fwnode.h
===================================================================
--- linux-pm.orig/include/linux/fwnode.h
+++ linux-pm/include/linux/fwnode.h
@@ -20,6 +20,7 @@ enum fwnode_type {
 
 struct fwnode_handle {
 	enum fwnode_type type;
+	struct fwnode_handle *secondary;
 };
 
 #endif
Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -940,6 +940,9 @@ extern void unlock_device_hotplug(void);
 extern int lock_device_hotplug_sysfs(void);
 extern int device_offline(struct device *dev);
 extern int device_online(struct device *dev);
+extern void set_primary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
+extern void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
+
 /*
  * Root device objects for grouping under /sys/devices
  */
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -54,8 +54,8 @@ static inline acpi_handle acpi_device_ha
 }
 
 #define ACPI_COMPANION(dev)		acpi_node((dev)->fwnode)
-#define ACPI_COMPANION_SET(dev, adev)	(dev)->fwnode = (adev) ? \
-	acpi_fwnode_handle(adev) : NULL
+#define ACPI_COMPANION_SET(dev, adev)	set_primary_fwnode(dev, (adev) ? \
+	acpi_fwnode_handle(adev) : NULL)
 #define ACPI_HANDLE(dev)		acpi_device_handle(ACPI_COMPANION(dev))
 
 static inline bool has_acpi_companion(struct device *dev)
Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -12,6 +12,7 @@
 
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/fwnode.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -2132,4 +2133,54 @@ define_dev_printk_level(dev_warn, KERN_W
 define_dev_printk_level(dev_notice, KERN_NOTICE);
 define_dev_printk_level(_dev_info, KERN_INFO);
 
+static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
+{
+	return fwnode && !IS_ERR(fwnode->secondary);
+}
+
+/**
+ * set_primary_fwnode - Change the primary firmware node of a given device.
+ * @dev: Device to handle.
+ * @fwnode: New primary firmware node of the device.
+ *
+ * Set the device's firmware node pointer to @fwnode, but if a secondary
+ * firmware node of the device is present, preserve it.
+ */
+void set_primary_fwnode(struct device *dev, struct fwnode_handle *fwnode)
+{
+	if (fwnode) {
+		struct fwnode_handle *fn = dev->fwnode;
+
+		if (fwnode_is_primary(fn))
+			fn = fn->secondary;
+
+		fwnode->secondary = fn;
+		dev->fwnode = fwnode;
+	} else {
+		dev->fwnode = fwnode_is_primary(dev->fwnode) ?
+			dev->fwnode->secondary : NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(set_primary_fwnode);
+
+/**
+ * set_secondary_fwnode - Change the secondary firmware node of a given device.
+ * @dev: Device to handle.
+ * @fwnode: New secondary firmware node of the device.
+ *
+ * If a primary firmware node of the device is present, set its secondary
+ * pointer to @fwnode.  Otherwise, set the device's firmware node pointer to
+ * @fwnode.
+ */
+void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode)
+{
+	if (fwnode)
+		fwnode->secondary = ERR_PTR(-ENODEV);
+
+	if (fwnode_is_primary(dev->fwnode))
+		dev->fwnode->secondary = fwnode;
+	else
+		dev->fwnode = fwnode;
+}
+
 #endif

[PATCH 2/3] device property: Introduce firmware node type for platform data

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce data structures and code allowing "built-in" properties
to be associated with devices in such a way that they will be used
by the device_property_* API if no proper firmware node (neither DT
nor ACPI) is present for the given device.

Each property is to be represented by a property_entry structure.
An array of property_entry structures (terminated with a null
entry) can be pointed to by the properties field of struct
property_set that can be added as a firmware node to a struct
device using device_add_property_set().  That will cause the
device_property_* API to use that property_set as the source
of properties if the given device does not have a DT node or
an ACPI companion device object associated with it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/property.c  |   98 ++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/fwnode.h   |    1 
 include/linux/property.h |   33 +++++++++++++++
 3 files changed, 127 insertions(+), 5 deletions(-)

Index: linux-pm/include/linux/fwnode.h
===================================================================
--- linux-pm.orig/include/linux/fwnode.h
+++ linux-pm/include/linux/fwnode.h
@@ -16,6 +16,7 @@ enum fwnode_type {
 	FWNODE_INVALID = 0,
 	FWNODE_OF,
 	FWNODE_ACPI,
+	FWNODE_PDATA,
 };
 
 struct fwnode_handle {
Index: linux-pm/include/linux/property.h
===================================================================
--- linux-pm.orig/include/linux/property.h
+++ linux-pm/include/linux/property.h
@@ -131,4 +131,37 @@ static inline int fwnode_property_read_u
 	return fwnode_property_read_u64_array(fwnode, propname, val, 1);
 }
 
+/**
+ * struct property_entry - "Built-in" device property representation.
+ * @name: Name of the property.
+ * @type: Type of the property.
+ * @nval: Number of items of type @type making up the value.
+ * @value: Value of the property (an array of @nval items of type @type).
+ */
+struct property_entry {
+	const char *name;
+	enum dev_prop_type type;
+	size_t nval;
+	union {
+		void *raw_data;
+		u8 *u8_data;
+		u16 *u16_data;
+		u32 *u32_data;
+		u64 *u64_data;
+		const char **str;
+	} value;
+};
+
+/**
+ * struct property_set - Collection of "built-in" device properties.
+ * @fwnode: Handle to be pointed to by the fwnode field of struct device.
+ * @properties: Array of properties terminated with a null entry.
+ */
+struct property_set {
+	struct fwnode_handle fwnode;
+	struct property_entry *properties;
+};
+
+void device_add_property_set(struct device *dev, struct property_set *pset);
+
 #endif /* _LINUX_PROPERTY_H_ */
Index: linux-pm/drivers/base/property.c
===================================================================
--- linux-pm.orig/drivers/base/property.c
+++ linux-pm/drivers/base/property.c
@@ -10,10 +10,96 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/property.h>
-#include <linux/export.h>
 #include <linux/acpi.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/property.h>
+
+/**
+ * device_add_property_set - Add a collection of properties to a device object.
+ * @dev: Device to add properties to.
+ * @pset: Collection of properties to add.
+ *
+ * Associate a collection of device properties represented by @pset with @dev
+ * as its secondary firmware node.
+ */
+void device_add_property_set(struct device *dev, struct property_set *pset)
+{
+	if (pset)
+		pset->fwnode.type = FWNODE_PDATA;
+
+	set_secondary_fwnode(dev, &pset->fwnode);
+}
+EXPORT_SYMBOL_GPL(device_add_property_set);
+
+static inline bool is_pset(struct fwnode_handle *fwnode)
+{
+	return fwnode && fwnode->type == FWNODE_PDATA;
+}
+
+static inline struct property_set *to_pset(struct fwnode_handle *fwnode)
+{
+	return is_pset(fwnode) ?
+		container_of(fwnode, struct property_set, fwnode) : NULL;
+}
+
+static struct property_entry *pset_prop_get(struct property_set *pset,
+					    const char *name)
+{
+	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 int pset_prop_read_array(struct property_set *pset, const char *name,
+				enum dev_prop_type type, void *val, size_t nval)
+{
+	struct property_entry *prop;
+	unsigned int item_size;
+
+	prop = pset_prop_get(pset, name);
+	if (!prop)
+		return -ENODATA;
+
+	if (prop->type != type)
+		return -EPROTO;
+
+	if (!val)
+		return prop->nval;
+
+	if (prop->nval < nval)
+		return -EOVERFLOW;
+
+	switch (type) {
+	case DEV_PROP_U8:
+		item_size = sizeof(u8);
+		break;
+	case DEV_PROP_U16:
+		item_size = sizeof(u16);
+		break;
+	case DEV_PROP_U32:
+		item_size = sizeof(u32);
+		break;
+	case DEV_PROP_U64:
+		item_size = sizeof(u64);
+		break;
+	case DEV_PROP_STRING:
+		item_size = sizeof(const char *);
+		break;
+	default:
+		return -EINVAL;
+	}
+	memcpy(val, prop->value.raw_data, nval * item_size);
+	return 0;
+}
 
 static inline struct fwnode_handle *dev_fwnode(struct device *dev)
 {
@@ -46,7 +132,7 @@ bool fwnode_property_present(struct fwno
 	else if (is_acpi_node(fwnode))
 		return !acpi_dev_prop_get(acpi_node(fwnode), propname, NULL);
 
-	return false;
+	return !!pset_prop_get(to_pset(fwnode), propname);
 }
 EXPORT_SYMBOL_GPL(fwnode_property_present);
 
@@ -205,7 +291,8 @@ EXPORT_SYMBOL_GPL(device_property_read_s
 		_ret_ = acpi_dev_prop_read(acpi_node(_fwnode_), _propname_, \
 					   _proptype_, _val_, _nval_); \
 	else \
-		_ret_ = -ENXIO; \
+		_ret_ = pset_prop_read_array(to_pset(_fwnode_), _propname_, \
+					     _proptype_, _val_, _nval_); \
 	_ret_; \
 })
 
@@ -344,7 +431,8 @@ int fwnode_property_read_string_array(st
 		return acpi_dev_prop_read(acpi_node(fwnode), propname,
 					  DEV_PROP_STRING, val, nval);
 
-	return -ENXIO;
+	return pset_prop_read_array(to_pset(fwnode), propname,
+				    DEV_PROP_STRING, val, nval);
 }
 EXPORT_SYMBOL_GPL(fwnode_property_read_string_array);
 

[PATCH 3/3][RFD] device property: Implement fallback to built-in properties

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify the fwnode_property_* accessors so as to make them fall back
to using "built-in" device properties from a property_set structure
associated with the given device (as the secondary firmware node)
if the property in question cannot be retrieved from the primary
firmware node (DT or ACPI).  That will make the device_properties_*
functions do the same thing as they use fwnode_property_* internally.

That should make it easier to provide default values for device
properties used by device drivers to be used in case the properties
in question are not provided by the platform firmare at all or
they cannot be retireved from the platform firmware due to errors.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

First of all, this should show why I thought it would be better to use
the primary/secondary scheme for firmware nodes instead of just having a
list of them with the head pointed to by the struct device's fwnode field.
Namely, within the primary/secondary scheme the ordering of property lookup
is always well defined and there's no overhead when, for example, somebody
specifically wants to access the ACPI companion firmware node etc.

Second, as I said in the intro message, this is not for immediate application
(apart from any review comments on [1-2/3]), because it would introduce an
artificial difference between DT and ACPI that I'd prefer to avoid.

Namely, dev_fwnode() (introduced by one of the patches in linux-next) returns
the address of the dev->of_node's fwnode field if dev->of_node is present and
that would not have the secondary pointer set.  On the other hand, since ACPI
is now using set_primary_fwnode() to add its firmware node companions to
devices, the secondary pointer in its firmware node handle will be set if the
"built-in" properties are present.  So, effectively, with this patch applied
ACPI would always fall back to the "built-in" properties (if present), while
DT would not do that (unless the DT node is missing entirely).

That difference could be worked around, though, if dev->fwnode is always set
whenever dev->of_node is set in essentially this way:

	dev->of_node = dn;
	set_primary_fwnode(dev, &dn->fwnode);

(that, of course, can be defined as a macro or static inline).  Now, I'm not
sure about that, but my somewhat educated guess is that while dev->of_node is
read in many many places, the number of places in which it is set is actually
much smaller and making this change in one go should be a workable thing.

Please let me know what you think.

---
 drivers/base/property.c |   61 ++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 30 deletions(-)

Index: linux-pm/drivers/base/property.c
===================================================================
--- linux-pm.orig/drivers/base/property.c
+++ linux-pm/drivers/base/property.c
@@ -127,12 +119,16 @@ EXPORT_SYMBOL_GPL(device_property_presen
  */
 bool fwnode_property_present(struct fwnode_handle *fwnode, const char *propname)
 {
-	if (is_of_node(fwnode))
-		return of_property_read_bool(of_node(fwnode), propname);
-	else if (is_acpi_node(fwnode))
-		return !acpi_dev_prop_get(acpi_node(fwnode), propname, NULL);
+	bool ret = false;
 
-	return !!pset_prop_get(to_pset(fwnode), propname);
+	if (is_of_node(fwnode)) {
+		ret = of_property_read_bool(of_node(fwnode), propname);
+		fwnode = fwnode->secondary;
+	} else if (is_acpi_node(fwnode)) {
+		ret = !acpi_dev_prop_get(acpi_node(fwnode), propname, NULL);
+		fwnode = fwnode->secondary;
+	}
+	return ret ? true : !!pset_prop_get(to_pset(fwnode), propname);
 }
 EXPORT_SYMBOL_GPL(fwnode_property_present);
 
@@ -283,17 +279,18 @@ EXPORT_SYMBOL_GPL(device_property_read_s
 
 #define FWNODE_PROP_READ_ARRAY(_fwnode_, _propname_, _type_, _proptype_, _val_, _nval_) \
 ({ \
-	int _ret_; \
-	if (is_of_node(_fwnode_)) \
+	int _ret_ = -ENXIO; \
+	if (is_of_node(_fwnode_)) { \
 		_ret_ = OF_DEV_PROP_READ_ARRAY(of_node(_fwnode_), _propname_, \
 					       _type_, _val_, _nval_); \
-	else if (is_acpi_node(_fwnode_)) \
+		_fwnode_ = _fwnode_->secondary; \
+	} else if (is_acpi_node(_fwnode_)) { \
 		_ret_ = acpi_dev_prop_read(acpi_node(_fwnode_), _propname_, \
 					   _proptype_, _val_, _nval_); \
-	else \
-		_ret_ = pset_prop_read_array(to_pset(_fwnode_), _propname_, \
-					     _proptype_, _val_, _nval_); \
-	_ret_; \
+		_fwnode_ = _fwnode_->secondary; \
+	} \
+	_ret_ ? pset_prop_read_array(to_pset(_fwnode_), _propname_, \
+				     _proptype_, _val_, _nval_) : 0; \
 })
 
 /**
@@ -422,17 +419,21 @@ int fwnode_property_read_string_array(st
 				      const char *propname, const char **val,
 				      size_t nval)
 {
-	if (is_of_node(fwnode))
-		return val ?
+	int ret = -ENXIO;
+
+	if (is_of_node(fwnode)) {
+		ret = val ?
 			of_property_read_string_array(of_node(fwnode), propname,
 						      val, nval) :
 			of_property_count_strings(of_node(fwnode), propname);
-	else if (is_acpi_node(fwnode))
-		return acpi_dev_prop_read(acpi_node(fwnode), propname,
-					  DEV_PROP_STRING, val, nval);
-
-	return pset_prop_read_array(to_pset(fwnode), propname,
-				    DEV_PROP_STRING, val, nval);
+		fwnode = fwnode->secondary;
+	} else if (is_acpi_node(fwnode)) {
+		ret = acpi_dev_prop_read(acpi_node(fwnode), propname,
+					 DEV_PROP_STRING, val, nval);
+		fwnode = fwnode->secondary;
+	}
+	return ret ? pset_prop_read_array(to_pset(fwnode), propname,
+					  DEV_PROP_STRING, val, nval) : 0;
 }
 EXPORT_SYMBOL_GPL(fwnode_property_read_string_array);
 

Re: [PATCH 3/3][RFD] device property: Implement fallback to built-in properties

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 3254 bytes --]

Hi,

On Sat, Mar 28, 2015 at 02:26:35AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify the fwnode_property_* accessors so as to make them fall back
> to using "built-in" device properties from a property_set structure
> associated with the given device (as the secondary firmware node)
> if the property in question cannot be retrieved from the primary
> firmware node (DT or ACPI).  That will make the device_properties_*
> functions do the same thing as they use fwnode_property_* internally.
> 
> That should make it easier to provide default values for device
> properties used by device drivers to be used in case the properties
> in question are not provided by the platform firmare at all or
> they cannot be retireved from the platform firmware due to errors.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

FWIW for the whole series:

Tested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

I'm attaching the little patch I used to test this with Synopsys
Designware USB Device Controller. Everything worked nicely.

> First of all, this should show why I thought it would be better to use
> the primary/secondary scheme for firmware nodes instead of just having a
> list of them with the head pointed to by the struct device's fwnode field.
> Namely, within the primary/secondary scheme the ordering of property lookup
> is always well defined and there's no overhead when, for example, somebody
> specifically wants to access the ACPI companion firmware node etc.
> 
> Second, as I said in the intro message, this is not for immediate application
> (apart from any review comments on [1-2/3]), because it would introduce an
> artificial difference between DT and ACPI that I'd prefer to avoid.
> 
> Namely, dev_fwnode() (introduced by one of the patches in linux-next) returns
> the address of the dev->of_node's fwnode field if dev->of_node is present and
> that would not have the secondary pointer set.  On the other hand, since ACPI
> is now using set_primary_fwnode() to add its firmware node companions to
> devices, the secondary pointer in its firmware node handle will be set if the
> "built-in" properties are present.  So, effectively, with this patch applied
> ACPI would always fall back to the "built-in" properties (if present), while
> DT would not do that (unless the DT node is missing entirely).
> 
> That difference could be worked around, though, if dev->fwnode is always set
> whenever dev->of_node is set in essentially this way:
> 
> 	dev->of_node = dn;
> 	set_primary_fwnode(dev, &dn->fwnode);
> 
> (that, of course, can be defined as a macro or static inline).  Now, I'm not
> sure about that, but my somewhat educated guess is that while dev->of_node is
> read in many many places, the number of places in which it is set is actually
> much smaller and making this change in one go should be a workable thing.
> 
> Please let me know what you think.

To me falling back to the "built-in" device properties is the most
interesting thing. I have already a set of mfd and probing drivers,
such as dwc3-pci.c in my example, in my mind, where I have the ACPI
companion but could still really use the build-in properties.


Thanks,

-- 
heikki

[-- Attachment #2: dwc3_pset_test.diff --]
[-- Type: text/plain, Size: 1483 bytes --]

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index b773fb5..b02b782 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -31,8 +31,41 @@
 #define PCI_DEVICE_ID_INTEL_SPTLP	0x9d30
 #define PCI_DEVICE_ID_INTEL_SPTH	0xa130
 
+static struct property_entry dwc3_pci_prop[5];
+static struct property_set dwc3_pci_pset = {
+	.properties = dwc3_pci_prop,
+};
+
+static u8 lpm_nyet_threshold[2];
+static u8 tx_de_emphasis[2];
+
 static int dwc3_pci_quirks(struct pci_dev *pdev)
 {
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+	    pdev->device == PCI_DEVICE_ID_INTEL_SPTLP) {
+		struct platform_device *dwc3 = pci_get_drvdata(pdev);
+
+		/* Let's test a couple of boolean properties */
+		dwc3_pci_prop[0].name = "snps,dis_u3_susphy_quirk";
+		dwc3_pci_prop[1].name = "snps,dis_u2_susphy_quirk";
+
+		/* And properties of type u8 */
+		lpm_nyet_threshold[0] = 0xf;
+		tx_de_emphasis[0] = 1;
+
+		dwc3_pci_prop[2].name = "snps,lpm-nyet-threshold";
+		dwc3_pci_prop[2].type = DEV_PROP_U8;
+		dwc3_pci_prop[2].nval = 1;
+		dwc3_pci_prop[2].value.u8_data = lpm_nyet_threshold;
+
+		dwc3_pci_prop[3].name = "snps,tx_de_emphasis";
+		dwc3_pci_prop[3].type = DEV_PROP_U8;
+		dwc3_pci_prop[3].nval = 1;
+		dwc3_pci_prop[3].value.u8_data = tx_de_emphasis;
+
+		device_add_property_set(&dwc3->dev, &dwc3_pci_pset);
+	}
+
 	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
 	    pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
 		struct dwc3_platform_data pdata;

Re: [PATCH 3/3][RFD] device property: Implement fallback to built-in properties

[Rafael J. Wysocki]

On Thursday, April 02, 2015 05:35:06 PM Heikki Krogerus wrote:
> 
> --AqsLC8rIMeq19msA
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> 
> Hi,
> 
> On Sat, Mar 28, 2015 at 02:26:35AM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Modify the fwnode_property_* accessors so as to make them fall back
> > to using "built-in" device properties from a property_set structure
> > associated with the given device (as the secondary firmware node)
> > if the property in question cannot be retrieved from the primary
> > firmware node (DT or ACPI).  That will make the device_properties_*
> > functions do the same thing as they use fwnode_property_* internally.
> > 
> > That should make it easier to provide default values for device
> > properties used by device drivers to be used in case the properties
> > in question are not provided by the platform firmare at all or
> > they cannot be retireved from the platform firmware due to errors.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> FWIW for the whole series:
> 
> Tested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> I'm attaching the little patch I used to test this with Synopsys
> Designware USB Device Controller. Everything worked nicely.

Thanks a lot for testing this!

> > First of all, this should show why I thought it would be better to use
> > the primary/secondary scheme for firmware nodes instead of just having a
> > list of them with the head pointed to by the struct device's fwnode field.
> > Namely, within the primary/secondary scheme the ordering of property lookup
> > is always well defined and there's no overhead when, for example, somebody
> > specifically wants to access the ACPI companion firmware node etc.
> > 
> > Second, as I said in the intro message, this is not for immediate application
> > (apart from any review comments on [1-2/3]), because it would introduce an
> > artificial difference between DT and ACPI that I'd prefer to avoid.
> > 
> > Namely, dev_fwnode() (introduced by one of the patches in linux-next) returns
> > the address of the dev->of_node's fwnode field if dev->of_node is present and
> > that would not have the secondary pointer set.  On the other hand, since ACPI
> > is now using set_primary_fwnode() to add its firmware node companions to
> > devices, the secondary pointer in its firmware node handle will be set if the
> > "built-in" properties are present.  So, effectively, with this patch applied
> > ACPI would always fall back to the "built-in" properties (if present), while
> > DT would not do that (unless the DT node is missing entirely).
> > 
> > That difference could be worked around, though, if dev->fwnode is always set
> > whenever dev->of_node is set in essentially this way:
> > 
> > 	dev->of_node = dn;
> > 	set_primary_fwnode(dev, &dn->fwnode);
> > 
> > (that, of course, can be defined as a macro or static inline).  Now, I'm not
> > sure about that, but my somewhat educated guess is that while dev->of_node is
> > read in many many places, the number of places in which it is set is actually
> > much smaller and making this change in one go should be a workable thing.
> > 
> > Please let me know what you think.
> 
> To me falling back to the "built-in" device properties is the most
> interesting thing. I have already a set of mfd and probing drivers,
> such as dwc3-pci.c in my example, in my mind, where I have the ACPI
> companion but could still really use the build-in properties.

Yes, this is the most interesting part to me too, but as I said, I wouldn't like
to introduce functional differences between DT and ACPI due to this and we need
response from the DT party to make changes along the lines of patch [3/3].

For now, let me resend patches [1-2/3] with your Tested-by: tag. :-)

Rafael

[PATCH 0/2] device property: firmware node type for platform data

[Rafael J. Wysocki]

On Saturday, March 28, 2015 02:05:19 AM Rafael J. Wysocki wrote:
> Hi,
> 
> Patches [1-2/3] add support for properties provided in a "pdata way"
> roughly along the lines of the Heikki's patch at
> 
> https://patchwork.kernel.org/patch/5709461/
> 
> but using the fwnode field in struct device introduced by one of my
> previous patches (currently in linux-next).
> 
> Patch [3/3] does one more interesting thing on top of that, but is mostly
> for discussion.  Namely, it changes the behavior of the unified device
> properties API to fall back to the "built-in" properties when it cannot
> find the requested data in the stuff provided by the platform firmware.
> 
> Currently, that patch would introduce an artificial difference between ACPI
> and DT, because DT uses the of_node pointer in struct device won't use the
> set_primary_fwnode() thing introduced buy the [1/3].  However, I have an
> idea about how that can be worked around, but more on that in the changelog
> of patch [3/3].

So to make some progress here I'd like to apply patches [1-2/3] (which I'm going
to resend shortly) if there are no objections.

Thanks,
Rafael

[PATCH 1/2] device property: Make it possible to use secondary firmware nodes

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Add a "secondary" pointer to struct fwnode_handle so as to make it
possible for a device to have two firmware nodes associated with
it at the same time, for example, an ACPI node and a node with
a set of properties provided by platform initialization code.

In the future that will allow device property lookup to fall back
from the primary firmware node to the secondary one if the given
property is not present there to make it easier to provide defaults
for device properties used by device drivers.

Introduce two helper routines, set_primary_fwnode() and
set_secondary_fwnode() allowing callers to add a primary/secondary
firmware node to the given device in such a way that

 (1) If there's only one firmware node for that device, it will be
     pointed to by the device's firmware node pointer.
 (2) If both the primary and secondary firmware nodes are present,
     the primary one will be pointed to by the device's firmware
     node pointer, while the secondary one will be pointed to by the
     primary node's secondary pointer.
 (3) If one of these nodes is removed (by calling one of the new
     nelpers with NULL as the second argument), the other one will
     be preserved.

Make ACPI use set_primary_fwnode() for attaching its firmware nodes
to devices.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---

Greg, any objections here?

---
 drivers/base/core.c    |   51 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h   |    4 +--
 include/linux/device.h |    3 ++
 include/linux/fwnode.h |    1 
 4 files changed, 57 insertions(+), 2 deletions(-)

Index: linux-pm/include/linux/fwnode.h
===================================================================
--- linux-pm.orig/include/linux/fwnode.h
+++ linux-pm/include/linux/fwnode.h
@@ -20,6 +20,7 @@ enum fwnode_type {
 
 struct fwnode_handle {
 	enum fwnode_type type;
+	struct fwnode_handle *secondary;
 };
 
 #endif
Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -940,6 +940,9 @@ extern void unlock_device_hotplug(void);
 extern int lock_device_hotplug_sysfs(void);
 extern int device_offline(struct device *dev);
 extern int device_online(struct device *dev);
+extern void set_primary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
+extern void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
+
 /*
  * Root device objects for grouping under /sys/devices
  */
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -54,8 +54,8 @@ static inline acpi_handle acpi_device_ha
 }
 
 #define ACPI_COMPANION(dev)		acpi_node((dev)->fwnode)
-#define ACPI_COMPANION_SET(dev, adev)	(dev)->fwnode = (adev) ? \
-	acpi_fwnode_handle(adev) : NULL
+#define ACPI_COMPANION_SET(dev, adev)	set_primary_fwnode(dev, (adev) ? \
+	acpi_fwnode_handle(adev) : NULL)
 #define ACPI_HANDLE(dev)		acpi_device_handle(ACPI_COMPANION(dev))
 
 static inline bool has_acpi_companion(struct device *dev)
Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -12,6 +12,7 @@
 
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/fwnode.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -2132,4 +2133,54 @@ define_dev_printk_level(dev_warn, KERN_W
 define_dev_printk_level(dev_notice, KERN_NOTICE);
 define_dev_printk_level(_dev_info, KERN_INFO);
 
+static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
+{
+	return fwnode && !IS_ERR(fwnode->secondary);
+}
+
+/**
+ * set_primary_fwnode - Change the primary firmware node of a given device.
+ * @dev: Device to handle.
+ * @fwnode: New primary firmware node of the device.
+ *
+ * Set the device's firmware node pointer to @fwnode, but if a secondary
+ * firmware node of the device is present, preserve it.
+ */
+void set_primary_fwnode(struct device *dev, struct fwnode_handle *fwnode)
+{
+	if (fwnode) {
+		struct fwnode_handle *fn = dev->fwnode;
+
+		if (fwnode_is_primary(fn))
+			fn = fn->secondary;
+
+		fwnode->secondary = fn;
+		dev->fwnode = fwnode;
+	} else {
+		dev->fwnode = fwnode_is_primary(dev->fwnode) ?
+			dev->fwnode->secondary : NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(set_primary_fwnode);
+
+/**
+ * set_secondary_fwnode - Change the secondary firmware node of a given device.
+ * @dev: Device to handle.
+ * @fwnode: New secondary firmware node of the device.
+ *
+ * If a primary firmware node of the device is present, set its secondary
+ * pointer to @fwnode.  Otherwise, set the device's firmware node pointer to
+ * @fwnode.
+ */
+void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode)
+{
+	if (fwnode)
+		fwnode->secondary = ERR_PTR(-ENODEV);
+
+	if (fwnode_is_primary(dev->fwnode))
+		dev->fwnode->secondary = fwnode;
+	else
+		dev->fwnode = fwnode;
+}
+
 #endif

Re: [PATCH 1/2] device property: Make it possible to use secondary firmware nodes

[Greg Kroah-Hartman]

On Fri, Apr 03, 2015 at 04:03:27PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Add a "secondary" pointer to struct fwnode_handle so as to make it
> possible for a device to have two firmware nodes associated with
> it at the same time, for example, an ACPI node and a node with
> a set of properties provided by platform initialization code.
> 
> In the future that will allow device property lookup to fall back
> from the primary firmware node to the secondary one if the given
> property is not present there to make it easier to provide defaults
> for device properties used by device drivers.
> 
> Introduce two helper routines, set_primary_fwnode() and
> set_secondary_fwnode() allowing callers to add a primary/secondary
> firmware node to the given device in such a way that
> 
>  (1) If there's only one firmware node for that device, it will be
>      pointed to by the device's firmware node pointer.
>  (2) If both the primary and secondary firmware nodes are present,
>      the primary one will be pointed to by the device's firmware
>      node pointer, while the secondary one will be pointed to by the
>      primary node's secondary pointer.
>  (3) If one of these nodes is removed (by calling one of the new
>      nelpers with NULL as the second argument), the other one will
>      be preserved.
> 
> Make ACPI use set_primary_fwnode() for attaching its firmware nodes
> to devices.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Tested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> 
> Greg, any objections here?

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 2/2] device property: Introduce firmware node type for platform data

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce data structures and code allowing "built-in" properties
to be associated with devices in such a way that they will be used
by the device_property_* API if no proper firmware node (neither DT
nor ACPI) is present for the given device.

Each property is to be represented by a property_entry structure.
An array of property_entry structures (terminated with a null
entry) can be pointed to by the properties field of struct
property_set that can be added as a firmware node to a struct
device using device_add_property_set().  That will cause the
device_property_* API to use that property_set as the source
of properties if the given device does not have a DT node or
an ACPI companion device object associated with it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---

Greg, any objections?

---
 drivers/base/property.c  |   98 ++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/fwnode.h   |    1 
 include/linux/property.h |   33 +++++++++++++++
 3 files changed, 127 insertions(+), 5 deletions(-)

Index: linux-pm/include/linux/fwnode.h
===================================================================
--- linux-pm.orig/include/linux/fwnode.h
+++ linux-pm/include/linux/fwnode.h
@@ -16,6 +16,7 @@ enum fwnode_type {
 	FWNODE_INVALID = 0,
 	FWNODE_OF,
 	FWNODE_ACPI,
+	FWNODE_PDATA,
 };
 
 struct fwnode_handle {
Index: linux-pm/include/linux/property.h
===================================================================
--- linux-pm.orig/include/linux/property.h
+++ linux-pm/include/linux/property.h
@@ -131,4 +131,37 @@ static inline int fwnode_property_read_u
 	return fwnode_property_read_u64_array(fwnode, propname, val, 1);
 }
 
+/**
+ * struct property_entry - "Built-in" device property representation.
+ * @name: Name of the property.
+ * @type: Type of the property.
+ * @nval: Number of items of type @type making up the value.
+ * @value: Value of the property (an array of @nval items of type @type).
+ */
+struct property_entry {
+	const char *name;
+	enum dev_prop_type type;
+	size_t nval;
+	union {
+		void *raw_data;
+		u8 *u8_data;
+		u16 *u16_data;
+		u32 *u32_data;
+		u64 *u64_data;
+		const char **str;
+	} value;
+};
+
+/**
+ * struct property_set - Collection of "built-in" device properties.
+ * @fwnode: Handle to be pointed to by the fwnode field of struct device.
+ * @properties: Array of properties terminated with a null entry.
+ */
+struct property_set {
+	struct fwnode_handle fwnode;
+	struct property_entry *properties;
+};
+
+void device_add_property_set(struct device *dev, struct property_set *pset);
+
 #endif /* _LINUX_PROPERTY_H_ */
Index: linux-pm/drivers/base/property.c
===================================================================
--- linux-pm.orig/drivers/base/property.c
+++ linux-pm/drivers/base/property.c
@@ -10,10 +10,96 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/property.h>
-#include <linux/export.h>
 #include <linux/acpi.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/property.h>
+
+/**
+ * device_add_property_set - Add a collection of properties to a device object.
+ * @dev: Device to add properties to.
+ * @pset: Collection of properties to add.
+ *
+ * Associate a collection of device properties represented by @pset with @dev
+ * as its secondary firmware node.
+ */
+void device_add_property_set(struct device *dev, struct property_set *pset)
+{
+	if (pset)
+		pset->fwnode.type = FWNODE_PDATA;
+
+	set_secondary_fwnode(dev, &pset->fwnode);
+}
+EXPORT_SYMBOL_GPL(device_add_property_set);
+
+static inline bool is_pset(struct fwnode_handle *fwnode)
+{
+	return fwnode && fwnode->type == FWNODE_PDATA;
+}
+
+static inline struct property_set *to_pset(struct fwnode_handle *fwnode)
+{
+	return is_pset(fwnode) ?
+		container_of(fwnode, struct property_set, fwnode) : NULL;
+}
+
+static struct property_entry *pset_prop_get(struct property_set *pset,
+					    const char *name)
+{
+	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 int pset_prop_read_array(struct property_set *pset, const char *name,
+				enum dev_prop_type type, void *val, size_t nval)
+{
+	struct property_entry *prop;
+	unsigned int item_size;
+
+	prop = pset_prop_get(pset, name);
+	if (!prop)
+		return -ENODATA;
+
+	if (prop->type != type)
+		return -EPROTO;
+
+	if (!val)
+		return prop->nval;
+
+	if (prop->nval < nval)
+		return -EOVERFLOW;
+
+	switch (type) {
+	case DEV_PROP_U8:
+		item_size = sizeof(u8);
+		break;
+	case DEV_PROP_U16:
+		item_size = sizeof(u16);
+		break;
+	case DEV_PROP_U32:
+		item_size = sizeof(u32);
+		break;
+	case DEV_PROP_U64:
+		item_size = sizeof(u64);
+		break;
+	case DEV_PROP_STRING:
+		item_size = sizeof(const char *);
+		break;
+	default:
+		return -EINVAL;
+	}
+	memcpy(val, prop->value.raw_data, nval * item_size);
+	return 0;
+}
 
 static inline struct fwnode_handle *dev_fwnode(struct device *dev)
 {
@@ -46,7 +132,7 @@ bool fwnode_property_present(struct fwno
 	else if (is_acpi_node(fwnode))
 		return !acpi_dev_prop_get(acpi_node(fwnode), propname, NULL);
 
-	return false;
+	return !!pset_prop_get(to_pset(fwnode), propname);
 }
 EXPORT_SYMBOL_GPL(fwnode_property_present);
 
@@ -205,7 +291,8 @@ EXPORT_SYMBOL_GPL(device_property_read_s
 		_ret_ = acpi_dev_prop_read(acpi_node(_fwnode_), _propname_, \
 					   _proptype_, _val_, _nval_); \
 	else \
-		_ret_ = -ENXIO; \
+		_ret_ = pset_prop_read_array(to_pset(_fwnode_), _propname_, \
+					     _proptype_, _val_, _nval_); \
 	_ret_; \
 })
 
@@ -344,7 +431,8 @@ int fwnode_property_read_string_array(st
 		return acpi_dev_prop_read(acpi_node(fwnode), propname,
 					  DEV_PROP_STRING, val, nval);
 
-	return -ENXIO;
+	return pset_prop_read_array(to_pset(fwnode), propname,
+				    DEV_PROP_STRING, val, nval);
 }
 EXPORT_SYMBOL_GPL(fwnode_property_read_string_array);
 

Re: [PATCH 2/2] device property: Introduce firmware node type for platform data

[Greg Kroah-Hartman]

On Fri, Apr 03, 2015 at 04:05:11PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce data structures and code allowing "built-in" properties
> to be associated with devices in such a way that they will be used
> by the device_property_* API if no proper firmware node (neither DT
> nor ACPI) is present for the given device.
> 
> Each property is to be represented by a property_entry structure.
> An array of property_entry structures (terminated with a null
> entry) can be pointed to by the properties field of struct
> property_set that can be added as a firmware node to a struct
> device using device_add_property_set().  That will cause the
> device_property_* API to use that property_set as the source
> of properties if the given device does not have a DT node or
> an ACPI companion device object associated with it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Tested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> 
> Greg, any objections?

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Heikki Krogerus]

> > 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.

OK.

<snip>

> > @@ -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"?

It sounds like a better Idea to me too.


Thanks,

-- 
heikki