Re: [PATCH 2/2] TVP514x Driver with Review comments fixed

[David Brownell <david-b@pacbell.net>]

On Friday 28 November 2008, hvaibhav@ti.com wrote:
> +static int tvp514x_read_reg(struct i2c_client *client, u8 reg, u8 *val)
> +{
> +       int err;
> +       struct i2c_msg msg[2];
> +       u8 data;
> +
> +       if (!client->adapter)
> +               return -ENODEV;
> +
> +       /* [MSG1] fill the register address data */
> +       data = reg;
> +       msg[0].addr = client->addr;
> +       msg[0].len = 1;
> +       msg[0].flags = 0;
> +       msg[0].buf = &data;
> +
> +       /* [MSG2] fill the data rx buffer */
> +       msg[1].addr = client->addr;
> +       msg[1].len = 1;         /* only 1 byte */
> +       msg[1].flags = I2C_M_RD;        /* Read the register values */
> +       msg[1].buf = val;
> +       err = i2c_transfer(client->adapter, msg, 2);

Better:

	err = i2c_smbus_read_byte_data(client, reg);
	if (err >= 0) {
		*val = err;
		err = 0;
	}
	return err;

Though maybe the calling convention should be simplified,
And of course, probe() should verify that the adapter can
support the SMBus byte_data operations.


> +       if (err >= 0)
> +               return 0;
> +
> +       v4l_dbg(1, debug, client,
> +               "read from device 0x%.2x, offset 0x%.2x error %d\n",
> +               client->addr, reg, err);
> +
> +       return err;
> +}
> +
> +/*
> + * Write a value to a register in an TVP5146/47 decoder device.
> + * Returns zero if successful, or non-zero otherwise.
> + */
> +static int tvp514x_write_reg(struct i2c_client *client, u8 reg, u8 val)
> +{
> +       int err;
> +       int retry = 0;
> +       struct i2c_msg msg[1];
> +       u8 data[2];
> +
> +       if (!client->adapter)
> +               return -ENODEV;
> +
> +again:
> +       data[0] = reg;          /* Register offset */
> +       data[1] = val;          /* Register value */
> +       msg->addr = client->addr;
> +       msg->len = 2;
> +       msg->flags = 0;         /* write operation */
> +       msg->buf = data;
> +
> +       err = i2c_transfer(client->adapter, msg, 1);
> +       if (err >= 0)
> +               return 0;

Likewise:

again:
	err = i2c_smbus_write_byte_data(client, reg, val);
	if (err == 0)
		return 0;
	/* else retry */

By the way, while I applaud actually *having* fault handling in
I2C driver code -- faults can happen, and pathetically few Linux
drivers tolerate them from I2C -- I'm curious why only the write
path handles them, not the read path.


> +
> +       v4l_dbg(1, debug, client,
> +               "wrote 0x%.2x to offset 0x%.2x error %d\n", val, reg, err);
> +       if (retry <= I2C_RETRY_COUNT) {
> +               v4l_warn(client, "retry ... %d\n", retry);
> +               retry++;
> +               schedule_timeout(msecs_to_jiffies(20));
> +               goto again;
> +       }
> +       return err;
> +}


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html