Hi Grant,
On 24 June 2011 01:38, Grant Likely grant.likely@secretlab.ca wrote:
Despite the fact that this is exactly what I asked you to write, this ends up being rather ugly. (I originally put in the '*4' to match the behaviour of the existing of_read_number(), but that was a mistake. tglx also pointed it out). How about this instead:
Your draft implementation below is suggesting that the offset should be in bytes and not cells and so maybe you are suggesting the new approach to support multi-format property values. I have changed the implementation as per your code below.
int of_read_property_u32(struct property *prop, unsigned int offset, u32 *out_value) { if (!prop || !out_value) return -EINVAL; if (!prop->value) return -ENODATA; if (offset + sizeof(*out_value) > prop->length) return -EOVERFLOW; *out_value = be32_to_cpup(prop->data + offset); return 0; }
[...]
+/**
- of_read_property_string - Returns a pointer to a indexed null terminated
- property value string
- @prop: property to read from.
- @offset: index of the property string to be read.
- @string: pointer to a null terminated string, valid only if the return
- value is 0.
- Returns a pointer to a indexed null terminated property cell string, -EINVAL
- in case the cell does not exist.
- */
+int of_read_property_string(struct property *prop, int offset, char **string) +{
- char *c;
- if (!prop || !prop->value)
- return -EINVAL;
Ditto here about return values.
- c = (char *)prop->value;
You don't need the cast. prop->value is a void* pointer. However, 'c' does need to be const char.
- while (offset--)
- while (*c++)
- ;
Rather than open coding this, you should use the string library functions. Something like:
c = prop->value; while (offset-- && (c - prop->value) < prop->size) c += strlen(c) + 1; if ((c - prop->value) + strlen(c) >= prop->size) return -EOVERFLOW; *out_value = c;
Plus it catches more error conditions that way.
If we change the offset to represent bytes and not cell index in the u32 and u64 versions, shouldn't offset represent bytes for the string version as well? In case of multi-format property values, there could be u32 or u64 values intermixed with string values. So, some modifications have been made to your above implementation of the string version.
The new diff is listed below. Would there be any changes required. If this is acceptable, I will submit a separate patch.
drivers/of/base.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/of.h | 12 ++++ 2 files changed, 154 insertions(+), 0 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c index 632ebae..e9acbea 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -596,6 +596,148 @@ struct device_node *of_find_node_by_phandle(phandle handle) EXPORT_SYMBOL(of_find_node_by_phandle);
/** + * of_read_property_u32 - Reads a 32-bit property value + * @prop: property to read from. + * @offset: offset into property value to read from (in bytes). + * @out_value: returned value. + * + * Returns a 32-bit value from a property. Returns -EINVAL, if the property does + * not exist, -ENODATA, if property does not have a value, -EOVERFLOW, if + * reading from offset overshoots the length of the property. + */ +int of_read_property_u32(struct property *prop, u32 offset, u32 *out_value) +{ + if (!prop || !out_value) + return -EINVAL; + if (!prop->value) + return -ENODATA; + if (offset + sizeof(*out_value) > prop->length) + return -EOVERFLOW; + + *out_value = be32_to_cpup(prop->value + offset); + return 0; +} +EXPORT_SYMBOL_GPL(of_read_property_u32); + +/** + * of_getprop_u32 - Find a property in a device node and read a 32-bit value + * @np: device node from which the property value is to be read. + * @propname name of the property to be searched. + * @offset: offset into property value to read from (in bytes). + * @out_value: returned value. + * + * Search for a property in a device node and read a 32-bit value from a + * property. Returns -EINVAL, if the property does not exist, -ENODATA, if + * property does not have a value, -EOVERFLOW, if reading from offset overshoots + * the length of the property. + */ +int of_getprop_u32(struct device_node *np, char *propname, int offset, + u32 *out_value) +{ + return of_read_property_u32(of_find_property(np, propname, NULL), + offset, out_value); +} +EXPORT_SYMBOL_GPL(of_getprop_u32); + +/** + * of_read_property_u64 - Reads a 64-bit property value + * @prop: property to read from. + * @offset: offset into property value to read from (in bytes). + * @out_value: returned value. + * + * Returns a 64-bit value from a property. Returns -EINVAL, if the property does + * not exist, -ENODATA, if property does not have a value, -EOVERFLOW, if + * reading from offset overshoots the length of the property. + */ +int of_read_property_u64(struct property *prop, int offset, u64 *out_value) +{ + if (!prop || !out_value) + return -EINVAL; + if (!prop->value) + return -ENODATA; + if (offset + sizeof(*out_value) > prop->length) + return -EOVERFLOW; + *out_value = be32_to_cpup(prop->value + offset); + *out_value = (*out_value << 32) | + be32_to_cpup(prop->value + offset + sizeof(u32)); + return 0; +} +EXPORT_SYMBOL_GPL(of_read_property_u64); + +/** + * of_getprop_u64 - Find a property in a device node and read a 64-bit value + * @np: device node from which the property value is to be read. + * @propname name of the property to be searched. + * @offset: offset into property value to read from (in bytes). + * @out_value: returned value. + * + * Search for a property in a device node and read a 64-bit value from a + * property. Returns -EINVAL, if the property does not exist, -ENODATA, if + * property does not have a value, -EOVERFLOW, if reading from offset overshoots + * the length of the property. + */ +int of_getprop_u64(struct device_node *np, char *propname, int offset, + u64 *out_value) +{ + return of_read_property_u64(of_find_property(np, propname, NULL), + offset, out_value); +} +EXPORT_SYMBOL_GPL(of_getprop_u64); + +/** + * of_read_property_string - Returns a pointer to a null terminated + * property string value + * @prop: property to read from. + * @offset: offset into property value to read from (in bytes). + * @string: pointer to a null terminated string, valid only if the return + * value is 0. + * + * Returns a pointer to a null terminated property string value. Returns -EINVAL, + * if the property does not exist, -ENODATA, if property does not have a value, + * -EOVERFLOW, if the offset overshoots the length of the property, -EILSEQ, if + * the string is not null-terminated within the length of the property. + */ +int +of_read_property_string(struct property *prop, int offset, char **out_string) +{ + if (!prop || !out_string) + return -EINVAL; + if (!prop->value) + return -ENODATA; + if (offset >= prop->length) + return -EOVERFLOW; + if (strnlen(prop->value + offset, prop->length - offset) + + offset == prop->length) + return -EILSEQ; + *out_string = prop->value + offset; + return 0; +} +EXPORT_SYMBOL_GPL(of_read_property_string); + +/** + * of_getprop_string - Find a property in a device node and read a null + * terminated string value + * @np: device node from which the property value is to be read. + * @propname name of the property to be searched. + * @offset: offset into property value to read from (in bytes). + * @out_string: pointer to a null terminated string, valid only if the return + * value is 0. + * + * Search for a property in a device node and return a pointer to a string + * value of a property. Returns -EINVAL, if the property does not exist, + * -ENODATA, if property does not have a value, -EOVERFLOW, if the offset + * overshoots the length of the property, -EILSEQ, if the string is not + * null-terminated within the length of the property. + */ +int of_getprop_string(struct device_node *np, char *propname, int offset, + char **out_string) +{ + return of_read_property_string(of_find_property(np, propname, NULL), + offset, out_string); +} +EXPORT_SYMBOL_GPL(of_getprop_string); + +/** * of_parse_phandle - Resolve a phandle property to a device_node pointer * @np: Pointer to device node holding phandle property * @phandle_name: Name of property holding a phandle value diff --git a/include/linux/of.h b/include/linux/of.h index bfc0ed1..f5c1065 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -195,6 +195,18 @@ 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_read_property_u32(struct property *prop, u32 offset, + u32 *out_value); +extern int of_getprop_u32(struct device_node *np, char *propname, int offset, + u32 *out_value); +extern int of_read_property_u64(struct property *prop, int offset, + u64 *out_value); +extern int of_getprop_u64(struct device_node *np, char *propname, int offset, + u64 *out_value); +extern int of_read_property_string(struct property *prop, int offset, + char **out_string); +extern int of_getprop_string(struct device_node *np, char *propname, int offset, + char **out_string); extern int of_device_is_compatible(const struct device_node *device, const char *); extern int of_device_is_available(const struct device_node *device);
Thanks, Thomas.