Message ID | 1357137512-8618-1-git-send-email-alexey.brodkin@gmail.com |
---|---|
State | Superseded |
Delegated to: | Wolfgang Denk |
Headers | show |
The reason for latter modification is to make code more readable. Do you think if I may submit this as a cosmetic change separately? 2013/1/2 Michal Simek <monstr@monstr.eu> > 2013/1/2 Alexey Brodkin <alexey.brodkin@gmail.com>: > > From: Alexey Brodkin <abrodkin@synopsys.com> > > > > 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 <abrodkin@synopsys.com> > > --- > > drivers/block/systemace.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/block/systemace.c b/drivers/block/systemace.c > > index 247cf06..88561a7 100644 > > --- a/drivers/block/systemace.c > > +++ b/drivers/block/systemace.c > > @@ -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 > > > > 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 >
Actually I meant to respin both patches. 1. Only add "else" thread for "ace_writew", keeping "ace_readw" unchanged 2. Replace "in16"/"out16" with "readw"/"writew" 2013/1/2 Michal Simek <monstr@monstr.eu> > Hi, > > 2013/1/2 Алексей Бродкин <alexey.brodkin@gmail.com>: > > 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 >
Dear Alexey Brodkin, In message <1357137512-8618-1-git-send-email-alexey.brodkin@gmail.com> you wrote: > From: Alexey Brodkin <abrodkin@synopsys.com> > > 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? Best regards, Wolfgang Denk
Hi Wolfgang, 2013/1/2 Wolfgang Denk <wd@denx.de>: > Dear Alexey Brodkin, > > In message <1357137512-8618-1-git-send-email-alexey.brodkin@gmail.com> you wrote: >> From: Alexey Brodkin <abrodkin@synopsys.com> >> >> 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? 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) static void ace_writew(u16 val, unsigned off) { if (width == 8) { #if !defined(__BIG_ENDIAN) writeb(val >> 8, base + off); writeb(val, base + off + 1); #else writeb(val, base + off); writeb(val >> 8, base + off + 1); #endif } <----------------------- missing else here writew(base + off, val); } For ace_readw is situation different because it always returns value for width 8 that's why it never reach 16bit read. Thanks, Michal
Dear Michal Simek, In message <CAHTX3d+shCmUjaXzS0oS5pT8a89cyA9UKemc+oRk5xk6qRNrdQ@mail.gmail.com> you wrote: > > > 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) I see. Well, I think the commit message is not really clear. I definitely misunderstood it. Maybe it can be improved? Thanks. Best regards, Wolfgang Denk
2013/1/3 Wolfgang Denk <wd@denx.de>: > Dear Michal Simek, > > In message <CAHTX3d+shCmUjaXzS0oS5pT8a89cyA9UKemc+oRk5xk6qRNrdQ@mail.gmail.com> you wrote: >> >> > 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) > > 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); } Thanks, Michal
Dear Michal Simek, In message <CAHTX3dLGmpBLxwYPdK2Fju4J7t8i+55L3h27O2HcxxcvJW-bxA@mail.gmail.com> you wrote: > > > I see. Well, I think the commit message is not really clear. I > > definitely misunderstood it. Maybe it can be improved? > > Yep. Thanks. > 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); > } 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. Best regards, Wolfgang Denk
2013/1/3 Wolfgang Denk <wd@denx.de>: > Dear Michal Simek, > > In message <CAHTX3dLGmpBLxwYPdK2Fju4J7t8i+55L3h27O2HcxxcvJW-bxA@mail.gmail.com> you wrote: >> >> > I see. Well, I think the commit message is not really clear. I >> > definitely misunderstood it. Maybe it can be improved? >> >> Yep. > > Thanks. > >> 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); >> } > > 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. For me too. Thanks, Michal
Dear Alexey Brodkin, In message <1357137512-8618-1-git-send-email-alexey.brodkin@gmail.com> you wrote: > From: Alexey Brodkin <abrodkin@synopsys.com> > > 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 <abrodkin@synopsys.com> > --- > drivers/block/systemace.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/systemace.c b/drivers/block/systemace.c > index 247cf06..88561a7 100644 > --- a/drivers/block/systemace.c > +++ b/drivers/block/systemace.c > @@ -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. > 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
Hi Wolfgang, I've already sent V2 patch with only 1 chunk and updated comments to the mailing list. -Alexey 03.01.2013 16:00 пользователь "Wolfgang Denk" <wd@denx.de> написал: > Dear Alexey Brodkin, > > In message <1357137512-8618-1-git-send-email-alexey.brodkin@gmail.com> > you wrote: > > From: Alexey Brodkin <abrodkin@synopsys.com> > > > > 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 <abrodkin@synopsys.com> > > --- > > drivers/block/systemace.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/block/systemace.c b/drivers/block/systemace.c > > index 247cf06..88561a7 100644 > > --- a/drivers/block/systemace.c > > +++ b/drivers/block/systemace.c > > @@ -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. > > > 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@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_ >
diff --git a/drivers/block/systemace.c b/drivers/block/systemace.c index 247cf06..88561a7 100644 --- a/drivers/block/systemace.c +++ b/drivers/block/systemace.c @@ -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); } 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); } static unsigned long systemace_read(int dev, unsigned long start,