Date:	Fri, 9 May 2008 10:08:41 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	"Maciej W. Rozycki" <macro@...ux-mips.org>
Cc:	David Brownell <david-b@...bell.net>, linux-mips@...ux-mips.org,
	mgreer@...sta.com, rtc-linux@...glegroups.com,
	Atsushi Nemoto <anemo@....ocn.ne.jp>,
	linux-kernel@...r.kernel.org, i2c@...sensors.org, ab@...able.de,
	Alessandro Zummo <alessandro.zummo@...ertech.it>
Subject: Re: [i2c] [RFC][PATCH 4/4] RTC: SMBus support for the M41T80,

Hi Maciej,

On Fri, 9 May 2008 01:43:32 +0100 (BST), Maciej W. Rozycki wrote:
>  Please do not remove other lists cc-ed as there are people interested in 
> this piece of hardware who are neither on i2c nor on rtc-linux (I am on 
> neither of the lists too).

Maybe you shouldn't have included 4 different mailing lists to start
with, especially for a patch which is rather hardware-specific and not
overly important.

> > >  Do you mean our generic I2C code emulates SMBus calls if the hardware
> > > does not support them directly?  Well, it looks to me it indeed does and
> > > you are right, but I have assumed, perhaps mistakenly, (but I generally
> > > trust if a piece of code is there, it is for a reason), that this driver
> > > checks for I2C_FUNC_SMBUS_BYTE_DATA for a purpose.
> > 
> > That purpose being:  it makes those SMBus calls explicitly.
> > (And it makes i2c_transfer calls explicitly too...)
> 
>  Then, given the emulation, the client should be satisfied with either of
> the flags instead of both at a time.  Exactly how I changed it.

You're going in the right direction, but it's not yet correct.

>  But as Atsushi-san pointed out, the block transfer transactions
> implemented by the M41T81 do not quite follow the rules of SMBus block
> transfers, so the call is out of question -- either i2c_transfer() or a
> sequence of byte transactions have to be used.

As I already wrote, what the M41T81 wants is _I2C_ block transactions,
not _SMBus_ block transactions. Both are supported by i2c-core, it's
just a matter of checking the right functionality flag and calling the
right helper function.

> (...)
>  Here is a new version of the patch.  I hope I have addressed all your
> concerns, but I am officially dead at the moment, so please do not disturb
> me until this is no longer the case.
> 
>   Maciej
> 
> patch-2.6.26-rc1-20080505-m41t80-smbus-13
> diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/drivers/rtc/rtc-m41t80.c linux-2.6.26-rc1-20080505/drivers/rtc/rtc-m41t80.c
> --- linux-2.6.26-rc1-20080505.macro/drivers/rtc/rtc-m41t80.c	2008-05-05 02:55:40.000000000 +0000
> +++ linux-2.6.26-rc1-20080505/drivers/rtc/rtc-m41t80.c	2008-05-09 00:32:39.000000000 +0000
> @@ -6,6 +6,8 @@
>   * Based on m41t00.c by Mark A. Greer <mgreer@...sta.com>
>   *
>   * 2006 (c) mycable GmbH
> + * Copyright (c) 2008  Maciej W. Rozycki
> + * Copyright (c) 2008  Atsushi Nemoto
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -15,6 +17,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/init.h>
> +#include <linux/kernel.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/i2c.h>
> @@ -36,6 +39,8 @@
>  #define M41T80_REG_DAY	5
>  #define M41T80_REG_MON	6
>  #define M41T80_REG_YEAR	7
> +#define M41T80_REG_CONTROL	8
> +#define M41T80_REG_WATCHDOG	9
>  #define M41T80_REG_ALARM_MON	0xa
>  #define M41T80_REG_ALARM_DAY	0xb
>  #define M41T80_REG_ALARM_HOUR	0xc
> @@ -58,7 +63,7 @@
>  #define M41T80_FEATURE_HT	(1 << 0)
>  #define M41T80_FEATURE_BL	(1 << 1)
>  
> -#define DRV_VERSION "0.05"
> +#define DRV_VERSION "0.06"
>  
>  static const struct i2c_device_id m41t80_id[] = {
>  	{ "m41t80", 0 },
> @@ -78,31 +83,104 @@ struct m41t80_data {
>  	struct rtc_device *rtc;
>  };
>  
> -static int m41t80_get_datetime(struct i2c_client *client,
> -			       struct rtc_time *tm)
> +
> +static int m41t80_i2c_write(struct i2c_client *client, u8 reg, u8 num, u8 *buf)
> +{
> +	u8 wbuf[num + 1];
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags	= 0,
> +			.len	= num + 1,
> +			.buf	= wbuf,
> +		},
> +	};
> +
> +	wbuf[0] = reg;
> +	memcpy(wbuf + 1, buf, num);
> +	return i2c_transfer(client->adapter, msgs, 1);
> +}

