[U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4

[ksi at koi8.net]

On Thu, 19 Feb 2009, Heiko Schocher wrote:

> Hello ksi,
> 
> ksi at koi8.net wrote:
> > On Wed, 18 Feb 2009, Wolfgang Denk wrote:
> > 
> >> Dear ksi at koi8.net,
> >>
> >> In message <Pine.LNX.4.64ksi.0902181112400.5002 at home-gw.koi8.net> you wrote:
> >>>> Duplicating the source code (and thus the object code, too) to create
> >>>> additional instances of basicly the same driver seems to be the wrong
> >>>> approach to me.
> >>>>
> >>>> It doesn't scale well, to say the least.
> >>> It is _NOT_ the same driver. Every bitbaged I2C driver is _DIFFERENT_. Only
> >>> the _NAME_ is the same.
> >> This is one way to implement it, but not the only one.
> >>
> >> Just to give  an  example  of  a  different  implementation  (without
> >> claiming  that that would be a better one): we could provide an array
> >> of functions (instead of macros) for each such adapter, so  we  could
> >> use  just  a single instance of the driver which takes the address of
> >> the respective array as argument.
> > 
> > Yes, it is possible, but it is not the best approach. Most of those macros
> > translate in 2 to 3 words. If you use functions you should add at very least
> > 2 more words to it, one for a function call another for a function return.
> > That is without array, just directly linked functions. If you are to use an
> > array it adds another 2 words -- one for the array element (the pointer)
> > itself and another one for the pointer to that array in i2c function code.
> > 
> > Linux uses functions for that but they are _INLINE_ ones in headers included
> > into the driver file i.e. they are essentially macros.
> 
> >From which Linux and which driver you speak?
> 
> If you use from actual 2.6 Linux the drivers/i2c/algos/i2c-algo-bit.c
> bitbang driver with gpio support (drivers/i2c/busses/i2c-gpio.c)
> you get called your gpio_get/set_value (pin, state) function in your
> board code (or gpio code)!
> 
> And in this function you have to switch dependent on the pin, and
> then again to switch dependent on the state, before you can do
> any action ... so tell me, where we are worser, when we making
> 
> #define I2C_SDA(bit) \
> 	switch(cur_adap_nr->hw_adapnr) { \
> 	case 0: \
> 		if (bit) { \
> 			/* set pin for adapter 0 = 1*/ \
> 		} else { \
> 			/* set pin for adapter 0 = 0*/ \
> 		} \
> 	[...] \
> 	} \
> } \
> 
> in board config file ... OK, we are worser against your approach, because
> we have for all I2C_SDA, I2C_SCL accesses + 1 switch, but I don;t think
> this is such a problem.

First of all, you are using an external global variable for object methods.
That is a VERY BAD practice and I can't even imagine a use case that would
justify this.

Second, what do you gain by commiting such a blasphemy?

> > That means that implementation is much worse than _EXISTING_ one. And out of
> > decent and much worse one which one would you choose?
> > 
> >>> The soft_i2c.c is _NOT_ a driver _SOURCE_. It is a _TEMPLATE_ that makes a
> >> This is your implementation. It is not the  only  possible  implemen-
> >> tation.  Please  try  and  open  your  mind  to  discuss  alternative
> >> possibilities as well.
> > 
> > No, it is NOT my implementation. It is _EXISTING_ driver in the main tree. I
> > did _NOT_ change the driver, I just made several copies of it.
> > 
> > And, BTW, you can see something very similar in Linux kernel
> > (i2c-algo-bit.c.)
> 
> Please have a deeper look in it, see above comment.

I did.

> > I'm open to any alternative possibilities but I can not see anything better.
> > That _EXISTING_ soft_i2c.c we have in the current tree is a little miracle
> > that was there since the start of times so I can't see any reason to throw
> > it away and reinvent the wheel.
> 
> Nobody wants to throw it away!

You want...

> >>> The former does not require additional adapter struct member, hwadap_no.
> >>> And, unlike the latter, it is self-contained, it doesn't require any
> >>> external global variable to decide what to do. One can initialize all
> >>> adapters by:
> >>>
> >>> 	for (i = 0; i < NUM_I2C_ADAPTERS; i++)
> >>> 		i2c_adap[i]->init(...);
> >> Most probably we never *want* to initialize all adapters...
> > 
> > It is easier that way and wouldn't do any harm in most cases...
> 
> But it is not needed when doing this in i2c_set_bus_num() !!
> It is sufficent to init a hardwareadapter, when switching to it.

That means you'll have to rewrite the entire U-Boot. 99% of the boards have
only one bus so they did not switch busses. That means they never called
that i2c_set_bus_num() relying on i2c_init() in libxxx/board.c instead.

Sorry guys, I do not have THAT much free time that my employer would let me
to spend on this.

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