[U-Boot] [PATCH v3 8/9] tegra: i2c: Enable new CONFIG_SYS_I2C framework

[Stephen Warren]

On 07/31/2013 10:32 PM, Heiko Schocher wrote:
> Hello Stephen,
> 
> Am 31.07.2013 21:31, schrieb Stephen Warren:
>> On 07/30/2013 11:46 PM, Heiko Schocher wrote:
>> ...
>>> Hmm.. each i2c adapter has its own init function ... why the tegra
>>> driver do not use it? And do the necessary inits in it? So we
>>> initialize an adapater only if we use it, which is also a rule
>>> for u-boot ...
>>>
>>> I have no hw, but it should be possible to add to process_nodes()
>>> a parameter "controller_number" and call
>>> process_nodes(controller_number, ...) from the i2c drivers init
>>> function ...
>>
>> There are two steps to initializing I2C on a Tegra system:
>>
>> 1) Parsing the DT to determine which/how-many I2C adapters exist in the
>> SoC. This will yield information such as the register address of the I2C
>> adapters, which clock/reset signal they rely on, perhaps the max bus
>> clock rate, etc.
>>
>> This is a single global operation that needs to happen one single time
>> before anything else.
> 
> Why?

That's simply the way the code is written.

>> This stage initializes the i2c_controllers[] array, since that's what
>> stores all the data parsed from DT.
>>
>> 2) Actually initializing or using the I2C HW. That could certainly be
>> part of the per-I2C-controller init() function you mention.
> 
> For that purpose is the i2c_init function.
> 
> And why we could not do step 1 when we do step 2? Why doing step 1
> for hw we later do not use?

I suppose you could. It seems conceptually /far/ simpler to just scan
the DT once up-front rather than having to defer all this stuff until
you actually use an I2C bus. If you do that, how can you know how many
I2C buses exist without trying to use every possible one and seeing
which fail? Also, the mapping from I2C bus number to register address is
only created by actually scanning the whole DT; there's no need to every
I2C DT node to have a /aliases entry that dictates its U-Boot device ID,
so you really do have to scan everything completely up-front before you
can determine which registers to use.

> saying something like this (just as an example, should be tested):
> 
> diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c
> index 9ac3969..7bbd3c7 100644
> --- a/drivers/i2c/tegra_i2c.c
> +++ b/drivers/i2c/tegra_i2c.c
> @@ -378,81 +378,91 @@ static int i2c_get_config(const void *blob, int
> node, struct i2c_bus *i2c_bus)
>   * @param is_scs       1 if this HW uses a single clock source (T114+)
>   * @return 0 if ok, -1 on error
>   */
> -static int process_nodes(const void *blob, int node_list[], int count,
> +static int process_nodes(const void *blob, int node_list[],
> +                        struct i2c_adapter *adap, int count,
>                          int is_dvc, int is_scs)
>  {
>         struct i2c_bus *i2c_bus;
> -       int i;
> -
> -       /* build the i2c_controllers[] for each controller */
> -       for (i = 0; i < count; i++) {
> -               int node = node_list[i];
> +       int node = node_list[i];

How do you determine i here? There's no guarantee that all I2C DT nodes
end up being processed in a single call to process_nodes(); there might
be some "Tegra20 I2C", some "Tegra20 I2C DVC", some "Tegra30 I2C"
modules in the same SoC.