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

Fri Jan 13 16:27:38 CET 2012

Hello Simon,

Simon Glass wrote:
> Hi Heiko,
> 
> On Thu, Jan 12, 2012 at 11:12 PM, Heiko Schocher <hs at 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 at nvidia.com> wrote:
>>>>> On 12/26/2011 11:11 AM, Simon Glass wrote:
>>>>>> From: Yen Lin <yelin at 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

Yep, there was some work on this ... but the last year, I just found
time for rebasing this branch, add new boards/i2c drivers and just
try compile clean ... so there are maybe some errors, which you can
solve only with a debugger ... Sorry for that ...

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

Yep, I know :-(

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

We should try to get rid of the checkpatch errors. After that we
should test it on some platforms (arm, powerpc) and if this step
is done, we can bring it to mainline in the next upcoming merge
window.

If we break i2c support on some boards, I hope board maintainers
will speak up then, and send bugfix patches ;-)

> 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

Hups, I run a "MAKEALL -a arm" without errors ... will look
into this next week.

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

I would prefer to get the hole branch to mainline, so we can get
rid off the CONFIG_HARD_I2C define ... but maybe it is worth to
try this in more smaller steps ...

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

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany