drivers/block/systemace: fixes read/write words for bus width = 8

From: Alexey Brodkin <[email protected]>

Current implementation works fine for bus width = 16 bits because we never get into "if" branch.

If one sets width to 8 bits there will be 2 consequent data accesses (read/write):
1. Correct data access for 8-bit bus
2. Unconditional (and in this case incorrect) data access as if data bus is 16-bit wide

Signed-off-by Alexey Brodkin <[email protected]>

---
 drivers/block/systemace.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/block/systemace.c +0 -0

index 247cf06..88561a7 100644

2013/1/2 Alexey Brodkin <[email protected]>:

Quoted 21 lines

From: Alexey Brodkin <[email protected]>

Current implementation works fine for bus width = 16 bits because we never get into "if" branch.

If one sets width to 8 bits there will be 2 consequent data accesses (read/write):
1. Correct data access for 8-bit bus
2. Unconditional (and in this case incorrect) data access as if data bus is 16-bit wide

Signed-off-by Alexey Brodkin <[email protected]>

---
 drivers/block/systemace.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/block/systemace.c +0 -0

index 247cf06..88561a7 100644

drivers/block/systemace.c static void ace_writew(u16 val, unsigned off) · line 71

Quoted 7 lines

drivers/block/systemace.c +2 -1

@@ -66,7 +66,8 @@ static void ace_writew(u16 val, unsigned off)
                writeb(val >> 8, base + off + 1);
 #endif
        }
-       out16(base + off, val);
+       else
+               out16(base + off, val);
 }

yes

drivers/block/systemace.c static u16 ace_readw(unsigned off) · line 84

Quoted 10 lines

drivers/block/systemace.c +2 -2

 static u16 ace_readw(unsigned off)

@@ -78,8 +79,8 @@ static u16 ace_readw(unsigned off)
                return readb(base + off) | (readb(base + off + 1) << 8);
 #endif
        }
-
-       return in16(base + off);
+       else
+               return in16(base + off);
 }

No need to use it - because for width=8 it will return above.

M

--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian

The reason for latter modification is to make code more readable. Do you think if I may submit this as a cosmetic change separately?

Hi,

2013/1/2 ??????? ??????? <[email protected]>:

Quoted 2 lines

The reason for latter modification is to make code more readable. Do you think if I may submit this as a cosmetic change separately?

If you mean create 3rd patch then no.

Thanks,
Michal

--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian

From: Alexey Brodkin <[email protected]>

Current implementation works fine for bus width = 16 bits because we never get into "if" branch.

If one sets width to 8 bits there will be 2 consequent data accesses (read/write):
1. Correct data access for 8-bit bus
2. Unconditional (and in this case incorrect) data access as if data bus is 16-bit wide

Dear Alexey Brodkin,

Quoted 10 lines · In message <[email protected]> you…

From: Alexey Brodkin <[email protected]>

Current implementation works fine for bus width = 16 bits because we never get into "if" branch.

If one sets width to 8 bits there will be 2 consequent data accesses (read/write):
1. Correct data access for 8-bit bus
2. Unconditional (and in this case incorrect) data access as if data bus is 16-bit wide

Sorry, I don't get this.

You say, if one sets width to 8 bits, then the code fails when the bus is actually 16 bits wide?

But this is just a bad configuration then - why do we need to change the code? All that is needed is to run with a correct configuration?

Quoted 3 lines

You say, if one sets width to 8 bits, then the code fails when the bus is actually 16 bits wide?

Here is that function with my comment about missing "else". In ace_writew function we shouldn't write 16bit value when width is 8. (It probably doesn't break anything but it is not correct - I will check it on the HW)

Dear Michal Simek,

Quoted 3 lines · In message <CAHTX3d+shCmUjaXzS0oS5pT8a89cyA9UKemc+oRk5xk6qRNrdQ@mail.gmail.com>…

Quoted 3 lines

You say, if one sets width to 8 bits, then the code fails when the bus is actually 16 bits wide?

...

Quoted 4 lines

