Re: [RFC][PATCH 4/4] RTC: SMBus support for the M41T80

[Jean Delvare]

Hi Maciej,

On Sat, 10 May 2008 02:35:18 +0100 (BST), Maciej W. Rozycki wrote:
> > >  Ah, I see -- I must have missed it from documentation or perhaps it is
> > > somewhat unclear.  You mean our I2C API implies a driver for a
> > 
> > It's documented in Documentation/i2c/functionality. If something is
> > unclear, please let me know and/or send a patch.
> 
>  Well, I had a look at this file while writing my changes and this is the 
> very thing that is unclear.  The only place the description refers to 
> emulation is the I2C_FUNC_SMBUS_EMUL flag and there is nothing said
> about any other I2C_FUNC_SMBUS_* flag in the context of emulation.  The 
> rest of the file refers to functionality provided by the adapter, which 
> can be reasonably assumed to be such provided directly by hardware.

OK, I've just spent some time trying to improve this piece of
documentation. I'll send it to you and the i2c list in a moment, to not
overload this thread. Please tell me if my proposed changes make the
document clearer.

> (...)
>  Will see.  For now here is a new version of the change -- aside taking 
> your and other people's comments into account I have improved the logic 
> behind required bus adapter's feature determination.

Only commenting on the i2c bits...

> -static int m41t80_get_datetime(struct i2c_client *client,
> -			       struct rtc_time *tm)
> +
> +static int m41t80_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,
> -		},
> -		{
> -			.addr	= client->addr,
> -			.flags	= I2C_M_RD,
> -			.len	= M41T80_DATETIME_REG_SIZE - M41T80_REG_SEC,
> -			.buf	= buf + M41T80_REG_SEC,
> -		},
> -	};
> +	int i, rc;
>  
> -	if (i2c_transfer(client->adapter, msgs, 2) < 0) {
> -		dev_err(&client->dev, "read error\n");
> -		return -EIO;
> +	if (write) {
> +		if (i2c_check_functionality(client->adapter,
> +					    I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> +			i = i2c_smbus_write_i2c_block_data(client,
> +							   reg, num, buf);
> +		} else {
> +			for (i = 0; i < num; i++) {
> +				rc = i2c_smbus_write_byte_data(client, reg + i,
> +							       buf[i]);
> +				if (rc < 0) {
> +					i = rc;
> +					goto out;
> +				}
> +			}
> +		}
> +	} else {
> +		if (i2c_check_functionality(client->adapter,
> +					    I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +			i = i2c_smbus_read_i2c_block_data(client,
> +							  reg, num, buf);
> +		} else {
> +			for (i = 0; i < num; i++) {
> +				rc = i2c_smbus_read_byte_data(client, reg + i);
> +				if (rc < 0) {
> +					i = rc;
> +					goto out;
> +				}
> +				buf[i] = rc;
> +			}
> +		}
>  	}
> +out:
> +	return i;
> +}

I don't understand why you insist on having a single m41t80_transfer()
function for read and write transactions, when the read and write cases
share zero code. Separate functions would perform better.

> (...)
> -	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C
> -				     | I2C_FUNC_SMBUS_BYTE_DATA)) {
> +	func = i2c_get_functionality(client->adapter);
> +	if (!(func & (I2C_FUNC_SMBUS_READ_I2C_BLOCK |
> +		      I2C_FUNC_SMBUS_READ_BYTE)) ||
> +	    !(func & (I2C_FUNC_SMBUS_WRITE_I2C_BLOCK |
> +		      I2C_FUNC_SMBUS_WRITE_BYTE))) {
>  		rc = -ENODEV;
>  		goto exit;
>  	}

Still not correct, sorry. The driver is still making unconditional
calls to i2c_smbus_read_byte_data() and i2c_smbus_write_byte_data(), so
the underlying adapter _must_ support I2C_FUNC_SMBUS_READ_BYTE_DATA and
I2C_FUNC_SMBUS_WRITE_BYTE_DATA (i.e. I2C_FUNC_SMBUS_BYTE_DATA), even if
it also supports the block transactions. Also, you don't have to check
for the availability of these block transactions at this point, because
you test for them at run-time in m41t80_transfer(), and the driver will
work find without them.

So the proper test here would simply be:

	if (!i2c_check_functionality(client->adapter,
				     I2C_FUNC_SMBUS_BYTE_DATA)) {
 		rc = -ENODEV;
 		goto exit;
 	}

-- 
Jean Delvare
--
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/