[U-Boot] [PATCH 1/2] tegra: i2c: Add function to know about current bus

[Simon Glass]

Hi Heiko,

On Tue, Oct 30, 2012 at 10:26 PM, Heiko Schocher <hs at denx.de> wrote:
> Hello Simon,
>
>
> On 30.10.2012 18:28, Simon Glass wrote:
>>
>> Rather than using a variable in various places, add a single function,
>> tegra_i2c_get_bus(), which returns a pointer to information about a
>> bus.
>>
>> This will make it easier to move to the new i2c framework.
>>
>> Signed-off-by: Simon Glass<sjg at chromium.org>
>> ---
>>   drivers/i2c/tegra_i2c.c |   78
>> ++++++++++++++++++++++++++++++++++++----------
>>   1 files changed, 61 insertions(+), 17 deletions(-)
>
>
> First question, did you tried the new i2c framework on some HW? Works it
> for you?

Yes, it works fine. Tried on a seaboard.

>
>
>> diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c
>> index efc77fa..3e157f4 100644
>> --- a/drivers/i2c/tegra_i2c.c
>> +++ b/drivers/i2c/tegra_i2c.c
>
> [...]
>>
>> @@ -302,18 +304,48 @@ static int tegra_i2c_read_data(u32 addr, u8 *data,
>> u32 len)
>>   #error "Please enable device tree support to use this driver"
>>   #endif
>>
>> +/**
>> + * Check that a bus number is valid and return a pointer to it
>> + *
>> + * @param bus_num      Bus number to check / return
>> + * @return pointer to bus, if valid, else NULL
>> + */
>> +static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num)
>> +{
>> +       struct i2c_bus *bus;
>> +
>> +       if (bus_num>= TEGRA_I2C_NUM_CONTROLLERS) {
>> +               debug("%s: Invalid bus number %u\n", __func__, bus_num);
>> +               return NULL;
>> +       }
>> +       bus =&i2c_controllers[bus_num];
>>
>> +       if (!bus->inited) {
>> +               debug("%s: Bus %u not available\n", __func__, bus_num);
>> +               return NULL;
>> +       }
>> +
>> +       return bus;
>> +}
>> +
>
>
> I added, as Stephen and you suggested, in the "struct i2c_adapter"
> to the driver callbacks a new parameter "struct i2c_adapter *adap":
>
> struct i2c_adapter {
>         void            (*init)(struct i2c_adapter *adap, int speed,
>                                 int slaveaddr);
>         int             (*probe)(struct i2c_adapter *adap, uint8_t chip);
>         int             (*read)(struct i2c_adapter *adap, uint8_t chip,
>                                 uint addr, int alen, uint8_t *buffer,
>                                 int len);
>         int             (*write)(struct i2c_adapter *adap, uint8_t chip,
>                                 uint addr, int alen, uint8_t *buffer,
>                                 int len);
>         uint            (*set_bus_speed)(struct i2c_adapter *adap,
>                                 uint speed);
> [...]
>
> so the probe/read/write and set_bus_speed() functions gets now a
> "struct i2c_adapter" pointer ... i2c driver internally, we need only
> the "struct i2c_adapter", which the i2c core passes to the i2c driver.
> So this function would look like now:
>
>> +static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap)

Sounds good. Are you going to send a new patch?

>
>> +{
>> +     struct i2c_bus *bus;
>         ^^^^^^^^^^^^^^
>         maybe a rename to "i2c_tegra_driver" would be good?
>> +     bus =&i2c_controllers[adap->hwadapnr];
>
>> +     if (!bus->inited) {
>> +             debug("%s: Bus %u not available\n", __func__, bus_num);
>> +             return NULL;
>> +     }
>> +
>> +     return bus;
>> +}
>
> With this, we can get rid of i2c_bus_num in this driver.
>
> Also the probe/read/write and set_bus_speed() functions needs to
> be adapted. I can do this for you, and add this patchset to my
> v2 post, if it is ok for you?

Of course.

>
>>   unsigned int i2c_get_bus_speed(void)
>
>
> This function is not needed for the new framework!
> [...]

Yes, I thought I removed it in 2/2, perhaps not.

>
>>   int i2c_set_bus_speed(unsigned int speed)
>>   {
>> -       struct i2c_bus *i2c_bus;
>> +       struct i2c_bus *bus;
>>
>> -       i2c_bus =&i2c_controllers[i2c_bus_num];
>>
>> -       i2c_bus->speed = speed;
>> -       i2c_init_controller(i2c_bus);
>> +       bus = tegra_i2c_get_bus(i2c_bus_num);
>> +       if (!bus)
>> +               return 0;
>> +       bus->speed = speed;
>> +       i2c_init_controller(bus);
>>
>>         return 0;
>>   }
>> @@ -426,7 +458,7 @@ void i2c_init(int speed, int slaveaddr)
>
> [...]
>
> Rest looks good to me.
>
> Thanks!

Will await your further patch, and I am ready to test. Thanks.

Regards,
Simon

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