+U-Boot, which I seemed to have dropped by mistake Hi Stephen,
On Thu, Jan 12, 2012 at 10:59 AM, Stephen Warren <swar...@nvidia.com> wrote: > Simon Glass wrote at Wednesday, January 11, 2012 8:00 PM: >> On Mon, Jan 9, 2012 at 1:45 PM, Stephen Warren <swar...@nvidia.com> wrote: >> > On 12/26/2011 11:11 AM, Simon Glass wrote: >> >> This enables I2C on Seaboard. >> > ... >> >> diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h >> > ... >> >> +#define CONFIG_SYS_I2C_INIT_BOARD >> > >> > I don't think that option is correct for Seaboard; the description in >> > the README indicates this causes a function named i2c_init_board() to be >> > called from boards/xxx/board.c, which is supposed to use GPIOs to unhang >> > the I2C bus. However, this raises a couple of issues: >> > >> > 1) Patch 5 in this series calls i2c_init_board() ifdef CONFIG_TEGRA2_I2C >> > rather than depending on this CONFIG option. >> > >> > 2) Tegra's i2c_init_board() doesn't appear to be anything to do with bus >> > unhanging, but is instead regular I2C initialization. Perhaps the >> > function should be renamed? >> >> I have had a bit of a look at this. From what I can see the problem is >> that i2c_init() is passed a bus speed, but this is just >> CONFIG_SYS_I2C_SPEED. We want to control the speed individually for >> each port. Yes would could use i2c_init() and ignore the speed, but >> that seems odd also. At least with the way it is we don't ignore the >> setting. >> >> We also don't define CONFIG_HARD_I2C which again seems odd, but I >> suppose is for the same reason (we don't want to call i2c_init()). > > Hmm. It sounds like we should replace CONFIG_TEGRA2_I2C with > CONFIG_HARD_I2C given the README description of the latter? Well we could, but we would need to ignore the speed argument. Do you think that is better? > >> Also U-Boot tends to call i2c_init() before every use of i2c, whereas >> we just want to init once and be done with it. >> >> I think the function name is correct, but perhaps I should change the >> #ifdef you mention in 1 above to CONFIG_SYS_I2C_INIT_BOARD. But for >> i2c to function on Tegra boards, this must be defined, so in fact this >> might be counterproductive. So perhaps a check that it is defined is >> best? > > But README explicitly says that i2c_init_board() is for bus unhanging > which isn't what the Tegra implementation does. How can the function > name be correct given that? > Well we should rename the function IMO. It seems to me that with a a name like that it is for initing i2c on the board. > Don't we just want to make i2c_init() use a global/static variable to > determine whether the device has already been initialized, and defer all > the init until first usage or something like that? That seems in line > with U-Boot's desire not to initialize drivers until they're actually > used. Actually that might work - if we keep i2c_init() a no-op, and wait until we get a request for a bus before we look up the fdt and init that port. But I suspect we might need to init port 0 immediately since there is no explicit call to i2c_set_bus_num() for that. It's a little wonky whatever we do. What do you think? (have just sent the series again with fdt changes, but can update once we sort this out). Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
