lkml.org 
[lkml]   [2009]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] rtc-ds1307: True SMBus compatibility
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@aristanetworks.com>

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


\
 
 \ /
  Last update: 2009-01-20 11:47    [W:0.143 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site