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

[Wolfgang Denk]

Dear ksi at koi8.net,

In message <Pine.LNX.4.64ksi.0902171117560.30222 at home-gw.koi8.net> you wrote:
> 
> > 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

Sorry that the code slipped through the review then.

> text smaller and easier to read. It is not used anywhere outside the driver

That's not easier to read. Macros with side  effects  are  dangerous.
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."

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

I am sure about it.

And while you are at it, I would appreciate if you could also post a
(separate) patch that fixes the same thing in the Davinci I2C driver
you were referring to above.  Thanks.

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

Yes, please.

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

I am aware that there are areas in the Linux kernel that could need a
thourough rework - like in any bigger project.

Borrowing good code from Linux is excellent. But there is  no  reason
not  to  improve  poor code when we run into it. Who knows, maybe one
day Linux might copy code from U-Boot -  now  would  that  be  a  bad
thing?

> 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?

Yes, we are - when it makes sense.

If you have free cycles you could try and post such a cleanup patch
for Linux too, so both keep in sync.

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

I agree with you - my personal  preference  is  quite  often  to  use
braces  there, too. But we try all to stick to a common coding style,
so everybody has to swallow his own set of bitter pills.

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

We try to be good citicens and follow the CodingStyle, that's all...

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

Since more changes are required to that file anyway: yes, please.

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

Yes, please.

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
Harrison's Postulate:
	For every action, there is an equal and opposite criticism.