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

[ksi at koi8.net]

On Sat, 14 Feb 2009, Heiko Schocher wrote:

> Hello ksi,
> 
> ksi at koi8.net wrote:
> > On Fri, 13 Feb 2009, Heiko Schocher wrote:
> >> ksi at koi8.net wrote:
> >>> Signed-off-by: Sergey Kubushyn <ksi at koi8.net>
> >>> ---
> >>> diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c u-boot-i2c/drivers/i2c/soft_i2c.c
> >>> --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c	2009-02-12 10:43:41.000000000 -0800
> >>> +++ u-boot-i2c/drivers/i2c/soft_i2c.c	2009-02-12 10:46:00.000000000 -0800
> >>> @@ -1,4 +1,8 @@
> >>>  /*
> >>> + * Copyright (c) 2009 Sergey Kubushyn <ksi at koi8.net>
> >>> + *
> >>> + * Changes for multibus/multiadapter I2C support.
> >>> + *
> >>>   * (C) Copyright 2001, 2002
> >>>   * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> >> [...]
> >>
> >> The following patch is based on your patches without 7/12 and
> >> adds multibus support for the soft_i2c driver without doing such
> >> a big change as you did. Maybe it is not yet perfect, because
> >> it is just a fast try, but I think we should go this way. What
> >> do you/others think?
> > 
> > The reason behind this patch is making SEVERAL different SOFT_I2C ADAPTERS
> > available. Not BUSSES but separate PHYSICAL I2C ADAPTERS made of different
> > pin pairs from different chips.
> 
> This you can also do with "my" suggestion ...
> 
> > OK, please explain how are you going to make different functions for
> > different adapters? Let's say you want to use 2 on-SoC GPIO pins for
> 
> You can do now the following for example in your
> include/configs/MPC8548CDS.h example:
> 
> you only have to define
> 
> #define I2C_SDA(bit)   (printf("hwadap: %d sda1: %d", cur_adap_nr->hwadapnr, bit))
> 
> if this is a real driver you can make a function in your board code
> say (just a fast thought):
> 
> void i2c_soft_sda (int bit)
> {
> 	switch(cur_adap_nr->hwadapnr) {
> 		case 0:
> 			/* adapter specfic code 0 */
> 			break;
> 		case 1:
> 			/* adapter specfic code 1 */
> 			break;
> 		[...]
> 	}
> }
> 
> and define in config file
> 
> #define I2C_SDA(bit)   i2c_soft_sda (bit)

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.

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.

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
"this" pointer... And that is just a tip of an iceberg...

And the million dollar question -- what is the potential gain?

> > adapter #0, 2 GPIOs from a PCI-PCI bridge for adapter #1, and 2 pins from
> > some chip sitting behind that bridge for adapter #2 if all those pin sets
> > are accessed totally different. I won't even start about using pins from
> > different chips for SDA and SCL (let's say you only have one GPIO available
> > on your SoC and another one on PCI Bridge.)
> > 
> > What your patch creates is just aliases to the SAME physical adapter.
> 
> No, it is not! I only use the same functions, but in the board
> specific code it is possible to made a switch and access
> the Pins where ever they are.

You are adding unnecessary complexity to the code. And you break uniformity.
Those defines in config/soft_i2c.c make real inline functions at _COMPILE_
time. Your approach shifts it to _LINK_ time. It also makes those drivers
come from 3 places ($(BOARD).c, include/configs/$(BOARD).h, and soft_i2c.c)
instead of just two. And it introduces a whole bunch of subtle problems with
no simple and elegant solutions...

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