Re: [PATCH] rtc-ds1307: True SMBus compatibility

[Jean Delvare <khali@linux-fr.org>]

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