Hello ksi, k...@koi8.net wrote: > On Thu, 19 Feb 2009, Heiko Schocher wrote: > >> 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. > > Argh... Do you understand that those send_start etc. are _NOT_ the > functions? One more time -- they are _NOT_ functions, they are _TEMPLATES_. > > Template is _NOT_ a code, it is a _TOOL_ that generates code.
I know this. >>> In my case you simply make N i2c_write() functions for N adapters and assign >> Thats not needed. > > Hm... Please explain how are you going to use 2 different sets of pins with > different access methods with one function? I explained you this in more than one EMail. You have one more "if" in your SDA, SCL functions/macro. And don;t say to me, your processor is not fast enough to do one more if for every SDA, SCL access. When running u-boot, you have full 100% from your CPU for doing this. If your CPU is to slow to do this, I bet you have no fun when starting Linux. And look deeper in the i2c-bitbang driver from linux. When using this with the gpio API you get for every SDA, SCL access a function call, in this fn you must decide which pin, and what state for that pin -> min 2 "if" ... and so we are not worser than Linux. >>> 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. > > So we should make a monster with switches off of each and every send_start() > and friends and pass them a value to decide which set to use? It is not in send_start(), it is in every SDA,SCL fn/macro, again, I sent you a soft-i2c driver proposal. If you look in this you see, no fn in it is changed! We want this, because we don;t need to change everything in Sourcecode, as Wolfgang mentioned to you. > That makes everything more messy and has a performance penalty -- instead of > simple branch to those simple blocks you are adding at least one register > load per call to load that argument. Look above. If speed is here your problem, you have a lot of other problems with that, when running Linux. > And what are the advantages? No Sourcecode change is needed. >>> 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. > > There IS a problem First, that means you have _ABSOLUTELY_ no way to access > any other adapter than default one until that variable made writable. And we make it writeable, so no problem. But you have the _same_ problem, and as I told you. + you cannot switch actually busses with your approach! > Second, all class functions must be self-sufficient, they should NOT rely on > some external global variable to work. That might sound C++ese but no matter > how I don't like C++ it does many things right. Hmm.. think this is not so a big problem here. >>> 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. > > No, it is you who didn't get it. As we russians use to say, the bride is > very good, just a little bit pregnant. You want to use a global variable in > I2C object member functions. Global variables are _EVIL_ by theirself and > should be only used as last resort, but in member functions they are not > just plain evil, they are absolute NO-NO. see above. >>> 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? > > No, I mean CPP. GCC is a frontend for cpp, as, ld etc. It does NOT > preprocess files, cpp does. Ah, okay, thanks. >>>>> 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. > > Please show me the code. which code? The soft-i2c driver I posted here as a proposal, also how a I2C_SDA macro can look, if you have more than one bitbang driver. Look in the archive. >>>>>>> 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. > > That means you have another design for that. Please explain it. I explained it. I posted the soft-i2c driver, also how i2c_set_bus_num can look, to support switching busses and init hardware adapters from it. >>>>> 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. > > Please show your code. posted here. >>>>> 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? > > I can call a member function for any adapter directly if needed with > adap[N]->function(). You can NOT because in your case that "N" does not have > any effect and your functions rely on an external global variable that you > can not change. > > BTW, in my design i2c_cur_bus variable is NOT global. It has file scope. Okay. >>>>> 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,.. > > That i2c_write uses a bunch of helper functions that are GENERATED from > I2C_SDA and other macros. All those helper functions are different for > different pin sets. And there is no compilable source code for them in > soft_i2c.c (_EXISTING_ one, not just new,) only TEMPLATEs. Function is > something you can get an address for. Templates do NOT have such address. > You can NOT make a pointer to a template. I give up. >>> 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. > > That's a separate issue. I can offer N other ways to achieve this other than > using that horror. And if they use the same binary image for different > boards those boards are not different. If they put a chip on different bus > that means the hardware is different. Different hardware means quite a > lengthy process involving changes to the schematics, then re-layout, new set > of gerbers, new PCBs, new BOM, new P&P programs etc. Such an effort > definitely deserves a separate config file and simple repompile of U-Boot > that takes less than a minute time. Thats your opinion, but there are others you cannot ignore. > But again, that is a separate issue. We do NOT have anything yet, just a > cloned repository with no new source. I agree here with you. First let us make some base, and then we can go on with that issue. >>>>> 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? > > Because you use an external global variable to choose which adapter's > function to call and that variable is not writable. You can NOT use > adap[N]->function() because in your case that N does not have any effect. But we can make this variable "writeable", so there is no problem with that. >>>>>>> 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'm not going to change busses when I'm running from flash. But I can call > functions on any adapter I want if needed. Is it needed? 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