Hello Dirk,
Dirk Behme wrote:
> Tom Rix wrote:
>> From: Syed Mohammed Khasim <kha...@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 <kha...@ti.com>
>> Signed-off-by: Jason Kridner <jkrid...@beagleboard.org>
>> Signed-off-by: Tom Rix <tom....@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_v2
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;h=e1bb67f1c136f5904c195d31d2916b17a30c9632;hb=6ef1bb6e9a0c03d5cbc8ef296b5022b5698207a6
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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
