Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752755Ab2KKOMx (ORCPT ); Sun, 11 Nov 2012 09:12:53 -0500 Received: from mail-ob0-f174.google.com ([209.85.214.174]:42973 "EHLO mail-ob0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751346Ab2KKOMv (ORCPT ); Sun, 11 Nov 2012 09:12:51 -0500 Message-ID: <509FB25F.3030307@gmail.com> Date: Sun, 11 Nov 2012 08:12:47 -0600 From: Rob Herring User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121028 Thunderbird/16.0.2 MIME-Version: 1.0 To: viresh kumar CC: rob.herring@calxeda.com, grant.likely@secretlab.ca, linaro-dev@lists.linaro.org, andriy.shevchenko@intel.com, patches@linaro.org, devicetree-discuss@lists.ozlabs.org, spear-devel@list.st.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays References: <64c5278ebdec503f83e9b7002bf13affb7f3260f.1351225085.git.viresh.kumar@linaro.org> <50991C41.50705@gmail.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4840 Lines: 124 On 11/06/2012 10:22 PM, viresh kumar wrote: > On Tue, Nov 6, 2012 at 7:48 PM, Rob Herring 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 > 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, > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/