This is reimplementing i2c_smbus_write_i2c_block_data().

> +
> +static int m41t80_i2c_read(struct i2c_client *client, 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,
> +			.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);
> +}

And this is reimplementing i2c_smbus_read_i2c_block_data(). So please
just use these standard helpers, which have the advantage that they can
work on a number of adapters that cannot do full I2C (many SMBus
controllers support I2C block transactions as long as the length
doesn't exceed 32 bytes.)

> +
> +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)) {

And then here you want to check for I2C_FUNC_SMBUS_I2C_BLOCK. Or even
better, check for I2C_FUNC_SMBUS_READ_I2C_BLOCK on read and
I2C_FUNC_SMBUS_WRITE_I2C_BLOCK on write, so you get the best of each
adapter in the unlikely event an adapter would support I2C block reads
but not writes or vice-versa.

> +		if (write)
> +			i = m41t80_i2c_write(client, reg, num, buf);
> +		else
> +			i = m41t80_i2c_read(client, reg, num, buf);
> +	} 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;
> +			}
> +		else
> +			for (i = 0; i < num; i++) {
> +				rc = i2c_smbus_read_byte_data(client, reg + i);
> +				if (rc < 0)
> +					return rc;
> +				buf[i] = rc;
> +			}
> +	}
> +	return i;
> +}

> (...)
> @@ -629,14 +610,12 @@ static int wdt_ioctl(struct inode *inode
>  			return -EFAULT;
>  
>  		if (rv & WDIOS_DISABLECARD) {
> -			printk(KERN_INFO
> -			       "rtc-m41t80: disable watchdog\n");
> +			pr_info("rtc-m41t80: disable watchdog\n");
>  			wdt_disable();
>  		}
>  
>  		if (rv & WDIOS_ENABLECARD) {
> -			printk(KERN_INFO
> -			       "rtc-m41t80: enable watchdog\n");
> +			pr_info("rtc-m41t80: enable watchdog\n");
>  			wdt_ping();
>  		}
>  

Mixing code cleanups with functional changes is a Bad Idea (TM).

> @@ -732,19 +711,28 @@ static struct notifier_block wdt_notifie
>  static int m41t80_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> -	int rc = 0;
>  	struct rtc_device *rtc = NULL;
>  	struct rtc_time tm;
>  	struct m41t80_data *clientdata = NULL;
> +	int rc = 0;
> +	int reg;
>  
> -	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C
> -				     | I2C_FUNC_SMBUS_BYTE_DATA)) {
> +	if ((i2c_get_functionality(client->adapter) &
> +	     (I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE_DATA)) == 0) {
>  		rc = -ENODEV;
>  		goto exit;
>  	}

That's not correct. The driver is making unconditional calls to
i2c_smbus_write_byte_data() and i2c_smbus_read_byte_data() so the
underlying adapter _must_ implement I2C_FUNC_SMBUS_BYTE_DATA. If it
additionally implements I2C_FUNC_I2C (or actually
I2C_FUNC_SMBUS_I2C_BLOCK), see above, then you can optimize some
transactions, but you don't have to check it here, the check should be
done at run-time (as you already do.)

You really shouldn't worry about making the I2C_FUNC_SMBUS_BYTE_DATA
check unconditional. I don't think I've ever seen an i2c bus driver not
supporting it.


>  
> +	/* Trivially check it's there; keep the result for the HT check.  */
> +	reg = i2c_smbus_read_byte_data(client, M41T80_REG_ALARM_HOUR);
> +	if (reg < 0) {
> +		rc = -ENXIO;
> +		goto exit;
> +	}
> +
>  	dev_info(&client->dev,
> -		 "chip found, driver version " DRV_VERSION "\n");
> +		 "%s chip found, driver version " DRV_VERSION "\n",
> +		 client->name);

