[U-Boot] [PATCH 4/7] tegra: Add I2C driver

[Heiko Schocher <hs@denx.de>]

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_v2_20111112

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany