On Thu, 19 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Wed, 18 Feb 2009, Wolfgang Denk wrote:
> >
> >> Dear k...@koi8.net,
> >>
> >> In message <pine.lnx.4.64ksi.0902181112400.5...@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.
---
******************************************************************
* k...@home KOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
******************************************************************
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
