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

[Heiko Schocher]

Hello ksi,

ksi at koi8.net wrote:
> On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> 
>> Dear ksi at koi8.net,
>>
>> In message <Pine.LNX.4.64ksi.0902142019520.6240 at home-gw.koi8.net> you wrote:
>>> That means you have to make changes in two places instead of one -- config
>>> file AND $(BOARD).c. Also you use functions instead of macros and you can
>>> NOT make them inline because they come from a separate object file. This
>>> essentially defeats the very purpose of that common soft_i2c.c driver. If
>>> you want to make functions for bitbanged I2C into the $(BOARD).c there is no
>>> reason to have them as a base for that driver. It is much more logical to do
>>> everything in reverse, i.e. instead of having soft_i2c.c as a bona fide
>>> drivers and those I2C_SDA and friends as its building blocks make those
>>> i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and
>>> build the actual driver in the $(BOARD).c itself. Just convert that
>>> soft_i2c.c into a header file with macros for real functions (soft_i2c_read
>>> etc.) and instantiate them in the $(BOARD).c.
>> Ecept that the code you posted is unreadable and you will need lots of
>> very good arguments to make me accept it.
> 
> What is unreadable in that code?

I wouldn;t say unreadable but unnecessary swollen.

> Take e.g. this:
> 
> === Cut ===
> #define I2C_SOFT_SEND_START(n) \
> static void send_start##n(void) \
> { \
[...]
>         I2C_DELAY2;
>         I2C_SDA2(0);
>         I2C_DELAY2;
> }
> === Cut ===
> 
> This will be generated at compile time and fed to gcc.
> 
> What is so unreadable here?
> 
> Sure I can make all the instances manually and avoid those #define's but it
> will not make that source file any more readable by simply repeating those
> functions several times with just that "##n" different. And it will make
> that source file 4 times bigger with 4 instances or twice as big if there
> are only two of them.

Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t
have to change anything in this driver (I posted such a patch as a proposal)

And again, you don;t need to do, as i did in this proposal, make this
I2C_SDA, ... in function. You can of course make this in macros. OK, you
have one more if but that shouldn;t be such a problem!

> Why should we avoid using CPP feature that is SPECIALLY made for cases like
> this?

What CPP feature?

> Not rocket science and not much of black magic, just simple and
> straightforward token pasting...
> 
>>> The only problem with that is it breaks uniformity and makes another mess.
>>> The whole idea was to bring _ALL_ I2C drivers to a single place and make
>>> them totally transparent and uniform. Something like e.g. Linux VFS.
>> This is a boot loader with limited resources, not a general purpose
>> OS.
> 
> It doesn't matter. It is much better to have a uniform API for all the
> future developers to use than to multiply horrible hacks and reinventing the
> wheel again and again.

? We didn;t want to change the API, you mix things. We only want to
prevent such a define monster in the bitbang driver.

>>> And remember, the devil is in details. How are you going to assign
>>> (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you
>>> going to work on an adapter other that "current" in a situation when you can
>>> NOT change "current" adapter (e.g. perform all I2C layer initialization
>>> while still running from flash?) Remember, this is plain C and there is no
>> What makes you insist that we cannot change a variable if we need to
>> be able to change one?
> 
> It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And
> number of busses can be bigger than number of adapters (e.g. when some
> busses a reached via muxes or switches.) When doing i2c_set_current_bus()
> you are switching _NOT_ adapters, but busses. That involves not only

What has this to do with soft_i2c.c?

> changing that global variable but also reprogramming muxes/switches for

Yes, and this is independent of changing also this current pointer.

> i2c_set_current_bus() to be consistent and hardware independent. Otherwise

It is this also with changing this current pointer!

> your code should know if that particular bus it is switching to is directly
> connected or switched and check the bus it is switching from for muxes. If
> they are switched, your code should disconnect the current bus switches,

Yes, and this you did perfectly in i2c-core.c, where is your problem?

> then do that i2c_set_current_bus() and connect the switches to the new bus
> after that.

I don;t understand you know, really. Nobody in this discussion criticize
the API, we just discuss the soft_i2c.c driver, and how we can prevent
this defines ... or I lost something ...

> That means that code MUST somehow know the topology to take appropriate
> actions and properly configure those switches. That means you should somehow
> describe that topology for each and every board in CONFIG_* terms and make
> each and every place at U-Boot that invokes _ANY_ i2c function to take care
> of that switching.

Yep, this we(you did it ;-) have this in i2c-core.c ...

(And, I want to start this discuss again, you just dropped the support for
adding new such busses per command shell ... you could not do this! But
I have a solution for this on top of your patches, but I want start this
discussion, if we have your patches in a testing branch in u-boot-i2c.git)

> My code does it transparently in the single place, i2c_set_current_bus() so
> higher level code doesn't have to bother with details.

Again, what has this to do with introducing a current pointer?

> Then, all those I2C multiplexers/switches are I2C devices theirself that
> means you can NOT talk to them if the adapter they connected to is not
> initialized.

Ok, come, read my previous EMail, you can init this adapter before
switching the muxes.

> And yes, we DO have some boards with switched I2C busses in U-Boot main tree
> so this is NOT a hypothetical situation.

Yes, and they add i2c busses frem env variables, which you dropped ...

>>> You are adding unnecessary complexity to the code. And you break uniformity.
>> He. I must have thought the same before about someone else's code ;-)
> 
> Eh, I'm trying to make things simpler... For that particular board I'm
> expecting from assembly house by the end of this week I can make its
> particular hardware work with a bunch of one-time hacks in its $(BOARD).c... 
> 
> But I think I'm not the first one to face such a problem and not the last
> one. So why wouldn't we make the proper API to get rid of all those hacks? I
> can do it now while paid by my current employer but there is no guarantee my
> next one would allow for such a waste from business standpoint...

I don;t understand why you have such problems with introducing a current
pointer. And again, that has nothing to do with the API.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany