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

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

[Maciej W. Rozycki]

Hi David,

 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).

> >  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.

> > The extensions are 16-bit commands 
> > (required by another RTC chip used on some of these boards) and an obscure
> > subset of the process call and block read/write commands (called an EEPROM
> > read and an extended write/read, respectively).
> 
> Subset of process call??  That's send-three-bytes, receive-two-bytes.
> Not possible to subset it ... anything else isn't a process call!!

 I misinterpreted the SMBus spec -- I have thought the receive part is
variable, sorry.  The controller implements a command which issues a write
byte transfer followed by a receive four bytes transfer.  Not quite a
process call although the idea is the same.

> Presumably those block read/write commands aren't quite enough
> for you to implement i2c_smbus_read_i2c_block_data() and friend?

 You can issue a block read of up to 5 bytes (6 if you add the PEC byte
which is not interpreted by the controller in any way).  And you can issue
a block write of up to 4 bytes (5 with PEC).  That's clearly not enough
for the m41t81 let alone a generic implementation.

 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.

> >  I feel a bit uneasy about unconditional emulation of SMBus block calls
> > with a series of corresponding byte read/write calls.  The reason is it
> > changes the semantics
> 
> No it doesn't.  The I2C signal transitions (SCL/SDA) will be
> exactly the same.  It's IMO very misleading to call it "emulation"
> since it's nothing more than a different software interface to
> the same functionality.

 It is not the same.  Assuming a write for a block transfer you have:

start:address:ack:command:ack:data:ack:data:ack:data:ack...stop

on the wire while for a series of byte calls you have:

start:address:ack:command:ack:data:ack:stop:start:address:ack:command:ack:data:ack:stop:start:address:ack:command:ack:data:ack...stop

This is what I mean here -- I gather you are thinking in the terms of raw 
I2C block vs SMBus block transfer.

> > Admittedly in this case the effect 
> > of the difference can be eliminated by rereading the least significant 
> > part of the timestamp considered, but it cannot be universally guaranteed.  
> > Which is why I would prefer not to hide these details behind an interface.
> 
> That would be internal to the m41t80 driver, not part of the
> i2c core.  (As your patch already does ...)

 It could be useful enough for other drivers to have the emulated calls 
available as a part of the API.  No need to rush adding that though 
obviously -- we can wait till we have a user (now that this driver has 
been ruled out).

> >  I think a pair of functions called i2c_smbus_read_i2c_byte_data() and
> > i2c_smbus_write_i2c_byte_data() could be added to the core though.
> 
> Too late.  Those calls have been in the I2C core for a long
> time now.  ;)

 A mental shortcut, sorry.  I meant these functions would perform block
transfers internally either by calling
i2c_smbus_xfer(I2C_SMBUS_I2C_BLOCK_DATA)/i2c_transfer() or, if neither
supported by a given controller, by performing an appropriate sequence of
i2c_smbus_xfer(I2C_SMBUS_BYTE_DATA) calls.

> > Their 
> > implementations might choose either byte or block transfers as available
> > within a given SMBus controller and they could be used by drivers known
> > for sure not to care about which kind of the transfers is uses (either
> > because the relevant piece of hardware does not care or because the driver
> > caters for either scenario).  What do you think?
> 
> You shouldn't think about changing the i2c core for that.

 It would not be a great idea to have the same piece of code duplicated in 
all the client drivers needing it, but we can certainly wait with that -- 
see above.

> >  BTW, as mentioned elsewhere the SMBus controller of the BCM1250A SOC can
> > be switched into a bit-banged raw I2C mode, where you can send whatever
> > you want over the bus and block tranfers would not be a problem at all,
> > but hopefully you agree this is not necessarily the best idea ever.
> 
> Between two drivers that both busy-wait and burn CPU cycles, I'd
> much rather have the one that provides the full functionality
> needed for the bus.  So I'd choose that bitbanger, no question.

 I suppose some care would have to be taken with the bit-banger so that 
the clock's frequency is not a complex function of time on its own.  Which 
means interrupts disabled, etc.

> If the SMBus driver were more functional, AND didn't busy-wait,
> then I'd consider using the native hardware.

 Oh, that can be rectified, no problem.  The controller does have
interrupt outputs that can be asserted on the busy-to-non-busy transition
as well as an error.  There is one per each of the two buses.  Plus the
silicon will retry aborted transfers itself up to 15 times before it gives
up with an error.  I can prepare a separate patch to address this issue --
I hate these devices operating in a polled manner unnecessarily too.

 BTW, the SOC has enough internal interrupt sources you can almost assume
if anything looks remotely like being able to make reasonable use of an
interrupt line it most likely has one.  It's somewhat worse outside the
SOC on this particular board -- to the best of my knowledge, the M41T81's
interrupt output is unfortunately not routed anywhere -- otherwise it's
alarm feature could be reasonably utilised; perhaps the periodic interrupt
as well.

 The reason is probably they did run out of external interrupt inputs --
of all the 16 allocatable GPIO lines, 10 are taken by a PCMCIA interface
and 2 are used for outputs.  The remaining four are actually used as
interrupts, but all taken by other devices.  There are some 16 (IIRC)  
unused interrupt inputs in the HT-PCI bridge, but that bridge was made by
some other company Broadcom might have not wanted to support too
aggresively. ;-)

 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@mvista.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);
