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

[Stephen Warren <swarren@nvidia.com>]

On 12/26/2011 11:11 AM, Simon Glass wrote:
> From: Yen Lin <yelin@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.

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

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

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

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

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

> +/* 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?

-- 
nvpublic