Here is that function with my comment about missing "else". In ace_writew function we shouldn't write 16bit value when width is 8. (It probably doesn't break anything but it is not correct - I will check it on the HW)

I see. Well, I think the commit message is not really clear. I definitely misunderstood it. Maybe it can be improved?

Quoted 3 lines

I see. Well, I think the commit message is not really clear. I definitely misunderstood it. Maybe it can be improved?

Yep.

btw: is there any coding style rule about this second function

Current (simplified) case.
static u16 ace_readw(unsigned off)

{
	if (width == 8)
		return readb(base + off) | (readb(base + off + 1) << 8);

	return readw(base + off);
}

or (with else)

static u16 ace_readw(unsigned off)

{
	if (width == 8)
		return readb(base + off) | (readb(base + off + 1) << 8);
        else
        	return readw(base + off);
}

Dear Michal Simek,

Quoted 5 lines · In message <CAHTX3dLGmpBLxwYPdK2Fju4J7t8i+55L3h27O2HcxxcvJW-bxA@mail.gmail.com>…

Quoted 3 lines

I see. Well, I think the commit message is not really clear. I definitely misunderstood it. Maybe it can be improved?

Yep.

Thanks.

Quoted 13 lines

btw: is there any coding style rule about this second function

Current (simplified) case.
static u16 ace_readw(unsigned off)

{
      if (width == 8)
              return readb(base + off) | (readb(base + off + 1) << 8);

      return readw(base + off);
}

or (with else)

Quoted 7 lines

static u16 ace_readw(unsigned off)

{
      if (width == 8)
              return readb(base + off) | (readb(base + off + 1) << 8);
        else
              return readw(base + off);
}

I am not aware of a CodingStyle rule; in this example there is also not much difference due to the simplicity of the code (note however that indentation of the second return is incorrect in the second example).

My personal preference is the first example, as it uses minimal nesting; I feel this is easier to read.

From: Alexey Brodkin <[email protected]>

Current implementation works fine for bus width = 16 bits because we never get into "if" branch.

If one sets width to 8 bits there will be 2 consequent data accesses (read/write):
1. Correct data access for 8-bit bus
2. Unconditional (and in this case incorrect) data access as if data bus is 16-bit wide

Signed-off-by Alexey Brodkin <[email protected]>

---
 drivers/block/systemace.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/block/systemace.c +0 -0

index 247cf06..88561a7 100644

Dear Alexey Brodkin,

In message <[email protected]>

Quoted 21 lines · you

From: Alexey Brodkin <[email protected]>

Current implementation works fine for bus width = 16 bits because we never get into "if" branch.

If one sets width to 8 bits there will be 2 consequent data accesses (read/write):
1. Correct data access for 8-bit bus
2. Unconditional (and in this case incorrect) data access as if data bus is 16-bit wide

Signed-off-by Alexey Brodkin <[email protected]>

---
 drivers/block/systemace.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/block/systemace.c +0 -0

index 247cf06..88561a7 100644

drivers/block/systemace.c static void ace_writew(u16 val, unsigned off) · line 71

Quoted 7 lines

drivers/block/systemace.c +2 -1

@@ -66,7 +66,8 @@ static void ace_writew(u16 val, unsigned off)
              writeb(val >> 8, base + off + 1);
 #endif
      }
-     out16(base + off, val);
+     else
+             out16(base + off, val);
 }

I agree with this first chunk.

drivers/block/systemace.c static u16 ace_readw(unsigned off) · line 84

Quoted 10 lines

drivers/block/systemace.c +2 -2

 static u16 ace_readw(unsigned off)

@@ -78,8 +79,8 @@ static u16 ace_readw(unsigned off)
              return readb(base + off) | (readb(base + off + 1) << 8);
 #endif
      }
-
-     return in16(base + off);
+     else
+             return in16(base + off);
 }

But please drop this second one; the "else" is not necessary and adds only unneeded nesting.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de If programming was easy, they wouldn't need something as complicated as a human being to do it, now would they?
- L. Wall & R. L. Schwartz, _Programming Perl_