From patchwork Fri Jun 24 12:27:59 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thomas Abraham X-Patchwork-Id: 916052 Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by demeter2.kernel.org (8.14.4/8.14.4) with ESMTP id p5OCTJTU017089 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Fri, 24 Jun 2011 12:29:40 GMT Received: from canuck.infradead.org ([2001:4978:20e::1]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1Qa5VO-0002AZ-Ih; Fri, 24 Jun 2011 12:29:02 +0000 Received: from localhost ([127.0.0.1] helo=canuck.infradead.org) by canuck.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1Qa5VO-0004Tp-7D; Fri, 24 Jun 2011 12:29:02 +0000 Received: from casper.infradead.org ([2001:770:15f::2]) by canuck.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1Qa5VL-0004Tk-6d for linux-arm-kernel@canuck.infradead.org; Fri, 24 Jun 2011 12:28:59 +0000 Received: from mail-qw0-f49.google.com ([209.85.216.49]) by casper.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1Qa5VC-0007hI-Ng for linux-arm-kernel@lists.infradead.org; Fri, 24 Jun 2011 12:28:51 +0000 Received: by qwi2 with SMTP id 2so1632818qwi.36 for ; Fri, 24 Jun 2011 05:27:59 -0700 (PDT) MIME-Version: 1.0 Received: by 10.224.196.199 with SMTP id eh7mr586051qab.231.1308918479333; Fri, 24 Jun 2011 05:27:59 -0700 (PDT) Received: by 10.224.60.71 with HTTP; Fri, 24 Jun 2011 05:27:59 -0700 (PDT) In-Reply-To: References: <1308567752-13451-1-git-send-email-thomas.abraham@linaro.org> <1308567752-13451-3-git-send-email-thomas.abraham@linaro.org> Date: Fri, 24 Jun 2011 17:57:59 +0530 Message-ID: Subject: Re: [PATCH 2/6] serial: samsung: Add device tree support for s5pv210 uart driver From: Thomas Abraham To: Grant Likely X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20110624_132851_125116_5DC069DD X-CRM114-Status: GOOD ( 36.00 ) X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2-r929478 on casper.infradead.org summary: Content analysis details: (-2.6 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.216.49 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: linux-samsung-soc@vger.kernel.org, linaro-dev@lists.linaro.org, patches@linaro.org, devicetree-discuss@lists.ozlabs.org, kgene.kim@samsung.com, ben-linux@fluff.org, Thomas Gleixner , linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter2.kernel.org [140.211.167.43]); Fri, 24 Jun 2011 12:29:40 +0000 (UTC) X-MIME-Autoconverted: from quoted-printable to 8bit by demeter2.kernel.org id p5OCTJTU017089 Hi Grant, On 24 June 2011 01:38, Grant Likely 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(-) extern int of_device_is_available(const struct device_node *device); Thanks, Thomas. 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 *);