OMAP: hwmod: Fix the addr spaces count API.

PATCH Sricharan R 3 participants 4 messages Sep 8, 2011 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sricharan R

author

2011-09-08 09:15 UTC #1 lore raw

Hi Benoit,

Quoted 9 lines

-----Original Message-----
From: Sricharan R [mailto:[email protected]]
Sent: Thursday, September 08, 2011 2:35 PM
To: Sricharan R
Subject: Re: [PATCH 1/8] OMAP: hwmod: Fix the addr spaces count API.
Hi Sricharan,
On 9/8/2011 7:22 AM, Shilimkar, Santosh wrote:

Quoted 8 lines

Quoted 5 lines
From: sricharan<[email protected]>
The address space count API returns the number of address space entries for a hwmod including a additional null value present in the address space structure introduced recently.
That's a minor nit, but you might give the commit you are referencing here.

Ok i will mention the below in detail and resend.
commit 78183f3fdf76f422431a81852468be01b36db325

Quoted 6 lines

Quoted 5 lines
The devices which
have multiple hwmods and use device_build_ss are broken with this, as their address resources are populated with a extra null value, subsequently the probe fails. So fix the API not to add the null value.

Quoted 8 lines

It seems that in every cases, we are adding an extra null resource for nothing. But it is true that will not crash if the driver is just expecting an unique entry.
What is happening with multiple hwmods is probably the introduction of that extra null resource in the middle of the real ones, hence shifting the resource index?
You might give more details here.

That is exactly correct.
So because of the introduction of the null structure in the middle,
the subsequent resource indexes gets shifted by one. So the
original index used by the drivers in those places would return a NULL resource now, hence the problem.

Quoted 29 lines

Review
Quoted 11 lines
Signed-off-by sricharan<[email protected]>
Signed-off-by Santosh Shilimkar<[email protected]>
Cc Benoit Cousson<[email protected]>
Cc Paul Walmsley<[email protected]>
Cc Kevin Hilman<[email protected]>
---
  arch/arm/mach-omap2/omap_hwmod.c |    8 +++++---
  1 files changed, 5 insertions(+), 3 deletions(-)
