[U-Boot] [PATCH] 0/12 Multiadapter/multibus I2C

[Heiko Schocher <hs@denx.de>]

Hello ksi,

ksi at koi8.net wrote:
> On Fri, 13 Feb 2009, Heiko Schocher wrote:
>> ksi at koi8.net wrote:
>>> Here is the second attempt for initial portion of multibus/multiadapter
>>> I2C support.
>>>   
>> Can you please send your patches with some better commit messages.
>> You only send your Signed-off-by, without any explanation. Please
>> change this.
> 
> There is not much sense in extensive commit messages in this case, IMHO. It
> is not a bug fix or added feature at one particular place; it is a major
> rework. The only message I can give is something like "Changes for
> multiadapter/multibus I2C support..."
> 
> I'll add it to the second attempt that I will make later today.
> 
>>> This includes a set of common files, all drivers in drivers/i2c and all
>>> boards affected by these changes (config files, board files, and lib_xx
>>> files.)
>>>
>>> There is an illustrative example of multiadapter multibus I2C config in
>>> MPC8548CDS.h config file (#if 0'd.) Definitions in that example are
>>> bogus so please don't expect it to work. It will compile though...
>>>
>>> This set also includes big rework for soft_i2c.c that makes it template
>>> version that allows up to 4 bitbanged adapters. This number can be
>>>   
>> Didn;t you try my suggestion? This is a really big define monster now,
>> which I think, we can avoid, and without to change nearly all lines of
>> the existing driver.
> 
> We can not avoid it. At least I can not see an easy way to do this. SOFT_I2C

Yes, we can. Look again deeper in my approach, this _is_ an easy way!

I also looked again in your changes in the fsl_i2c.c driver, and we
can make this also simplier, by using cur_adap_nr->hwadapnr!

We have not to define for both hardware adapter each function in
i2c_adap_t! For example:

You did:
static void fsl_i2c1_init(int speed, int slaveadd)
{
	__i2c_init(0, speed, slaveadd);
}

instead we only need

i2c_init(cur_adap_nr->hwadapnr, speed, slaveadd);

with

i2c_adap_t	fsl_i2c_adap[] = {
	{
		.init		=	i2c_init,
[...]
		.hwadapnr	=	0,
		.name		=	FSL_NAME(CONFIG_SYS_FSL_I2C_OFFSET)
	},
#ifdef CONFIG_SYS_FSL_I2C2_OFFSET
	{
		.init		=	i2c_init,
[...]
		.hwadapnr	=	1,
		.name		=	FSL_NAME(CONFIG_SYS_FSL_I2C2_OFFSET)
	},
#endif

Please change this driver also!

If I think more, we never even need to change the function parameters
like you did for example for i2c_int ()! We can use at the beginning
of every function who go in i2c_adap_t, the "cur_adap_nr->hwadapnr"
and make the settings we need for this function... and wow we saved
one function parameter.

> is special. Those multiple e.g. MXC or OMAP3 adapters can be parameterized
> because different channels do only differ in their base address that can be
> made into a parameter. Software I2C is totally different because it has

Why is this different? If you change a base or the way to the pins?

> totally different functions for different channels, there is nothing we can

Think about my explanation to the soft_i2c.c driver in previous EMail and
above function.

It also works!

> make into a parameter. All those I2C_SDA etc. are NOT DEFINES, they are
> MACROS. Every function for every channel is built of those macros that can

I know this in your approach, but we _don;t_ need this. We simply can make
one "common" board function and switch in this function dependent on the
"cur_adap_nr->hwadapnr" to the particular GPIO pin functions, wherever the
are!

> be absolutely different for each channel. They define NOT some PARAMETERS
> but function TEXT that will be compiled into executable code.

And this additional TEXT I save too!
I think, you think too much in C++?

> This is how it is done in Linux kernel (see e.g. drivers/hwmon/lm93.c.)

Thats no argument.

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