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

[ksi at koi8.net]

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.

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

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.

> > And it can _NOT_ scale because it is impossible to make a generic driver
> > _SOURCE_ for each and every hardware configuration imaginable. That existing
> 
> Impossible. Famous last words.

Well, it looks like it is you are Russian, not me :) It is well known
Russian passion to scrap everything and design a Universal Server Of
Everything from scratch :) I'm trying to avoid that...

> > soft_i2c.c makes _GENERATION_ of such a driver trivial. The only problem
> > with it is it only makes _ONE_ such interface.
> 
> And it duplicates the source code for each additional instance that is
> needed.
> 
> > And no, we are _NOT_ duplicating source code. Source code is _DIFFERENT_ for
> > different adapters. We just creating several _INSTANCES_ using that template
> > with _DIFFERENT_ parameters. And those instances are all different. The
> > template itself does _NOT_ go into the final code.
> 
> Call it what you want, I call it duplication of code.

It might be a duplication of SOURCE TEXT, but not CODE.

> > > Is this theory or did you perform code size measurements?
> > 
> > It is obvious. Furthermore, it doesn't make sence to count size difference
> > here because it is miniscule -- how many I2C adapters do we have on a board?
> 
> It is obvious. Famous last words again.

Eh, do you think anyone really has time to make such comparisons to find out
that size difference is zero?

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

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