[U-Boot] [PATCH 5/6] I2C: Driver changes for FDT support

[Simon Glass]

Hi Rajeshwari,

On Mon, Nov 12, 2012 at 5:03 AM, Rajeshwari Birje
<rajeshwari.birje at gmail.com> wrote:
> Hi Simon Glass,
>
> Thank you for comments.
>
> On Fri, Nov 9, 2012 at 6:34 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Rajeshwari,
>>
>> On Wed, Nov 7, 2012 at 9:30 PM, Rajeshwari Shinde
>> <rajeshwari.s at samsung.com> wrote:
>>> Functions added to get the I2C bus number and reset I2C bus using
>>> FDT node.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> Signed-off-by: Rajeshwari Shinde <rajeshwari.s at samsung.com>
>>> ---
>>>  drivers/i2c/s3c24x0_i2c.c |   89 ++++++++++++++++++++++++++++++++++++++++++++-
>>>  drivers/i2c/s3c24x0_i2c.h |    7 ++++
>>>  include/i2c.h             |   28 ++++++++++++++
>>>  3 files changed, 123 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c
>>> index 9bc4c7f..952f76c 100644
>>> --- a/drivers/i2c/s3c24x0_i2c.c
>>> +++ b/drivers/i2c/s3c24x0_i2c.c
>>> @@ -27,6 +27,7 @@
>>>   */
>>>
>>>  #include <common.h>
>>> +#include <fdtdec.h>
>>>  #ifdef CONFIG_EXYNOS5
>>>  #include <asm/arch/clk.h>
>>>  #include <asm/arch/cpu.h>
>>> @@ -60,7 +61,14 @@
>>>  #define I2C_TIMEOUT 1          /* 1 second */
>>>
>>>
>>> -static unsigned int g_current_bus;     /* Stores Current I2C Bus */
>>> +/*
>>> + * For SPL boot some boards need i2c before SDRAM is initialised so force
>>> + * variables to live in SRAM
>>> + */
>>> +static unsigned int g_current_bus __attribute__((section(".data")));
>>> +static struct s3c24x0_i2c_bus i2c_bus[CONFIG_MAX_I2C_NUM]
>>> +                       __attribute__((section(".data")));
>>> +static int i2c_busses __attribute__((section(".data")));
>>>
>>>  #ifndef CONFIG_EXYNOS5
>>>  static int GetI2CSDA(void)
>>> @@ -507,4 +515,83 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
>>>                 (i2c, I2C_WRITE, chip << 1, &xaddr[4 - alen], alen, buffer,
>>>                  len) != 0);
>>>  }
>>> +
>>> +#ifdef CONFIG_OF_CONTROL
>>> +unsigned int i2c_fdt_bus_values(const void *blob)
>>> +{
>>> +
>>> +       int node_list[CONFIG_MAX_I2C_NUM];
>>> +       int count, i;
>>> +
>>> +       count = fdtdec_find_aliases_for_id(blob, "i2c",
>>> +               COMPAT_SAMSUNG_S3C2440_I2C, node_list,
>>> +               CONFIG_MAX_I2C_NUM);
>>> +       for (i = 0; i < count; i++) {
>>> +               struct s3c24x0_i2c_bus *bus;
>>> +               int node = node_list[i];
>>> +
>>> +               if (node < 0)
>>> +                       continue;
>>> +               bus = &i2c_bus[i];
>>> +               bus->regs = (struct s3c24x0_i2c *)
>>> +                       fdtdec_get_addr(blob, node, "reg");
>>> +               bus->node = node;
>>> +               bus->bus_num = i2c_busses++;
>>> +       }
>>> +
>>> +       return count;
>>> +}
>>> +
>>
>> This seems to be the init function for i2c, so how about calling it
>> board_i2c_init() or similar. I don't think you need to return the
>> count of i2c ports - you can just use CONFIG_MAX_I2C_NUM anywhere that
>> it is needed.
>>
>>> +static struct s3c24x0_i2c_bus *get_bus(int bus_idx)
>>
>> should probably be unsigned so that you don't need to check < 0
>>
>>> +{
>>> +       if (bus_idx < i2c_busses)
>>> +               return &i2c_bus[bus_idx];
>>> +       debug("Undefined bus: %d\n", bus_idx);
>>> +       return NULL;
>>> +}
>>> +
>>> +int i2c_get_bus_num_fdt(const void *blob, int node)
>>> +{
>>> +       enum fdt_compat_id compat;
>>> +       fdt_addr_t reg;
>>> +       int i;
>>> +
>>> +       compat = fdtdec_lookup(blob, node);
>>> +       if (compat != COMPAT_SAMSUNG_S3C2440_I2C) {
>>> +               debug("%s: Not a supported I2C node\n", __func__);
>>> +               return -1;
>>> +       }
>>> +       reg = fdtdec_get_addr(blob, node, "reg");
>>> +       for (i = 0; i < i2c_busses; i++) {
>>> +               if (reg == (fdt_addr_t)(uintptr_t)i2c_bus[i].regs)
>>> +                       return i;
>>> +       }
>>
>> This seems painful. You already have a table of node versus bus
>> number, created in the function above, so why not just look in that?
>>
> You want me to use get_bus function? but that function takes the bus
> number and gives the handle to the structure and not the i2c channel
> register address. Hence we cannot use that function here, instead I am
> using the structures populated in board_i2c_init.
> Do clarify if I have misunderstood the comment.

What I mean is that you have set up a table of node versus bus number
in i2c_fdt_bus_values().  So there should be no need to go and look
again at the fdt in i2c_get_bus_num_fdt() - you can just write a
function to search your table.

[snip]

Regards,
Simon