Hello ksi, k...@koi8.net wrote: > On Wed, 18 Feb 2009, Heiko Schocher wrote: > >> Hello ksi, >> >> k...@koi8.net wrote: >>> On Mon, 16 Feb 2009, Wolfgang Denk wrote: >>> >>>> Dear k...@koi8.net, >>>> >>>> In message <pine.lnx.4.64ksi.0902142019520.6...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot