Hi Ed,
On Mon, 19 Jan 2009 15:59:37 -0800, Ed Swierk wrote:
> Here is an updated patch for rtc-ds1307 that allows the driver to work
> with SMBus controllers like nforce2 that do not support i2c block
> transfers. The driver now checks the capabilities of the controller and
> emulates the block transfers with SMBus byte transfers only if
> necessary.
>
> Signed-off-by: Ed Swierk <eswierk@xxxxxxxxxxxxxxxxxx>
Patch looks reasonable, with just minor objections:
> Index: linux-2.6.27.4/drivers/rtc/rtc-ds1307.c
> ===================================================================
> --- linux-2.6.27.4.orig/drivers/rtc/rtc-ds1307.c
> +++ linux-2.6.27.4/drivers/rtc/rtc-ds1307.c
> @@ -94,6 +94,10 @@ struct ds1307 {
> struct i2c_client *client;
> struct rtc_device *rtc;
> struct work_struct work;
> + s32 (*read_block_data)(struct i2c_client *client, u8 command,
> + u8 length, u8 *values);
> + s32 (*write_block_data)(struct i2c_client *client, u8 command,
> + u8 length, const u8 *values);
> };
>
> struct chip_desc {
> @@ -132,6 +136,61 @@ MODULE_DEVICE_TABLE(i2c, ds1307_id);
>
> /*----------------------------------------------------------------------*/
>
> +static s32 ds1307_read_block_data_once(struct i2c_client *client, u8 command,
> + u8 length, u8 *values)
> +{
> + s32 i, data;
> + for (i = 0; i < length; i++) {
> + data = i2c_smbus_read_byte_data(client, command + i);
> + if (data < 0)
> + return data;
> + *(values + i) = data;
> + }
> + return i;
> +}
> +
> +static s32 ds1307_read_block_data(struct i2c_client *client, u8 command,
> + u8 length, u8 *values)
> +{
> + u8 oldvalues[I2C_SMBUS_BLOCK_MAX];
> + s32 ret;
> + pr_debug("ds1307_read_block_data (length=%d)\n", length);
Please use dev_dbg(&client->dev, ...).
> + ret = ds1307_read_block_data_once(client, command, length, values);
> + if (ret < 0)
> + return ret;
> + do {
> + s32 ret;
Redeclaring a variable that already exists?
> + memcpy(oldvalues, values, length);
> + ret = ds1307_read_block_data_once(client, command, length, values);
> + if (ret < 0)
> + return ret;
> + } while (memcmp(oldvalues, values, length));
What guarantee do you have that you'll ever leave this loop?
> + return length;
> +}
> +
> +static s32 ds1307_write_block_data(struct i2c_client *client, u8 command,
> + u8 length, const u8 *values)
> +{
> + u8 currvalues[I2C_SMBUS_BLOCK_MAX];
> + pr_debug("ds1307_write_block_data (length=%d)\n", length);
dev_dbg
> + do {
> + s32 i, ret;
> + for (i = 0; i < length; i++) {
> + ret = i2c_smbus_write_byte_data(client, command + i,
> + *(values + i));
> + if (ret < 0)
> + return ret;
> + }
> + ret = ds1307_read_block_data_once(client, command, length,
> + currvalues);
> + if (ret < 0)
> + return ret;
> + } while (memcmp(currvalues, values, length));
Same question as above, what if you get stuck in the loop?
> + return length;
> +}
> +
> +/*----------------------------------------------------------------------*/
> +
> /*
All the rest is fine.
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
