From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Tue, 10 Feb 2009 09:20:54 +0100 Subject: [U-Boot] PATCH 3/8 Multi-adapter multi-bus I2C In-Reply-To: References: <49901FDE.8050405@denx.de> Message-ID: <499138E6.5090603@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> --- >>> 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 >>> + * [...] >>> +#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