Hello ksi,
ksi@koi8.net wrote:
On Sat, 14 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
On Fri, 13 Feb 2009, Heiko Schocher wrote:
ksi@koi8.net wrote:
Signed-off-by: Sergey Kubushyn ksi@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@koi8.net
- Changes for multibus/multiadapter I2C support.
- (C) Copyright 2001, 2002
- Wolfgang Denk, DENX Software Engineering, wd@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
Yes. But that was just a thought, it should be possible to do this also in macros.
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
Maybe more logical, but not needed.
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.
Just, if we do this, but we don;t need to do it so.
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
Yes, good point. But do we need more then one i2c adapter when running from flash? I see only one reason to use i2c when running from flash: accessing SPD EEprom ... and this "bus" could always be the first hw adapter. All other accesses to i2c should be moved to run when we are in ram.
"this" pointer... And that is just a tip of an iceberg...
And the million dollar question -- what is the potential gain?
I want to avoid such a big change in soft_i2c.c. Also if you have 4 bitbang instances with your version you have 3 times more code.
But if others are on your side, I have no problem with your approach.
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.
not really.
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)
Thats not really a problem.
bye Heiko