[PATCH] pinctrl: bcm2835: make use of of_property_read_u32_index()

[PATCH] pinctrl: bcm2835: make use of of_property_read_u32_index()

[Stephen Warren]

Use the new standard API of_property_read_u32_index() instead of open-
coding it.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
Note: This depends on the proposed patch "of: Add support for reading
a u32 from a multi-value property" by Tony Prisk.

BTW, I realized why I didn't create that standard API, but wrote custom
prop_u32() instead; the code has already looked up the properties, and
validated their length, so prop_u32() can simply index into the property
data, whereas of_property_read_u32_index() needs to go search for the
property and re-validate it every time, so there's a bunch more overhead.
It also means duplicating the property name, although I could use a
define for that. I'm not entirely convinced that using this standard API
is a win in this case. LinusW, Tony, what do you think?
---
 drivers/pinctrl/pinctrl-bcm2835.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-bcm2835.c b/drivers/pinctrl/pinctrl-bcm2835.c
index f28d4b0..c8f20a3 100644
--- a/drivers/pinctrl/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/pinctrl-bcm2835.c
@@ -699,11 +699,6 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc,
 	return 0;
 }
 
-static inline u32 prop_u32(struct property *p, int i)
-{
-	return be32_to_cpup(((__be32 *)p->value) + i);
-}
-
 static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		struct device_node *np,
 		struct pinctrl_map **map, unsigned *num_maps)
@@ -761,7 +756,9 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		return -ENOMEM;
 
 	for (i = 0; i < num_pins; i++) {
-		pin = prop_u32(pins, i);
+		err = of_property_read_u32_index(np, "brcm,pins", i, &pin);
+		if (err)
+			goto out;
 		if (pin >= ARRAY_SIZE(bcm2835_gpio_pins)) {
 			dev_err(pc->dev, "%s: invalid brcm,pins value %d\n",
 				of_node_full_name(np), pin);
@@ -770,14 +767,20 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		}
 
 		if (num_funcs) {
-			func = prop_u32(funcs, (num_funcs > 1) ? i : 0);
+			err = of_property_read_u32_index(np, "brcm,function",
+					(num_funcs > 1) ? i : 0, &func);
+			if (err)
+				goto out;
 			err = bcm2835_pctl_dt_node_to_map_func(pc, np, pin,
 							func, &cur_map);
 			if (err)
 				goto out;
 		}
 		if (num_pulls) {
-			pull = prop_u32(pulls, (num_pulls > 1) ? i : 0);
+			err = of_property_read_u32_index(np, "brcm,pull",
+					(num_funcs > 1) ? i : 0, &pull);
+			if (err)
+				goto out;
 			err = bcm2835_pctl_dt_node_to_map_pull(pc, np, pin,
 							pull, &cur_map);
 			if (err)
-- 
1.7.10.4

Re: [PATCH] pinctrl: bcm2835: make use of of_property_read_u32_index()

[Tony Prisk]

On Wed, 2013-03-27 at 23:09 -0600, Stephen Warren wrote:
> Use the new standard API of_property_read_u32_index() instead of open-
> coding it.
> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> Note: This depends on the proposed patch "of: Add support for reading
> a u32 from a multi-value property" by Tony Prisk.
> 
> BTW, I realized why I didn't create that standard API, but wrote custom
> prop_u32() instead; the code has already looked up the properties, and
> validated their length, so prop_u32() can simply index into the property
> data, whereas of_property_read_u32_index() needs to go search for the
> property and re-validate it every time, so there's a bunch more overhead.
> It also means duplicating the property name, although I could use a
> define for that. I'm not entirely convinced that using this standard API
> is a win in this case. LinusW, Tony, what do you think?
> ---

When I was writing the function I had a similar thought about the fact
we need to work out the length first, which as you mentioned, requires
all the prior work on the property anyway.

I didn't bring it up, because I thought someone might say 'hey, you
should add a function to get the count as well' :)

In both the brcm and vt8500 use cases, we will have the issue of
multiple lookups on the same property using 'generic' functions. Price
we have to pay for generic code?

Regards
Tony P

Re: [PATCH] pinctrl: bcm2835: make use of of_property_read_u32_index()

[Linus Walleij]

On Thu, Mar 28, 2013 at 7:18 AM, Tony Prisk <linux@prisktech.co.nz> wrote:
> On Wed, 2013-03-27 at 23:09 -0600, Stephen Warren wrote:
>> Use the new standard API of_property_read_u32_index() instead of open-
>> coding it.
>>
>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
>> Note: This depends on the proposed patch "of: Add support for reading
>> a u32 from a multi-value property" by Tony Prisk.
>>
>> BTW, I realized why I didn't create that standard API, but wrote custom
>> prop_u32() instead; the code has already looked up the properties, and
>> validated their length, so prop_u32() can simply index into the property
>> data, whereas of_property_read_u32_index() needs to go search for the
>> property and re-validate it every time, so there's a bunch more overhead.
>> It also means duplicating the property name, although I could use a
>> define for that. I'm not entirely convinced that using this standard API
>> is a win in this case. LinusW, Tony, what do you think?
>> ---
>
> When I was writing the function I had a similar thought about the fact
> we need to work out the length first, which as you mentioned, requires
> all the prior work on the property anyway.
>
> I didn't bring it up, because I thought someone might say 'hey, you
> should add a function to get the count as well' :)
>
> In both the brcm and vt8500 use cases, we will have the issue of
> multiple lookups on the same property using 'generic' functions. Price
> we have to pay for generic code?

My take on this is that if you look in for example:
drivers/of/platform.c you find that for every node in the device
tree when it creates a platform device with a resource it will
for example call of_address_to_resource() twice. The same
for IRQs IIRC.

So basically this behaviour is inherent in all OF/DT code,
and no point trying to work around it.

I don't know if there is a way to fix the problem at the root,
like have the DT parser annotate nodes with the number of
resources of each type (like a decorated tree) so it can be
looked up quickly somehow. I'm not smart enough on DT for
such things...

Yours,
Linus Walleij