[U-Boot] [PATCH] Davinci I2C code cleanup
Removed CHECK_NACK macro from Davinci I2C driver for code cleanup.
Signed-off-by: Sergey Kubushyn ksi@koi8.net --- cpu/arm926ejs/davinci/i2c.c | 62 +++++++++++++++++++++++++--------- 1 files changed, 45 insertions(+), 17 deletions(-)
diff --git a/cpu/arm926ejs/davinci/i2c.c b/cpu/arm926ejs/davinci/i2c.c index 3ba20ef..f48aae2 100644 --- a/cpu/arm926ejs/davinci/i2c.c +++ b/cpu/arm926ejs/davinci/i2c.c @@ -32,14 +32,6 @@ #include <asm/arch/hardware.h> #include <asm/arch/i2c_defs.h>
-#define CHECK_NACK() \ - do {\ - if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) {\ - REG(I2C_CON) = 0;\ - return(1);\ - }\ - } while (0) -
static int wait_for_bus(void) { @@ -179,7 +171,11 @@ int i2c_read(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len)
tmp = poll_i2c_irq(I2C_STAT_XRDY | I2C_STAT_NACK);
- CHECK_NACK(); + /* Check for NACK */ + if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) { + REG(I2C_CON) = 0; + return(1); + }
switch (alen) { case 2: @@ -193,7 +189,11 @@ int i2c_read(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len)
tmp = poll_i2c_irq(I2C_STAT_XRDY | I2C_STAT_NACK);
- CHECK_NACK(); + /* Check for NACK */ + if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) { + REG(I2C_CON) = 0; + return(1); + } /* No break, fall through */ case 1: /* Send address LSByte */ @@ -206,7 +206,11 @@ int i2c_read(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len)
tmp = poll_i2c_irq(I2C_STAT_XRDY | I2C_STAT_NACK | I2C_STAT_ARDY);
- CHECK_NACK(); + /* Check for NACK */ + if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) { + REG(I2C_CON) = 0; + return(1); + }
if (!(tmp & I2C_STAT_ARDY)) { REG(I2C_CON) = 0; @@ -224,7 +228,11 @@ int i2c_read(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len) for (i = 0; i < len; i++) { tmp = poll_i2c_irq(I2C_STAT_RRDY | I2C_STAT_NACK | I2C_STAT_ROVR);
- CHECK_NACK(); + /* Check for NACK */ + if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) { + REG(I2C_CON) = 0; + return(1); + }
if (tmp & I2C_STAT_RRDY) { buf[i] = REG(I2C_DRR); @@ -236,7 +244,11 @@ int i2c_read(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len)
tmp = poll_i2c_irq(I2C_STAT_SCD | I2C_STAT_NACK);
- CHECK_NACK(); + /* Check for NACK */ + if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) { + REG(I2C_CON) = 0; + return(1); + }
if (!(tmp & I2C_STAT_SCD)) { REG(I2C_CON) = 0; @@ -279,7 +291,11 @@ int i2c_write(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len) /* Send address MSByte */ tmp = poll_i2c_irq(I2C_STAT_XRDY | I2C_STAT_NACK);
- CHECK_NACK(); + /* Check for NACK */ + if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) { + REG(I2C_CON) = 0; + return(1); + }
if (tmp & I2C_STAT_XRDY) { REG(I2C_DXR) = (addr >> 8) & 0xff; @@ -292,7 +308,11 @@ int i2c_write(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len) /* Send address LSByte */ tmp = poll_i2c_irq(I2C_STAT_XRDY | I2C_STAT_NACK);
- CHECK_NACK(); + /* Check for NACK */ + if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) { + REG(I2C_CON) = 0; + return(1); + }
if (tmp & I2C_STAT_XRDY) { REG(I2C_DXR) = addr & 0xff; @@ -305,7 +325,11 @@ int i2c_write(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len) for (i = 0; i < len; i++) { tmp = poll_i2c_irq(I2C_STAT_XRDY | I2C_STAT_NACK);
- CHECK_NACK(); + /* Check for NACK */ + if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) { + REG(I2C_CON) = 0; + return(1); + }
if (tmp & I2C_STAT_XRDY) { REG(I2C_DXR) = buf[i]; @@ -316,7 +340,11 @@ int i2c_write(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len)
tmp = poll_i2c_irq(I2C_STAT_SCD | I2C_STAT_NACK);
- CHECK_NACK(); + /* Check for NACK */ + if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) { + REG(I2C_CON) = 0; + return(1); + }
if (!(tmp & I2C_STAT_SCD)) { REG(I2C_CON) = 0;
On 13:52 Thu 19 Feb , ksi@koi8.net wrote:
Removed CHECK_NACK macro from Davinci I2C driver for code cleanup.
Signed-off-by: Sergey Kubushyn ksi@koi8.net
cpu/arm926ejs/davinci/i2c.c | 62 +++++++++++++++++++++++++--------- 1 files changed, 45 insertions(+), 17 deletions(-)
NACK
please explain why IMHO duplicate code is really wrong
Best Regards, J.
Dear Jean-Christophe PLAGNIOL-VILLARD,
In message 20090222124436.GA9867@game.jcrosoft.org you wrote:
On 13:52 Thu 19 Feb , ksi@koi8.net wrote:
Removed CHECK_NACK macro from Davinci I2C driver for code cleanup.
Signed-off-by: Sergey Kubushyn ksi@koi8.net
cpu/arm926ejs/davinci/i2c.c | 62 +++++++++++++++++++++++++--------- 1 files changed, 45 insertions(+), 17 deletions(-)
NACK
please explain why
Please see previous discussion about multibus I2C support.
IMHO duplicate code is really wrong
I agree. The macro should indeed NOT be deleted, but it needs fixing. It is magically accessing the local variable "tmp" which [Quoting the CodingStyle] "...might look like a good thing, but it's confusing as hell when one reads the code and it's prone to breakage from seemingly innocent changes."
I suggest that "tmp" gets passed as argument to the macro.
Best regards,
Wolfgang Denk
On Sun, 22 Feb 2009, Wolfgang Denk wrote:
Dear Jean-Christophe PLAGNIOL-VILLARD,
In message 20090222124436.GA9867@game.jcrosoft.org you wrote:
On 13:52 Thu 19 Feb , ksi@koi8.net wrote:
Removed CHECK_NACK macro from Davinci I2C driver for code cleanup.
Signed-off-by: Sergey Kubushyn ksi@koi8.net
cpu/arm926ejs/davinci/i2c.c | 62 +++++++++++++++++++++++++--------- 1 files changed, 45 insertions(+), 17 deletions(-)
NACK
please explain why
Please see previous discussion about multibus I2C support.
IMHO duplicate code is really wrong
I agree. The macro should indeed NOT be deleted, but it needs fixing. It is magically accessing the local variable "tmp" which [Quoting the CodingStyle] "...might look like a good thing, but it's confusing as hell when one reads the code and it's prone to breakage from seemingly innocent changes."
I suggest that "tmp" gets passed as argument to the macro.
It is _NOT_ a macro working on some variable. It is simply repeating _LITERAL_ text that not only accesses a local variable tmp but even performs a function return with an error if condition is not met. That is, why, BTW, it should not be made into an inline function because it would require additional checking of its return code where it is used (lot of places in Davinci I2C driver) that, in turn, would've added a lot of unnecessary overhead for local variable, register loads, second conditional checking etc.
It is _NOT_ supposed to be any function-like macro. It is just a fragment of _LITERAL_ text.
I don't think it is a right thing to blindly apply "Coding Style" to everything no matter if it applies to it or not.
I also NACK this :)
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902221346290.5031@home-gw.koi8.net you wrote:
I suggest that "tmp" gets passed as argument to the macro.
It is _NOT_ a macro working on some variable. It is simply repeating
Oh yes, of course it is. It references the variable "tmp", and this is nasty as it is not clear to the reader of the code.
If I think it would be better to rename "tmp" into "foo" the code will magically break.
_LITERAL_ text that not only accesses a local variable tmp but even performs a function return with an error if condition is not met. That is,
You are right. That's even worse.
why, BTW, it should not be made into an inline function because it would require additional checking of its return code where it is used (lot of places in Davinci I2C driver) that, in turn, would've added a lot of unnecessary overhead for local variable, register loads, second conditional checking etc.
I am sure you could come up with an efficient *and* clean solution - if you really wanted to try.
It is _NOT_ supposed to be any function-like macro. It is just a fragment of _LITERAL_ text.
The code as is is not acceptable and needs to be cleaned up.
Best regards,
Wolfgang Denk
On Sun, 22 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902221346290.5031@home-gw.koi8.net you wrote:
I suggest that "tmp" gets passed as argument to the macro.
It is _NOT_ a macro working on some variable. It is simply repeating
Oh yes, of course it is. It references the variable "tmp", and this is nasty as it is not clear to the reader of the code.
If I think it would be better to rename "tmp" into "foo" the code will magically break.
It won't magically break, it will fail to compile. It is totally different case.
This macro does not come from some header file, it is in the same relatively small source file so it is easy to trace if compilation broke. Also it is not used anywhere else so there is no risk it could break anything else. And there is absolutely no reason to rename tmp to foo.
_LITERAL_ text that not only accesses a local variable tmp but even performs a function return with an error if condition is not met. That is,
You are right. That's even worse.
Yep. That's if we are to follow those dogmas to the letter, blindly. But we don't have to.
why, BTW, it should not be made into an inline function because it would require additional checking of its return code where it is used (lot of places in Davinci I2C driver) that, in turn, would've added a lot of unnecessary overhead for local variable, register loads, second conditional checking etc.
I am sure you could come up with an efficient *and* clean solution - if you really wanted to try.
There is no way you can avoid that very same code repeated several times (7 or so? didn't count them) -- that is how the driver works.
It is _NOT_ supposed to be any function-like macro. It is just a fragment of _LITERAL_ text.
The code as is is not acceptable and needs to be cleaned up.
Why not? What is wrong with that code?
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902221504450.5270@home-gw.koi8.net you wrote:
I am sure you could come up with an efficient *and* clean solution - if you really wanted to try.
There is no way you can avoid that very same code repeated several times (7 or so? didn't count them) -- that is how the driver works.
See above - I wrote "if you really wanted to try." You don't.
The code as is is not acceptable and needs to be cleaned up.
Why not? What is wrong with that code?
I will not explain it again.
Best regards,
Wolfgang Denk
On Mon, 23 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902221504450.5270@home-gw.koi8.net you wrote:
I am sure you could come up with an efficient *and* clean solution - if you really wanted to try.
There is no way you can avoid that very same code repeated several times (7 or so? didn't count them) -- that is how the driver works.
See above - I wrote "if you really wanted to try." You don't.
The code as is is not acceptable and needs to be cleaned up.
Why not? What is wrong with that code?
I will not explain it again.
We are splitting hairs here...
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
participants (3)
-
Jean-Christophe PLAGNIOL-VILLARD -
ksi＠koi8.net -
Wolfgang Denk