Hello ksi,
ksi@koi8.net wrote:
On Mon, 9 Feb 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net wrote:
Signed-off-by: Sergey Kubushyn ksi@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@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