[U-Boot] [PATCH 7/7] tegra: Enable I2C on Seaboard

[Simon Glass]

+U-Boot, which I seemed to have dropped by mistake

Hi Stephen,

On Thu, Jan 12, 2012 at 10:59 AM, Stephen Warren <swarren at 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 <swarren at 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