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

[Simon Glass]

Hi Stephem,

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/arch/arm/include/asm/arch-tegra2/tegra2_i2c.h b/arch/arm/include/asm/arch-tegra2/tegra2_i2c.h
> ...
>> +#ifndef CONFIG_OF_CONTROL
>> +enum {
>> +       I2CSPEED_KHZ = 100,             /* in KHz */
>> +};
>> +#endif
>
> The patch description says the board needs to define I2CSPEED_KHZ, yet
> that seems to allow the board to override it, otherwise a default is
> used. The patch description probably needs to be fixed.
>
> The way the optional override is implemented is a little confusing. I'd
> rather see:
>
> #ifndef CONFIG_OF_CONTROL
> #ifndef I2CSPEED_KHZ
> #define I2CSPEED_KHZ 100
> #endif
> #endif
>
> That makes it a lot more obvious it's a default. And actually, why not
> put that within some other ifndef CONFIG_OF_CONTROL where the
> enum/define is used, since it's presumably only used very locally in the
> non-OF initialization code.

I'm removing the non-OF code since we don't use it.

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

>
> I didn't really review the I2C driver code itself, since I'm not
> terribly familiar with the I2C HW details.

It had quite a bit of review before it went into our tree, and it
certainly has had a lot of testing.

>
>> +static int i2c_get_config(int *index, struct i2c_bus *i2c_bus)
>> +{
>> +       const void *blob = gd->fdt_blob;
>> +       int node;
>> +
>> +       node = fdtdec_next_alias(blob, "i2c", COMPAT_NVIDIA_TEGRA20_I2C,
>> +                                index);
>> +       if (node < 0)
>> +               return -1;
>
> I assume the I2C patches will also be reworked to enumerate based on
> nodes with a specific compatible flag, and then optionally using aliases
> to number them, just like you said you're rework USB?

Yes, will do. The hard bit of that was creating an fdtdec function to handle it.

>
>> +int i2c_init_board(void)
>> +{
>> +       struct i2c_bus *i2c_bus;
>> +       int index = 0;
>> +       int i;
>> +
>> +       /* build the i2c_controllers[] for each controller */
>> +       for (i = 0; i < CONFIG_SYS_MAX_I2C_BUS; ++i) {
>> +               i2c_bus = &i2c_controllers[i];
>> +               i2c_bus->id = i;
>> +
>> +               if (i2c_get_config(&index, i2c_bus)) {
>> +                       printf("i2c_init_board: failed to find bus %d\n", i);
>> +                       return -1;
>> +               }
>> +
>> +               if (i2c_bus->periph_id == PERIPH_ID_DVC_I2C)
>> +                       i2c_bus->control =
>> +                               &((struct dvc_ctlr *)i2c_bus->regs)->control;
>> +               else
>> +                       i2c_bus->control = &i2c_bus->regs->control;
>
> When instantiating controllers from device tree (as opposed to the
> static !CONFIG_OF_CONTROL case), that conditional should be driven by
> device tree properties. The kernel already represents this using two
> separate compatible values for the different HW: nvidia,tegra20-i2c and
> nvidia,tegra20-i2c-dvc.

Not in the device tree file I got from the kernel...has it changed?

I will make this change. By dropping non-fdt use it will make this
work almost for free.

>
>> +
>> +               i2c_init_controller(i2c_bus);
>> +       }
>> +
>> +       return 0;
>> +}
>
>> +void i2c_init(int speed, int slaveaddr)
>> +{
>> +       debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
>> +}
>
> 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.

If fdt use grows then perhaps U-Boot might accept patches to
understand the concept of initing i2c as a unit, and once at the start
(or first use), rather than the current setup.

>
>> +/* Probe to see if a chip is present. */
>> +int i2c_probe(uchar chip)
>> +{
>> +       int rc;
>> +       uchar reg;
>> +
>> +       debug("i2c_probe: addr=0x%x\n", chip);
>> +       reg = 0;
>> +       rc = i2c_write_data(chip, &reg, 1);
>> +       if (rc) {
>> +               debug("Error probing 0x%x.\n", chip);
>> +               return 1;
>> +       }
>> +       return 0;
>> +}
>> +
>
>> +static int i2c_addr_ok(const uint addr, const int alen)
>> +{
>> +       if (alen < 0 || alen > sizeof(addr))
>> +               return 0;
>> +       if (alen != sizeof(addr)) {
>> +               uint max_addr = (1 << (8 * alen)) - 1;
>> +               if (addr > max_addr)
>> +                       return 0;
>> +       }
>> +       return 1;
>> +}
>
> That seems rather roundabout. Only two address lengths are valid; 7 and
> 10-bit, so wouldn't it be better to only accept those specific values?

Yes I agree - I will change it to just check that one or two address
bytes are provided.

Regards,
Simon

>
> --
> nvpublic