[U-Boot] [PATCH] 6/12 Multiadapter/multibus I2C, drivers part 3

[ksi at koi8.net]

On Mon, 16 Feb 2009, Wolfgang Denk wrote:

> Dear ksi at koi8.net,
> 
> In message <Pine.LNX.4.64ksi.0902121420120.21067 at home-gw.koi8.net> you wrote:
> ...
> > +/* I2C registers field definitions */
> > +/* --------------------------------*/
> > +/* I2C_CONTROL (R/W) */
> 
> Incorrect multi-line comment style. Please fix.

OK.

> > +#define SM501_CHECK_NACK() \
> > +	do {\
> > +		if (tmp & (SM501_I2C_NACK | SM501_I2C_BUS_ERROR)) {\
> > +			tmp = read_i2c_reg(SM501_I2C_CONTROL) & SM501_I2C_SPEED;\
> > +			write_i2c_reg(SM501_I2C_CONTROL, tmp);\
> > +			return(1);\
> > +		}\
> > +	} while (0)
> 
> Macros with magic side effects (here on the variable "tmp" are stronly
> deprecated. Please fix this.

There is exactly the same code in Davinci I2C driver that had been accepted
and made it in the main tree. The reason behind this macro is to make source
text smaller and easier to read. It is not used anywhere outside the driver
file. Sure I can remove it and put that text as is anywhere where it is used
(3 places in sm502_i2c.c) but will it make it any better or more readable? I
seriously doubt it.

And that side effect on tmp is accounted for so there is no magic. Nothing
in the code relies on tmp content after that code fragment is executed. This
is just a text fragment that gets inserted literally at appropriate places.

Please let me know if you still want that changed. I will remove that
standard "do {...} while(0)"  wrapper and paste the "if{...}" block at
appropriate places.

Are we going to do the same for Davinci I2C driver for consistency? 

> > +static __inline__ void write_i2c_reg(unsigned long offset, u_int8_t data)
> > +{
> > +	writeb(data, sm501_iomem_base + SM501_I2C + offset);
> > +	__asm__("sync;isync;msync");
> 
> Are you sure the "sync;isync;msync" is needed? The accessor functions
> should make sure this is not necessary.

OK. It would do any harm anyways but I will remove that.

> Also, instead of register offsets ("...base + SM501_I2C") please use
> a proper data structure.

Are we dropping Linux compatibility or what? That sm501-regs.h file with
those offsets has been directly stolen from Linux kernel source and that's
the way they do it in the kernel. I don't have any problems with making a
data structure for SM501/502 I2C controller but aren't we supposed to keep
parts stolen from Linux kernel as close to the source as possible?

Please advise.

> > +	do {
> > +		stat = read_i2c_reg(SM501_I2C_STATUS);
> > +		if (stat & mask) {
> > +			return(stat);
> > +		}
> 
> No curly braces for single line statements. Same goes everywhere esle,
> too.

What's wrong with bracing a single line statement? I personally think it
makes code more readable and less prone for errors. But sure, I can change
this, no problems...

> And "return" is not a function - please omit the parens (here and
> elsewhere).

There is a lot of places where parens are used with "return" in U-Boot (and
elsewhere.) C does not REQUIRE parens but also does not forbid or discourage
them. I personally think it is better to use excessive parens than not to
use them when they are required.

But sure I can change it if it really matters...

> > +	while ((read_i2c_reg(SM501_I2C_STATUS) & SM501_I2C_BUS_BUSY) && timeout--) {
> ...
> > diff -purN u-boot-i2c.orig/include/sm501-regs.h u-boot-i2c/include/sm501-regs.h
> > --- u-boot-i2c.orig/include/sm501-regs.h	1969-12-31 16:00:00.000000000 -0800
> > +++ u-boot-i2c/include/sm501-regs.h	2009-02-12 10:46:00.000000000 -0800
> > @@ -0,0 +1,394 @@
> > +/* sm501-regs.h
> 
> Incorrect multiline comment style.

This again comes from Linux kernel source. Should I fix such borrowed code?

> ...
> > +#define SM501_GPIO31_0_CONTROL		(0x000008)
> > +#define SM501_GPIO63_32_CONTROL		(0x00000C)
> > +#define SM501_DRAM_CONTROL		(0x000010)
> > +
> > +/* command list */
> > +#define SM501_ARBTRTN_CONTROL		(0x000014)
> > +
> > +/* command list */
> > +#define SM501_COMMAND_LIST_STATUS	(0x000024)
> > +
> > +/* interrupt debug */
> > +#define SM501_RAW_IRQ_STATUS		(0x000028)
> > +#define SM501_RAW_IRQ_CLEAR		(0x000028)
> > +#define SM501_IRQ_STATUS		(0x00002C)
> > +#define SM501_IRQ_MASK			(0x000030)
> > +#define SM501_DEBUG_CONTROL		(0x000034)
> 
> Please do not use register offsets, define a C structure instead.

Again, this came from Linux kernel. Should I throw it away and make my own
header file? Please advise.

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************