[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions
All implementations of the functions i2c_reg_read() and i2c_reg_write() are identical. We can save space and simplify the code by converting these functions into inlines and putting them in i2c.h.
Signed-off-by: Timur Tabi timur@freescale.com ---
v3: add 8xx-specific code, and include PRINTDs from blackfin and pxa
I'm posting this patch because I'm enhancing the I2C routines to support multiple I2C busses more easily, but I need to clean up the existing code first.
I'm going to be on vacation when the next merge window opens, so I'm posting this now in the hopes that it will be picked up when the window does open.
cpu/arm920t/at91rm9200/i2c.c | 14 --------- cpu/arm926ejs/davinci/i2c.c | 17 ----------- cpu/blackfin/i2c.c | 16 ---------- cpu/mpc512x/i2c.c | 17 ----------- cpu/mpc5xxx/i2c.c | 16 ---------- cpu/mpc8220/i2c.c | 16 ---------- cpu/mpc824x/drivers/i2c/i2c.c | 14 --------- cpu/mpc8260/i2c.c | 16 ---------- cpu/mpc8xx/i2c.c | 33 --------------------- cpu/ppc4xx/i2c.c | 20 ------------- cpu/pxa/i2c.c | 15 ---------- drivers/i2c/fsl_i2c.c | 16 ---------- drivers/i2c/soft_i2c.c | 19 ------------ include/i2c.h | 62 +++++++++++++++++++++++++++++++++++++++- 14 files changed, 60 insertions(+), 231 deletions(-)
diff --git a/cpu/arm920t/at91rm9200/i2c.c b/cpu/arm920t/at91rm9200/i2c.c index b68c5dd..9fd72d3 100644 --- a/cpu/arm920t/at91rm9200/i2c.c +++ b/cpu/arm920t/at91rm9200/i2c.c @@ -189,20 +189,6 @@ i2c_init(int speed, int slaveaddr) return; }
-uchar i2c_reg_read(uchar i2c_addr, uchar reg) -{ - unsigned char buf; - - i2c_read(i2c_addr, reg, 1, &buf, 1); - - return(buf); -} - -void i2c_reg_write(uchar i2c_addr, uchar reg, uchar val) -{ - i2c_write(i2c_addr, reg, 1, &val, 1); -} - int i2c_set_bus_speed(unsigned int speed) { return -1; diff --git a/cpu/arm926ejs/davinci/i2c.c b/cpu/arm926ejs/davinci/i2c.c index d220a4c..3ba20ef 100644 --- a/cpu/arm926ejs/davinci/i2c.c +++ b/cpu/arm926ejs/davinci/i2c.c @@ -331,21 +331,4 @@ int i2c_write(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len) return(0); }
- -u_int8_t i2c_reg_read(u_int8_t chip, u_int8_t reg) -{ - u_int8_t tmp; - - i2c_read(chip, reg, 1, &tmp, 1); - return(tmp); -} - - -void i2c_reg_write(u_int8_t chip, u_int8_t reg, u_int8_t val) -{ - u_int8_t tmp; - - i2c_write(chip, reg, 1, &tmp, 1); -} - #endif /* CONFIG_DRIVER_DAVINCI_I2C */ diff --git a/cpu/blackfin/i2c.c b/cpu/blackfin/i2c.c index 60f03d4..2a3e223 100644 --- a/cpu/blackfin/i2c.c +++ b/cpu/blackfin/i2c.c @@ -425,20 +425,4 @@ int i2c_write(uchar chip, uint addr, int alen, uchar * buffer, int len)
}
-uchar i2c_reg_read(uchar chip, uchar reg) -{ - uchar buf; - - PRINTD("i2c_reg_read: chip=0x%02x, reg=0x%02x\n", chip, reg); - i2c_read(chip, reg, 0, &buf, 1); - return (buf); -} - -void i2c_reg_write(uchar chip, uchar reg, uchar val) -{ - PRINTD("i2c_reg_write: chip=0x%02x, reg=0x%02x, val=0x%02x\n", chip, - reg, val); - i2c_write(chip, reg, 0, &val, 1); -} - #endif /* CONFIG_HARD_I2C */ diff --git a/cpu/mpc512x/i2c.c b/cpu/mpc512x/i2c.c index 77a6f0d..4f6bc86 100644 --- a/cpu/mpc512x/i2c.c +++ b/cpu/mpc512x/i2c.c @@ -382,23 +382,6 @@ Done: return ret; }
-uchar i2c_reg_read (uchar chip, uchar reg) -{ - uchar buf; - - i2c_read (chip, reg, 1, &buf, 1); - - return buf; -} - -void i2c_reg_write (uchar chip, uchar reg, uchar val) -{ - i2c_write (chip, reg, 1, &val, 1); - - return; -} - - int i2c_set_bus_num (unsigned int bus) { if (bus >= I2C_BUS_CNT) { diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c index 4d16bbe..7d76274 100644 --- a/cpu/mpc5xxx/i2c.c +++ b/cpu/mpc5xxx/i2c.c @@ -380,20 +380,4 @@ Done: return ret; }
-uchar i2c_reg_read(uchar chip, uchar reg) -{ - uchar buf; - - i2c_read(chip, reg, 1, &buf, 1); - - return buf; -} - -void i2c_reg_write(uchar chip, uchar reg, uchar val) -{ - i2c_write(chip, reg, 1, &val, 1); - - return; -} - #endif /* CONFIG_HARD_I2C */ diff --git a/cpu/mpc8220/i2c.c b/cpu/mpc8220/i2c.c index d67936d..76ecdf1 100644 --- a/cpu/mpc8220/i2c.c +++ b/cpu/mpc8220/i2c.c @@ -387,20 +387,4 @@ int i2c_write (uchar chip, uint addr, int alen, uchar * buf, int len) return ret; }
-uchar i2c_reg_read (uchar chip, uchar reg) -{ - uchar buf; - - i2c_read (chip, reg, 1, &buf, 1); - - return buf; -} - -void i2c_reg_write (uchar chip, uchar reg, uchar val) -{ - i2c_write (chip, reg, 1, &val, 1); - - return; -} - #endif /* CONFIG_HARD_I2C */ diff --git a/cpu/mpc824x/drivers/i2c/i2c.c b/cpu/mpc824x/drivers/i2c/i2c.c index 854345e..637ae4c 100644 --- a/cpu/mpc824x/drivers/i2c/i2c.c +++ b/cpu/mpc824x/drivers/i2c/i2c.c @@ -267,18 +267,4 @@ int i2c_probe (uchar chip) return i2c_read (chip, 0, 1, (uchar *) &tmp, 1); }
-uchar i2c_reg_read (uchar i2c_addr, uchar reg) -{ - uchar buf[1]; - - i2c_read (i2c_addr, reg, 1, buf, 1); - - return (buf[0]); -} - -void i2c_reg_write (uchar i2c_addr, uchar reg, uchar val) -{ - i2c_write (i2c_addr, reg, 1, &val, 1); -} - #endif /* CONFIG_HARD_I2C */ diff --git a/cpu/mpc8260/i2c.c b/cpu/mpc8260/i2c.c index a934193..68c37e6 100644 --- a/cpu/mpc8260/i2c.c +++ b/cpu/mpc8260/i2c.c @@ -753,22 +753,6 @@ i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) return 0; }
-uchar -i2c_reg_read(uchar chip, uchar reg) -{ - uchar buf; - - i2c_read(chip, reg, 1, &buf, 1); - - return (buf); -} - -void -i2c_reg_write(uchar chip, uchar reg, uchar val) -{ - i2c_write(chip, reg, 1, &val, 1); -} - #if defined(CONFIG_I2C_MULTI_BUS) /* * Functions for multiple I2C bus handling diff --git a/cpu/mpc8xx/i2c.c b/cpu/mpc8xx/i2c.c index 29c7c71..338caba 100644 --- a/cpu/mpc8xx/i2c.c +++ b/cpu/mpc8xx/i2c.c @@ -42,19 +42,6 @@ DECLARE_GLOBAL_DATA_PTR; /* define to enable debug messages */ #undef DEBUG_I2C
-/*----------------------------------------------------------------------- - * Set default values - */ -#ifndef CONFIG_SYS_I2C_SPEED -#define CONFIG_SYS_I2C_SPEED 50000 -#endif - -#ifndef CONFIG_SYS_I2C_SLAVE -#define CONFIG_SYS_I2C_SLAVE 0xFE -#endif -/*----------------------------------------------------------------------- - */ - /* tx/rx timeout (we need the i2c early, so we don't use get_timer()) */ #define TOUT_LOOP 1000000
@@ -717,24 +704,4 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) return 0; }
-uchar -i2c_reg_read(uchar i2c_addr, uchar reg) -{ - uchar buf; - - i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); - - i2c_read(i2c_addr, reg, 1, &buf, 1); - - return (buf); -} - -void -i2c_reg_write(uchar i2c_addr, uchar reg, uchar val) -{ - i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); - - i2c_write(i2c_addr, reg, 1, &val, 1); -} - #endif /* CONFIG_HARD_I2C */ diff --git a/cpu/ppc4xx/i2c.c b/cpu/ppc4xx/i2c.c index 0deb149..4456f3f 100644 --- a/cpu/ppc4xx/i2c.c +++ b/cpu/ppc4xx/i2c.c @@ -420,26 +420,6 @@ int i2c_write(uchar chip, uint addr, int alen, uchar * buffer, int len) return (i2c_transfer(0, chip<<1, &xaddr[4-alen], alen, buffer, len ) != 0); }
-/*----------------------------------------------------------------------- - * Read a register - */ -uchar i2c_reg_read(uchar i2c_addr, uchar reg) -{ - uchar buf; - - i2c_read(i2c_addr, reg, 1, &buf, 1); - - return (buf); -} - -/*----------------------------------------------------------------------- - * Write a register - */ -void i2c_reg_write(uchar i2c_addr, uchar reg, uchar val) -{ - i2c_write(i2c_addr, reg, 1, &val, 1); -} - #if defined(CONFIG_I2C_MULTI_BUS) /* * Functions for multiple I2C bus handling diff --git a/cpu/pxa/i2c.c b/cpu/pxa/i2c.c index 08042be..6b72ba1 100644 --- a/cpu/pxa/i2c.c +++ b/cpu/pxa/i2c.c @@ -455,19 +455,4 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
}
-uchar i2c_reg_read (uchar chip, uchar reg) -{ - uchar buf; - - PRINTD(("i2c_reg_read(chip=0x%02x, reg=0x%02x)\n",chip,reg)); - i2c_read(chip, reg, 1, &buf, 1); - return (buf); -} - -void i2c_reg_write(uchar chip, uchar reg, uchar val) -{ - PRINTD(("i2c_reg_write(chip=0x%02x, reg=0x%02x, val=0x%02x)\n",chip,reg,val)); - i2c_write(chip, reg, 1, &val, 1); -} - #endif /* CONFIG_HARD_I2C */ diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c index 281a88b..79fdc6d 100644 --- a/drivers/i2c/fsl_i2c.c +++ b/drivers/i2c/fsl_i2c.c @@ -368,22 +368,6 @@ i2c_probe(uchar chip) return i2c_read(chip, 0, 0, NULL, 0); }
-uchar -i2c_reg_read(uchar i2c_addr, uchar reg) -{ - uchar buf[1]; - - i2c_read(i2c_addr, reg, 1, buf, 1); - - return buf[0]; -} - -void -i2c_reg_write(uchar i2c_addr, uchar reg, uchar val) -{ - i2c_write(i2c_addr, reg, 1, &val, 1); -} - int i2c_set_bus_num(unsigned int bus) { #ifdef CONFIG_SYS_I2C2_OFFSET diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c index ebe60e2..a863348 100644 --- a/drivers/i2c/soft_i2c.c +++ b/drivers/i2c/soft_i2c.c @@ -435,22 +435,3 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) return(failures); }
-/*----------------------------------------------------------------------- - * Read a register - */ -uchar i2c_reg_read(uchar i2c_addr, uchar reg) -{ - uchar buf; - - i2c_read(i2c_addr, reg, 1, &buf, 1); - - return(buf); -} - -/*----------------------------------------------------------------------- - * Write a register - */ -void i2c_reg_write(uchar i2c_addr, uchar reg, uchar val) -{ - i2c_write(i2c_addr, reg, 1, &val, 1); -} diff --git a/include/i2c.h b/include/i2c.h index 8d6f867..fad2d57 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -76,6 +76,20 @@ # define I2C_SOFT_DECLARATIONS # endif #endif + +#ifdef CONFIG_8xx +/* Set default values for the I2C bus speed and slave address on 8xx. In the + * future, we'll define these in all 8xx board config files. + */ +#ifndef CONFIG_SYS_I2C_SPEED +#define CONFIG_SYS_I2C_SPEED 50000 +#endif + +#ifndef CONFIG_SYS_I2C_SLAVE +#define CONFIG_SYS_I2C_SLAVE 0xFE +#endif +#endif + /* * Initialization, must be called once on start up, may be called * repeatedly to change the speed and slave addresses. @@ -132,8 +146,52 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len); /* * Utility routines to read/write registers. */ -uchar i2c_reg_read (uchar chip, uchar reg); -void i2c_reg_write(uchar chip, uchar reg, uchar val); +static inline u8 i2c_reg_read(u8 addr, u8 reg) +{ + u8 buf; + +#ifdef CONFIG_8xx + /* MPC8xx needs this. Maybe one day we can get rid of it. */ + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); +#endif + +#ifdef DEBUG + printf("%s: addr=0x%02x, reg=0x%02x\n", __func__, addr, reg); +#endif + +#ifdef CONFIG_BLACKFIN + /* This ifdef will become unneccessary in a future version of the + * blackfin I2C driver. + */ + i2c_read(addr, reg, 0, &buf, 1); +#else + i2c_read(addr, reg, 1, &buf, 1); +#endif + + return buf; +} + +static inline void i2c_reg_write(u8 addr, u8 reg, u8 val) +{ +#ifdef CONFIG_8xx + /* MPC8xx needs this. Maybe one day we can get rid of it. */ + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); +#endif + +#ifdef DEBUG + printf("%s: addr=0x%02x, reg=0x%02x, val=0x%02x\n", + __func__, addr, reg, val); +#endif + +#ifdef CONFIG_BLACKFIN + /* This ifdef will become unneccessary in a future version of the + * blackfin I2C driver. + */ + i2c_write(addr, reg, 0, &val, 1); +#else + i2c_write(addr, reg, 1, &val, 1); +#endif +}
/* * Functions for setting the current I2C bus and its speed
On 11:28 Wed 03 Dec , Timur Tabi wrote:
All implementations of the functions i2c_reg_read() and i2c_reg_write() are identical. We can save space and simplify the code by converting these functions into inlines and putting them in i2c.h.
Signed-off-by: Timur Tabi timur@freescale.com
Ack-By: Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com
Best Regards, J.
Dear Timur Tabi,
In message 1228325310-19275-1-git-send-email-timur@freescale.com you wrote:
All implementations of the functions i2c_reg_read() and i2c_reg_write() are identical. We can save space and simplify the code by converting these functions into inlines and putting them in i2c.h.
Signed-off-by: Timur Tabi timur@freescale.com
v3: add 8xx-specific code, and include PRINTDs from blackfin and pxa
I'm posting this patch because I'm enhancing the I2C routines to support multiple I2C busses more easily, but I need to clean up the existing code first.
I'm going to be on vacation when the next merge window opens, so I'm posting this now in the hopes that it will be picked up when the window does open.
We've been discussing this long enough when the previous window was open, so ...
cpu/arm920t/at91rm9200/i2c.c | 14 --------- cpu/arm926ejs/davinci/i2c.c | 17 ----------- cpu/blackfin/i2c.c | 16 ---------- cpu/mpc512x/i2c.c | 17 ----------- cpu/mpc5xxx/i2c.c | 16 ---------- cpu/mpc8220/i2c.c | 16 ---------- cpu/mpc824x/drivers/i2c/i2c.c | 14 --------- cpu/mpc8260/i2c.c | 16 ---------- cpu/mpc8xx/i2c.c | 33 --------------------- cpu/ppc4xx/i2c.c | 20 ------------- cpu/pxa/i2c.c | 15 ---------- drivers/i2c/fsl_i2c.c | 16 ---------- drivers/i2c/soft_i2c.c | 19 ------------ include/i2c.h | 62 +++++++++++++++++++++++++++++++++++++++- 14 files changed, 60 insertions(+), 231 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
On Mon, 15 Dec 2008, Wolfgang Denk wrote:
Are you going to implement support for multiple I2C busses on some boards? I'm working on something like this now so it would be nice to coordinate our efforts somehow...
There is a provision for using 2 I2C busses in fsl-i2c.c now but I'm trying to make it more uniform to allow for different controllers on the same board. I have the first prototype of our MPC8548E based motherboard in PCB fab right now and I do have 3 I2C busses on it all of which are required for U-Boot. That is 2 busses off of 2 MPC8548E I2C controllers and the third one off of Silicon Motion SM502 MFD.
I have SM502 I2C driver written but not tested (no real hardware to test it on yet, will be ready in something like a month) but the entire multibus multicontroller I2C framework is not in U-Boot yet.
Are you working on something like this or just on moving i2c_reg_read() and i2c_reg_write() functions to inlines?
Dear Timur Tabi,
In message 1228325310-19275-1-git-send-email-timur@freescale.com you wrote:
All implementations of the functions i2c_reg_read() and
i2c_reg_write() are
identical. We can save space and simplify the code by converting
these
functions into inlines and putting them in i2c.h.
Signed-off-by: Timur Tabi timur@freescale.com
v3: add 8xx-specific code, and include PRINTDs from blackfin and pxa
I'm posting this patch because I'm enhancing the I2C routines to
support
multiple I2C busses more easily, but I need to clean up the existing
code
first.
I'm going to be on vacation when the next merge window opens, so I'm
posting
this now in the hopes that it will be picked up when the window does
open.
We've been discussing this long enough when the previous window was open, so ...
cpu/arm920t/at91rm9200/i2c.c | 14 --------- cpu/arm926ejs/davinci/i2c.c | 17 ----------- cpu/blackfin/i2c.c | 16 ---------- cpu/mpc512x/i2c.c | 17 ----------- cpu/mpc5xxx/i2c.c | 16 ---------- cpu/mpc8220/i2c.c | 16 ---------- cpu/mpc824x/drivers/i2c/i2c.c | 14 --------- cpu/mpc8260/i2c.c | 16 ---------- cpu/mpc8xx/i2c.c | 33 --------------------- cpu/ppc4xx/i2c.c | 20 ------------- cpu/pxa/i2c.c | 15 ---------- drivers/i2c/fsl_i2c.c | 16 ---------- drivers/i2c/soft_i2c.c | 19 ------------ include/i2c.h | 62
+++++++++++++++++++++++++++++++++++++++-
14 files changed, 60 insertions(+), 231 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de All these theories, diverse as they are, have two things in common: they explain the observed facts, and they are completeley and utterly wrong. - Terry Pratchett, _The Light Fantastic_ _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
ksi@koi8.net wrote:
On Mon, 15 Dec 2008, Wolfgang Denk wrote:
Are you going to implement support for multiple I2C busses on some boards? I'm working on something like this now so it would be nice to coordinate our efforts somehow...
Yes, my goal is to add low-level i2c functions that take an i2c bus as a parameter. So instead of
i2c_set_bus_num(1); i2c_read(...);
you can do
i2c_read(1, ...)
or something like that. My goal is to eliminate i2c_set_bus_num() and everything related to it.
It sounds like we're working on something very similar. This is very low-priority for me, though.
On Mon, 15 Dec 2008, Timur Tabi wrote:
ksi@koi8.net wrote:
On Mon, 15 Dec 2008, Wolfgang Denk wrote:
Are you going to implement support for multiple I2C busses on some
boards?
I'm working on something like this now so it would be nice to
coordinate our
efforts somehow...
Yes, my goal is to add low-level i2c functions that take an i2c bus as a parameter. So instead of
i2c_set_bus_num(1); i2c_read(...);
you can do
i2c_read(1, ...)
or something like that. My goal is to eliminate i2c_set_bus_num() and everything related to it.
It sounds like we're working on something very similar. This is very low-priority for me, though.
That looks similar. But why do you want to remove i2c_set_bus_num()? I think it would be less work to keep it. This way you can leave 90% or so of existing I2C code unchanged by setting bus number to 0 at init. In your case you will have to find each and every call to i2c_read... and change it to the new format that includes bus number in parameters list.
My idea is to have global bus number variable in a single place and a single i2c_set_bus_num() that can be excluded for most boards with a single bus with #ifdef...
Then, we could use some kind of array of I2C structures each containing pointers to appropriate i2c-{read,write,probe,init}() functions with generic i2c functions just calling those pointers using bus number as index into that array.
That would allow for unlimited number of different adapters for any board. Initial code for initializing such an array would have to go into each and every $(BOARD).c board specific file.
We could even make some default #ifdef'd structure that would've included automagically if something like CONFIG_I2C_MULTIBUS was not defined thus making most of the boards (actually almost all of them) work without modifications.
Please let me know what you think...
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
ksi@koi8.net wrote:
That looks similar. But why do you want to remove i2c_set_bus_num()? I think it would be less work to keep it.
Perhaps, but it would be even better to get rid of it. IMHO, it's a kludge. It was a hack added to allow existing I2C routines to function while adding minimal support for multiple buses on those platforms that needed it.
This way you can leave 90% or so of existing I2C code unchanged by setting bus number to 0 at init.
I only intend on exporting the multiple-bus versions of the I2C function if CONFIG_I2C_MULTI_BUS is defined.
My idea is to have global bus number variable in a single place and a single i2c_set_bus_num() that can be excluded for most boards with a single bus with #ifdef...
We already have something like that. A global variable is inconvenient because every time you want to access the bus, you need to do something like this:
bus = i2c_get_bus_num(); i2c_set_bus_num(x); i2c_write(...) i2c_set_bus_num(bus);
We need to save/restore the current bus number, because the I2C command-line has the concept of a
Then, we could use some kind of array of I2C structures each containing pointers to appropriate i2c-{read,write,probe,init}() functions with generic i2c functions just calling those pointers using bus number as index into that array.
Sounds complicated.
That would allow for unlimited number of different adapters for any board.
Ah, now this is something else entirely. I don't think U-boot supports this at all. I think you're being too ambitious. It's a noble idea, and I think U-boot should support it, but I think we need to simplify the support for multiple buses first.
Initial code for initializing such an array would have to go into each and every $(BOARD).c board specific file.
Ugh.
On Tue, 16 Dec 2008, Timur Tabi wrote:
ksi@koi8.net wrote:
That looks similar. But why do you want to remove i2c_set_bus_num()? I
think
it would be less work to keep it.
Perhaps, but it would be even better to get rid of it. IMHO, it's a kludge. It was a hack added to allow existing I2C routines to function while adding minimal support for multiple buses on those platforms that needed it.
I have nothing against changing parameters list for i2c_read/write(), it's just more work and less elegant.
This way you can leave 90% or so of existing I2C code unchanged by setting bus number to 0 at init.
I only intend on exporting the multiple-bus versions of the I2C function if CONFIG_I2C_MULTI_BUS is defined.
That looks messy... Why would we use two different versions if we can make everything uniform?
My idea is to have global bus number variable in a single place and a
single
i2c_set_bus_num() that can be excluded for most boards with a single
bus
with #ifdef...
We already have something like that. A global variable is inconvenient because every time you want to access the bus, you need to do something like this:
bus = i2c_get_bus_num(); i2c_set_bus_num(x); i2c_write(...) i2c_set_bus_num(bus);
We need to save/restore the current bus number, because the I2C command-line has the concept of a
Eh, you can just set bus number every time you're gonna do i2c read/write... That i2c_get_bus_num() doesn't make any sence at all. Just set bus number every time you access i2c device. U-boot is single-task so there is no other process that can change it from under you and you do not save anything with checking that bus number. I can't see any practical use for i2c_get_bus_num() except for showing it with an interactive command. That is, in turn, is also redundant -- one would not save any keystrokes by typing something like "i2c bus" and not typing "i2c bus X" if that X is what he wants. Just type the second command every time when in doubt and you won't have to type the first one :)
Also, for all boards with a single I2C bus we can assume bus=0 so there is no need for bus-related functions/commands at all...
Then, we could use some kind of array of I2C structures each
containing
pointers to appropriate i2c-{read,write,probe,init}() functions with
generic
i2c functions just calling those pointers using bus number as index
into
that array.
Sounds complicated.
What is complicated? It is something like 3-4 lines of code per I2C bus. Look how e.g. NAND subsystem is initialized...
And, BTW, this initialization can even go into include/configs/$(BOARD).h.
That would allow for unlimited number of different adapters for any
board.
Ah, now this is something else entirely. I don't think U-boot supports this at all. I think you're being too ambitious. It's a noble idea, and I think U-boot should support it, but I think we need to simplify the support for multiple buses first.
It ain't rocket science :) The reason why I'm doing this is exactly what you said -- U-boot does NOT support it at all. And I do really need this and I think I'm not the only one.
Initial code for initializing such an array would have to go into each
and
every $(BOARD).c board specific file.
Ugh.
Ah, that's not biggie. And I volunteer to do this and come up with patches.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
ksi@koi8.net wrote:
That looks messy... Why would we use two different versions if we can make everything uniform?
Because we already have something that makes it uniform, and it's broken. The idea of having a "current i2c bus" that needs to be set before read/write operations can be performed is the broken part!
Eh, you can just set bus number every time you're gonna do i2c read/write...
Not with the current i2c command line. We would need another global variable in the i2c command line code to store what IT thinks is the current bus.
That i2c_get_bus_num() doesn't make any sence at all. Just set bus number every time you access i2c device.
That's risky. Sooner or later, you will want to know what the current bus number is, at least for debugging or status purposes.
U-boot is single-task so there is no other process that can change it from under you and you do not save anything with checking that bus number.
Sounds to me like you haven't really looked at the U-Boot code. There are plenty of places where one function does I2C operations, then calls another function that does its own.
I think all this boils down to one core disagreement we have: I think the idea of a "current" i2c bus is a bad one.
On Tue, 16 Dec 2008, Timur Tabi wrote:
ksi@koi8.net wrote:
That looks messy... Why would we use two different versions if we can
make
everything uniform?
Because we already have something that makes it uniform, and it's broken. The idea of having a "current i2c bus" that needs to be set before read/write operations can be performed is the broken part!
Eh, we don't have any uniformity. That "uniformity" we do have is only for a trivial case of a single I2C bus. Everything else is a bunch of SoC specific hacks made differently fo different platforms. The fsl-i2c.c e.g. uses local bus number variable.
Eh, you can just set bus number every time you're gonna do i2c
read/write...
Not with the current i2c command line. We would need another global variable in the i2c command line code to store what IT thinks is the current bus.
That can be taken care of.
That i2c_get_bus_num() doesn't make any sence at all. Just set bus
number
every time you access i2c device.
That's risky. Sooner or later, you will want to know what the current bus number is, at least for debugging or status purposes.
So what? Just use i2c_get_bus() for this... We can even add a string to i2c_func structure for textual representation.
U-boot is single-task so there is no other process that can change it from under you and you do not save anything
with
checking that bus number.
Sounds to me like you haven't really looked at the U-Boot code. There are plenty of places where one function does I2C operations, then calls another function that does its own.
So what? Does it make it multitasking? Can that another function preempt the first one and change bus number from under it?
I think all this boils down to one core disagreement we have: I think the idea of a "current" i2c bus is a bad one.
I can see nothing wrong with that but I also have nothing against changing all i2c_{read,write,probe}() functions to take bus number as an argument.
I offered 4 possible scenarios and additional parameter to i2c functions was one of them. Wolfgang said that current bus approach looks better than others and I agree with him. But it is not rocket science to use an additional parameter either. The only thing is it MUST be consistent, i.e. we should NOT have 2 different sets of functions based on CONFIG_MULTIBUS or whatever. If we are to make this change ALL boards must be multibus with majority of them having bus count of 1.
Does anybody else have something to say on this? I'm going to write code so let's make some decision. I don't want this to end up as a company hack and making it properly affects a lot of U-boot...
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
ksi@koi8.net wrote:
On Tue, 16 Dec 2008, Timur Tabi wrote:
ksi@koi8.net wrote:
That looks messy... Why would we use two different versions if we can
make
everything uniform?
Because we already have something that makes it uniform, and it's broken. The idea of having a "current i2c bus" that needs to be set before read/write operations can be performed is the broken part!
Eh, we don't have any uniformity. That "uniformity" we do have is only for a trivial case of a single I2C bus. Everything else is a bunch of SoC specific hacks made differently fo different platforms. The fsl-i2c.c e.g. uses local bus number variable.
[snip]
I think all this boils down to one core disagreement we have: I think the idea of a "current" i2c bus is a bad one.
I can see nothing wrong with that but I also have nothing against changing all i2c_{read,write,probe}() functions to take bus number as an argument.
I offered 4 possible scenarios and additional parameter to i2c functions was one of them. Wolfgang said that current bus approach looks better than others and I agree with him. But it is not rocket science to use an additional parameter either. The only thing is it MUST be consistent, i.e. we should NOT have 2 different sets of functions based on CONFIG_MULTIBUS or whatever. If we are to make this change ALL boards must be multibus with majority of them having bus count of 1.
Does anybody else have something to say on this? I'm going to write code so let's make some decision. I don't want this to end up as a company hack and making it properly affects a lot of U-boot...
IMHO Sergey's approach is reasonable. It is pragmatic in that it builds on and improves the existing method. It only touches boards with multiple i2c buses.
The best is the enemy of the good. - Voltaire http://www.bartleby.com/66/2/63002.html
My two cents, gvb
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0812161102580.28667@home-gw.koi8.net you wrote:
I offered 4 possible scenarios and additional parameter to i2c functions was one of them. Wolfgang said that current bus approach looks better than others and I agree with him. But it is not rocket science to use an
You and me agree on this.
additional parameter either. The only thing is it MUST be consistent, i.e. we should NOT have 2 different sets of functions based on CONFIG_MULTIBUS or
We agree on this, too.
whatever. If we are to make this change ALL boards must be multibus with majority of them having bus count of 1.
I don't think this makes sense - it would just add complexity and memory footprint without added benefit.
Does anybody else have something to say on this? I'm going to write code so let's make some decision. I don't want this to end up as a company hack and making it properly affects a lot of U-boot...
I support your position, not Timur's.
Best regards,
Wolfgang Denk
On Tue, 16 Dec 2008, Wolfgang Denk wrote:
Dear ksi@koi8.net,
In message Pine.LNX.4.64ksi.0812161102580.28667@home-gw.koi8.net you wrote:
I offered 4 possible scenarios and additional parameter to i2c
functions was
one of them. Wolfgang said that current bus approach looks better than others and I agree with him. But it is not rocket science to use an
You and me agree on this.
additional parameter either. The only thing is it MUST be consistent,
i.e.
we should NOT have 2 different sets of functions based on
CONFIG_MULTIBUS or
We agree on this, too.
whatever. If we are to make this change ALL boards must be multibus
with
majority of them having bus count of 1.
I don't think this makes sense - it would just add complexity and memory footprint without added benefit.
Does anybody else have something to say on this? I'm going to write
code so
let's make some decision. I don't want this to end up as a company
hack and
making it properly affects a lot of U-boot...
I support your position, not Timur's.
OK, so I'm on it. It might take a couple of weeks for patches to show up.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear Timur,
In message 4947F8B4.8070804@freescale.com you wrote:
ksi@koi8.net wrote:
That looks messy... Why would we use two different versions if we can make everything uniform?
Because we already have something that makes it uniform, and it's broken. The idea of having a "current i2c bus" that needs to be set before read/write operations can be performed is the broken part!
Hm... what exactly is broken with the concept of having a "current device" or a "current bus"? We use it elasewhere, too (like for selection IDE or S-ATA devices and such), and so far I am not aware of fundamental issues because of that.
You sound as if we were designing a system where tens of independend users could log in simultaneously and concurrently access different devices on different busses.
But this is NOT the case. We're strictly single user, single threaded, and when one command is runnign we are certain that there cannot be any interference from other I2C accesses.
Eh, you can just set bus number every time you're gonna do i2c read/write...
Not with the current i2c command line. We would need another global variable in the i2c command line code to store what IT thinks is the current bus.
Not really. It is really sufficient to set the bus for each command. There is no need to save and restore any previous state, because accesses cannot be nested because of the way how U-Boot is designed.
That i2c_get_bus_num() doesn't make any sence at all. Just set bus number every time you access i2c device.
That's risky. Sooner or later, you will want to know what the current bus number is, at least for debugging or status purposes.
The design has been discussed before, and I see little reason why we should not throw all this away and come up with something new, more complicated.
Sounds to me like you haven't really looked at the U-Boot code. There are plenty of places where one function does I2C operations, then calls another function that does its own.
Really? Where exactly does such a thing happen?
I tend to call this a bug that needs to be fixed, if there is ssuch code.
I think all this boils down to one core disagreement we have: I think the idea of a "current" i2c bus is a bad one.
This statement is probably correct: I don't share your point of view here, either.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Hm... what exactly is broken with the concept of having a "current device" or a "current bus"? We use it elasewhere, too (like for selection IDE or S-ATA devices and such), and so far I am not aware of fundamental issues because of that.
I think it's a kludge because you have to set the current device before you can access it. It seems ridiculous that you have to do this:
i2c_set_bus_num(x) i2c_write(...)
when you could do this:
i2c_write(x, ...)
Sounds to me like you haven't really looked at the U-Boot code. There are plenty of places where one function does I2C operations, then calls another function that does its own.
Really? Where exactly does such a thing happen?
Well, I can't find a real example, but here's something close. Take a look at function pib_init() in mpc8568mds.c. This function needs to talk to the 2nd I2C bus. It has no idea who called it, so it has no idea what the current I2C bus is. Therefore, it has to save and restore it.
Now consider the code that calls pib_init(). Technically, this code cannot know that pib_init() does I2C operations. So it doesn't know that pib_init() could change the current I2C bus. So it wouldn't know to save and restore what it needs.
This situation could happen anywhere.
I tend to call this a bug that needs to be fixed, if there is ssuch code.
It's not a bug. Function X does I2C operations, and function Y also does I2C operations, but on a different device. If function X calls function Y, then Y needs to save and restore the current bus number. You will never get rid of this requirement as long as you have the concept of a current I2C bus number.
So you're still going to need i2c_get_bus_num() and i2c_set_bus_num(), and you're still going to have code like this:
bus = i2c_get_bus_num(); i2c_set_bus_num(x); i2c_write(...) i2c_set_bus_num(bus);
This statement is probably correct: I don't share your point of view here, either.
In that case, I have no interest in working on this new I2C design. You guys obviously have everything under control, so good luck.
On Tue, 16 Dec 2008, Timur Tabi wrote:
Wolfgang Denk wrote:
Hm... what exactly is broken with the concept of having a "current device" or a "current bus"? We use it elasewhere, too (like for selection IDE or S-ATA devices and such), and so far I am not aware of fundamental issues because of that.
I think it's a kludge because you have to set the current device before you can access it. It seems ridiculous that you have to do this:
i2c_set_bus_num(x) i2c_write(...)
when you could do this:
i2c_write(x, ...)
Only if you have more than one I2C bus that 95% of supported boards don't have (95% is a wild guess; in reality, methinks, it is even closer to 99%.) That means you should only do this for less than 5% of existing boards.
Adding a parameter would require you to make changes for 100% of boards.
common/cmd_i2c.c is a single file, common for all platforms so it is a special case.
Sounds to me like you haven't really looked at the U-Boot code.
There are
plenty of places where one function does I2C operations, then calls another function that does its own.
Really? Where exactly does such a thing happen?
Well, I can't find a real example, but here's something close. Take a look at function pib_init() in mpc8568mds.c. This function needs to talk to the 2nd I2C bus. It has no idea who called it, so it has no idea what the current I2C bus is. Therefore, it has to save and restore it.
Now consider the code that calls pib_init(). Technically, this code cannot know that pib_init() does I2C operations. So it doesn't know that pib_init() could change the current I2C bus. So it wouldn't know to save and restore what it needs.
This situation could happen anywhere.
No, that is not an issue. That pib_init() does not happen out of a blue, it is YOU who call it in $(BOARD).c file so you MUST know what you are doing.
I tend to call this a bug that needs to be fixed, if there is ssuch code.
It's not a bug. Function X does I2C operations, and function Y also does I2C operations, but on a different device. If function X calls function Y, then Y needs to save and restore the current bus number. You will never get rid of this requirement as long as you have the concept of a current I2C bus number.
So you're still going to need i2c_get_bus_num() and i2c_set_bus_num(), and you're still going to have code like this:
bus = i2c_get_bus_num(); i2c_set_bus_num(x); i2c_write(...) i2c_set_bus_num(bus);
This statement is probably correct: I don't share your point of view here, either.
In that case, I have no interest in working on this new I2C design. You guys obviously have everything under control, so good luck.
-- Timur Tabi Linux Kernel Developer @ Freescale
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Dear Timur,
In message 49483DD7.5080908@freescale.com you wrote:
I think it's a kludge because you have to set the current device before you can access it. It seems ridiculous that you have to do this:
i2c_set_bus_num(x) i2c_write(...)
when you could do this:
i2c_write(x, ...)
That's your argument. The other side's argument is that it's overhead (note that I do not use such a strong term as "ridiculous" as you did) to add a bus number argument to each and every board, where only a tiny fraction of them will ever have mnore than one I2C bus used.
So there is arguments bor both implementations, and it was decided to implement the first approach.
We've been trhough this before, it makes little sense that yous tir this up again.
Really? Where exactly does such a thing happen?
Well, I can't find a real example, but here's something close. Take a look at function pib_init() in mpc8568mds.c. This function needs to talk to the 2nd I2C bus. It has no idea who called it, so it has no idea what the current I2C bus is. Therefore, it has to save and restore it.
IUt has to set it, yes. But there is no need to restore it, because there are no nested I2C calls.
This situation could happen anywhere.
It could, if you intentionally create such a case. In real life, it never happens. Or can trivially be avoided.
It's not a bug. Function X does I2C operations, and function Y also does I2C operations, but on a different device. If function X calls function Y, then Y needs to save and restore the current bus number. You will never get rid of this requirement as long as you have the concept of a current I2C bus number.
There are no examples for such code. And I cannot see where it would be necessary.
So you're still going to need i2c_get_bus_num() and i2c_set_bus_num(), and you're still going to have code like this:
bus = i2c_get_bus_num(); i2c_set_bus_num(x); i2c_write(...) i2c_set_bus_num(bus);
Nope. This is not necessary.
I repeat: there are no nested I2C calls.
That's full in line with the design philosophy that we are strictly single tasking. There are no multiple tasks using the same driver or bus.
In that case, I have no interest in working on this new I2C design. You guys obviously have everything under control, so good luck.
C'me on. Don't act like a prima donna.
Best regards,
Wolfgang Denk
On Mon, Dec 15, 2008 at 04:24:26PM -0800, ksi@koi8.net wrote:
Then, we could use some kind of array of I2C structures each containing pointers to appropriate i2c-{read,write,probe,init}() functions with generic i2c functions just calling those pointers using bus number as index into that array.
Why not just pass a pointer to the i2c device, rather than an index?
-Scott
On Tue, 16 Dec 2008, Scott Wood wrote:
On Mon, Dec 15, 2008 at 04:24:26PM -0800, ksi@koi8.net wrote:
Then, we could use some kind of array of I2C structures each
containing
pointers to appropriate i2c-{read,write,probe,init}() functions with
generic
i2c functions just calling those pointers using bus number as index
into
that array.
Why not just pass a pointer to the i2c device, rather than an index?
Because it is easier to change bus with i2c_set_bus() function and then use regular i2c_{read,write,probe}() functions. We are NOT passing anything. One global variable, bus_num or so, that is changed with i2c_set_bus() is used as an index for any i2c_{read,write,probe}() function. Those functions are just one line inlines calling appropriate pointers in i2c_func[bus_num] area. This way one can still use regular e.g. i2c_write() that translates to i2c_func[bus_num].i2c_write.
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
participants (6)
-
Jean-Christophe PLAGNIOL-VILLARD -
Jerry Van Baren -
ksi＠koi8.net -
Scott Wood -
Timur Tabi -
Wolfgang Denk