[U-Boot] [PATCH v4 5/9] tegra: i2c: Add I2C driver

[Simon Glass]

Hi Mike,

On Thu, Mar 1, 2012 at 12:29 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Wednesday 29 February 2012 12:31:25 Simon Glass wrote:
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arch-tegra2/tegra_i2c.h
>>
>> +/* Convert i2c slave address to be put on bus  */
>> +#define I2C_ADDR_ON_BUS(chip)                (chip << 1)
>
> i'm not sure the desc here is correct ... it's at least a little bit
> misleading.  addresses are 7bits, and the 8th bit is for telling the device to
> read/write.  since it only gets used in two places, might be better to inline
> the bit shift there ?  hard to say.

Yes I agree with you, have changed it.

>
>> --- /dev/null
>> +++ b/drivers/i2c/tegra_i2c.c
>>
>> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
>
> guess you need to -2012 that now ;)

Changed; pray I don't have to change it again.

>
>> +#include <common.h>
>> +#include <asm/io.h>
>> +#include <asm/arch/clk_rst.h>
>> +#include <asm/arch/clock.h>
>> +#include <asm/arch/funcmux.h>
>> +#include <asm/arch/gpio.h>
>> +#include <asm/arch/pinmux.h>
>> +#include <asm/arch/tegra_i2c.h>
>> +#include <fdtdec.h>
>
> needs to include i2c.h to make sure your local defs stay inline with the
> prototypes everyone else is using.  also, asm/* generally should come last (so
> after ftdec.h).

Done, which threw up an error in the i2c_init_board() signature. No
error checking :-(

>
>> +static int send_recv_packets(
>> +     struct i2c_bus *i2c_bus,
>> +     struct i2c_trans_info *trans)
>> +{
>> +     struct i2c_control *control = i2c_bus->control;
>> +     u32 int_status;
>> +     u32 words;
>> +     u8 *dptr;
>> +     u32 local;
>> +     uchar last_bytes;
>> +     int error = 0;
>> +     int is_write = trans->flags & I2C_IS_WRITE;
>> +
>> +     /* clear status from previous transaction, XFER_COMPLETE, NOACK, etc. */
>> +     int_status = readl(&control->int_status);
>> +     writel(int_status, &control->int_status);
>> +
>> +     send_packet_headers(i2c_bus, trans, 1);
>> +
>> +     words = DIV_ROUND_UP(trans->num_bytes, 4);
>> +     last_bytes = trans->num_bytes & 3;
>> +     dptr = trans->buf;
>> +
>> +     while (words) {
>> +             if (is_write) {
>> +                     /* deal with word alignment */
>> +                     if ((unsigned)dptr & 3) {
>> +                             memcpy(&local, dptr, sizeof(u32));
>> +                             writel(local, &control->tx_fifo);
>> +                             debug("pkt data sent (0x%x)\n", local);
>> +                     } else {
>> +                             writel(*(u32 *)dptr, &control->tx_fifo);
>> +                             debug("pkt data sent (0x%x)\n", *(u32 *)dptr);
>
> generally inlining these types of bitsized casts are discouraged ...

OK I have added a separate variable for this.

Regards,
Simon

> -mike