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

[Heiko Schocher]

Hello ksi,

ksi at koi8.net wrote:
> On Wed, 18 Feb 2009, Heiko Schocher wrote:
> 
>> 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!
> 
> What problem? Look, for a generic case _ALL_ those I2C_SDA etc. building
> blocks are _ABSOLUTELY_ different for different adapters. You can _NOT_
> parameterize them, you will have _ABSOLUTELY_ different i2c_write() etc. for
> different adapters. There is _NOTHING_ common between them.

How SDA, SCL are get/set is common, just how SDA and SCL are accessed is
different! So there is no need for different i2c_write(), ... only SDA,SCL
accessors are different.

> In my case you simply make N i2c_write() functions for N adapters and assign

Thats not needed.

> pointers to those functions to appropriate adapter struct members at
> _COMPLILE_ time. And that's all to it.
> 
> In your case you stick those functions in one monstrous i2c_write() where
> you still have those N functions as "case" bodies of some switch so you

No, just the SDA,SCL accesses have such a switch.

> still have that same code size _PLUS_ switch. Than, you assign a pointer to
> that monstrous function to i2c_write() member of _ALL_ adapter structures so
> a call to i2c_write() ends up calling that function. Now you have to somehow
> find out which switch case to execute. For that you need an additional
> global variable and additional member in each adapter struct. And those must

No problem with this.

> be writeable because you don't have any means of executing something like
> "adap[N]->i2c_write(....)", you must prepend it with i2c_set_bus_num(M)
> because in your case that "N" in "adap[N]" has absolutely no effect...

? you again mixing thinks. With my approach no need for changing the API
in i2c-core.c. I think, thats you don;t got.

> Please explain how it is better than simple and straightforward function per
> adapter? Which one is more complex?

> It looks like I simply don't understand something. You must've meant
> something else that I didn't get because the above comparison is SO obvious
> that it is almost impossible to somehow misunderstand it...
> 
>>> Why should we avoid using CPP feature that is SPECIALLY made for cases like
>>> this?
>> What CPP feature?
> 
> Source file preprocessing.

I think you mean gcc, right?

>>> 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.
> 
> Make several of those for several drivers if it looks easier. Multiply it.

No need for it.

>>>>> 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?
> 
> Please read above.

So you to ;-)

>>> changing that global variable but also reprogramming muxes/switches for
>> Yes, and this is independent of changing also this current pointer.
> 
> No, it is _NOT_.

It is.

>>> i2c_set_current_bus() to be consistent and hardware independent. Otherwise
>> It is this also with changing this current pointer!
> 
> No, it is _NOT_.

It is.

>>> 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?
> 
> The problem is you can _NOT_ change the bus if adapter is not initialized
> and you can _NOT_ initialize adapter because you _MUST_ change the bus to do
> this. No matter running from RAM or from ROM, you have exactly the same
> Catch 22.

You have the same problem. I wrote you in an EMail, that your i2c_set_bus_number()
will not work from flash, so how works this for you?

>>> 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 ...
> 
> You can _NOT_. The soft_i2c.c file is _ALREADY_ a template, that builds
> actual source code from set of external defines. One more time -- it is
> _ALREADY_ a template.

Yes, I know! But different adapters just differnet in how they access
SDA, SCL not in i2c_write,..

> I did nothing to that file, just added an option of generating several
> drivers instead of one. There is absolutely nothing I changed in that driver
> per se, that is _EXACTLY_ the same code.
> 
> You can make N such files for N adapters, or you can multiply those
> functions in that file N times to make N adapters. I'm doing the latter and
> nothing else.
> 
> What are you trying to prevent? I simply don't understand. Do you want me to
> just make several sets of functions in soft_i2c.c file because those defines
> look scary? No problems, just say it and I will do that. But there is no
> need for reinventing a wheel or adding sophisticated crutches to the
> obvious...
> 
>>> 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)
> 
> We'll return to those dynamic busses later. I personally can't see any
> viable reason for that.

Maybe you not, but I told you, that there is a hardware manufacturer, that
has his DTTs, EEProms,... on different hardware on different I2C busses.
With this dynamic busses, he can have one image for all his boardvariants,
and this is a need.

>>> 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?
> 
> It solves Catch 22.
> 
>>> 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.
> 
> You can _NOT_. Please read above.

Why?

>>> 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 ...
> 
> Again, I can't see any reason for such a feature. If you want to attach
> something to the already running board and do some accesses you can easily
> do it with existing commands. If you want to use it for U-Boot itself, then
> it does NOT belong to the environment; it belongs to board config file.

I think, you don;t read my EMails ...

>>>>> 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.
> 
> We do already have current bus. There is absolutely no need for such a
> pointer. Occam's razor.

OK, do we not make a pointer, just an integer, but it is the same
problem, the integer must also be writeable! How would you change busses
when running in flash?

(I prefer to have such a pointer)

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