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

[Heiko Schocher <hs@denx.de>]

Hello ksi,

ksi at koi8.net wrote:
> 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@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 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
>   

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany