[U-Boot] PATCH 3/8 Multi-adapter multi-bus I2C

[Heiko Schocher]

Hello ksi,

ksi at koi8.net wrote:
> On Mon, 9 Feb 2009, Heiko Schocher wrote:
> 
>> Hello ksi,
>>
>> ksi at koi8.net wrote:
>>> Signed-off-by: Sergey Kubushyn <ksi at koi8.net>      
>>> ---
>>> diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c
>>> index da6cec1..f0c1771 100644
>>> --- a/drivers/i2c/soft_i2c.c
>>> +++ b/drivers/i2c/soft_i2c.c
>>> @@ -1,4 +1,8 @@
>>>  /*
>>> + * Copyright (c) 2009 Sergey Kubushyn <ksi at koi8.net>
>>> + *

[...]

>>> +#endif
>>> +I2C_SOFT_WRITE_BYTE(3)
>>> +I2C_SOFT_READ_BYTE(3)
>>> +I2C_SOFT_INIT_ADAPTER(3)
>>> +I2C_SOFT_PROBE(3)
>>> +I2C_SOFT_READ(3)
>>> +I2C_SOFT_WRITE(3)
>>> +I2C_SOFT_GET_BUS_SPEED(3)
>>> +I2C_SOFT_SET_BUS_SPEED(3)
>>> +#endif
>>> +
>>>   
>> Hmm... are this lots of defines really necessary? Couldn't we add
>> something like
>>
>> int    hw_adapnr; /* hardware adapter number */
>>
>> to the i2c_adap_t struct, and have an pointer (cur_i2c_adap?) to the
>> current used i2c_adap_t? Then you have where you need it, your
>> hw_adapnr and need not all of this defines.
>>
>> For example you need in the config for MPC8548CDS.h just this define:
>>
>> #define I2C_SDA(bit)	(printf("HW adap: %d sda: %d", cur_i2c_adap->hw_adapnr, bit))
>>
>> and not  I2C_SDA(bit) and I2C_SDA1(bit)
> 
> Eh, those are _NOT_ defines, those are _INSTANCES_.
> 
> First of all, you need real functions to make pointers to them at compile
> time.

Obvious.

> Second, SOFT_I2C is special; it is _NOT_ possible to make generic
> paratemerized functions. Each, e.g., I2C_SDA is different and those config
> file defines are _MACROS_, not defines. One can have one I2C_SOFT adapter
> made of a couple of on-SoC GPIOs and another one constructed of, e.g.,
> unused GPIOs from PCI bridge or whatever. That means that _ALL_ those I2C_*
> macros will be totally different for those 2 adapters thus making 2 sets of
> access functions that have absolutely nothing in common. You can not
> parameterize this...

For example soft_i2c_read():

static int soft_i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
{
[...]
	send_start();
	if(alen > 0) {
		if(write_byte(chip << 1)) {	/* write cycle */
[...]
}

This is now a real function you can make a pointer for

i2c_adap_t	soft_i2c_adap[] = {
{
	.read		=	soft_i2c_read,
}
}

In soft_i2c_read() there are calls for example for send_start().

static void send_start(void)
{
[...]
	I2C_DELAY;
	I2C_SDA(1);
	I2C_ACTIVE;
	I2C_DELAY;
}

and in this function, there are the calls for I2C_SDA(), I2C_ACTIVE,... which
are look as I described. So where is the problem?

And we have not to change aprox. all lines of code from this driver!

> I agree that those 4 #ifdef'ed instances are not the prettiest and 4 is an
> arbitrary number but I'm not THAT good in CPP trickery to come up with a
> generic template that would be good for arbitrary number of instances if it
> can be done at all...
> 
> And that template allows for using existing SOFT_I2C macros in existing
> config files without any changes to them.

So, I think, on my suggestion to.

> Also let's not forget that all those function sets are instantiated at
> _COMPILE_ time so they can be run from ROM.

Why should my functions not run from ROM?

> I would like to hear suggestions on that from real CPP gurus. That would've
> made the code prettier and I would've learned new neat tricks...

Hmm.. I am not a CPP Guru, but it should work without these "define monster".

I look for a little time to try out my suggestion.

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