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

[ksi at koi8.net]

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.

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 at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************