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

[Heiko Schocher <hs@denx.de>]

Hello Simon,

Simon Glass wrote:
> Hi Heiko,
> 
> On Thu, Jan 12, 2012 at 11:25 PM, Heiko Schocher <hs@denx.de> wrote:
>> Hello Simon,
>>
>> Simon Glass wrote:
>>> From: Yen Lin <yelin@nvidia.com>
>>>
>>> Add basic i2c driver for Tegra2 with 8- and 16-bit address support.
>>> The driver requires CONFIG_OF_CONTROL to obtain its configuration
>>> from the device tree.
>>>
>>> (Simon Glass: sjg at chromium.org modified for upstream)
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>> Changes in v2:
>>> - Use DIV_ROUND_UP() instead of a home-grown macro
>>> - Tidy comment style
>>> - Change i2c array to static
>>> - Adjust definitions to fit new peripheral clock bindings
>>> - Remove i2c configuring using CONFIG (use fdt instead)
>> Why? Ah found it ... Hmm.. why we don't need the non OF
>> case?
> 
> The intent is that Tegra boards will run with fdt turned on. It means
> that we should be able to build U-Boot for a Tegra platform using the
> Linux fdt file and not lots of manual config. Of course this works
> better for some peripherals than others, but I2C works well enough.

Ah, Ok, nice!

>>> - Make i2c/dvc decision come from fdt
>>> - Use new fdtdec alias decode function
>>> - Simplify code in i2c_addr_ok()
>>> - Return an error if an unavailable i2c bus is selected
>>>
>>>  arch/arm/include/asm/arch-tegra2/tegra2_i2c.h |  160 +++++++
>>>  drivers/i2c/Makefile                          |    1 +
>>>  drivers/i2c/tegra2_i2c.c                      |  551 +++++++++++++++++++++++++
>>>  include/fdtdec.h                              |    2 +
>>>  lib/fdtdec.c                                  |    2 +
>>>  5 files changed, 716 insertions(+), 0 deletions(-)
>>>  create mode 100644 arch/arm/include/asm/arch-tegra2/tegra2_i2c.h
>>>  create mode 100644 drivers/i2c/tegra2_i2c.c
>> [...]
>>> diff --git a/drivers/i2c/tegra2_i2c.c b/drivers/i2c/tegra2_i2c.c
>>> new file mode 100644
>>> index 0000000..a7db714
>>> --- /dev/null
>>> +++ b/drivers/i2c/tegra2_i2c.c
>>> @@ -0,0 +1,551 @@
>> [...]
>>> +static void i2c_init_controller(struct i2c_bus *i2c_bus)
>>> +{
>>> +     /* TODO: Fix bug which makes us need to do this */
>>> +     clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_OSC,
>>> +                            i2c_bus->speed * (8 * 2 - 1));
>>                                                 Can you use here some defines?
>> What is (8 * 2 - 1) ?
> 
> I will look up the bug and find out.

Not a bug, just I did not know what this values are for ...

>>> +#ifndef CONFIG_OF_CONTROL
>>> +#error "Please enable device tree support to use this driver"
>>> +#endif
>> Hmm.. see above question. Ok, if somebody need want to use this
>> driver without CONFIG_OF_CONTROL it must be added ...
> 
> Yes and they need a way of getting the configuration in there. The fdt
> is nicer...

;-)

[...]
>>> +void i2c_init(int speed, int slaveaddr)
>>> +{
>>> +     debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
>>> +}
>> Hmm... i2c_init is called to init the i2c subsystem ... you do nothing
>> here ... and use i2c_init_board for init the i2c bus, right?
>>
>> But i2c_init_board is not called from the driver ... ah, you do this
>> in board code ... Ok ...
>>
>> I think, you do this, because i2c_init is called very early, and
>> so processing fdt is slow?
> 
> That's not the main reason but it is true. We have no need of early
> I2C on the platform. Also we don't want to set the speed as this is
> defined individually per port in the fdt.

Ok.

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