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

[ksi at koi8.net]

On Thu, 19 Feb 2009, Wolfgang Denk wrote:

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

OK, posted a patch.

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

OK, will replace it with a structure, no problems. I do personally prefer a
structure too; that code was against my nature but I tried to keep Linux
kernel files intact.

[dd]

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

OK, 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...
> 
> We try to be good citicens and follow the CodingStyle, that's all...

OK.

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

OK.

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

OK, not a big deal, will do.

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