Re: [RFC][PATCH 4/4] RTC: SMBus support for the M41T80, etc. driver

[Atsushi Nemoto]

Added CC to authers of the driver.

On Wed, 7 May 2008 01:40:41 +0100 (BST), "Maciej W. Rozycki" <macro@xxxxxxxxxxxxxx> wrote:
>  The BCM1250A SOC which is used on the SWARM board utilising an M41T81
> chip only supports pure I2C in the raw bit-banged mode.  Nobody sane
> really wants to use it unless absolutely necessary and the M41T80, etc.  
> chips work just fine with an SMBus controller which is what the standard
> mode of operation of the BCM1250A.  The only drawback of byte accesses
> with the M41T80 is the chip only latches clock data registers for the
> duration of an I2C transaction which works fine with a block transfers,
> but not byte-wise accesses.
> 
>  The driver currently requires an I2C controller providing both SMBus and
> pure I2C access.  This is a set of changes to make it work with either,
> with a preference to pure I2C.  The problem of unlatched clock data if
> SMBus transactions are used is resolved in the standard way.
> 
>  Whether a choice between SMBus and pure I2C access is possible is 
> obviously device dependent.  If the code proved useful for other I2C 
> devices, then it could be moved to a more generic place.  It is a code
> simplification regardless -- similar I2C packet definitions are not 
> scattered throughout the driver anymore.
> 
> Signed-off-by: Maciej W. Rozycki <macro@xxxxxxxxxxxxxx>
> ---
>  Please note I have no means to test these changes with a pure I2C 
> controller -- anybody interested, please help.

I tested this with i2c-gpio.  It needs some fix.  Detailed below.

> -static int m41t80_get_datetime(struct i2c_client *client,
> -			       struct rtc_time *tm)
> +
> +static int m41t80_i2c_transfer(struct i2c_client *client, int write,
> +			       u8 reg, u8 num, u8 *buf)
>  {
> -	u8 buf[M41T80_DATETIME_REG_SIZE], dt_addr[1] = { M41T80_REG_SEC };
>  	struct i2c_msg msgs[] = {
>  		{
>  			.addr	= client->addr,
>  			.flags	= 0,
>  			.len	= 1,
> -			.buf	= dt_addr,
> +			.buf	= &reg,
>  		},
>  		{
>  			.addr	= client->addr,
> -			.flags	= I2C_M_RD,
> -			.len	= M41T80_DATETIME_REG_SIZE - M41T80_REG_SEC,
> -			.buf	= buf + M41T80_REG_SEC,
> +			.flags	= write ? 0 : I2C_M_RD,
> +			.len	= num,
> +			.buf	= buf,
>  		},
>  	};
>  
> -	if (i2c_transfer(client->adapter, msgs, 2) < 0) {
> -		dev_err(&client->dev, "read error\n");
> -		return -EIO;
> +	return i2c_transfer(client->adapter, msgs, 2);
> +}

For write transfer, you must use only one i2c_msg.  If two i2c_msg
were used, the slave address is inserted between register number and
first data.  This chip cannot accept such sequence.

> +static int m41t80_transfer(struct i2c_client *client, int write,
> +			   u8 reg, u8 num, u8 *buf)
> +{
> +	int i, rc;
> +
> +	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return m41t80_i2c_transfer(client, write, reg, num, buf);

On success, m41t80_i2c_transfer() returns positive value, but
m41t80_transfer() should return 0.


Here is a proposed fix on top of your patch.

--- drivers/rtc/rtc-m41t80.c.orig	2008-05-08 11:08:57.000000000 +0900
+++ drivers/rtc/rtc-m41t80.c	2008-05-08 12:30:49.000000000 +0900
@@ -83,8 +83,8 @@ struct m41t80_data {
 };
 
 
-static int m41t80_i2c_transfer(struct i2c_client *client, int write,
-			       u8 reg, u8 num, u8 *buf)
+static int m41t80_i2c_read_transfer(struct i2c_client *client,
+				    u8 reg, u8 num, u8 *buf)
 {
 	struct i2c_msg msgs[] = {
 		{
@@ -95,7 +95,7 @@ static int m41t80_i2c_transfer(struct i2
 		},
 		{
 			.addr	= client->addr,
-			.flags	= write ? 0 : I2C_M_RD,
+			.flags	= I2C_M_RD,
 			.len	= num,
 			.buf	= buf,
 		},
@@ -104,13 +104,29 @@ static int m41t80_i2c_transfer(struct i2
 	return i2c_transfer(client->adapter, msgs, 2);
 }
 
+static int m41t80_i2c_write_transfer(struct i2c_client *client,
+				     u8 reg, u8 num, u8 *buf)
+{
+	u8 wbuf[M41T80_DATETIME_REG_SIZE + 1];
+	struct i2c_msg msg = {
+		.addr	= client->addr,
+		.flags	= 0,
+		.len	= num + 1,
+		.buf	= wbuf,
+	};
+
+	wbuf[0] = reg;
+	memcpy(wbuf + 1, buf, num);
+	return i2c_transfer(client->adapter, &msg, 1);
+}
+
 static int m41t80_read_byte_data(struct i2c_client *client, u8 reg)
 {
 	int rc;
 	u8 val;
 
 	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
-		rc = m41t80_i2c_transfer(client, 0, reg, 1, &val);
+		rc = m41t80_i2c_read_transfer(client, reg, 1, &val);
 		if (rc < 0)
 			return rc;
 		else
@@ -122,7 +138,7 @@ static int m41t80_read_byte_data(struct 
 static int m41t80_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
 {
 	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
-		return m41t80_i2c_transfer(client, 1, reg, 1, &val);
+		return m41t80_i2c_write_transfer(client, reg, 1, &val);
 	else
 		return i2c_smbus_write_byte_data(client, reg, val);
 }
@@ -132,16 +148,20 @@ static int m41t80_transfer(struct i2c_cl
 {
 	int i, rc;
 
-	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
-		return m41t80_i2c_transfer(client, write, reg, num, buf);
-	else if (write) {
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		if (write)
+			rc = m41t80_i2c_write_transfer(client, reg, num, buf);
+		else
+			rc = m41t80_i2c_read_transfer(client, reg, num, buf);
+		if (rc < 0)
+			return rc;
+	} else if (write) {
 		for (i = 0; i < num; i++) {
 			rc = i2c_smbus_write_byte_data(client, reg + i,
 						       buf[i]);
 			if (rc < 0)
 				return rc;
 		}
-		return 0;
 	} else {
 		for (i = 0; i < num; i++) {
 			rc = i2c_smbus_read_byte_data(client, reg + i);
@@ -149,8 +169,8 @@ static int m41t80_transfer(struct i2c_cl
 				return rc;
 			buf[i] = rc;
 		}
-		return 0;
 	}
+	return 0;
 }
 
 static int m41t80_get_datetime(struct i2c_client *client, struct rtc_time *tm)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/