On Wed, 18 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0902142019520.6240@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.
In my case you simply make N i2c_write() functions for N adapters and assign 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 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 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...
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.
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.
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.
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_.
i2c_set_current_bus() to be consistent and hardware independent. Otherwise
It is this also with changing this current pointer!
No, it is _NOT_.
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.
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.
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.
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.
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.
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.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************