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

[ksi at koi8.net]

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?

Take e.g. this:

=== Cut ===
#define I2C_SOFT_SEND_START(n) \
static void send_start##n(void) \
{ \
	I2C_SOFT_DECLARATIONS##n \
	I2C_DELAY##n; \
	I2C_SDA##n(1); \
	I2C_ACTIVE##n; \
	I2C_DELAY##n; \
	I2C_SCL##n(1); \
	I2C_DELAY##n; \
	I2C_SDA##n(0); \
	I2C_DELAY##n; \
}

...

I2C_SOFT_SEND_START()

I2C_SOFT_SEND_START(2)
=== Cut ===

That will make two functions replacing "##n" with literal argument:

=== Cut ===
static void send_start(void)
{
        I2C_SOFT_DECLARATIONS
        I2C_DELAY;
        I2C_SDA(1);
        I2C_ACTIVE;
        I2C_DELAY;
        I2C_SCL(1);
        I2C_DELAY;
        I2C_SDA(0);
        I2C_DELAY;
}

static void send_start2(void)
{
        I2C_SOFT_DECLARATIONS2
        I2C_DELAY2;
        I2C_SDA2(1);
        I2C_ACTIVE2;
        I2C_DELAY2;
        I2C_SCL2(1);
        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.

Why should we avoid using CPP feature that is SPECIALLY made for cases like
this?

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.

> > 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
changing that global variable but also reprogramming muxes/switches for
i2c_set_current_bus() to be consistent and hardware independent. Otherwise
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,
then do that i2c_set_current_bus() and connect the switches to the new bus
after that.

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.

My code does it transparently in the single place, i2c_set_current_bus() so
higher level code doesn't have to bother with details.

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.

And yes, we DO have some boards with switched I2C busses in U-Boot main tree
so this is NOT a hypothetical situation.

> > And the million dollar question -- what is the potential gain?
> 
> Indeed. The  same  question  goes  to  you  -  where  is  this  added
> complexity  really  needed? So far nowhere. Are we just talking about
> hypothetical cases, or about a real design? How  many  such  designs?
> Just a single one?

Please see above.

> > 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...

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