(quoted diff) +0 -0
b/arch/arm/mach-omap2/omap_hwmod.c
arch/arm/mach-omap2/omap_hwmod.c static int _count_ocp_if_addr_spaces(struct
Quoted 4 lines
index 84cc0bd..32a0f48a 100644
arch/arm/mach-omap2/omap_hwmod.c +0 -0
@@ -791,9 +791,11 @@ static int _count_ocp_if_addr_spaces(struct
omap_hwmod_ocp_if *os)
Quoted 12 lines
arch/arm/mach-omap2/omap_hwmod.c +5 -3
  	if (!os || !os->addr)
  		return 0;
 
-	do {
-		mem =&os->addr[i++];
-	} while (mem->pa_start != mem->pa_end);
+	mem =&os->addr[i];
+
+	while (mem->pa_start != mem->pa_end) {
+		mem =&os->addr[++i];
+	};
 

Quoted 3 lines

arch/arm/mach-omap2/omap_hwmod.c static int _count_ocp_if_addr_spaces(struct · line 800
Quoted 1 line
arch/arm/mach-omap2/omap_hwmod.c +0 -0
  	return i;
Cannot you just do "return i - 1"?

Right. That was the first idea.
But after some discussion concluded that right way is to, not to take in to account, the null structure itself, rather than incrementing and decrementing.

Quoted 3 lines

Regards,
Benoit

Thanks,
Sricharan

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Santosh

2011-09-08 09:32 UTC #2 lore raw

Quoted 8 lines · Sricharan R

Hi Benoit,
Quoted 6 lines
-----Original Message-----
From: Sricharan R [mailto:[email protected]]
Sent: Thursday, September 08, 2011 2:35 PM
To: Sricharan R
Subject: Re: [PATCH 1/8] OMAP: hwmod: Fix the addr spaces count API.

[...]

Quoted 8 lines

Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +0 -0
b/arch/arm/mach-omap2/omap_hwmod.c
arch/arm/mach-omap2/omap_hwmod.c static int _count_ocp_if_addr_spaces(struct
Quoted 4 lines
index 84cc0bd..32a0f48a 100644
arch/arm/mach-omap2/omap_hwmod.c +0 -0
@@ -791,9 +791,11 @@ static int _count_ocp_if_addr_spaces(struct
omap_hwmod_ocp_if *os)

Quoted 8 lines

Quoted 8 lines
arch/arm/mach-omap2/omap_hwmod.c static int _count_ocp_if_addr_spaces(struct · line 795
Quoted 8 lines
arch/arm/mach-omap2/omap_hwmod.c +2 -3
   	if (!os || !os->addr)
   		return 0;
 
-	do {
-		mem =&os->addr[i++];
-	} while (mem->pa_start != mem->pa_end);
+	mem =&os->addr[i];
+

This extran line isn't needed Sricharan.

Quoted 8 lines

Quoted 7 lines
arch/arm/mach-omap2/omap_hwmod.c static int _count_ocp_if_addr_spaces(struct · line 800
Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod.c +3 -0
+	while (mem->pa_start != mem->pa_end) {
+		mem =&os->addr[++i];
+	};
 
   	return i;
Cannot you just do "return i - 1"?

Quoted 6 lines

Right. That was the first idea.
But after some discussion concluded that right way is to, not to take in to account, the null structure itself, rather than incrementing and decrementing.

I was the one who objected to the i-1 or i--
change. The count logic itself should
handle all the scenario's.

Regards
Santosh

Sricharan R

author

2011-09-08 13:03 UTC #3 lore raw

[...]

Quoted 20 lines

Quoted 6 lines
Quoted 6 lines
-----Original Message-----
From: Sricharan R [mailto:[email protected]]
Sent: Thursday, September 08, 2011 2:35 PM
To: Sricharan R
Subject: Re: [PATCH 1/8] OMAP: hwmod: Fix the addr spaces count API.
[...]
Quoted 11 lines
Quoted 11 lines
Review
Quoted 2 lines
(quoted diff) +0 -0
b/arch/arm/mach-omap2/omap_hwmod.c
arch/arm/mach-omap2/omap_hwmod.c static int _count_ocp_if_addr_spaces(struct
Quoted 4 lines
index 84cc0bd..32a0f48a 100644
arch/arm/mach-omap2/omap_hwmod.c +0 -0
@@ -791,9 +791,11 @@ static int _count_ocp_if_addr_spaces(struct
omap_hwmod_ocp_if *os)
Quoted 3 lines
arch/arm/mach-omap2/omap_hwmod.c +0 -0
   	if (!os || !os->addr)
   		return 0;
 

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod.c static int _count_ocp_if_addr_spaces(struct · line 795
Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod.c +2 -3
-	do {
-		mem =&os->addr[i++];
-	} while (mem->pa_start != mem->pa_end);
+	mem =&os->addr[i];
+
This extran line isn't needed Sricharan.

Will correct this, along with benoit's commmets.

Thanks,
Sricharan

Cousson, Benoit

2011-09-08 15:06 UTC #4 lore raw

In reply to Santosh

Quoted 44 lines · Santosh

Quoted 8 lines · Sricharan R
Hi Benoit,
Quoted 6 lines
-----Original Message-----
From: Sricharan R [mailto:[email protected]]
Sent: Thursday, September 08, 2011 2:35 PM
To: Sricharan R
Subject: Re: [PATCH 1/8] OMAP: hwmod: Fix the addr spaces count API.
[...]
Quoted 8 lines
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +0 -0
b/arch/arm/mach-omap2/omap_hwmod.c
arch/arm/mach-omap2/omap_hwmod.c static int _count_ocp_if_addr_spaces(struct
Quoted 4 lines
index 84cc0bd..32a0f48a 100644
arch/arm/mach-omap2/omap_hwmod.c +0 -0
@@ -791,9 +791,11 @@ static int _count_ocp_if_addr_spaces(struct
omap_hwmod_ocp_if *os)
Quoted 8 lines
Quoted 8 lines
arch/arm/mach-omap2/omap_hwmod.c static int _count_ocp_if_addr_spaces(struct · line 795
Quoted 8 lines
arch/arm/mach-omap2/omap_hwmod.c +2 -3
    	if (!os || !os->addr)
    		return 0;
 
-	do {
-		mem =&os->addr[i++];
-	} while (mem->pa_start != mem->pa_end);
+	mem =&os->addr[i];
+
This extran line isn't needed Sricharan.
Quoted 8 lines
Quoted 7 lines
arch/arm/mach-omap2/omap_hwmod.c static int _count_ocp_if_addr_spaces(struct · line 800
Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod.c +3 -0
+	while (mem->pa_start != mem->pa_end) {
+		mem =&os->addr[++i];
+	};
 
    	return i;
Cannot you just do "return i - 1"?
Quoted 6 lines
Right. That was the first idea.
But after some discussion concluded that right way is to, not to take in to account, the null structure itself, rather than incrementing and decrementing.

Quoted 3 lines

I was the one who objected to the i-1 or i--
change. The count logic itself should
handle all the scenario's.

But this is the case even with the i-1.
That's a details, but it looks to me nicer to do a return i-1 to highlight the extra empty entry that is added, instead of doing twice "mem =&os->addr[i];" The loop is counting the real number of entries, but you return only the relevant number by removing the terminator.

BTW, don't we have the same issue with _count_sdma_reqs and _count_mpu_irqs?

Regards,
Benoit