Hello Dirk,
Dirk Behme wrote:
Tom Rix wrote:
From: Syed Mohammed Khasim khasim@ti.com
This was cherry-picked from
repo: http://www.beagleboard.org/u-boot-arm.git commit: 52eddcd07c2e7ad61d15bab2cf2d0d21466eaca2
In addition to adding multibus support, this patch also cleans up the register access. The register access has been changed from #defines to a structure.
Have you looked at my proposal I sent some hours before your patch?
http://lists.denx.de/pipermail/u-boot/2009-October/063556.html
Signed-off-by: Syed Mohammed Khasim khasim@ti.com Signed-off-by: Jason Kridner jkridner@beagleboard.org Signed-off-by: Tom Rix Tom.Rix@windriver.com
drivers/i2c/omap24xx_i2c.c | 220 ++++++++++++++++++++++------------- include/asm-arm/arch-omap24xx/i2c.h | 52 ++++++--- include/asm-arm/arch-omap3/i2c.h | 48 +++++--- include/configs/omap3_beagle.h | 1 + 4 files changed, 209 insertions(+), 112 deletions(-)
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index 1a4c8c9..763d2f8 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c @@ -1,7 +1,7 @@ /*
- Basic I2C functions
- Copyright (c) 2004 Texas Instruments
- Copyright (c) 2004, 2009 Texas Instruments
- This package is free software; you can redistribute it and/or
- modify it under the terms of the license found in the file
@@ -25,10 +25,18 @@ #include <asm/arch/i2c.h> #include <asm/io.h>
+#ifdef CONFIG_OMAP34XX +#define I2C_NUM_IF 3 +#else +#define I2C_NUM_IF 2 +#endif
I prefer something like I2C_BUS_MAX for this. And move it to header file. Moving it OMAP2 and OMAP3 i2c.h headers will remove the #ifdef, too.
Yep, I2C_BUS_MAX would be better.
[...]
@@ -398,8 +406,58 @@ static u16 wait_for_pin (void)
if (timeout <= 0) { printf ("timed out in wait_for_pin: I2C_STAT=%x\n",
readw (I2C_STAT));writew(0xFFFF, I2C_STAT);
readw(&i2c->stat));writew(0xFFFF, &i2c->stat);} return status; }
+int i2c_set_bus_num(unsigned int bus)
Do we need an extern declaration in i2c.h for this? To be able to call it from somewhere else without warning?
Hmm.. isn;t it declared in i2c.h?
+{
- if ((bus < 0) || (bus >= I2C_NUM_IF)) {
As mentioned above, I'd like something like bus max here.
printf("Bad bus ID-%d\n", bus);return -1;- }
+#if defined(CONFIG_OMAP34XX)
- if (bus == 2)
i2c = (i2c_t *)I2C_BASE3;- else
+#endif
- if (bus == 1)
i2c = (i2c_t *)I2C_BASE2;- else
i2c = (i2c_t *)I2C_BASE1;- return 0;
+}
I would remove the following three functions as they are not needed at the moment (i.e. without command line interface).
Hmm... really? If someone uses multibus support, this functions are needed, or?
+unsigned int i2c_get_bus_num(void) +{
- if (i2c == (i2c_t *)I2C_BASE1)
return 0;- else
- if (i2c == (i2c_t *)I2C_BASE2)
return 1;+#if defined(CONFIG_OMAP34XX)
- else
- if (i2c == (i2c_t *)I2C_BASE3)
return 2;+#endif
- else
return 0xFFFFFFFF;+}
+/*
- To be Implemented
- */
+int i2c_set_bus_speed(unsigned int speed) +{
- return 0;
+}
+unsigned int i2c_get_bus_speed(void) +{
- return 0;
+}
[...]
diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h index 19a5ec9..d5a0d49 100644 --- a/include/configs/omap3_beagle.h +++ b/include/configs/omap3_beagle.h @@ -128,6 +128,7 @@ #define CONFIG_SYS_I2C_BUS 0 #define CONFIG_SYS_I2C_BUS_SELECT 1 #define CONFIG_DRIVER_OMAP34XX_I2C 1 +#define CONFIG_I2C_MULTI_BUS 1
Not needed.
While all above are only style questions, what's about the main functionality topic:
Do we have to call i2c_init() for bus 1 and 2 if switching to it? If yes, who does it? For bus 0 it's already in the code. I'd like that the user doesn't have to care about it. Therefore, I added
I solved this in the multibus_v2 branch, see:
http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus...
i2c_init() gets just called if switching to an I2C bus. This is done in i2c_set_bus_num()
http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=blob;f=drivers/i2c/i2c_core.c;...
pro:
a I2C adapter gets only initialized, if used, and there is the option to extend this functionality to: - when switching to another i2c adapter call (a not yet existing) i2c adapter deinit function ...
static unsigned int bus_initialized[3] = {0, 0, 0}; static unsigned int current_bus = 0;
void i2c_init (int speed, int slaveadd) { ... bus_initialized[current_bus] = 1; }
int i2c_set_bus_num(unsigned int bus) { ... current_bus = bus;
if(!bus_initialized[current_bus]) i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); ... }
Ah, you also tend to this direction ;-) Great!
bye Heiko