On Thu, 19 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902181112400.5002@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@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************