Hello Stephen,
Stephen Warren wrote:
Simon Glass wrote ednesday, January 11, 2012 9:18 PM:
On Mon, Jan 9, 2012 at 2:07 PM, Stephen Warren swarren@nvidia.com wrote:
On 12/26/2011 11:11 AM, Simon Glass wrote:
From: Yen Lin yelin@nvidia.com
Add basic i2c driver for Tegra2 with 8- and 16-bit address support. The driver supports building both with and without CONFIG_OF_CONTROL.
Without CONFIG_OF_CONTROL a number of CONFIG options must be supplied in the board config header file:
I2CSPEED_KHZ - speed to run I2C bus at (typically 100000) CONFIG_I2Cx_PIN_MUX - pin mux setting for each port (P, 1, 2, 3) (typically this will be 0 to bring the port out the common pins)
...
...
diff --git a/drivers/i2c/tegra2_i2c.c b/drivers/i2c/tegra2_i2c.c
...
+struct i2c_bus i2c_controllers[CONFIG_SYS_MAX_I2C_BUS];
What if there are I2C bus extenders/muxes/... such that there are more I2C buses in the system than Tegra I2C controllers? I'd rather see an explicit TEGRA_I2C_NUM_CONTROLLERS define used throughout this patch.
We don't actually support CONFIG_I2C_MUX, so I can't see how that could happen. Can you please explain a bit more?
We may not support it now, but I see no reason we won't in the future. If we confuse the two defines now, it'll make it harder to allow muxes in the future. The fix is simply using the correct define name within the I2C driver isn't it; pretty simple.
If we really have this case, we *must* get the multibus/multiadapter branch from here:
http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus...
to uboot mainline! With that approach we could handle this case in a clean way ... This branch is not in sync with current TOT, and it has to be tested again, as I didn't found time for this in the last year :-( Also, as its base is >2 years old, there are a lot of checkpatch errors in this patchserie, which have to be cleaned up ... hope I get some time for it ... help is welcome.
+int i2c_init_board(void) +{
[...]
Surely that function needs to actually do something, at least set up the clocks so that the (user?) requested rate is honored, or print an error message if you're only allowed to use the hard-coded bus rate.
See my other message. I suppose we could reinit, but we really don't want to honour the speed, since the fdt speed setting is then lost and irrecoverable. For now it feels like we should ignore it.
Hmm. I suspect the answer here is roughly to override the following in cmd_i2c.c:
/* TODO: Implement architecture-specific get/set functions */ unsigned int __def_i2c_get_bus_speed(void) { return CONFIG_SYS_I2C_SPEED; } unsigned int i2c_get_bus_speed(void) __attribute__((weak, alias("__def_i2c_get_bus_speed")));
int __def_i2c_set_bus_speed(unsigned int speed) { if (speed != CONFIG_SYS_I2C_SPEED) return -1;
return 0;} int i2c_set_bus_speed(unsigned int) __attribute__((weak, alias("__def_i2c_set_bus_speed")));
To actually read/write the rate in use by the driver.
Yep, for this reason this functions are weak, and you can define a driver specific function which returns the current used settings in the driver.
Code should compile, if tegra2 not define CONFIG_SYS_I2C_SPEED, as a "default" value for CONFIG_SYS_I2C_SPEED is defined in include/i2c.h ...
Then, fix do_i2c_reset() to use i2c_get_bus_speed() so it interacts correctly with those functions.
Yep, I think, you are right here, that should be fixed. As the default function for i2c_get_bus_speed returns CONFIG_SYS_I2C_SPEED, that should be OK.
There may be other places that need to be updates to use those function instead of hard-coding CONFIG_SYS_I2C_SPEED too. Perhaps we could even get away without defining CONFIG_SYS_I2C_SPEED for Tegra since it's meaningless. Instead, ifdef those default function definitions above based on whether CONFIG_SYS_I2C_SPEED is defined or not.
bye, Heiko