+}
+
+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);
+}
+
+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)) {
+		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;
+}
+
+static int m41t80_get_datetime(struct i2c_client *client, struct rtc_time *tm)
+{
+	u8 buf[M41T80_DATETIME_REG_SIZE];
+	int loops = 2;
+	int sec0, sec1;
+
+	/*
+	 * Time registers are latched by this chip if an I2C block
+	 * transfer is used, but with SMBus-style byte accesses
+	 * this is not the case, so check seconds for a wraparound.
+	 */
+	do {
+		if (m41t80_transfer(client, 0, M41T80_REG_SEC,
+				    M41T80_DATETIME_REG_SIZE - M41T80_REG_SEC,
+				    buf + M41T80_REG_SEC) < 0) {
+			dev_err(&client->dev, "read error\n");
+			return -EIO;
+		}
+		sec0 = buf[M41T80_REG_SEC];
+
+		sec1 = i2c_smbus_read_byte_data(client, M41T80_REG_SEC);
+		if (sec1 < 0) {
+			dev_err(&client->dev, "read error\n");
+			return -EIO;
+		}
+
+		sec0 = BCD2BIN(sec0 & 0x7f);
+		sec1 = BCD2BIN(sec1 & 0x7f);
+	} while (sec1 < sec0 && --loops);
 
-	tm->tm_sec = BCD2BIN(buf[M41T80_REG_SEC] & 0x7f);
+	tm->tm_sec = sec1;
 	tm->tm_min = BCD2BIN(buf[M41T80_REG_MIN] & 0x7f);
 	tm->tm_hour = BCD2BIN(buf[M41T80_REG_HOUR] & 0x3f);
 	tm->tm_mday = BCD2BIN(buf[M41T80_REG_DAY] & 0x3f);
@@ -117,39 +195,16 @@ static int m41t80_get_datetime(struct i2
 /* Sets the given date and time to the real time clock. */
 static int m41t80_set_datetime(struct i2c_client *client, struct rtc_time *tm)
 {
-	u8 wbuf[1 + M41T80_DATETIME_REG_SIZE];
-	u8 *buf = &wbuf[1];
-	u8 dt_addr[1] = { M41T80_REG_SEC };
-	struct i2c_msg msgs_in[] = {
-		{
-			.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,
-		},
-	};
-	struct i2c_msg msgs[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1 + M41T80_DATETIME_REG_SIZE,
-			.buf	= wbuf,
-		 },
-	};
+	u8 buf[M41T80_DATETIME_REG_SIZE];
 
 	/* Read current reg values into buf[1..7] */
-	if (i2c_transfer(client->adapter, msgs_in, 2) < 0) {
+	if (m41t80_transfer(client, 0, M41T80_REG_SEC,
+			    M41T80_DATETIME_REG_SIZE - M41T80_REG_SEC,
+			    buf + M41T80_REG_SEC) < 0) {
 		dev_err(&client->dev, "read error\n");
 		return -EIO;
 	}
 
-	wbuf[0] = 0; /* offset into rtc's regs */
 	/* Merge time-data and register flags into buf[0..7] */
 	buf[M41T80_REG_SSEC] = 0;
 	buf[M41T80_REG_SEC] =
@@ -167,7 +222,8 @@ static int m41t80_set_datetime(struct i2
 	/* assume 20YY not 19YY */
 	buf[M41T80_REG_YEAR] = BIN2BCD(tm->tm_year % 100);
 
-	if (i2c_transfer(client->adapter, msgs, 1) != 1) {
+	if (m41t80_transfer(client, 1, M41T80_REG_SSEC,
+			    M41T80_DATETIME_REG_SIZE, buf) < 0) {
 		dev_err(&client->dev, "write error\n");
 		return -EIO;
 	}
@@ -241,34 +297,11 @@ err:
 static int m41t80_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	u8 wbuf[1 + M41T80_ALARM_REG_SIZE];
-	u8 *buf = &wbuf[1];
+	u8 buf[M41T80_ALARM_REG_SIZE];
 	u8 *reg = buf - M41T80_REG_ALARM_MON;
-	u8 dt_addr[1] = { M41T80_REG_ALARM_MON };
-	struct i2c_msg msgs_in[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1,
-			.buf	= dt_addr,
-		},
-		{
-			.addr	= client->addr,
-			.flags	= I2C_M_RD,
-			.len	= M41T80_ALARM_REG_SIZE,
-			.buf	= buf,
-		},
-	};
-	struct i2c_msg msgs[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1 + M41T80_ALARM_REG_SIZE,
-			.buf	= wbuf,
-		 },
-	};
 
-	if (i2c_transfer(client->adapter, msgs_in, 2) < 0) {
+	if (m41t80_transfer(client, 0, M41T80_REG_ALARM_MON,
+			    M41T80_ALARM_REG_SIZE, buf) < 0) {
 		dev_err(&client->dev, "read error\n");
 		return -EIO;
 	}
@@ -278,7 +311,6 @@ static int m41t80_rtc_set_alarm(struct d
 	reg[M41T80_REG_ALARM_MIN] = 0;
 	reg[M41T80_REG_ALARM_SEC] = 0;
 
-	wbuf[0] = M41T80_REG_ALARM_MON; /* offset into rtc's regs */
 	reg[M41T80_REG_ALARM_SEC] |= t->time.tm_sec >= 0 ?
 		BIN2BCD(t->time.tm_sec) : 0x80;
 	reg[M41T80_REG_ALARM_MIN] |= t->time.tm_min >= 0 ?
@@ -292,7 +324,8 @@ static int m41t80_rtc_set_alarm(struct d
 	else
 		reg[M41T80_REG_ALARM_DAY] |= 0x40;
 
-	if (i2c_transfer(client->adapter, msgs, 1) != 1) {
+	if (m41t80_transfer(client, 1, M41T80_REG_ALARM_MON,
+			    M41T80_ALARM_REG_SIZE, buf) < 0) {
 		dev_err(&client->dev, "write error\n");
 		return -EIO;
 	}
@@ -311,25 +344,11 @@ static int m41t80_rtc_set_alarm(struct d
 static int m41t80_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	u8 buf[M41T80_ALARM_REG_SIZE + 1]; /* all alarm regs and flags */
-	u8 dt_addr[1] = { M41T80_REG_ALARM_MON };
+	u8 buf[M41T80_ALARM_REG_SIZE + 1];	/* all alarm regs and flags */
 	u8 *reg = buf - M41T80_REG_ALARM_MON;
-	struct i2c_msg msgs[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1,
-			.buf	= dt_addr,
-		},
-		{
-			.addr	= client->addr,
-			.flags	= I2C_M_RD,
-			.len	= M41T80_ALARM_REG_SIZE + 1,
-			.buf	= buf,
-		},
-	};
 
-	if (i2c_transfer(client->adapter, msgs, 2) < 0) {
+	if (m41t80_transfer(client, 0, M41T80_REG_ALARM_MON,
+			    M41T80_ALARM_REG_SIZE + 1, buf) < 0) {
 		dev_err(&client->dev, "read error\n");
 		return -EIO;
 	}
@@ -488,26 +507,16 @@ static int boot_flag;
  */
 static void wdt_ping(void)
 {
-	unsigned char i2c_data[2];
-	struct i2c_msg msgs1[1] = {
-		{
-			.addr	= save_client->addr,
-			.flags	= 0,
-			.len	= 2,
-			.buf	= i2c_data,
-		},
-	};
-	i2c_data[0] = 0x09;		/* watchdog register */
+	u8 wdt = 0x80;				/* WDS = 1 (0x80)  */
 
 	if (wdt_margin > 31)
-		i2c_data[1] = (wdt_margin & 0xFC) | 0x83; /* resolution = 4s */
+		/* mulitplier = WD_TIMO / 4, resolution = 4s (0x3)  */
+		wdt |= (wdt_margin & 0xfc) | 0x3;
 	else
-		/*
-		 * WDS = 1 (0x80), mulitplier = WD_TIMO, resolution = 1s (0x02)
-		 */
-		i2c_data[1] = wdt_margin<<2 | 0x82;
+		/* mulitplier = WD_TIMO, resolution = 1s (0x2)  */
+		wdt |= wdt_margin << 2 | 0x2;
 
-	i2c_transfer(save_client->adapter, msgs1, 1);
+	i2c_smbus_write_byte_data(save_client, M41T80_REG_WATCHDOG, wdt);
 }
 
 /**
@@ -517,36 +526,8 @@ static void wdt_ping(void)
  */
 static void wdt_disable(void)
 {
-	unsigned char i2c_data[2], i2c_buf[0x10];
-	struct i2c_msg msgs0[2] = {
-		{
-			.addr	= save_client->addr,
-			.flags	= 0,
-			.len	= 1,
-			.buf	= i2c_data,
-		},
-		{
-			.addr	= save_client->addr,
-			.flags	= I2C_M_RD,
-			.len	= 1,
-			.buf	= i2c_buf,
-		},
-	};
-	struct i2c_msg msgs1[1] = {
-		{
-			.addr	= save_client->addr,
-			.flags	= 0,
-			.len	= 2,
-			.buf	= i2c_data,
-		},
-	};
-
-	i2c_data[0] = 0x09;
-	i2c_transfer(save_client->adapter, msgs0, 2);
-
-	i2c_data[0] = 0x09;
-	i2c_data[1] = 0x00;
-	i2c_transfer(save_client->adapter, msgs1, 1);
+	i2c_smbus_read_byte_data(save_client, M41T80_REG_WATCHDOG);
+	i2c_smbus_write_byte_data(save_client, M41T80_REG_WATCHDOG, 0);
 }
 
 /**
@@ -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();
 		}
 
@@ -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;
 	}
 
+	/* 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);
 
 	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;
 	}
 
 	/* 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;
 	}
 
@@ -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;

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

[Jean Delvare]

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@mvista.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

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

[David Brownell]

On Thursday 08 May 2008, Maciej W. Rozycki wrote:
> Hi David,
> 
>  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).

I didn't.  I responded to a message from list archives, and could
not tell how many lists were copied ... "WAY too many", clearly.


> > >  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.

No; as Jean also noted, since it makes some explicit calls,
it should test for the functionality of those calls.  It should
not call i2c_transfer() unless the underlying adapter accepts
those calls.

 
> > > The extensions are 16-bit commands 
> > > (required by another RTC chip used on some of these boards) and an obscure
> > > subset of the process call and block read/write commands (called an EEPROM
> > > read and an extended write/read, respectively).
> > 
> > Subset of process call??  That's send-three-bytes, receive-two-bytes.
> > Not possible to subset it ... anything else isn't a process call!!
> 
>  I misinterpreted the SMBus spec -- I have thought the receive part is
> variable, sorry.  The controller implements a command which issues a write
> byte transfer followed by a receive four bytes transfer.  Not quite a
> process call although the idea is the same.

That is, no STOP in between, just a repeated START?  In which
case that's a subset of i2c_smbus_read_i2c_block_data().

 
> > Presumably those block read/write commands aren't quite enough
> > for you to implement i2c_smbus_read_i2c_block_data() and friend?
> 
>  You can issue a block read of up to 5 bytes (6 if you add the PEC byte
> which is not interpreted by the controller in any way).  And you can issue
> a block write of up to 4 bytes (5 with PEC).  That's clearly not enough
> for the m41t81 let alone a generic implementation.

Right.  Possibly worth updating i2c-sibyte to be able to perform
those calls through the "smbus i2c_block" calls; but maybe not.
(Those calls aren't true SMBus calls, but many otherwise-SMBus-only
controllers can handle them, hence the i2c_smbus_* prefix.)

 
>  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.

See above:  they sound like subsets of the Linux "smbus i2c block"
calls.  (Those calls allow up to 32 bytes, much more than what the
i2c-sibyte hardware allows.)


> > >  I feel a bit uneasy about unconditional emulation of SMBus block calls
> > > with a series of corresponding byte read/write calls.  The reason is it
> > > changes the semantics
> > 
> > No it doesn't.  The I2C signal transitions (SCL/SDA) will be
> > exactly the same.  It's IMO very misleading to call it "emulation"
> > since it's nothing more than a different software interface to
> > the same functionality.
> 
>  It is not the same.  Assuming a write for a block transfer you have:
> 
> start:address:ack:command:ack:data:ack:data:ack:data:ack...stop
> 
> on the wire

That's true using both native SMBus calls and the
SMBus "emulated" (by native I2C) implementation of
the i2c_smbus_write_i2c_block_data() interface.


You introduced something entirely different:

> while for a series of byte calls you have: 
> 
> start:address:ack:command:ack:data:ack:stop
> start:address:ack:command:ack:data:ack:stop
> start:address:ack:command:ack:data:ack...stop

(I broke each separate I2C message onto its own line for clarity.)

 
> This is what I mean here -- I gather you are thinking in the terms of raw 
> I2C block vs SMBus block transfer.

I was talking in terms of i2c_smbus_write_i2c_block_data()
and i2c_smbus_read_i2c_block_data() ... which m41t80 should
be happy with.  (But i2c-sibyte won't be.)

If that second protocol sequence (many messages) happens to
work for a given chip, it can be done *portably* too using
pure SMBus "write byte" calls:  i2c_smbus_write_byte_data().

And that knowledge is very chip-specific, so it's IMO more
appropriate to keep it out of infrastructure like i2c-core.


>  It could be useful enough for other drivers to have the emulated calls 
> available as a part of the API.  No need to rush adding that though 
> obviously -- we can wait till we have a user (now that this driver has 
> been ruled out).

I can't quite see the point though.  Any driver can write a loop
that calls i2c_smbus_write_byte_data(), if that's an alternative
way to achieve the task on that chip.

It can be rather annoying to try getting some I2C drivers to cope
with a variety of broken I2C adapters.  But that's exactly why
there's a way to ask adapters what they can do.  If they can't
execute the I2C_FUNC_SMBUS_I2C_BLOCK protocols, then M41T80 code
must provide less efficient substitutes ... like looping over
byte-at-a-time calls, and coping with various time roll-over cases
that such substitutes will surface.

- Dave

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

[Atsushi Nemoto]

On Fri, 9 May 2008 01:43:32 +0100 (BST), "Maciej W. Rozycki" <macro@linux-mips.org> wrote:
>  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.

This version works fine for me (with i2c-gpio).  And as Jean said,
using i2c_smbus_(write|read)_i2c_block_data instead of
m41t80_i2c_(write|read) works fine too.

---
Atsushi Nemoto

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

[Maciej W. Rozycki]

Hi Jean,

> 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.

 Well, there is more interest in these changes on the linux-mips mailing
list than on any other one -- I seriously doubt there is any user of
hardware based around the BCM1250A SOC on either of the i2c and rtc-linux
lists.  And the LKML is to be cc-ed on all patch submissions.

> > @@ -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().

 Where does it come from?  I fail to see this type of transfer being 
defined anywhere in the SMBus spec.  I checked the spec before I referred 
to the implementation in our I2C core and I hope you agree I may not have 
expected any extensions beyond what the SMBus spec defines.

 That written, you are of course correct WRT the reimplementation and I am 
eager to remove it -- thanks for the point.  I'll skip all your other 
comments related as obviously implied by this change.

 Given the function and friends make use of apparently a non-standard
SMBus transfer, I think they should be called differently, perhaps
i2c_smbusext_write_i2c_block_data(), etc. or suchlike.

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

 I am happy to bother you with a separate patch including style fixes.  I
can even create a handful of them, grouping functionally consistent
changes.

> >  	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.

 My system must be a notable exception then, as this change modifies 
output:

rtc-m41t80 1-0068: chip found, driver version 0.05

to:

rtc-m41t80 1-0068: m41t81 chip found, driver version 0.05

here.

> 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.

 You seem to have your boundary set differently to me and a few other
people.  This is perfectly fine, as the line is thin here and each of the
subsystems follows slightly different rules.  You cannot always satisfy
everybody and if something makes your life easier and does not make mine
more difficult, I see no problem with adapting myself. :-)

> > @@ -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.

 No problem.

  Maciej

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

[Jean Delvare]

Hi Maciej,

On Fri, 9 May 2008 21:55:38 +0100 (BST), Maciej W. Rozycki wrote:
> > This is reimplementing i2c_smbus_write_i2c_block_data().
> 
>  Where does it come from?  I fail to see this type of transfer being 
> defined anywhere in the SMBus spec.

It is indeed not.

>                                      I checked the spec before I referred 
> to the implementation in our I2C core and I hope you agree I may not have 
> expected any extensions beyond what the SMBus spec defines.

The "smbus" in these function names are more about "what SMBus
controllers usually can do" than about the SMBus specification. But I
admit you couldn't guess.

>  That written, you are of course correct WRT the reimplementation and I am 
> eager to remove it -- thanks for the point.  I'll skip all your other 
> comments related as obviously implied by this change.
> 
>  Given the function and friends make use of apparently a non-standard
> SMBus transfer, I think they should be called differently, perhaps
> i2c_smbusext_write_i2c_block_data(), etc. or suchlike.

This was an option when the functions where introduced 9 years ago.
But now that it was done, renaming them would cause even more
confusion, I think. I would be fine with adding comments in i2c-core.c
or improving Documentation/i2c/smbus-protocol to make it more obious,
though.

On a related note, you will notice that the other i2c_smbus_* functions
do not follow the naming of SMBus transactions. Again that's something
I regret but I feel that changing the names now would cause a lot of
confusion amongst developers, so I'm not doing it.

> > Mixing code cleanups with functional changes is a Bad Idea (TM).
> 
>  I am happy to bother you with a separate patch including style fixes.  I
> can even create a handful of them, grouping functionally consistent
> changes.

Just one patch should be enough, if I agree with all the changes. You
might make a separate patch with the things I may not agree with, so
that you don't have to cherry-revert them if I indeed don't agree, and
we just merge them if I do agree.

> > >  	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.
> 
>  My system must be a notable exception then, as this change modifies 
> output:
> 
> rtc-m41t80 1-0068: chip found, driver version 0.05
> 
> to:
> 
> rtc-m41t80 1-0068: m41t81 chip found, driver version 0.05
> 
> here.

My bad, for some reason I thought that dev_printk() included the device
name but it in fact includes the driver name. I was wrong, just ignore
me.

-- 
Jean Delvare

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

[Maciej W. Rozycki]

Hi David,

> >  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).
> 
> I didn't.  I responded to a message from list archives, and could

 I just asked not to remove from now on -- no implied double meaning and 
thanks for respecting my request. :-)

> not tell how many lists were copied ... "WAY too many", clearly.

 Just enough plus the usual LKML everyone is free to ignore if they 
cannot stand the volume.  And I got responses from linux-mips, which means 
my choice was right.

> No; as Jean also noted, since it makes some explicit calls,
> it should test for the functionality of those calls.  It should
> not call i2c_transfer() unless the underlying adapter accepts
> those calls.

 As mentioned elsewhere I misunderstood the semantics of the flags in the 
API.

> >  I misinterpreted the SMBus spec -- I have thought the receive part is
> > variable, sorry.  The controller implements a command which issues a write
> > byte transfer followed by a receive four bytes transfer.  Not quite a
> > process call although the idea is the same.
> 
> That is, no STOP in between, just a repeated START?  In which
> case that's a subset of i2c_smbus_read_i2c_block_data().

 There is the usual second START in between to turn around the direction.  
There is no STOP in the process call either, which is what makes it
different from an ordinary write transaction followed by a read
transaction.

> >  You can issue a block read of up to 5 bytes (6 if you add the PEC byte
> > which is not interpreted by the controller in any way).  And you can issue
> > a block write of up to 4 bytes (5 with PEC).  That's clearly not enough
> > for the m41t81 let alone a generic implementation.
> 
> Right.  Possibly worth updating i2c-sibyte to be able to perform
> those calls through the "smbus i2c_block" calls; but maybe not.
> (Those calls aren't true SMBus calls, but many otherwise-SMBus-only
> controllers can handle them, hence the i2c_smbus_* prefix.)

 I am not sure such a limited functionality is worth the hassle of making 
it available to clients in a reasonably clean way.  How common an 
extension of this kind is among SMBus controllers?  I would say if there 
are other controllers providing it (perhaps for a different range of 
transfer lengths) and clients benefitting from it, it might be worth 
adding it for this controller as well.  Otherwise perhaps let's wait till 
somebody complains about the lack of this functionality?

> If that second protocol sequence (many messages) happens to
> work for a given chip, it can be done *portably* too using
> pure SMBus "write byte" calls:  i2c_smbus_write_byte_data().
> 
> And that knowledge is very chip-specific, so it's IMO more
> appropriate to keep it out of infrastructure like i2c-core.
[...]
> I can't quite see the point though.  Any driver can write a loop
> that calls i2c_smbus_write_byte_data(), if that's an alternative
> way to achieve the task on that chip.

 Well, it seems generic enough we may provide wrappers around loops using
i2c_smbus_read_byte_data() and i2c_smbus_write_byte_data() to perform
transactions involving consecutive values of commands for clients to pick.  
You may be right it may be too trivial to bother though -- I am not sure.  
In any case, as suggested elsewhere, the core does not seem the right
place indeed, but a header file or drivers/i2c/lib/ should be appropriate.

> It can be rather annoying to try getting some I2C drivers to cope
> with a variety of broken I2C adapters.  But that's exactly why
> there's a way to ask adapters what they can do.  If they can't
> execute the I2C_FUNC_SMBUS_I2C_BLOCK protocols, then M41T80 code
> must provide less efficient substitutes ... like looping over
> byte-at-a-time calls, and coping with various time roll-over cases
> that such substitutes will surface.

 Of course.

  Maciej

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

[Maciej W. Rozycki]

Hi Jean,

> Welcome back to the world of the living creatures then. How was the
> other world? :)

 I don't remember, sorry. ;-)

> Really? I don't remember any such rule, so this must have been before
> 2003.

 Could be -- my memory happily still extends back beyond that moment of
time.

> You're wrong in your assumption that adding coding style cleanups to a
> patch is "for free". It makes reviewing the patch more difficult, and it
> makes backporting the patch more difficult, too (cleanups increase the
> risks that the patch won't apply to an older kernel, and you have to
> filter out all cleanups for -stable.) Compared to this, the time spent
> to handle a separate, cleanup-only patch is very small by my maintainer
> experience.

 Now this is certainly a valid point.  But the concept of -stable is
recent enough this is only the second or third time I see it mentioned.

> >                                             I think anybody clever enough
> > to notice an obvious rule having been applied will follow it.
> 
> You're wrong again, sorry. Developers are always in a hurry, they often
> fail to follow the written rules, they just won't care about unwritten
> ones.

 Well, you have spotted one counter-example now who does not hurry enough
for it to spoil his work.  Besides, the time required to put a header
inclusion at the right place in an ordered sequence is the same as for an
insertion at a random position.

> Want an example? Look at MAINTAINERS. It is supposed to be sorted
> alphabetically, the header says so and pretty much everyone should be
> aware of it by now, still you can check by yourself that of the 600
> entries in that file, about 100 are not at the correct place.

 Well, some people do not read user manuals either -- perhaps they have 
missed the annotation too?

> I sure hope that they are all gone by now. Headers which need to
> included in a specific order are a bug.

 Well, this is the first time I hear it stated by someone else.  Good 
progress, I would say...

> I think we have a script to find duplicates. I admit that sorting the
> headers in alphabetical order solve the problem. But OTOH, if
> developers can't be bothered to make sure they don't add a duplicate
> header, I doubt that they can be bothered to keep the includes sorted
> alphabetically.

 Well, if developers cannot be bothered to produce quality results, then
perhaps they should be doing something else?  There is no obligation to be
a software developer, is it?

> >  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.

> More exactly: if there is anything to share _and the added cost of
> sharing is less than the benefit_ then it should be done. Additional
> kernel modules have a cost. Even just exporting additional functions,
> has a cost. Going through an interface often has a cost. The benefit on
> the maintenance front must overcome these, otherwise it's not worth
> doing it.

 Having improvements and bug fixes applied to some of the repeated code
sequences only has its cost as well.  If the piece of code is small then
it can go to an inline function and the interface cost is null.  If it is
big, then it can go to a library from which all the interested modules can
import it with no need for an additional module.  Even pieces of code as
small as the Ethernet CRC calculation have been decided worth being put
into a single place even though there is little room for improvement or
breakage there.

 Go chase all the copies of code to support the SCC or the LANCE scattered
throughout our tree and figure out which has the least bugs and/or the
most features if you have any doubts about how to classify code
duplication.

> You assume that there is roughly only one way to emulate reading or
> writing a block from/to an I2C or SMBus device, with only slight
> variations. I would love it the I2C world was that simple, but
> unfortunately it is not. Each device has its own constraints on how a
> block read/write can be emulated. For example on some devices you can
> use word reads as an alternative to block reads, but other devices will
> only accept byte reads as an alternative. Some devices will accept byte
> reads but not at the same address as the block read (see for example
> drivers/hwmon/lm93.c - actually this is mandatory for SMBus block
> reads), etc. So I am still skeptical that we can come up with one or
> more emulation functions that can be used by more than 2 drivers and
> still get the best of each device.

 I am not assuming anything here.  I have only stated that if this
approach to transfers involving a range of commands was useful elsewhere,
then code to do so should not be duplicated throughout all the interested
drivers.  I do not know if there are any other than this -- it is up to
the users and/or maintainers of code in question to determine.

> But well, if you want to give it a try, just go on, prepare a patch,
> and send it for review.

 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.

 Everybody interested please give it a try.  It certainly works for me.  
Comments are welcome.  I will submit all the clean-up changes separately,
independently or when the whole set is ready as appropriate, based on
dependencies or the lack of.

  Maciej

patch-2.6.26-rc1-20080505-m41t80-smbus-15
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-10 00:57:13.000000000 +0000
@@ -6,6 +6,7 @@
  * Based on m41t00.c by Mark A. Greer <mgreer@mvista.com>
  *
  * 2006 (c) mycable GmbH
+ * Copyright (c) 2008  Maciej W. Rozycki
  *
  * 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
@@ -36,6 +37,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 +61,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 +81,78 @@ struct m41t80_data {
 	struct rtc_device *rtc;
 };
 
-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;
+}
 
-	tm->tm_sec = BCD2BIN(buf[M41T80_REG_SEC] & 0x7f);
+static int m41t80_get_datetime(struct i2c_client *client, struct rtc_time *tm)
+{
+	u8 buf[M41T80_DATETIME_REG_SIZE];
+	int loops = 2;
+	int sec0, sec1;
+
+	/*
+	 * Time registers are latched by this chip if an I2C block
+	 * transfer is used, but with SMBus-style byte accesses
+	 * this is not the case, so check seconds for a wraparound.
+	 */
+	do {
+		if (m41t80_transfer(client, 0, M41T80_REG_SEC,
+				    M41T80_DATETIME_REG_SIZE - M41T80_REG_SEC,
+				    buf + M41T80_REG_SEC) < 0) {
+			dev_err(&client->dev, "read error\n");
+			return -EIO;
+		}
+		sec0 = buf[M41T80_REG_SEC];
+
+		sec1 = i2c_smbus_read_byte_data(client, M41T80_REG_SEC);
+		if (sec1 < 0) {
+			dev_err(&client->dev, "read error\n");
+			return -EIO;
+		}
+
+		sec0 = BCD2BIN(sec0 & 0x7f);
+		sec1 = BCD2BIN(sec1 & 0x7f);
+	} while (sec1 < sec0 && --loops);
+
+	tm->tm_sec = sec1;
 	tm->tm_min = BCD2BIN(buf[M41T80_REG_MIN] & 0x7f);
 	tm->tm_hour = BCD2BIN(buf[M41T80_REG_HOUR] & 0x3f);
 	tm->tm_mday = BCD2BIN(buf[M41T80_REG_DAY] & 0x3f);
@@ -117,39 +167,16 @@ static int m41t80_get_datetime(struct i2
 /* Sets the given date and time to the real time clock. */
 static int m41t80_set_datetime(struct i2c_client *client, struct rtc_time *tm)
 {
-	u8 wbuf[1 + M41T80_DATETIME_REG_SIZE];
-	u8 *buf = &wbuf[1];
-	u8 dt_addr[1] = { M41T80_REG_SEC };
-	struct i2c_msg msgs_in[] = {
-		{
-			.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,
-		},
-	};
-	struct i2c_msg msgs[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1 + M41T80_DATETIME_REG_SIZE,
-			.buf	= wbuf,
-		 },
-	};
+	u8 buf[M41T80_DATETIME_REG_SIZE];
 
 	/* Read current reg values into buf[1..7] */
-	if (i2c_transfer(client->adapter, msgs_in, 2) < 0) {
+	if (m41t80_transfer(client, 0, M41T80_REG_SEC,
+			    M41T80_DATETIME_REG_SIZE - M41T80_REG_SEC,
+			    buf + M41T80_REG_SEC) < 0) {
 		dev_err(&client->dev, "read error\n");
 		return -EIO;
 	}
 
-	wbuf[0] = 0; /* offset into rtc's regs */
 	/* Merge time-data and register flags into buf[0..7] */
 	buf[M41T80_REG_SSEC] = 0;
 	buf[M41T80_REG_SEC] =
@@ -167,7 +194,8 @@ static int m41t80_set_datetime(struct i2
 	/* assume 20YY not 19YY */
 	buf[M41T80_REG_YEAR] = BIN2BCD(tm->tm_year % 100);
 
-	if (i2c_transfer(client->adapter, msgs, 1) != 1) {
+	if (m41t80_transfer(client, 1, M41T80_REG_SSEC,
+			    M41T80_DATETIME_REG_SIZE, buf) < 0) {
 		dev_err(&client->dev, "write error\n");
 		return -EIO;
 	}
@@ -241,34 +269,11 @@ err:
 static int m41t80_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	u8 wbuf[1 + M41T80_ALARM_REG_SIZE];
-	u8 *buf = &wbuf[1];
+	u8 buf[M41T80_ALARM_REG_SIZE];
 	u8 *reg = buf - M41T80_REG_ALARM_MON;
-	u8 dt_addr[1] = { M41T80_REG_ALARM_MON };
-	struct i2c_msg msgs_in[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1,
-			.buf	= dt_addr,
-		},
-		{
-			.addr	= client->addr,
-			.flags	= I2C_M_RD,
-			.len	= M41T80_ALARM_REG_SIZE,
-			.buf	= buf,
-		},
-	};
-	struct i2c_msg msgs[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1 + M41T80_ALARM_REG_SIZE,
-			.buf	= wbuf,
-		 },
-	};
 
-	if (i2c_transfer(client->adapter, msgs_in, 2) < 0) {
+	if (m41t80_transfer(client, 0, M41T80_REG_ALARM_MON,
+			    M41T80_ALARM_REG_SIZE, buf) < 0) {
 		dev_err(&client->dev, "read error\n");
 		return -EIO;
 	}
@@ -278,7 +283,6 @@ static int m41t80_rtc_set_alarm(struct d
 	reg[M41T80_REG_ALARM_MIN] = 0;
 	reg[M41T80_REG_ALARM_SEC] = 0;
 
-	wbuf[0] = M41T80_REG_ALARM_MON; /* offset into rtc's regs */
 	reg[M41T80_REG_ALARM_SEC] |= t->time.tm_sec >= 0 ?
 		BIN2BCD(t->time.tm_sec) : 0x80;
 	reg[M41T80_REG_ALARM_MIN] |= t->time.tm_min >= 0 ?
@@ -292,7 +296,8 @@ static int m41t80_rtc_set_alarm(struct d
 	else
 		reg[M41T80_REG_ALARM_DAY] |= 0x40;
 
-	if (i2c_transfer(client->adapter, msgs, 1) != 1) {
+	if (m41t80_transfer(client, 1, M41T80_REG_ALARM_MON,
+			    M41T80_ALARM_REG_SIZE, buf) < 0) {
 		dev_err(&client->dev, "write error\n");
 		return -EIO;
 	}
@@ -312,24 +317,10 @@ static int m41t80_rtc_read_alarm(struct 
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	u8 buf[M41T80_ALARM_REG_SIZE + 1]; /* all alarm regs and flags */
-	u8 dt_addr[1] = { M41T80_REG_ALARM_MON };
 	u8 *reg = buf - M41T80_REG_ALARM_MON;
-	struct i2c_msg msgs[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1,
-			.buf	= dt_addr,
-		},
-		{
-			.addr	= client->addr,
-			.flags	= I2C_M_RD,
-			.len	= M41T80_ALARM_REG_SIZE + 1,
-			.buf	= buf,
-		},
-	};
 
-	if (i2c_transfer(client->adapter, msgs, 2) < 0) {
+	if (m41t80_transfer(client, 0, M41T80_REG_ALARM_MON,
+			    M41T80_ALARM_REG_SIZE + 1, buf) < 0) {
 		dev_err(&client->dev, "read error\n");
 		return -EIO;
 	}
@@ -488,26 +479,16 @@ static int boot_flag;
  */
 static void wdt_ping(void)
 {
-	unsigned char i2c_data[2];
-	struct i2c_msg msgs1[1] = {
-		{
-			.addr	= save_client->addr,
-			.flags	= 0,
-			.len	= 2,
-			.buf	= i2c_data,
-		},
-	};
-	i2c_data[0] = 0x09;		/* watchdog register */
+	u8 wdt = 0x80;				/* WDS = 1 (0x80)  */
 
 	if (wdt_margin > 31)
-		i2c_data[1] = (wdt_margin & 0xFC) | 0x83; /* resolution = 4s */
+		/* mulitplier = WD_TIMO / 4, resolution = 4s (0x3)  */
+		wdt |= (wdt_margin & 0xfc) | 0x3;
 	else
-		/*
-		 * WDS = 1 (0x80), mulitplier = WD_TIMO, resolution = 1s (0x02)
-		 */
-		i2c_data[1] = wdt_margin<<2 | 0x82;
+		/* mulitplier = WD_TIMO, resolution = 1s (0x2)  */
+		wdt |= wdt_margin << 2 | 0x2;
 
-	i2c_transfer(save_client->adapter, msgs1, 1);
+	i2c_smbus_write_byte_data(save_client, M41T80_REG_WATCHDOG, wdt);
 }
 
 /**
@@ -517,36 +498,8 @@ static void wdt_ping(void)
  */
 static void wdt_disable(void)
 {
-	unsigned char i2c_data[2], i2c_buf[0x10];
-	struct i2c_msg msgs0[2] = {
-		{
-			.addr	= save_client->addr,
-			.flags	= 0,
-			.len	= 1,
-			.buf	= i2c_data,
-		},
-		{
-			.addr	= save_client->addr,
-			.flags	= I2C_M_RD,
-			.len	= 1,
-			.buf	= i2c_buf,
-		},
-	};
-	struct i2c_msg msgs1[1] = {
-		{
-			.addr	= save_client->addr,
-			.flags	= 0,
-			.len	= 2,
-			.buf	= i2c_data,
-		},
-	};
-
-	i2c_data[0] = 0x09;
-	i2c_transfer(save_client->adapter, msgs0, 2);
-
-	i2c_data[0] = 0x09;
-	i2c_data[1] = 0x00;
-	i2c_transfer(save_client->adapter, msgs1, 1);
+	i2c_smbus_read_byte_data(save_client, M41T80_REG_WATCHDOG);
+	i2c_smbus_write_byte_data(save_client, M41T80_REG_WATCHDOG, 0);
 }
 
 /**
@@ -736,13 +689,25 @@ static int m41t80_probe(struct i2c_clien
 	struct rtc_device *rtc = NULL;
 	struct rtc_time tm;
 	struct m41t80_data *clientdata = NULL;
+	u32 func;
+	int reg;
 
-	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;
 	}
 
+	/* 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");
 
@@ -765,11 +730,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");
@@ -782,18 +743,18 @@ static int m41t80_probe(struct i2c_clien
 		}
 		if (i2c_smbus_write_byte_data(client,
 					      M41T80_REG_ALARM_HOUR,
-					      rc & ~M41T80_ALHOUR_HT) < 0)
+					      reg & ~M41T80_ALHOUR_HT) < 0)
 			goto ht_err;
 	}
 
 	/* 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;
 	}
 

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

[Maciej W. Rozycki]

Hi Jean,

> This was an option when the functions where introduced 9 years ago.
> But now that it was done, renaming them would cause even more
> confusion, I think. I would be fine with adding comments in i2c-core.c
> or improving Documentation/i2c/smbus-protocol to make it more obious,
> though.
> 
> On a related note, you will notice that the other i2c_smbus_* functions
> do not follow the naming of SMBus transactions. Again that's something
> I regret but I feel that changing the names now would cause a lot of
> confusion amongst developers, so I'm not doing it.

 It may not be worth the effort, but if done in bulk for all the users in
the tree, there should be no problem with that.  I am fairly sure there
were changes of this kind from time to time, with occasional screams heard
in response from some dark corners, but no big pain.  We obviously
explicitly disregard out-of-tree users and for occasional contributors
asking: "Where the * has this function gone?" there is the Documentation/
tree to provide a greppable reference, so generally not a big deal.

> Just one patch should be enough, if I agree with all the changes. You
> might make a separate patch with the things I may not agree with, so
> that you don't have to cherry-revert them if I indeed don't agree, and
> we just merge them if I do agree.

 Hmm, technically you do not seem to be responsible to accept changes
under drivers/rtc/, so I will split them anyway for others to decide.  In
particular I do plan to submit a separate change for header ordering for
the driver, just out of curiosity to see how long it will stay unspoilt.  
Somehow most if not all changes of this kind that I ever made to files in
our tree have survived to the present day. :-)

> My bad, for some reason I thought that dev_printk() included the device
> name but it in fact includes the driver name. I was wrong, just ignore
> me.

 It will go separately though as not directly related to this
modification, now that I have started splitting it.

  Maciej

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

[Jean Delvare]

On Sat, 10 May 2008 03:21:35 +0100 (BST), Maciej W. Rozycki wrote:
> > This was an option when the functions where introduced 9 years ago.
> > But now that it was done, renaming them would cause even more
> > confusion, I think. I would be fine with adding comments in i2c-core.c
> > or improving Documentation/i2c/smbus-protocol to make it more obious,
> > though.
> > 
> > On a related note, you will notice that the other i2c_smbus_* functions
> > do not follow the naming of SMBus transactions. Again that's something
> > I regret but I feel that changing the names now would cause a lot of
> > confusion amongst developers, so I'm not doing it.
> 
>  It may not be worth the effort, but if done in bulk for all the users in
> the tree, there should be no problem with that.  I am fairly sure there
> were changes of this kind from time to time, with occasional screams heard
> in response from some dark corners, but no big pain.  We obviously
> explicitly disregard out-of-tree users and for occasional contributors
> asking: "Where the * has this function gone?" there is the Documentation/
> tree to provide a greppable reference, so generally not a big deal.

It's not that easy. There are some drivers which are both in-tree and
out-of-tree, for which such a change means adding ifdefs. And there is
i2c-dev.h (the user-space one) which has similar functions, if we
rename only the kernel variants, there will be some confusion. But if
we rename also the user-space variants, then it's up to 2.4 kernel
users to have different names for kernel-space and user-space functions.

All in all I'd say it is not worth the effort. There are many other
tasks where our time will be better used.

> > Just one patch should be enough, if I agree with all the changes. You
> > might make a separate patch with the things I may not agree with, so
> > that you don't have to cherry-revert them if I indeed don't agree, and
> > we just merge them if I do agree.
> 
>  Hmm, technically you do not seem to be responsible to accept changes
> under drivers/rtc/, so I will split them anyway for others to decide.

Huu, sorry, for some reason I thought that we were still speaking about
i2c-sibyte. Of course I don't have my say about what happens in
drivers/rtc.

-- 
Jean Delvare

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

[Jean Delvare]

On Fri, 9 May 2008 22:22:11 +0100 (BST), Maciej W. Rozycki wrote:
> > >  You can issue a block read of up to 5 bytes (6 if you add the PEC byte
> > > which is not interpreted by the controller in any way).  And you can issue
> > > a block write of up to 4 bytes (5 with PEC).  That's clearly not enough
> > > for the m41t81 let alone a generic implementation.
> > 
> > Right.  Possibly worth updating i2c-sibyte to be able to perform
> > those calls through the "smbus i2c_block" calls; but maybe not.
> > (Those calls aren't true SMBus calls, but many otherwise-SMBus-only
> > controllers can handle them, hence the i2c_smbus_* prefix.)
> 
>  I am not sure such a limited functionality is worth the hassle of making 
> it available to clients in a reasonably clean way.  How common an 
> extension of this kind is among SMBus controllers?  I would say if there 
> are other controllers providing it (perhaps for a different range of 
> transfer lengths) and clients benefitting from it, it might be worth 
> adding it for this controller as well.  Otherwise perhaps let's wait till 
> somebody complains about the lack of this functionality?

The problem is that the interface for client drivers to query the
adapters capabilities is rather limited. There's just one bit for I2C
block read, so if an adapter has limitations in the size of requests it
can accept (beyond the traditional 32-byte limit that comes from SMBus)
it can't express it. This means that client drivers should expect
transaction requests to fail even if they checked that the transaction
type in question was supported. Most client drivers don't actually
expect that.

My advice would be to only bother implementing restricted support for a
transaction type if there's a big benefit in doing so. And then, double
check that all the client drivers that are likely to be used with the
adapter in question, are robust enough to deal with the restrictions
gracefully.

-- 
Jean Delvare

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

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

[David Brownell]

On Friday 09 May 2008, Jean Delvare wrote:
> 
> > > On a related note, you will notice that the other i2c_smbus_* functions
> > > do not follow the naming of SMBus transactions. Again that's something
> > > I regret but I feel that changing the names now would cause a lot of
> > > confusion amongst developers, so I'm not doing it.
> > 
> > It may not be worth the effort, but if done in bulk for all the users in
> > the tree, there should be no problem with that.  ...
> 
> It's not that easy. There are some drivers which are both in-tree and
> out-of-tree, for which such a change means adding ifdefs.

Actually, I thought it *WAS* that easy.  See the appended patch,
which goes on top of the various doc and (mostly SMBus) fault
handling patches I've sent ... the old names are still available
(only needed by out-of-tree drivers), but marked as __deprecated.
So:  no #ifdefs required.

I agree it's not worth while for *all* the SMBus functions that
use names-made-up-for-Linux.  But for the two which are really
badly misnamed (after *different* SMBus operations), I suggest
fixing would be a reasonable thing.


> And there is 
> i2c-dev.h (the user-space one) which has similar functions,

That's problematic in its own right though.  Not only that
the *kernel* file Documentation/i2c/dev-interface referring
to those functions, which are unavailable from the header
provided by the kernel.  Or that there's no relationship
between the kernel and userspace files of the same name.
But also that those functions are actually a bit too large
to be appropriate as inlines, even once you manage to track
it down (as part of a "tools" package, not a "library").


> if we 
> rename only the kernel variants, there will be some confusion. But if
> we rename also the user-space variants, then it's up to 2.4 kernel
> users to have different names for kernel-space and user-space functions.

True, but that would be a question for a "libsmbus" or somesuch
to deal with.  Not a kernel issue.

- Dave


===========
Two of the SMBus operations are using confusingly inappropriate names:

  i2c_smbus_read_byte() does not execute the SMBus "Read Byte" protocol
  	... it implements the SMBus "Receive Byte" protocol instead!!

  i2c_smbus_write_byte() does not execute the SMBus "Write Byte" protocol
  	... it implements the SMBus "Send Byte" protocol instead!!

This patch changes the names of those functions, so they no longer
use names of different operations (which they do not implement).

---
 Documentation/i2c/chips/max6875   |    4 ++--
 Documentation/i2c/smbus-protocol  |   12 +++++-------
 Documentation/i2c/writing-clients |    4 ++--
 drivers/gpio/pcf857x.c            |    8 ++++----
 drivers/hwmon/ds1621.c            |    2 +-
 drivers/hwmon/lm90.c              |    2 +-
 drivers/i2c/chips/eeprom.c        |   10 +++++-----
 drivers/i2c/chips/max6875.c       |    2 +-
 drivers/i2c/chips/pcf8574.c       |    4 ++--
 drivers/i2c/chips/pcf8591.c       |   10 +++++-----
 drivers/i2c/chips/tsl2550.c       |   16 ++++++++--------
 drivers/i2c/i2c-core.c            |   12 ++++++------
 drivers/media/video/saa7110.c     |    2 +-
 drivers/media/video/saa7185.c     |    2 +-
 drivers/media/video/tda9840.c     |    8 ++++----
 drivers/media/video/tea6415c.c    |    4 ++--
 drivers/media/video/tea6420.c     |    4 ++--
 drivers/w1/masters/ds2482.c       |   10 +++++-----
 include/linux/i2c.h               |   20 ++++++++++++++++++--
 19 files changed, 75 insertions(+), 61 deletions(-)

--- g26.orig/include/linux/i2c.h	2008-05-07 16:32:18.000000000 -0700
+++ g26/include/linux/i2c.h	2008-05-10 00:13:48.000000000 -0700
@@ -73,8 +73,8 @@ extern s32 i2c_smbus_xfer (struct i2c_ad
    conventions of smbus_access. */
 
 extern s32 i2c_smbus_write_quick(struct i2c_client * client, u8 value);
-extern s32 i2c_smbus_read_byte(struct i2c_client * client);
-extern s32 i2c_smbus_write_byte(struct i2c_client * client, u8 value);
+extern s32 i2c_smbus_receive_byte(struct i2c_client *client);
+extern s32 i2c_smbus_send_byte(struct i2c_client *client, u8 value);
 extern s32 i2c_smbus_read_byte_data(struct i2c_client * client, u8 command);
 extern s32 i2c_smbus_write_byte_data(struct i2c_client * client,
                                      u8 command, u8 value);
@@ -94,6 +94,22 @@ extern s32 i2c_smbus_write_i2c_block_dat
 					  u8 command, u8 length,
 					  const u8 *values);
 
+/* Stop using these legacy names in new code.
+ * - the SMBus "Read Byte" operation is i2c_smbus_read_byte_data()
+ * - the SMBus "Write Byte" operation is i2c_smbus_write_byte_data()
+ *
+ */
+static inline s32 __deprecated i2c_smbus_read_byte(struct i2c_client *client)
+{
+	return i2c_smbus_receive_byte(client);
+}
+
+static inline s32 __deprecated i2c_smbus_write_byte(struct i2c_client *client,
+		u8 value)
+{
+	return i2c_smbus_send_byte(client, value);
+}
+
 /*
  * A driver is capable of handling one or more physical devices present on
  * I2C adapters. This information is used to inform the driver of adapter
--- g26.orig/drivers/i2c/i2c-core.c	2008-05-07 16:32:18.000000000 -0700
+++ g26/drivers/i2c/i2c-core.c	2008-05-10 00:08:45.000000000 -0700
@@ -1529,13 +1529,13 @@ s32 i2c_smbus_write_quick(struct i2c_cli
 EXPORT_SYMBOL(i2c_smbus_write_quick);
 
 /**
- * i2c_smbus_read_byte - SMBus "receive byte" protocol
+ * i2c_smbus_receive_byte - SMBus "receive byte" protocol
  * @client: Handle to slave device
  *
  * This executes the SMBus "receive byte" protocol, returning negative errno
  * else the byte received from the device.
  */
-s32 i2c_smbus_read_byte(struct i2c_client *client)
+s32 i2c_smbus_receive_byte(struct i2c_client *client)
 {
 	union i2c_smbus_data data;
 	int status;
@@ -1545,22 +1545,22 @@ s32 i2c_smbus_read_byte(struct i2c_clien
 			I2C_SMBUS_BYTE, &data);
 	return (status < 0) ? status : data.byte;
 }
-EXPORT_SYMBOL(i2c_smbus_read_byte);
+EXPORT_SYMBOL(i2c_smbus_receive_byte);
 
 /**
- * i2c_smbus_write_byte - SMBus "send byte" protocol
+ * i2c_smbus_send_byte - SMBus "send byte" protocol
  * @client: Handle to slave device
  * @value: Byte to be sent
  *
  * This executes the SMBus "send byte" protocol, returning negative errno
  * else zero on success.
  */
-s32 i2c_smbus_write_byte(struct i2c_client *client, u8 value)
+s32 i2c_smbus_send_byte(struct i2c_client *client, u8 value)
 {
 	return i2c_smbus_xfer(client->adapter,client->addr,client->flags,
 	                      I2C_SMBUS_WRITE, value, I2C_SMBUS_BYTE, NULL);
 }
-EXPORT_SYMBOL(i2c_smbus_write_byte);
+EXPORT_SYMBOL(i2c_smbus_send_byte);
 
 /**
  * i2c_smbus_read_byte_data - SMBus "read byte" protocol
--- g26.orig/Documentation/i2c/chips/max6875	2007-07-12 16:32:15.000000000 -0700
+++ g26/Documentation/i2c/chips/max6875	2008-05-10 00:07:50.000000000 -0700
@@ -88,14 +88,14 @@ To write 0x5a to address 0x8003:
 
 Reading data from the EEPROM is a little more complicated.
 Use i2c_smbus_write_byte_data() to set the read address and then
-i2c_smbus_read_byte() or i2c_smbus_read_i2c_block_data() to read the data.
+i2c_smbus_receive_byte() or i2c_smbus_read_i2c_block_data() to read the data.
 
 Example:
 To read data starting at offset 0x8100, first set the address:
   i2c_smbus_write_byte_data(fd, 0x81, 0x00);
 
 And then read the data
-  value = i2c_smbus_read_byte(fd);
+  value = i2c_smbus_receive_byte(fd);
 
   or
 
--- g26.orig/Documentation/i2c/smbus-protocol	2008-05-10 00:13:59.000000000 -0700
+++ g26/Documentation/i2c/smbus-protocol	2008-05-10 00:15:14.000000000 -0700
@@ -18,9 +18,7 @@ handled at all on most pure SMBus adapte
 
 Below is a list of SMBus protocol operations, and the functions executing
 them.  Note that the names used in the SMBus protocol specifications usually
-don't match these function names.  For some of the operations which pass a
-single data byte, the functions using SMBus protocol operation names execute
-a different protocol operation entirely.
+don't match these function names.
 
 
 Key to symbols
@@ -49,8 +47,8 @@ This sends a single bit to the device, a
 A Addr Rd/Wr [A] P
 
 
-SMBus Receive Byte:  i2c_smbus_read_byte()
-==========================================
+SMBus Receive Byte:  i2c_smbus_receive_byte()
+=============================================
 
 This reads a single byte from a device, without specifying a device
 register. Some devices are so simple that this interface is enough; for
@@ -60,8 +58,8 @@ the previous SMBus command.
 S Addr Rd [A] [Data] NA P
 
 
-SMBus Send Byte:  i2c_smbus_write_byte()
-========================================
+SMBus Send Byte:  i2c_smbus_send_byte()
+=======================================
 
 This operation is the reverse of Receive Byte: it sends a single byte
 to a device.  See Receive Byte for more information.
--- g26.orig/Documentation/i2c/writing-clients	2008-04-29 21:51:55.000000000 -0700
+++ g26/Documentation/i2c/writing-clients	2008-05-10 00:07:50.000000000 -0700
@@ -560,8 +560,8 @@ SMBus communication
 
 
   extern s32 i2c_smbus_write_quick(struct i2c_client * client, u8 value);
-  extern s32 i2c_smbus_read_byte(struct i2c_client * client);
-  extern s32 i2c_smbus_write_byte(struct i2c_client * client, u8 value);
+  extern s32 i2c_smbus_receive_byte(struct i2c_client * client);
+  extern s32 i2c_smbus_send_byte(struct i2c_client * client, u8 value);
   extern s32 i2c_smbus_read_byte_data(struct i2c_client * client, u8 command);
   extern s32 i2c_smbus_write_byte_data(struct i2c_client * client,
                                        u8 command, u8 value);
--- g26.orig/drivers/gpio/pcf857x.c	2008-05-07 16:32:18.000000000 -0700
+++ g26/drivers/gpio/pcf857x.c	2008-05-10 00:07:50.000000000 -0700
@@ -68,7 +68,7 @@ static int pcf857x_input8(struct gpio_ch
 	struct pcf857x	*gpio = container_of(chip, struct pcf857x, chip);
 
 	gpio->out |= (1 << offset);
-	return i2c_smbus_write_byte(gpio->client, gpio->out);
+	return i2c_smbus_send_byte(gpio->client, gpio->out);
 }
 
 static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
@@ -76,7 +76,7 @@ static int pcf857x_get8(struct gpio_chip
 	struct pcf857x	*gpio = container_of(chip, struct pcf857x, chip);
 	s32		value;
 
-	value = i2c_smbus_read_byte(gpio->client);
+	value = i2c_smbus_receive_byte(gpio->client);
 	return (value < 0) ? 0 : (value & (1 << offset));
 }
 
@@ -89,7 +89,7 @@ static int pcf857x_output8(struct gpio_c
 		gpio->out |= bit;
 	else
 		gpio->out &= ~bit;
-	return i2c_smbus_write_byte(gpio->client, gpio->out);
+	return i2c_smbus_send_byte(gpio->client, gpio->out);
 }
 
 static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
@@ -202,7 +202,7 @@ static int pcf857x_probe(struct i2c_clie
 
 		/* fail if there's no chip present */
 		else
-			status = i2c_smbus_read_byte(client);
+			status = i2c_smbus_receive_byte(client);
 
 	/* '75/'75c addresses are 0x20..0x27, just like the '74;
 	 * the '75c doesn't have a current source pulling high.
--- g26.orig/drivers/hwmon/ds1621.c	2008-03-28 11:08:28.000000000 -0700
+++ g26/drivers/hwmon/ds1621.c	2008-05-10 00:07:50.000000000 -0700
@@ -132,7 +132,7 @@ static void ds1621_init_client(struct i2
 	ds1621_write_value(client, DS1621_REG_CONF, reg);
 	
 	/* start conversion */
-	i2c_smbus_write_byte(client, DS1621_COM_START);
+	i2c_smbus_send_byte(client, DS1621_COM_START);
 }
 
 static ssize_t show_temp(struct device *dev, struct device_attribute *da,
--- g26.orig/drivers/hwmon/lm90.c	2008-03-28 11:08:29.000000000 -0700
+++ g26/drivers/hwmon/lm90.c	2008-05-10 00:07:50.000000000 -0700
@@ -463,7 +463,7 @@ static int lm90_read_reg(struct i2c_clie
  	if (client->flags & I2C_CLIENT_PEC) {
  		err = adm1032_write_byte(client, reg);
  		if (err >= 0)
- 			err = i2c_smbus_read_byte(client);
+			err = i2c_smbus_receive_byte(client);
  	} else
  		err = i2c_smbus_read_byte_data(client, reg);
 
--- g26.orig/drivers/i2c/chips/eeprom.c	2008-03-28 11:08:29.000000000 -0700
+++ g26/drivers/i2c/chips/eeprom.c	2008-05-10 00:07:50.000000000 -0700
@@ -93,12 +93,12 @@ static void eeprom_update_client(struct 
 							!= 32)
 					goto exit;
 		} else {
-			if (i2c_smbus_write_byte(client, slice << 5)) {
+			if (i2c_smbus_send_byte(client, slice << 5)) {
 				dev_dbg(&client->dev, "eeprom read start has failed!\n");
 				goto exit;
 			}
 			for (i = slice << 5; i < (slice + 1) << 5; i++) {
-				j = i2c_smbus_read_byte(client);
+				j = i2c_smbus_receive_byte(client);
 				if (j < 0)
 					goto exit;
 				data->data[i] = (u8) j;
@@ -208,9 +208,9 @@ static int eeprom_detect(struct i2c_adap
 		char name[4];
 
 		name[0] = i2c_smbus_read_byte_data(new_client, 0x80);
-		name[1] = i2c_smbus_read_byte(new_client);
-		name[2] = i2c_smbus_read_byte(new_client);
-		name[3] = i2c_smbus_read_byte(new_client);
+		name[1] = i2c_smbus_receive_byte(new_client);
+		name[2] = i2c_smbus_receive_byte(new_client);
+		name[3] = i2c_smbus_receive_byte(new_client);
 
 		if (!memcmp(name, "PCG-", 4) || !memcmp(name, "VGN-", 4)) {
 			dev_info(&new_client->dev, "Vaio EEPROM detected, "
--- g26.orig/drivers/i2c/chips/max6875.c	2008-03-28 10:42:32.000000000 -0700
+++ g26/drivers/i2c/chips/max6875.c	2008-05-10 00:07:50.000000000 -0700
@@ -112,7 +112,7 @@ static void max6875_update_slice(struct 
 			}
 		} else {
 			for (i = 0; i < SLICE_SIZE; i++) {
-				j = i2c_smbus_read_byte(client);
+				j = i2c_smbus_receive_byte(client);
 				if (j < 0) {
 					goto exit_up;
 				}
--- g26.orig/drivers/i2c/chips/pcf8574.c	2008-03-28 11:08:29.000000000 -0700
+++ g26/drivers/i2c/chips/pcf8574.c	2008-05-10 00:07:50.000000000 -0700
@@ -75,7 +75,7 @@ static struct i2c_driver pcf8574_driver 
 static ssize_t show_read(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	return sprintf(buf, "%u\n", i2c_smbus_read_byte(client));
+	return sprintf(buf, "%u\n", i2c_smbus_receive_byte(client));
 }
 
 static DEVICE_ATTR(read, S_IRUGO, show_read, NULL);
@@ -101,7 +101,7 @@ static ssize_t set_write(struct device *
 		return -EINVAL;
 
 	data->write = val;
-	i2c_smbus_write_byte(client, data->write);
+	i2c_smbus_send_byte(client, data->write);
 	return count;
 }
 
--- g26.orig/drivers/i2c/chips/pcf8591.c	2008-03-28 11:08:29.000000000 -0700
+++ g26/drivers/i2c/chips/pcf8591.c	2008-05-10 00:07:50.000000000 -0700
@@ -149,7 +149,7 @@ static ssize_t set_out0_enable(struct de
 		data->control |= PCF8591_CONTROL_AOEF;
 	else
 		data->control &= ~PCF8591_CONTROL_AOEF;
-	i2c_smbus_write_byte(client, data->control);
+	i2c_smbus_send_byte(client, data->control);
 	mutex_unlock(&data->update_lock);
 	return count;
 }
@@ -288,7 +288,7 @@ static void pcf8591_init_client(struct i
 	
 	/* The first byte transmitted contains the conversion code of the 
 	   previous read cycle. FLUSH IT! */
-	i2c_smbus_read_byte(client);
+	i2c_smbus_receive_byte(client);
 }
 
 static int pcf8591_read_channel(struct device *dev, int channel)
@@ -302,13 +302,13 @@ static int pcf8591_read_channel(struct d
 	if ((data->control & PCF8591_CONTROL_AICH_MASK) != channel) {
 		data->control = (data->control & ~PCF8591_CONTROL_AICH_MASK)
 			      | channel;
-		i2c_smbus_write_byte(client, data->control);
+		i2c_smbus_send_byte(client, data->control);
 	
 		/* The first byte transmitted contains the conversion code of 
 		   the previous read cycle. FLUSH IT! */
-		i2c_smbus_read_byte(client);
+		i2c_smbus_receive_byte(client);
 	}
-	value = i2c_smbus_read_byte(client);
+	value = i2c_smbus_receive_byte(client);
 
 	mutex_unlock(&data->update_lock);
 
--- g26.orig/drivers/i2c/chips/tsl2550.c	2008-04-29 21:51:56.000000000 -0700
+++ g26/drivers/i2c/chips/tsl2550.c	2008-05-10 00:07:50.000000000 -0700
@@ -68,7 +68,7 @@ static int tsl2550_set_operating_mode(st
 {
 	struct tsl2550_data *data = i2c_get_clientdata(client);
 
-	int ret = i2c_smbus_write_byte(client, TSL2550_MODE_RANGE[mode]);
+	int ret = i2c_smbus_send_byte(client, TSL2550_MODE_RANGE[mode]);
 
 	data->operating_mode = mode;
 
@@ -81,9 +81,9 @@ static int tsl2550_set_power_state(struc
 	int ret;
 
 	if (state == 0)
-		ret = i2c_smbus_write_byte(client, TSL2550_POWER_DOWN);
+		ret = i2c_smbus_send_byte(client, TSL2550_POWER_DOWN);
 	else {
-		ret = i2c_smbus_write_byte(client, TSL2550_POWER_UP);
+		ret = i2c_smbus_send_byte(client, TSL2550_POWER_UP);
 
 		/* On power up we should reset operating mode also... */
 		tsl2550_set_operating_mode(client, data->operating_mode);
@@ -107,14 +107,14 @@ static int tsl2550_get_adc_value(struct 
 	 */
 	end = jiffies + msecs_to_jiffies(400);
 	while (time_before(jiffies, end)) {
-		i2c_smbus_write_byte(client, cmd);
+		i2c_smbus_send_byte(client, cmd);
 
 		if (loop++ < 5)
 			mdelay(1);
 		else
 			msleep(1);
 
-		ret = i2c_smbus_read_byte(client);
+		ret = i2c_smbus_receive_byte(client);
 		if (ret < 0)
 			return ret;
 		else if (ret & 0x0080)
@@ -342,16 +342,16 @@ static int tsl2550_init_client(struct i2
 	 * Probe the chip. To do so we try to power up the device and then to
 	 * read back the 0x03 code
 	 */
-	err = i2c_smbus_write_byte(client, TSL2550_POWER_UP);
+	err = i2c_smbus_send_byte(client, TSL2550_POWER_UP);
 	if (err < 0)
 		return err;
 	mdelay(1);
-	if (i2c_smbus_read_byte(client) != TSL2550_POWER_UP)
+	if (i2c_smbus_receive_byte(client) != TSL2550_POWER_UP)
 		return -ENODEV;
 	data->power_state = 1;
 
 	/* Set the default operating mode */
-	err = i2c_smbus_write_byte(client,
+	err = i2c_smbus_send_byte(client,
 				   TSL2550_MODE_RANGE[data->operating_mode]);
 	if (err < 0)
 		return err;
--- g26.orig/drivers/media/video/saa7110.c	2008-04-24 13:45:59.000000000 -0700
+++ g26/drivers/media/video/saa7110.c	2008-05-10 00:07:51.000000000 -0700
@@ -127,7 +127,7 @@ saa7110_write_block (struct i2c_client *
 static inline int
 saa7110_read (struct i2c_client *client)
 {
-	return i2c_smbus_read_byte(client);
+	return i2c_smbus_receive_byte(client);
 }
 
 /* ----------------------------------------------------------------------- */
--- g26.orig/drivers/media/video/saa7185.c	2008-04-24 13:45:59.000000000 -0700
+++ g26/drivers/media/video/saa7185.c	2008-05-10 00:07:51.000000000 -0700
@@ -82,7 +82,7 @@ struct saa7185 {
 static inline int
 saa7185_read (struct i2c_client *client)
 {
-	return i2c_smbus_read_byte(client);
+	return i2c_smbus_receive_byte(client);
 }
 
 static int
--- g26.orig/drivers/media/video/tda9840.c	2008-04-24 13:45:59.000000000 -0700
+++ g26/drivers/media/video/tda9840.c	2008-05-10 00:07:51.000000000 -0700
@@ -74,7 +74,7 @@ static int command(struct i2c_client *cl
 
 		result = i2c_smbus_write_byte_data(client, SWITCH, byte);
 		if (result)
-			dprintk("i2c_smbus_write_byte() failed, ret:%d\n", result);
+			dprintk("i2c_smbus_write_byte_data() failed, ret:%d\n", result);
 		break;
 
 	case TDA9840_LEVEL_ADJUST:
@@ -94,7 +94,7 @@ static int command(struct i2c_client *cl
 
 		result = i2c_smbus_write_byte_data(client, LEVEL_ADJUST, byte);
 		if (result)
-			dprintk("i2c_smbus_write_byte() failed, ret:%d\n", result);
+			dprintk("i2c_smbus_write_byte_data() failed, ret:%d\n", result);
 		break;
 
 	case TDA9840_STEREO_ADJUST:
@@ -114,7 +114,7 @@ static int command(struct i2c_client *cl
 
 		result = i2c_smbus_write_byte_data(client, STEREO_ADJUST, byte);
 		if (result)
-			dprintk("i2c_smbus_write_byte() failed, ret:%d\n", result);
+			dprintk("i2c_smbus_write_byte_data() failed, ret:%d\n", result);
 		break;
 
 	case TDA9840_DETECT: {
@@ -144,7 +144,7 @@ static int command(struct i2c_client *cl
 
 		result = i2c_smbus_write_byte_data(client, TEST, byte);
 		if (result)
-			dprintk("i2c_smbus_write_byte() failed, ret:%d\n", result);
+			dprintk("i2c_smbus_write_byte_data() failed, ret:%d\n", result);
 		break;
 	default:
 		return -ENOIOCTLCMD;
--- g26.orig/drivers/media/video/tea6415c.c	2008-04-24 13:45:59.000000000 -0700
+++ g26/drivers/media/video/tea6415c.c	2008-05-10 00:07:51.000000000 -0700
@@ -165,9 +165,9 @@ static int switch_matrix(struct i2c_clie
 		break;
 	};
 
-	ret = i2c_smbus_write_byte(client, byte);
+	ret = i2c_smbus_send_byte(client, byte);
 	if (ret) {
-		dprintk("i2c_smbus_write_byte() failed, ret:%d\n", ret);
+		dprintk("i2c_smbus_send_byte() failed, ret:%d\n", ret);
 		return -EIO;
 	}
 
--- g26.orig/drivers/media/video/tea6420.c	2008-04-24 13:45:59.000000000 -0700
+++ g26/drivers/media/video/tea6420.c	2008-05-10 00:07:51.000000000 -0700
@@ -79,9 +79,9 @@ static int tea6420_switch(struct i2c_cli
 		break;
 	}
 
-	ret = i2c_smbus_write_byte(client, byte);
+	ret = i2c_smbus_send_byte(client, byte);
 	if (ret) {
-		dprintk("i2c_smbus_write_byte() failed, ret:%d\n", ret);
+		dprintk("i2c_smbus_send_byte() failed, ret:%d\n", ret);
 		return -EIO;
 	}
 
--- g26.orig/drivers/w1/masters/ds2482.c	2008-03-28 10:42:44.000000000 -0700
+++ g26/drivers/w1/masters/ds2482.c	2008-05-10 00:07:51.000000000 -0700
@@ -167,7 +167,7 @@ static inline int ds2482_select_register
  */
 static inline int ds2482_send_cmd(struct ds2482_data *pdev, u8 cmd)
 {
-	if (i2c_smbus_write_byte(&pdev->client, cmd) < 0)
+	if (i2c_smbus_send_byte(&pdev->client, cmd) < 0)
 		return -1;
 
 	pdev->read_prt = DS2482_PTR_CODE_STATUS;
@@ -216,7 +216,7 @@ static int ds2482_wait_1wire_idle(struct
 
 	if (!ds2482_select_register(pdev, DS2482_PTR_CODE_STATUS)) {
 		do {
-			temp = i2c_smbus_read_byte(&pdev->client);
+			temp = i2c_smbus_receive_byte(&pdev->client);
 		} while ((temp >= 0) && (temp & DS2482_REG_STS_1WB) &&
 			 (++retries < DS2482_WAIT_IDLE_TIMEOUT));
 	}
@@ -244,7 +244,7 @@ static int ds2482_set_channel(struct ds2
 
 	pdev->read_prt = DS2482_PTR_CODE_CHANNEL;
 	pdev->channel = -1;
-	if (i2c_smbus_read_byte(&pdev->client) == ds2482_chan_rd[channel]) {
+	if (i2c_smbus_receive_byte(&pdev->client) == ds2482_chan_rd[channel]) {
 		pdev->channel = channel;
 		return 0;
 	}
@@ -368,7 +368,7 @@ static u8 ds2482_w1_read_byte(void *data
 	ds2482_select_register(pdev, DS2482_PTR_CODE_DATA);
 
 	/* Read the data byte */
-	result = i2c_smbus_read_byte(&pdev->client);
+	result = i2c_smbus_receive_byte(&pdev->client);
 
 	mutex_unlock(&pdev->access_lock);
 
@@ -463,7 +463,7 @@ static int ds2482_detect(struct i2c_adap
 	ndelay(525);
 
 	/* Read the status byte - only reset bit and line should be set */
-	temp1 = i2c_smbus_read_byte(new_client);
+	temp1 = i2c_smbus_receive_byte(new_client);
 	if (temp1 != (DS2482_REG_STS_LL | DS2482_REG_STS_RST)) {
 		dev_dbg(&adapter->dev, "DS2482 (0x%02x) reset status "
 			"0x%02X - not a DS2482\n", address, temp1);

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

[Maciej W. Rozycki]

Hi Jean,

> 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.

 Certainly.

> 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.

 Nothing to understand here and I do not insist on anything.  You are
right of course -- I must have got too much influenced by i2c_transfer()  
and have not thought of splitting.

> 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

 Well, as I understand the support for I2C_FUNC_SMBUS_I2C_BLOCK
(read/write, as appropriate) implies I2C_FUNC_SMBUS_BYTE_DATA as the
latter is a special case of the former, where the length of the transfer
equals one.  But I agree -- in the light of what you wrote previously a
bus adapter that supports say I2C_FUNC_SMBUS_READ_I2C_BLOCK is meant to
have I2C_FUNC_SMBUS_READ_BYTE set as well, so no need to check for it
here.

 If we agree on this one, I will retest and submit the whole batch again,
updated as needed.

  Maciej

patch-2.6.26-rc1-20080505-m41t80-smbus-16
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-11 00:16:36.000000000 +0000
@@ -6,6 +6,7 @@
  * Based on m41t00.c by Mark A. Greer <mgreer@mvista.com>
  *
  * 2006 (c) mycable GmbH
+ * Copyright (c) 2008  Maciej W. Rozycki
  *
  * 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
@@ -36,6 +37,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 +61,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 +81,83 @@ struct m41t80_data {
 	struct rtc_device *rtc;
 };
 
-static int m41t80_get_datetime(struct i2c_client *client,
-			       struct rtc_time *tm)
+
+static int m41t80_write_block_data(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,
-		},
-		{
-			.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 (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;
+			}
+		}
 	}
+out:
+	return i;
+}
+
+static int m41t80_read_block_data(struct i2c_client *client,
+				  u8 reg, u8 num, u8 *buf)
+{
+	int i, rc;
+
+	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;
+}
+
+static int m41t80_get_datetime(struct i2c_client *client, struct rtc_time *tm)
+{
+	u8 buf[M41T80_DATETIME_REG_SIZE];
+	int loops = 2;
+	int sec0, sec1;
+
+	/*
+	 * Time registers are latched by this chip if an I2C block
+	 * transfer is used, but with SMBus-style byte accesses
+	 * this is not the case, so check seconds for a wraparound.
+	 */
+	do {
+		if (m41t80_read_block_data(client, M41T80_REG_SEC,
+					   M41T80_DATETIME_REG_SIZE -
+					   M41T80_REG_SEC,
+					   buf + M41T80_REG_SEC) < 0) {
+			dev_err(&client->dev, "read error\n");
+			return -EIO;
+		}
+		sec0 = buf[M41T80_REG_SEC];
+
+		sec1 = i2c_smbus_read_byte_data(client, M41T80_REG_SEC);
+		if (sec1 < 0) {
+			dev_err(&client->dev, "read error\n");
+			return -EIO;
+		}
 
-	tm->tm_sec = BCD2BIN(buf[M41T80_REG_SEC] & 0x7f);
+		sec0 = BCD2BIN(sec0 & 0x7f);
+		sec1 = BCD2BIN(sec1 & 0x7f);
+	} while (sec1 < sec0 && --loops);
+
+	tm->tm_sec = sec1;
 	tm->tm_min = BCD2BIN(buf[M41T80_REG_MIN] & 0x7f);
 	tm->tm_hour = BCD2BIN(buf[M41T80_REG_HOUR] & 0x3f);
 	tm->tm_mday = BCD2BIN(buf[M41T80_REG_DAY] & 0x3f);
@@ -117,39 +172,16 @@ static int m41t80_get_datetime(struct i2
 /* Sets the given date and time to the real time clock. */
 static int m41t80_set_datetime(struct i2c_client *client, struct rtc_time *tm)
 {
-	u8 wbuf[1 + M41T80_DATETIME_REG_SIZE];
-	u8 *buf = &wbuf[1];
-	u8 dt_addr[1] = { M41T80_REG_SEC };
-	struct i2c_msg msgs_in[] = {
-		{
-			.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,
-		},
-	};
-	struct i2c_msg msgs[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1 + M41T80_DATETIME_REG_SIZE,
-			.buf	= wbuf,
-		 },
-	};
+	u8 buf[M41T80_DATETIME_REG_SIZE];
 
 	/* Read current reg values into buf[1..7] */
-	if (i2c_transfer(client->adapter, msgs_in, 2) < 0) {
+	if (m41t80_read_block_data(client, M41T80_REG_SEC,
+				   M41T80_DATETIME_REG_SIZE - M41T80_REG_SEC,
+				   buf + M41T80_REG_SEC) < 0) {
 		dev_err(&client->dev, "read error\n");
 		return -EIO;
 	}
 
-	wbuf[0] = 0; /* offset into rtc's regs */
 	/* Merge time-data and register flags into buf[0..7] */
 	buf[M41T80_REG_SSEC] = 0;
 	buf[M41T80_REG_SEC] =
@@ -167,7 +199,8 @@ static int m41t80_set_datetime(struct i2
 	/* assume 20YY not 19YY */
 	buf[M41T80_REG_YEAR] = BIN2BCD(tm->tm_year % 100);
 
-	if (i2c_transfer(client->adapter, msgs, 1) != 1) {
+	if (m41t80_write_block_data(client, M41T80_REG_SSEC,
+				    M41T80_DATETIME_REG_SIZE, buf) < 0) {
 		dev_err(&client->dev, "write error\n");
 		return -EIO;
 	}
@@ -241,34 +274,11 @@ err:
 static int m41t80_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	u8 wbuf[1 + M41T80_ALARM_REG_SIZE];
-	u8 *buf = &wbuf[1];
+	u8 buf[M41T80_ALARM_REG_SIZE];
 	u8 *reg = buf - M41T80_REG_ALARM_MON;
-	u8 dt_addr[1] = { M41T80_REG_ALARM_MON };
-	struct i2c_msg msgs_in[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1,
-			.buf	= dt_addr,
-		},
-		{
-			.addr	= client->addr,
-			.flags	= I2C_M_RD,
-			.len	= M41T80_ALARM_REG_SIZE,
-			.buf	= buf,
-		},
-	};
-	struct i2c_msg msgs[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1 + M41T80_ALARM_REG_SIZE,
-			.buf	= wbuf,
-		 },
-	};
 
-	if (i2c_transfer(client->adapter, msgs_in, 2) < 0) {
+	if (m41t80_read_block_data(client, M41T80_REG_ALARM_MON,
+				   M41T80_ALARM_REG_SIZE, buf) < 0) {
 		dev_err(&client->dev, "read error\n");
 		return -EIO;
 	}
@@ -278,7 +288,6 @@ static int m41t80_rtc_set_alarm(struct d
 	reg[M41T80_REG_ALARM_MIN] = 0;
 	reg[M41T80_REG_ALARM_SEC] = 0;
 
-	wbuf[0] = M41T80_REG_ALARM_MON; /* offset into rtc's regs */
 	reg[M41T80_REG_ALARM_SEC] |= t->time.tm_sec >= 0 ?
 		BIN2BCD(t->time.tm_sec) : 0x80;
 	reg[M41T80_REG_ALARM_MIN] |= t->time.tm_min >= 0 ?
@@ -292,7 +301,8 @@ static int m41t80_rtc_set_alarm(struct d
 	else
 		reg[M41T80_REG_ALARM_DAY] |= 0x40;
 
-	if (i2c_transfer(client->adapter, msgs, 1) != 1) {
+	if (m41t80_write_block_data(client, M41T80_REG_ALARM_MON,
+				    M41T80_ALARM_REG_SIZE, buf) < 0) {
 		dev_err(&client->dev, "write error\n");
 		return -EIO;
 	}
@@ -312,24 +322,10 @@ static int m41t80_rtc_read_alarm(struct 
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	u8 buf[M41T80_ALARM_REG_SIZE + 1]; /* all alarm regs and flags */
-	u8 dt_addr[1] = { M41T80_REG_ALARM_MON };
 	u8 *reg = buf - M41T80_REG_ALARM_MON;
-	struct i2c_msg msgs[] = {
-		{
-			.addr	= client->addr,
-			.flags	= 0,
-			.len	= 1,
-			.buf	= dt_addr,
-		},
-		{
-			.addr	= client->addr,
-			.flags	= I2C_M_RD,
-			.len	= M41T80_ALARM_REG_SIZE + 1,
-			.buf	= buf,
-		},
-	};
 
-	if (i2c_transfer(client->adapter, msgs, 2) < 0) {
+	if (m41t80_read_block_data(client, M41T80_REG_ALARM_MON,
+				   M41T80_ALARM_REG_SIZE + 1, buf) < 0) {
 		dev_err(&client->dev, "read error\n");
 		return -EIO;
 	}
@@ -488,26 +484,16 @@ static int boot_flag;
  */
 static void wdt_ping(void)
 {
-	unsigned char i2c_data[2];
-	struct i2c_msg msgs1[1] = {
-		{
-			.addr	= save_client->addr,
-			.flags	= 0,
-			.len	= 2,
-			.buf	= i2c_data,
-		},
-	};
-	i2c_data[0] = 0x09;		/* watchdog register */
+	u8 wdt = 0x80;				/* WDS = 1 (0x80)  */
 
 	if (wdt_margin > 31)
-		i2c_data[1] = (wdt_margin & 0xFC) | 0x83; /* resolution = 4s */
+		/* mulitplier = WD_TIMO / 4, resolution = 4s (0x3)  */
+		wdt |= (wdt_margin & 0xfc) | 0x3;
 	else
-		/*
-		 * WDS = 1 (0x80), mulitplier = WD_TIMO, resolution = 1s (0x02)
-		 */
-		i2c_data[1] = wdt_margin<<2 | 0x82;
+		/* mulitplier = WD_TIMO, resolution = 1s (0x2)  */
+		wdt |= wdt_margin << 2 | 0x2;
 
-	i2c_transfer(save_client->adapter, msgs1, 1);
+	i2c_smbus_write_byte_data(save_client, M41T80_REG_WATCHDOG, wdt);
 }
 
 /**
@@ -517,36 +503,8 @@ static void wdt_ping(void)
  */
 static void wdt_disable(void)
 {
-	unsigned char i2c_data[2], i2c_buf[0x10];
-	struct i2c_msg msgs0[2] = {
-		{
-			.addr	= save_client->addr,
-			.flags	= 0,
-			.len	= 1,
-			.buf	= i2c_data,
-		},
-		{
-			.addr	= save_client->addr,
-			.flags	= I2C_M_RD,
-			.len	= 1,
-			.buf	= i2c_buf,
-		},
-	};
-	struct i2c_msg msgs1[1] = {
-		{
-			.addr	= save_client->addr,
-			.flags	= 0,
-			.len	= 2,
-			.buf	= i2c_data,
-		},
-	};
-
-	i2c_data[0] = 0x09;
-	i2c_transfer(save_client->adapter, msgs0, 2);
-
-	i2c_data[0] = 0x09;
-	i2c_data[1] = 0x00;
-	i2c_transfer(save_client->adapter, msgs1, 1);
+	i2c_smbus_read_byte_data(save_client, M41T80_REG_WATCHDOG);
+	i2c_smbus_write_byte_data(save_client, M41T80_REG_WATCHDOG, 0);
 }
 
 /**
@@ -736,13 +694,21 @@ static int m41t80_probe(struct i2c_clien
 	struct rtc_device *rtc = NULL;
 	struct rtc_time tm;
 	struct m41t80_data *clientdata = NULL;
+	int reg;
 
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C
-				     | I2C_FUNC_SMBUS_BYTE_DATA)) {
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_BYTE_DATA)) {
 		rc = -ENODEV;
 		goto exit;
 	}
 
+	/* 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");
 
@@ -765,11 +731,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");
@@ -782,18 +744,18 @@ static int m41t80_probe(struct i2c_clien
 		}
 		if (i2c_smbus_write_byte_data(client,
 					      M41T80_REG_ALARM_HOUR,
-					      rc & ~M41T80_ALHOUR_HT) < 0)
+					      reg & ~M41T80_ALHOUR_HT) < 0)
 			goto ht_err;
 	}
 
 	/* 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;
 	}
 

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

[Jean Delvare]

Hi Maciej,

On Sun, 11 May 2008 02:59:34 +0100 (BST), Maciej W. Rozycki wrote:
> > 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
> 
>  Well, as I understand the support for I2C_FUNC_SMBUS_I2C_BLOCK
> (read/write, as appropriate) implies I2C_FUNC_SMBUS_BYTE_DATA as the
> latter is a special case of the former, where the length of the transfer
> equals one.

In theory you are right, yes. But as I wrote before, functionality are
expressed in a boolean way, so adapters can't express their limitations
if there are any. Think of an adapter which could only transfer blocks
of even size, it would most certainly declare itself
I2C_FUNC_SMBUS_I2C_BLOCK capable (even though it can't do all of it)
but wouldn't declare I2C_FUNC_SMBUS_BYTE_DATA as it can't do it. This
is just an example of course, in practice I just can't remember of any
I2C or SMBus adapter not implementing I2C_FUNC_SMBUS_BYTE_DATA.

The bottom line is that you should never assume that support for a
given transaction type implies support for another transaction type.

>              But I agree -- in the light of what you wrote previously a
> bus adapter that supports say I2C_FUNC_SMBUS_READ_I2C_BLOCK is meant to
> have I2C_FUNC_SMBUS_READ_BYTE set as well, so no need to check for it
> here.
> 
>  If we agree on this one, I will retest and submit the whole batch again,
> updated as needed.

Yes, you code looks correct to me now, i2c-wise.

-- 
Jean Delvare

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

[Atsushi Nemoto]

On Sun, 11 May 2008 02:59:34 +0100 (BST), "Maciej W. Rozycki" <macro@linux-mips.org> wrote:
>  If we agree on this one, I will retest and submit the whole batch again,
> updated as needed.

Works OK for me (m41t80 + i2c-gpio).

Tested-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

One minor style comment.

> +	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;
> +			}
> +		}
>  	}
> +out:
> +	return i;

This part can be a bit shorter.

	if (i2c_check_functionality(client->adapter,
				    I2C_FUNC_SMBUS_WRITE_I2C_BLOCK))
		return i2c_smbus_write_i2c_block_data(client, reg, num, buf);
	for (i = 0; i < num; i++) {
		rc = i2c_smbus_write_byte_data(client, reg + i, buf[i]);
		if (rc < 0)
			return rc;
	}
	return i;

Saves 6 lines.  Not a big issue.

---
Atsushi Nemoto

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

[Jean Delvare]

Hi David,

On Sat, 10 May 2008 09:36:50 -0700, David Brownell wrote:
> > It's not that easy. There are some drivers which are both in-tree and
> > out-of-tree, for which such a change means adding ifdefs.
> 
> Actually, I thought it *WAS* that easy.  See the appended patch,
> which goes on top of the various doc and (mostly SMBus) fault
> handling patches I've sent ... the old names are still available
> (only needed by out-of-tree drivers), but marked as __deprecated.
> So:  no #ifdefs required.

No #ifdefs required until we drop the deprecated helpers, which will
inevitably happen, even if only in several years. So in practice,
#ifdefs would be required for out-of-tree drivers (or semi-out-of-tree
drivers such as v4l/dvb drivers).

> I agree it's not worth while for *all* the SMBus functions that
> use names-made-up-for-Linux.  But for the two which are really
> badly misnamed (after *different* SMBus operations), I suggest
> fixing would be a reasonable thing.

That's tempting, but see below.

> > And there is 
> > i2c-dev.h (the user-space one) which has similar functions,
> 
> That's problematic in its own right though.  Not only that
> the *kernel* file Documentation/i2c/dev-interface referring
> to those functions, which are unavailable from the header
> provided by the kernel.  Or that there's no relationship
> between the kernel and userspace files of the same name.
> But also that those functions are actually a bit too large
> to be appropriate as inlines, even once you manage to track
> it down (as part of a "tools" package, not a "library").

I agree that the i2c-dev.h mess needs some love and this is on my to-do
list for this year. That's not necessarily related to the problem at
hand though.

> > if we 
> > rename only the kernel variants, there will be some confusion. But if
> > we rename also the user-space variants, then it's up to 2.4 kernel
> > users to have different names for kernel-space and user-space functions.
> 
> True, but that would be a question for a "libsmbus" or somesuch
> to deal with.  Not a kernel issue.

Whether it's in the kernel or outside the kernel is irrelevant. The
developers are likely to be the same, and the person who will have to
deal with the confusion is the same (i.e.: me.)

> ===========
> Two of the SMBus operations are using confusingly inappropriate names:
> 
>   i2c_smbus_read_byte() does not execute the SMBus "Read Byte" protocol
>   	... it implements the SMBus "Receive Byte" protocol instead!!
> 
>   i2c_smbus_write_byte() does not execute the SMBus "Write Byte" protocol
>   	... it implements the SMBus "Send Byte" protocol instead!!
> 
> This patch changes the names of those functions, so they no longer
> use names of different operations (which they do not implement).
> 
> ---
>  Documentation/i2c/chips/max6875   |    4 ++--
>  Documentation/i2c/smbus-protocol  |   12 +++++-------
>  Documentation/i2c/writing-clients |    4 ++--
>  drivers/gpio/pcf857x.c            |    8 ++++----
>  drivers/hwmon/ds1621.c            |    2 +-
>  drivers/hwmon/lm90.c              |    2 +-
>  drivers/i2c/chips/eeprom.c        |   10 +++++-----
>  drivers/i2c/chips/max6875.c       |    2 +-
>  drivers/i2c/chips/pcf8574.c       |    4 ++--
>  drivers/i2c/chips/pcf8591.c       |   10 +++++-----
>  drivers/i2c/chips/tsl2550.c       |   16 ++++++++--------
>  drivers/i2c/i2c-core.c            |   12 ++++++------
>  drivers/media/video/saa7110.c     |    2 +-
>  drivers/media/video/saa7185.c     |    2 +-
>  drivers/media/video/tda9840.c     |    8 ++++----
>  drivers/media/video/tea6415c.c    |    4 ++--
>  drivers/media/video/tea6420.c     |    4 ++--
>  drivers/w1/masters/ds2482.c       |   10 +++++-----
>  include/linux/i2c.h               |   20 ++++++++++++++++++--
>  19 files changed, 75 insertions(+), 61 deletions(-)

I'm not going to take this, sorry. I still believe that it will cause
more trouble than is worth at this point in time. For example, after
applying your patch, we'd have functionality defines no longer matching
the SMBus helper names (I2C_FUNC_SMBUS_READ_BYTE,
I2C_FUNC_SMBUS_WRITE_BYTE). And you can't easily rename these because
they are part of the i2c-dev API.

I'm not saying that the suggested cleanup shouldn't be done, nor that
it cannot be done. All I'm saying is that it will be a lot of work if
we do it properly, and potentially a lot of trouble for many developers
out there, all for a marginal benefit, and I think that our time can be
better used to solve other, more important problems. And you've done a
very good job at documenting the current implementation, so I hope that
it is clear enough for everyone now.

Thanks,
-- 
Jean Delvare