Hi Mike,
On Thu, Mar 1, 2012 at 12:29 PM, Mike Frysinger <vapier@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