Incorrect change, dev_info() already includes the chip name.

>  
>  	clientdata = kzalloc(sizeof(*clientdata), GFP_KERNEL);
>  	if (!clientdata) {
> @@ -765,11 +753,7 @@ static int m41t80_probe(struct i2c_clien
>  	i2c_set_clientdata(client, clientdata);
>  
>  	/* Make sure HT (Halt Update) bit is cleared */
> -	rc = i2c_smbus_read_byte_data(client, M41T80_REG_ALARM_HOUR);
> -	if (rc < 0)
> -		goto ht_err;
> -
> -	if (rc & M41T80_ALHOUR_HT) {
> +	if (reg & M41T80_ALHOUR_HT) {
>  		if (clientdata->features & M41T80_FEATURE_HT) {
>  			m41t80_get_datetime(client, &tm);
>  			dev_info(&client->dev, "HT bit was set!\n");
> @@ -780,20 +764,19 @@ static int m41t80_probe(struct i2c_clien
>  				 tm.tm_mon + 1, tm.tm_mday, tm.tm_hour,
>  				 tm.tm_min, tm.tm_sec);
>  		}
> -		if (i2c_smbus_write_byte_data(client,
> -					      M41T80_REG_ALARM_HOUR,
> -					      rc & ~M41T80_ALHOUR_HT) < 0)
> +		if (i2c_smbus_write_byte_data(client, M41T80_REG_ALARM_HOUR,
> +					      reg & ~M41T80_ALHOUR_HT) < 0)
>  			goto ht_err;
>  	}

Again coding style fixes...

>  
>  	/* Make sure ST (stop) bit is cleared */
> -	rc = i2c_smbus_read_byte_data(client, M41T80_REG_SEC);
> -	if (rc < 0)
> +	reg = i2c_smbus_read_byte_data(client, M41T80_REG_SEC);
> +	if (reg < 0)
>  		goto st_err;
>  
> -	if (rc & M41T80_SEC_ST) {
> +	if (reg & M41T80_SEC_ST) {
>  		if (i2c_smbus_write_byte_data(client, M41T80_REG_SEC,
> -					      rc & ~M41T80_SEC_ST) < 0)
> +					      reg & ~M41T80_SEC_ST) < 0)
>  			goto st_err;
>  	}

And here again...

I'm not the one who will take your patch, I'll leave it to Alessandro
to tell you what he wants, but one thing for sure: it would be much
easier to review and validate your patches if you were not mixing
functional changes and code cleanups.

>  
> @@ -803,6 +786,7 @@ static int m41t80_probe(struct i2c_clien
>  
>  #ifdef CONFIG_RTC_DRV_M41T80_WDT
>  	if (clientdata->features & M41T80_FEATURE_HT) {
> +		save_client = client;
>  		rc = misc_register(&wdt_dev);
>  		if (rc)
>  			goto exit;
> @@ -811,7 +795,6 @@ static int m41t80_probe(struct i2c_clien
>  			misc_deregister(&wdt_dev);
>  			goto exit;
>  		}
> -		save_client = client;
>  	}
>  #endif
>  	return 0;

This one deserves a patch on its own IMHO.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists