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

[Wolfgang Denk <wd@denx.de>]

Dear ksi at koi8.net,

In message <Pine.LNX.4.64ksi.0902121420120.21067@home-gw.koi8.net> you wrote:
...
> +/* I2C registers field definitions */
> +/* --------------------------------*/
> +/* I2C_CONTROL (R/W) */

Incorrect multi-line comment style. Please fix.

> +#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.

> +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.

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


> +	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.

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

> +	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.

...
> +#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.


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
Fantasy is like alcohol - too much is bad for you, a little bit makes
the world a better place. Like  an  exercise  bicycle  it  takes  you
nowhere, but it just might tone up the muscles that will. Daydreaming
got  us  where we are today; early in our evolution we learned to let
our minds wander so well that they started coming  back  with  souve-
nirs.   - Terry Pratchett & Stephen Briggs, _The Discworld Companion_