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

[Simon Glass <sjg@chromium.org>]

Hi Heiko,

On Thu, Jan 12, 2012 at 11:12 PM, Heiko Schocher <hs@denx.de> wrote:
> 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.
>

Hmm quite a few patches. I see this email from 2.5 years ago
indicating that it was ready but needed more testing:

http://lists.denx.de/pipermail/u-boot/2009-July/056726.html

2a083b7 i2c: get rid of HARD_i2C
ccb680c i2c: add dynamic bus allocation
b68480a i2c, u8500: added new multibus/multiadapter support
692f037 i2c, marvell: added new multibus/multiadapter support
3b0258c i2c, spear: added new multibus/multiadapter support
a06deff i2c, at91rm9200: added new multibus/multiadapter support
0758f6b i2c, kirkwood: added new multibus/multiadapter support
7919863 i2c, mv: added new multibus/multiadapter support
b164243 i2c, mpc824x: added new multibus/multiadapter support
a5f8bd1 i2c, tsi108: added new multibus/multiadapter support
9db40d8 i2c, s3c44b0: added new multibus/multiadapter support
67b95ef i2c, s3c24x0: added new multibus/multiadapter support
6b2c3f5 i2c, omap24xx: added new multibus/multiadapter support
1f8ffed i2c, omap1510: added new multibus/multiadapter support
c41757c i2c, mxc: added new multibus/multiadapter support
5c22159 i2c, davinci: added new multibus/multiadapter support
3dd28ab i2c, bfin: added new multibus/multiadapter support
3d1c119 i2c, mpc8220: added new multibus/multiadapter support
8f4d062 i2c, mpc512x: added new multibus/multiadapter support
1bd40d4 i2c, mpc5xxx: added new multibus/multiadapter support
6225ccb i2c, 8xx: added new multibus/multiadapter support
d94a632 i2c, ppc4xx_i2c: added new multibus/multiadapter support
c34a705 i2c, mpc8260_i2c: added new multibus/multiadapter support
832b409 i2c, fsl_i2c: switch to new multibus/multiadapter support
06de378 i2c, soft-i2c: switch to new multibus/multiadapter support
e47115f i2c, common: common changes for multibus/multiadapter support
d7a70d7 i2c: add i2c_core and prepare for new multibus support

 489 files changed, 8512 insertions(+), 7063 deletions(-)

checkpatch.pl found 295 error(s), 653 warning(s)

I have had a look through the patches. I agree that i2c needs a clean
up and this seems to fit the bill. How can we get this committed?

The first two commits aren't too hard to bring over but it doesn't
actually build. I could perhaps have a look at getting it to build
correctly with and without CONFIG_SYS_I2C, and see if I can get the
latter working on Tegra.

Then perhaps we could add the new support as just another option so we
have CONFIG_HARD_I2C, CONFIG_SOFT_I2C and CONFIG_SYS_I2C. That might
be a start, and drivers can move over to the new system. What do you
think?

>>>>> +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.

Yes ok I will take a look.

Regards,
Simon

>
>> 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