On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902180945100.5002@home-gw.koi8.net you wrote:
OK, for bitbang driver it is just a source file size reduction. We can simply duplicate (triplicate etc.) that file for more than one adapter. What I did makes CPP make that duplication instead of us. But I can simply do it manually. It will make soft_i2c.c 4 times bigger (for 4 adapters) and the resulting object code will remain exactly the same, byte-to-byte.
The soft_i2c.c source file makes ONE adapter. If we want 2 adapters we should have 2 such files. If we want 4 adapters there should be 4 such files. It is that simple.
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.
The soft_i2c.c is _NOT_ a driver _SOURCE_. It is a _TEMPLATE_ that makes a source from set of macros that come from an external file (in our case it is board config file.) And that is a beauty of this driver -- one doesn't have to write his own driver for each and every board; he just defines a set of basic operations and CPP takes it from there.
And it can _NOT_ scale because it is impossible to make a generic driver _SOURCE_ for each and every hardware configuration imaginable. That existing soft_i2c.c makes _GENERATION_ of such a driver trivial. The only problem with it is it only makes _ONE_ such interface.
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.
For the functions that can be parameterized I'm adding a simple wrapper function that simply passes additional argument to that parameterizable function and calls it. That is in no way worse, or bigger, or more complex than accessing some pointer, then getting adapter number, then branching to the appropriate function. The wrapper is simpler and smaller. Codewise it is even smaller if all arguments fit into registers because when you are preparing that actual function call from a wrapper function ALL arguments are already loaded into appropriate registers; you only have to load one additional register with an immediate constant (channel number) and perform a branch.
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?
Look what I do:
=== Cut === static int _i2c_probe(chip, adap_no) { ... }
static int xx_i2c_probe1(chip) { return _i2c_probe(chip, 0); }
static int xx_i2c_probe2(chip) { return _i2c_probe(chip, 1); } === Cut ===
Looks like overhead to me.
Sure it is. There is no way how you can avoid overhead without writing a totally custom U-Boot for each and every board. But it is no worse than this:
=== Cut === static int _i2c_probe(chip) { adap_no = ADAP(i2c_cur_bus)->hwadap_no; .... }
xx_i2c_probe1 = xx_i2c_probe2 = _i2c_probe; === Cut ===
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(...);
and there is no Catch 22 with changing current bus number to initialize adapter that requires that adapter must be initialized to change the current bus number.
As a rule of thumb, member functions should _NEVER_ use any global variables. It is up to higher layer to work on those if they are required.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************