[PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Viresh Kumar]

This adds following helper routines:
- of_property_read_u8_array()
- of_property_read_u16_array()
- of_property_read_u8()
- of_property_read_u16()

First two actually share most of the code with of_property_read_u32_array(), so
the common part is taken out into a macro, which can be used by all three
*_array() routines.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V1->V2:
-----
- Use typeof() in of_property_read_array() macro instead of passing type to it

 drivers/of/base.c  | 73 +++++++++++++++++++++++++++++++++++++++++++-----------
 include/linux/of.h | 30 ++++++++++++++++++++++
 2 files changed, 89 insertions(+), 14 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index af3b22a..039e178 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -670,6 +670,64 @@ struct device_node *of_find_node_by_phandle(phandle handle)
 }
 EXPORT_SYMBOL(of_find_node_by_phandle);
 
+#define of_property_read_array(_np, _pname, _out, _sz)			\
+	struct property *_prop = of_find_property(_np, _pname, NULL);	\
+	const __be32 *_val;						\
+									\
+	if (!_prop)							\
+		return -EINVAL;						\
+	if (!_prop->value)						\
+		return -ENODATA;					\
+	if ((_sz * sizeof(*_out)) > _prop->length)			\
+		return -EOVERFLOW;					\
+									\
+	_val = _prop->value;						\
+	while (_sz--)							\
+		*_out++ = (typeof(*_out))be32_to_cpup(_val++);		\
+	return 0;
+
+/**
+ * of_property_read_u8_array - Find and read an array of u8 from a property.
+ *
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ * @out_value:	pointer to return value, modified only if return value is 0.
+ *
+ * Search for a property in a device node and read 8-bit value(s) from
+ * it. Returns 0 on success, -EINVAL if the property does not exist,
+ * -ENODATA if property does not have a value, and -EOVERFLOW if the
+ * property data isn't large enough.
+ *
+ * The out_value is modified only if a valid u8 value can be decoded.
+ */
+int of_property_read_u8_array(const struct device_node *np,
+			const char *propname, u8 *out_values, size_t sz)
+{
+	of_property_read_array(np, propname, out_values, sz);
+}
+EXPORT_SYMBOL_GPL(of_property_read_u8_array);
+
+/**
+ * of_property_read_u16_array - Find and read an array of u16 from a property.
+ *
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ * @out_value:	pointer to return value, modified only if return value is 0.
+ *
+ * Search for a property in a device node and read 16-bit value(s) from
+ * it. Returns 0 on success, -EINVAL if the property does not exist,
+ * -ENODATA if property does not have a value, and -EOVERFLOW if the
+ * property data isn't large enough.
+ *
+ * The out_value is modified only if a valid u16 value can be decoded.
+ */
+int of_property_read_u16_array(const struct device_node *np,
+			const char *propname, u16 *out_values, size_t sz)
+{
+	of_property_read_array(np, propname, out_values, sz);
+}
+EXPORT_SYMBOL_GPL(of_property_read_u16_array);
+
 /**
  * of_property_read_u32_array - Find and read an array of 32 bit integers
  * from a property.
@@ -689,20 +747,7 @@ int of_property_read_u32_array(const struct device_node *np,
 			       const char *propname, u32 *out_values,
 			       size_t sz)
 {
-	struct property *prop = of_find_property(np, propname, NULL);
-	const __be32 *val;
-
-	if (!prop)
-		return -EINVAL;
-	if (!prop->value)
-		return -ENODATA;
-	if ((sz * sizeof(*out_values)) > prop->length)
-		return -EOVERFLOW;
-
-	val = prop->value;
-	while (sz--)
-		*out_values++ = be32_to_cpup(val++);
-	return 0;
+	of_property_read_array(np, propname, out_values, sz);
 }
 EXPORT_SYMBOL_GPL(of_property_read_u32_array);
 
diff --git a/include/linux/of.h b/include/linux/of.h
index 72843b7..e2d9b40 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -223,6 +223,10 @@ extern struct device_node *of_find_node_with_property(
 extern struct property *of_find_property(const struct device_node *np,
 					 const char *name,
 					 int *lenp);
+extern int of_property_read_u8_array(const struct device_node *np,
+			const char *propname, u8 *out_values, size_t sz);
+extern int of_property_read_u16_array(const struct device_node *np,
+			const char *propname, u16 *out_values, size_t sz);
 extern int of_property_read_u32_array(const struct device_node *np,
 				      const char *propname,
 				      u32 *out_values,
@@ -357,6 +361,18 @@ static inline struct device_node *of_find_compatible_node(
 	return NULL;
 }
 
+static inline int of_property_read_u8_array(const struct device_node *np,
+			const char *propname, u8 *out_values, size_t sz)
+{
+	return -ENOSYS;
+}
+
+static inline int of_property_read_u16_array(const struct device_node *np,
+			const char *propname, u16 *out_values, size_t sz)
+{
+	return -ENOSYS;
+}
+
 static inline int of_property_read_u32_array(const struct device_node *np,
 					     const char *propname,
 					     u32 *out_values, size_t sz)
@@ -463,6 +479,20 @@ static inline bool of_property_read_bool(const struct device_node *np,
 	return prop ? true : false;
 }
 
+static inline int of_property_read_u8(const struct device_node *np,
+				       const char *propname,
+				       u8 *out_value)
+{
+	return of_property_read_u8_array(np, propname, out_value, 1);
+}
+
+static inline int of_property_read_u16(const struct device_node *np,
+				       const char *propname,
+				       u16 *out_value)
+{
+	return of_property_read_u16_array(np, propname, out_value, 1);
+}
+
 static inline int of_property_read_u32(const struct device_node *np,
 				       const char *propname,
 				       u32 *out_value)
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Viresh Kumar]

[-- Attachment #1.1: Type: text/plain, Size: 540 bytes --]

On 26 October 2012 09:50, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> This adds following helper routines:
> - of_property_read_u8_array()
> - of_property_read_u16_array()
> - of_property_read_u8()
> - of_property_read_u16()
>
> First two actually share most of the code with
> of_property_read_u32_array(), so
> the common part is taken out into a macro, which can be used by all three
> *_array() routines.
>
> Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>

Ping!!

[-- Attachment #1.2: Type: text/html, Size: 989 bytes --]

[-- Attachment #2: Type: text/plain, Size: 175 bytes --]

_______________________________________________
linaro-dev mailing list
linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Rob Herring]

On 10/25/2012 11:20 PM, Viresh Kumar wrote:
> This adds following helper routines:
> - of_property_read_u8_array()
> - of_property_read_u16_array()
> - of_property_read_u8()
> - of_property_read_u16()
> 
> First two actually share most of the code with of_property_read_u32_array(), so
> the common part is taken out into a macro, which can be used by all three
> *_array() routines.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V1->V2:
> -----
> - Use typeof() in of_property_read_array() macro instead of passing type to it
> 
>  drivers/of/base.c  | 73 +++++++++++++++++++++++++++++++++++++++++++-----------
>  include/linux/of.h | 30 ++++++++++++++++++++++
>  2 files changed, 89 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index af3b22a..039e178 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -670,6 +670,64 @@ struct device_node *of_find_node_by_phandle(phandle handle)
>  }
>  EXPORT_SYMBOL(of_find_node_by_phandle);
>  
> +#define of_property_read_array(_np, _pname, _out, _sz)			\
> +	struct property *_prop = of_find_property(_np, _pname, NULL);	\
> +	const __be32 *_val;						\
> +									\
> +	if (!_prop)							\
> +		return -EINVAL;						\
> +	if (!_prop->value)						\
> +		return -ENODATA;					\
> +	if ((_sz * sizeof(*_out)) > _prop->length)			\
> +		return -EOVERFLOW;					\
> +									\
> +	_val = _prop->value;						\
> +	while (_sz--)							\
> +		*_out++ = (typeof(*_out))be32_to_cpup(_val++);		\

This will not work. You are incrementing _out by 1, 2, or 4 bytes, but
_val is always incremented by 4 bytes.

According to the dtc commit adding this feature, the values are packed:

With this patch the following property assignment:

    property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;

is equivalent to:

    property = <0x12345678 0x0000ffff>;


> +	return 0;
> +
> +/**
> + * of_property_read_u8_array - Find and read an array of u8 from a property.
> + *
> + * @np:		device node from which the property value is to be read.
> + * @propname:	name of the property to be searched.
> + * @out_value:	pointer to return value, modified only if return value is 0.
> + *

Missing sz

> + * Search for a property in a device node and read 8-bit value(s) from
> + * it. Returns 0 on success, -EINVAL if the property does not exist,
> + * -ENODATA if property does not have a value, and -EOVERFLOW if the
> + * property data isn't large enough.
> + *
> + * The out_value is modified only if a valid u8 value can be decoded.
> + */
> +int of_property_read_u8_array(const struct device_node *np,
> +			const char *propname, u8 *out_values, size_t sz)
> +{
> +	of_property_read_array(np, propname, out_values, sz);
> +}
> +EXPORT_SYMBOL_GPL(of_property_read_u8_array);
> +
> +/**
> + * of_property_read_u16_array - Find and read an array of u16 from a property.
> + *
> + * @np:		device node from which the property value is to be read.
> + * @propname:	name of the property to be searched.
> + * @out_value:	pointer to return value, modified only if return value is 0.
> + *

Ditto.

> + * Search for a property in a device node and read 16-bit value(s) from
> + * it. Returns 0 on success, -EINVAL if the property does not exist,
> + * -ENODATA if property does not have a value, and -EOVERFLOW if the
> + * property data isn't large enough.
> + *
> + * The out_value is modified only if a valid u16 value can be decoded.
> + */
> +int of_property_read_u16_array(const struct device_node *np,
> +			const char *propname, u16 *out_values, size_t sz)
> +{
> +	of_property_read_array(np, propname, out_values, sz);
> +}
> +EXPORT_SYMBOL_GPL(of_property_read_u16_array);
> +
>  /**
>   * of_property_read_u32_array - Find and read an array of 32 bit integers
>   * from a property.
> @@ -689,20 +747,7 @@ int of_property_read_u32_array(const struct device_node *np,
>  			       const char *propname, u32 *out_values,
>  			       size_t sz)
>  {
> -	struct property *prop = of_find_property(np, propname, NULL);
> -	const __be32 *val;
> -
> -	if (!prop)
> -		return -EINVAL;
> -	if (!prop->value)
> -		return -ENODATA;
> -	if ((sz * sizeof(*out_values)) > prop->length)
> -		return -EOVERFLOW;
> -
> -	val = prop->value;
> -	while (sz--)
> -		*out_values++ = be32_to_cpup(val++);
> -	return 0;
> +	of_property_read_array(np, propname, out_values, sz);
>  }
>  EXPORT_SYMBOL_GPL(of_property_read_u32_array);
>  
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 72843b7..e2d9b40 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -223,6 +223,10 @@ extern struct device_node *of_find_node_with_property(
>  extern struct property *of_find_property(const struct device_node *np,
>  					 const char *name,
>  					 int *lenp);
> +extern int of_property_read_u8_array(const struct device_node *np,
> +			const char *propname, u8 *out_values, size_t sz);
> +extern int of_property_read_u16_array(const struct device_node *np,
> +			const char *propname, u16 *out_values, size_t sz);
>  extern int of_property_read_u32_array(const struct device_node *np,
>  				      const char *propname,
>  				      u32 *out_values,
> @@ -357,6 +361,18 @@ static inline struct device_node *of_find_compatible_node(
>  	return NULL;
>  }
>  
> +static inline int of_property_read_u8_array(const struct device_node *np,
> +			const char *propname, u8 *out_values, size_t sz)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int of_property_read_u16_array(const struct device_node *np,
> +			const char *propname, u16 *out_values, size_t sz)
> +{
> +	return -ENOSYS;
> +}
> +
>  static inline int of_property_read_u32_array(const struct device_node *np,
>  					     const char *propname,
>  					     u32 *out_values, size_t sz)
> @@ -463,6 +479,20 @@ static inline bool of_property_read_bool(const struct device_node *np,
>  	return prop ? true : false;
>  }
>  
> +static inline int of_property_read_u8(const struct device_node *np,
> +				       const char *propname,
> +				       u8 *out_value)
> +{
> +	return of_property_read_u8_array(np, propname, out_value, 1);
> +}
> +
> +static inline int of_property_read_u16(const struct device_node *np,
> +				       const char *propname,
> +				       u16 *out_value)
> +{
> +	return of_property_read_u16_array(np, propname, out_value, 1);
> +}
> +
>  static inline int of_property_read_u32(const struct device_node *np,
>  				       const char *propname,
>  				       u32 *out_value)
> 

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Viresh Kumar]

[-- Attachment #1.1: Type: text/plain, Size: 2080 bytes --]

On 6 November 2012 19:48, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> > diff --git a/drivers/of/base.c b/drivers/of/base.c
>


> > +#define of_property_read_array(_np, _pname, _out, _sz)
>       \
> > +     struct property *_prop = of_find_property(_np, _pname, NULL);   \
> > +     const __be32 *_val;                                             \
> > +                                                                     \
> > +     if (!_prop)                                                     \
> > +             return -EINVAL;                                         \
> > +     if (!_prop->value)                                              \
> > +             return -ENODATA;                                        \
> > +     if ((_sz * sizeof(*_out)) > _prop->length)                      \
> > +             return -EOVERFLOW;                                      \
> > +                                                                     \
> > +     _val = _prop->value;                                            \
> > +     while (_sz--)                                                   \
> > +             *_out++ = (typeof(*_out))be32_to_cpup(_val++);          \
>
> This will not work. You are incrementing _out by 1, 2, or 4 bytes, but
> _val is always incremented by 4 bytes.
>
> According to the dtc commit adding this feature, the values are packed:
>
> With this patch the following property assignment:
>
>     property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
>
> is equivalent to:
>
>     property = <0x12345678 0x0000ffff>;
>

Something which i haven't expected :(
I will fix and test it well for all types before sending it now.


> > +/**
> > + * of_property_read_u8_array - Find and read an array of u8 from a
> property.
> > + *
> > + * @np:              device node from which the property value is to be
> read.
> > + * @propname:        name of the property to be searched.
> > + * @out_value:       pointer to return value, modified only if return
> value is 0.
> > + *
>
> Missing sz
>

Yes for both misses.

[-- Attachment #1.2: Type: text/html, Size: 3003 bytes --]

[-- Attachment #2: Type: text/plain, Size: 175 bytes --]

_______________________________________________
linaro-dev mailing list
linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[viresh kumar]

On Tue, Nov 6, 2012 at 7:48 PM, Rob Herring <robherring2@gmail.com> wrote:
>> +#define of_property_read_array(_np, _pname, _out, _sz)                       \

>> +     while (_sz--)                                                   \
>> +             *_out++ = (typeof(*_out))be32_to_cpup(_val++);          \

> This will not work. You are incrementing _out by 1, 2, or 4 bytes, but
> _val is always incremented by 4 bytes.
>
> According to the dtc commit adding this feature, the values are packed:
>
> With this patch the following property assignment:
>
>     property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
>
> is equivalent to:
>
>     property = <0x12345678 0x0000ffff>;

I thought of it a bit more and wasn't actually aligned with your explanation :(

If that is the case, how will current implementation of u32 array will work
if we pass something like: 0x88 0x84000000 0x5890 from DT?

So, i did a dummy test of my current implementation, with following changes
in one of my drivers:

dts changes:

                cluster0: cluster@0 {
+                       data1 = <0x50 0x60 0x70>;
+                       data2 = <0x5000 0x6000 0x7000>;
+                       data3 = <0x50000000 0x60000000 0x70000000>;
               }

driver changes:

+void test(struct device_node *cluster)
+{
+       u8 data1[3];
+       u16 data2[3];
+       u32 data3[3], i;
+
+       of_property_read_u8_array(cluster, "data1", data1, 3);
+       of_property_read_u16_array(cluster, "data2", data2, 3);
+       of_property_read_u32_array(cluster, "data3", data3, 3);
+
+       for (i = 0; i < 3; i++) {
+               printk(KERN_INFO "u8 %d: %x\n", i, data1[i]);
+               printk(KERN_INFO "u16 %d: %x\n", i, data2[i]);
+               printk(KERN_INFO "u32 %d: %x\n", i, data3[i]);
+       }
+}

And following is the output

[    4.087205] u8 0: 50
[    4.093746] u16 0: 5000
[    4.101067] u32 0: 50000000
[    4.109512] u8 1: 60
[    4.116036] u16 1: 6000
[    4.123357] u32 1: 60000000
[    4.131718] u8 2: 70
[    4.138241] u16 2: 7000
[    4.145573] u32 2: 70000000

which looks to be what we were looking for, isn't it?

Following is fixup for the doc comment missing:

commit 00803aed0781de451048df0d15a3e8c814a343c8
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Wed Nov 7 09:48:46 2012 +0530

    fixup! dt: add helper function to read u8 & u16 variables & arrays
---
 drivers/of/base.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index fbb634b..4a6632e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -669,6 +669,7 @@ EXPORT_SYMBOL(of_find_node_by_phandle);
  * @np:                device node from which the property value is to be read.
  * @propname:  name of the property to be searched.
  * @out_value: pointer to return value, modified only if return value is 0.
+ * @sz:                number of array elements to read
  *
  * Search for a property in a device node and read 8-bit value(s) from
  * it. Returns 0 on success, -EINVAL if the property does not exist,
@@ -690,6 +691,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u8_array);
  * @np:                device node from which the property value is to be read.
  * @propname:  name of the property to be searched.
  * @out_value: pointer to return value, modified only if return value is 0.
+ * @sz:                number of array elements to read
  *
  * Search for a property in a device node and read 16-bit value(s) from
  * it. Returns 0 on success, -EINVAL if the property does not exist,
@@ -712,6 +714,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u16_array);
  * @np:                device node from which the property value is to be read.
  * @propname:  name of the property to be searched.
  * @out_value: pointer to return value, modified only if return value is 0.
+ * @sz:                number of array elements to read
  *
  * Search for a property in a device node and read 32-bit value(s) from
  * it. Returns 0 on success, -EINVAL if the property does not exist,

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Viresh Kumar]

Ping!!

On 7 November 2012 09:52, viresh kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Nov 6, 2012 at 7:48 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> +#define of_property_read_array(_np, _pname, _out, _sz)                       \
>
>>> +     while (_sz--)                                                   \
>>> +             *_out++ = (typeof(*_out))be32_to_cpup(_val++);          \
>
>> This will not work. You are incrementing _out by 1, 2, or 4 bytes, but
>> _val is always incremented by 4 bytes.
>>
>> According to the dtc commit adding this feature, the values are packed:
>>
>> With this patch the following property assignment:
>>
>>     property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
>>
>> is equivalent to:
>>
>>     property = <0x12345678 0x0000ffff>;
>
> I thought of it a bit more and wasn't actually aligned with your explanation :(
>
> If that is the case, how will current implementation of u32 array will work
> if we pass something like: 0x88 0x84000000 0x5890 from DT?
>
> So, i did a dummy test of my current implementation, with following changes
> in one of my drivers:
>
> dts changes:
>
>                 cluster0: cluster@0 {
> +                       data1 = <0x50 0x60 0x70>;
> +                       data2 = <0x5000 0x6000 0x7000>;
> +                       data3 = <0x50000000 0x60000000 0x70000000>;
>                }
>
> driver changes:
>
> +void test(struct device_node *cluster)
> +{
> +       u8 data1[3];
> +       u16 data2[3];
> +       u32 data3[3], i;
> +
> +       of_property_read_u8_array(cluster, "data1", data1, 3);
> +       of_property_read_u16_array(cluster, "data2", data2, 3);
> +       of_property_read_u32_array(cluster, "data3", data3, 3);
> +
> +       for (i = 0; i < 3; i++) {
> +               printk(KERN_INFO "u8 %d: %x\n", i, data1[i]);
> +               printk(KERN_INFO "u16 %d: %x\n", i, data2[i]);
> +               printk(KERN_INFO "u32 %d: %x\n", i, data3[i]);
> +       }
> +}
>
> And following is the output
>
> [    4.087205] u8 0: 50
> [    4.093746] u16 0: 5000
> [    4.101067] u32 0: 50000000
> [    4.109512] u8 1: 60
> [    4.116036] u16 1: 6000
> [    4.123357] u32 1: 60000000
> [    4.131718] u8 2: 70
> [    4.138241] u16 2: 7000
> [    4.145573] u32 2: 70000000
>
> which looks to be what we were looking for, isn't it?
>
> Following is fixup for the doc comment missing:
>
> commit 00803aed0781de451048df0d15a3e8c814a343c8
> Author: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Date:   Wed Nov 7 09:48:46 2012 +0530
>
>     fixup! dt: add helper function to read u8 & u16 variables & arrays
> ---
>  drivers/of/base.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index fbb634b..4a6632e 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -669,6 +669,7 @@ EXPORT_SYMBOL(of_find_node_by_phandle);
>   * @np:                device node from which the property value is to be read.
>   * @propname:  name of the property to be searched.
>   * @out_value: pointer to return value, modified only if return value is 0.
> + * @sz:                number of array elements to read
>   *
>   * Search for a property in a device node and read 8-bit value(s) from
>   * it. Returns 0 on success, -EINVAL if the property does not exist,
> @@ -690,6 +691,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u8_array);
>   * @np:                device node from which the property value is to be read.
>   * @propname:  name of the property to be searched.
>   * @out_value: pointer to return value, modified only if return value is 0.
> + * @sz:                number of array elements to read
>   *
>   * Search for a property in a device node and read 16-bit value(s) from
>   * it. Returns 0 on success, -EINVAL if the property does not exist,
> @@ -712,6 +714,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u16_array);
>   * @np:                device node from which the property value is to be read.
>   * @propname:  name of the property to be searched.
>   * @out_value: pointer to return value, modified only if return value is 0.
> + * @sz:                number of array elements to read
>   *
>   * Search for a property in a device node and read 32-bit value(s) from
>   * it. Returns 0 on success, -EINVAL if the property does not exist,
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Rob Herring]

On 11/06/2012 10:22 PM, viresh kumar wrote:
> On Tue, Nov 6, 2012 at 7:48 PM, Rob Herring <robherring2@gmail.com> wrote:
>>> +#define of_property_read_array(_np, _pname, _out, _sz)                       \
> 
>>> +     while (_sz--)                                                   \
>>> +             *_out++ = (typeof(*_out))be32_to_cpup(_val++);          \
> 
>> This will not work. You are incrementing _out by 1, 2, or 4 bytes, but
>> _val is always incremented by 4 bytes.
>>
>> According to the dtc commit adding this feature, the values are packed:
>>
>> With this patch the following property assignment:
>>
>>     property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
>>
>> is equivalent to:
>>
>>     property = <0x12345678 0x0000ffff>;
> 
> I thought of it a bit more and wasn't actually aligned with your explanation :(
> 
> If that is the case, how will current implementation of u32 array will work
> if we pass something like: 0x88 0x84000000 0x5890 from DT?
> 
> So, i did a dummy test of my current implementation, with following changes
> in one of my drivers:
> 
> dts changes:
> 
>                 cluster0: cluster@0 {
> +                       data1 = <0x50 0x60 0x70>;
> +                       data2 = <0x5000 0x6000 0x7000>;
> +                       data3 = <0x50000000 0x60000000 0x70000000>;

So there is a mismatch in our assumptions. You are just truncating
32-bit values. I assumed you were using the 8 and 16 bit sizes that are
now supported in dts. I don't think we should just truncate values
blindly. We have support for specifying 8 and 16 values now so you
should use that and define that as part of a binding.

Rob

>                }
> 
> driver changes:
> 
> +void test(struct device_node *cluster)
> +{
> +       u8 data1[3];
> +       u16 data2[3];
> +       u32 data3[3], i;
> +
> +       of_property_read_u8_array(cluster, "data1", data1, 3);
> +       of_property_read_u16_array(cluster, "data2", data2, 3);
> +       of_property_read_u32_array(cluster, "data3", data3, 3);
> +
> +       for (i = 0; i < 3; i++) {
> +               printk(KERN_INFO "u8 %d: %x\n", i, data1[i]);
> +               printk(KERN_INFO "u16 %d: %x\n", i, data2[i]);
> +               printk(KERN_INFO "u32 %d: %x\n", i, data3[i]);
> +       }
> +}
> 
> And following is the output
> 
> [    4.087205] u8 0: 50
> [    4.093746] u16 0: 5000
> [    4.101067] u32 0: 50000000
> [    4.109512] u8 1: 60
> [    4.116036] u16 1: 6000
> [    4.123357] u32 1: 60000000
> [    4.131718] u8 2: 70
> [    4.138241] u16 2: 7000
> [    4.145573] u32 2: 70000000
> 
> which looks to be what we were looking for, isn't it?
> 
> Following is fixup for the doc comment missing:
> 
> commit 00803aed0781de451048df0d15a3e8c814a343c8
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Wed Nov 7 09:48:46 2012 +0530
> 
>     fixup! dt: add helper function to read u8 & u16 variables & arrays
> ---
>  drivers/of/base.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index fbb634b..4a6632e 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -669,6 +669,7 @@ EXPORT_SYMBOL(of_find_node_by_phandle);
>   * @np:                device node from which the property value is to be read.
>   * @propname:  name of the property to be searched.
>   * @out_value: pointer to return value, modified only if return value is 0.
> + * @sz:                number of array elements to read
>   *
>   * Search for a property in a device node and read 8-bit value(s) from
>   * it. Returns 0 on success, -EINVAL if the property does not exist,
> @@ -690,6 +691,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u8_array);
>   * @np:                device node from which the property value is to be read.
>   * @propname:  name of the property to be searched.
>   * @out_value: pointer to return value, modified only if return value is 0.
> + * @sz:                number of array elements to read
>   *
>   * Search for a property in a device node and read 16-bit value(s) from
>   * it. Returns 0 on success, -EINVAL if the property does not exist,
> @@ -712,6 +714,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u16_array);
>   * @np:                device node from which the property value is to be read.
>   * @propname:  name of the property to be searched.
>   * @out_value: pointer to return value, modified only if return value is 0.
> + * @sz:                number of array elements to read
>   *
>   * Search for a property in a device node and read 32-bit value(s) from
>   * it. Returns 0 on success, -EINVAL if the property does not exist,
> 

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Viresh Kumar]

On 11 November 2012 19:42, Rob Herring <robherring2@gmail.com> wrote:
> On 11/06/2012 10:22 PM, viresh kumar wrote:

>>                 cluster0: cluster@0 {
>> +                       data1 = <0x50 0x60 0x70>;
>> +                       data2 = <0x5000 0x6000 0x7000>;
>> +                       data3 = <0x50000000 0x60000000 0x70000000>;
>
> So there is a mismatch in our assumptions. You are just truncating
> 32-bit values. I assumed you were using the 8 and 16 bit sizes that are
> now supported in dts. I don't think we should just truncate values
> blindly. We have support for specifying 8 and 16 values now so you
> should use that and define that as part of a binding.

Sorry couldn't get your point at all :(
What did you mean by "truncating 32 bit values" and how should we
tell via DT, that the value passed is 8 bit, 16 bit or 32 bit?

--
viresh

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Rob Herring]

On 11/11/2012 11:27 AM, Viresh Kumar wrote:
> On 11 November 2012 19:42, Rob Herring <robherring2@gmail.com> wrote:
>> On 11/06/2012 10:22 PM, viresh kumar wrote:
> 
>>>                 cluster0: cluster@0 {
>>> +                       data1 = <0x50 0x60 0x70>;
>>> +                       data2 = <0x5000 0x6000 0x7000>;
>>> +                       data3 = <0x50000000 0x60000000 0x70000000>;
>>
>> So there is a mismatch in our assumptions. You are just truncating
>> 32-bit values. I assumed you were using the 8 and 16 bit sizes that are
>> now supported in dts. I don't think we should just truncate values
>> blindly. We have support for specifying 8 and 16 values now so you
>> should use that and define that as part of a binding.
> 
> Sorry couldn't get your point at all :(
> What did you mean by "truncating 32 bit values" and how should we
> tell via DT, that the value passed is 8 bit, 16 bit or 32 bit?
> 

You are trying to retrieve an array of 8 or 16-bit values which are
stored as 32-bit values in dtb. Why not define them in the binding as 8
or 16 bit to begin with. Then there is never any ambiguity about their size.

I don't think the size is stored in the dtb. It is only in the dts. You
need to define the size in the binding definitions and use '/bits/'
annotation. With this the data is packed. Then the array function used
should match what the binding defines.

Rob

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Viresh Kumar]

On 12 November 2012 01:12, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 11/11/2012 11:27 AM, Viresh Kumar wrote:
>> On 11 November 2012 19:42, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 11/06/2012 10:22 PM, viresh kumar wrote:
>>
>>>>                 cluster0: cluster@0 {
>>>> +                       data1 = <0x50 0x60 0x70>;
>>>> +                       data2 = <0x5000 0x6000 0x7000>;
>>>> +                       data3 = <0x50000000 0x60000000 0x70000000>;
>>>
>>> So there is a mismatch in our assumptions. You are just truncating
>>> 32-bit values. I assumed you were using the 8 and 16 bit sizes that are
>>> now supported in dts. I don't think we should just truncate values
>>> blindly. We have support for specifying 8 and 16 values now so you
>>> should use that and define that as part of a binding.
>>
>> Sorry couldn't get your point at all :(
>> What did you mean by "truncating 32 bit values" and how should we
>> tell via DT, that the value passed is 8 bit, 16 bit or 32 bit?
>>
>
> You are trying to retrieve an array of 8 or 16-bit values which are
> stored as 32-bit values in dtb. Why not define them in the binding as 8
> or 16 bit to begin with. Then there is never any ambiguity about their size.
>
> I don't think the size is stored in the dtb. It is only in the dts. You
> need to define the size in the binding definitions and use '/bits/'
> annotation. With this the data is packed. Then the array function used
> should match what the binding defines.

Aha, and in that case incrementing address by 4 in my patch will fail. Right?
Will fix it. Thanks for increasing my knowledge on this :)

--
viresh

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Viresh Kumar]

On 12 November 2012 09:03, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 12 November 2012 01:12, Rob Herring <robherring2@gmail.com> wrote:
>> I don't think the size is stored in the dtb. It is only in the dts. You
>> need to define the size in the binding definitions and use '/bits/'
>> annotation. With this the data is packed. Then the array function used
>> should match what the binding defines.

Hi Rob,

Can you please help me in getting the correct format to do this from dts.
I tried:

data1 = /bits/ 8 <0x50 0x60 0x70>;

as written in your earlier mail... but it didn't worked. Tried to
search too, but
couldn't find it.

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Rajanikanth HV]

On 19 November 2012 09:24, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 12 November 2012 09:03, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 12 November 2012 01:12, Rob Herring <robherring2@gmail.com> wrote:
>>> I don't think the size is stored in the dtb. It is only in the dts. You
>>> need to define the size in the binding definitions and use '/bits/'
>>> annotation. With this the data is packed. Then the array function used
>>> should match what the binding defines.
>
> Hi Rob,
>
> Can you please help me in getting the correct format to do this from dts.
> I tried:
>
> data1 = /bits/ 8 <0x50 0x60 0x70>;
as per spec, format for data byte defines will be:
data1 = [ 0x50 0x60 0x70 ];
however, i see a parse error from device tree compiler when i tried.
>
> as written in your earlier mail... but it didn't worked. Tried to
> search too, but
> couldn't find it.
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Viresh Kumar]

On 19 November 2012 11:54, Rajanikanth HV <rajanikanth.hv-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> data1 = /bits/ 8 <0x50 0x60 0x70>;
> as per spec, format for data byte defines will be:
> data1 = [ 0x50 0x60 0x70 ];
> however, i see a parse error from device tree compiler when i tried.

Firstly you tried square braces [ ], I am not sure if that is allowed.
Can you point me to the specification?

And simply passing 0x50, 0x60 etc.. will make them u32 instead of
u8. i.e. they will be stored as 0x00000050, etc.

--
viresh

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Rajanikanth HV]

On 19 November 2012 12:00, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Firstly you tried square braces [ ], I am not sure if that is allowed.
> Can you point me to the specification?
http://www.devicetree.org/Device_Tree_Usage
"
a-byte-data-property = [0x01 0x23 0x34 0x56];
"
>
> And simply passing 0x50, 0x60 etc.. will make them u32 instead of
> u8. i.e. they will be stored as 0x00000050, etc.
>
> --
> viresh

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Viresh Kumar]

On 19 November 2012 12:05, Rajanikanth HV <rajanikanth.hv@linaro.org> wrote:
> On 19 November 2012 12:00, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> Firstly you tried square braces [ ], I am not sure if that is allowed.
>> Can you point me to the specification?
> http://www.devicetree.org/Device_Tree_Usage
> "
> a-byte-data-property = [0x01 0x23 0x34 0x56];
> "

Ok, but what about 16 bit then {} :)

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Stephen Warren]

On 11/18/2012 11:41 PM, Viresh Kumar wrote:
> On 19 November 2012 12:05, Rajanikanth HV <rajanikanth.hv@linaro.org> wrote:
>> On 19 November 2012 12:00, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> Firstly you tried square braces [ ], I am not sure if that is allowed.
>>> Can you point me to the specification?
>> http://www.devicetree.org/Device_Tree_Usage
>> "
>> a-byte-data-property = [0x01 0x23 0x34 0x56];
>> "
> 
> Ok, but what about 16 bit then {} :)

Support for byte- and word- properties is relatively recent I believe
(or at least, the /bits/ syntax is). Which dtc version are you using?

Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays

[Viresh Kumar]

On 19 November 2012 21:58, Stephen Warren <swarren@wwwdotorg.org> wrote:
> Support for byte- and word- properties is relatively recent I believe
> (or at least, the /bits/ syntax is). Which dtc version are you using?

Ok, i was on a older version. I just saw this patch now:

commit cd296721a9645f9f28800a072490fa15458d1fb7
Author: Stephen Warren <swarren@nvidia.com>
Date:   Fri Sep 28 21:25:59 2012 +0000

    dtc: import latest upstream dtc

    This updates scripts/dtc to commit 317a5d9 "dtc: zero out new label
    objects" from git://git.jdl.com/software/dtc.git.

    This adds features such as:
    * /bits/ syntax for cell data.
    * Math expressions within cell data.
    * The ability to delete properties or nodes.
    * Support for #line directives in the input file, which allows the use of
      cpp on *.dts.
    * -i command-line option (/include/ path)
    * -W/-E command-line options for error/warning control.
    * Removal of spew to STDOUT containing the filename being compiled.
    * Many additions to the libfdt API.

    Signed-off-by: Stephen Warren <swarren@nvidia.com>
    Acked-by: Jon Loeliger <jdl@jdl.com>
    Signed-off-by: Rob Herring <rob.herring@calxeda.com>

Will try it tomorrow

--
viresh