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

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

[David Brownell]

This patch is way too big for what it does.  Lots of whitespace
and similar noise (did those headers *need* rearranging?).


But most importantly, that driver already uses SMBus calls.
So you shouldn't *stop* it from using them by adding wrapper
code like this:

> +static int m41t80_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
> +{ 
> +	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return m41t80_i2c_transfer(client, 1, reg, 1, &val);
> +	else
> +		return i2c_smbus_write_byte_data(client, reg, val);
> +}

That's because if an I2C controller can execute that protocol,
it can do so through the SMBus calls too.  And the current code
already verifies that will work ... those wrappers are pointless,
as well as untested.


The chip docs basically tell us that if the underlying SMBus
controller can issue i2c_smbus_read_i2c_block_data() and its
write-side sibling, it can *easily* talk to this chip.

So wouldn't it be a better idea to just replace the existing
calls to i2c_transfer() with those pseudo-SMBus block calls?

And then *separately* add workarounds for i2c-sibyte, and
similar controllers that don't offer block transfers?  Maybe
using your existing structure ... just not replacing those
existing perfectly-fine SMBus calls.

- Dave



_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Maciej W. Rozycki]

On Wed, 7 May 2008, David Brownell wrote:

> This patch is way too big for what it does.  Lots of whitespace
> and similar noise (did those headers *need* rearranging?).

 I can submit the formatting fix for the register offsets separately, no 
problem.  Similarly about the headers -- it is just that kind of the 
changes that hardly deserve a separate patch, but if changes are made in 
the area, it may well be a good time to tidy up.

 Personally I prefer to keep header inclusions sorted as it makes it
easier to immediately see what is included and what is not, but if you 
prefer them in a random order, then my preference is not strong enough for 
me to resist.

> But most importantly, that driver already uses SMBus calls.
> So you shouldn't *stop* it from using them by adding wrapper
> code like this:

 Well, I have to admit what I know in detail about I2C and how it is 
handled by Linux is what I gathered over the last weekend.  The idea of 
the change, given a change had to be done, was to make the driver a bit 
more flexible.  Of all the I2C RTC drivers this seems to be one of the two 
only that require *both* I2C *and* SMBus functionality in the driving 
controller at the same time.  With this change in place only either of the 
two is needed.  Please correct me if I am wrong.

> > +static int m41t80_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
> > +{ 
> > +	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > +		return m41t80_i2c_transfer(client, 1, reg, 1, &val);
> > +	else
> > +		return i2c_smbus_write_byte_data(client, reg, val);
> > +}
> 
> That's because if an I2C controller can execute that protocol,
> it can do so through the SMBus calls too.  And the current code
> already verifies that will work ... those wrappers are pointless,
> as well as untested.

 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.

> The chip docs basically tell us that if the underlying SMBus
> controller can issue i2c_smbus_read_i2c_block_data() and its
> write-side sibling, it can *easily* talk to this chip.

 Well, I have obviously missed the existence of the SMBus block transfer
and you have correctly inferred this means the controller used here does
not support them.  The SOC implements a "subset of a standard SMBus
controller with some extensions" -- the subset being send/receive byte,
write/read byte and write/read word.  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).

> So wouldn't it be a better idea to just replace the existing
> calls to i2c_transfer() with those pseudo-SMBus block calls?

 I can do that, no problem.  It looks like a good way to eliminate code 
duplication here.

> And then *separately* add workarounds for i2c-sibyte, and
> similar controllers that don't offer block transfers?  Maybe
> using your existing structure ... just not replacing those
> existing perfectly-fine SMBus calls.

 Given the driver uses a mixture of SMBus calls and raw I2C calls, I would
assume the other controller currently used with the driver (that's the
D-Link DNS-323 platform -- I cannot immediately figure out which I2C
controller this one would use) does not support these SMBus block calls
either.

 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 -- given how the I2C bus is specified, a device is
free to interpret a series of byte calls differently to seemingly
equivalent block calls.  It may not support one or the other altogether as
well.  Even this simple RTC device differs slightly in this respect, by
latching or not the clock registers.  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.

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

 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.

 Thanks a lot for your review!

  Maciej

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Wednesday 07 May 2008, Maciej W. Rozycki wrote:
> On Wed, 7 May 2008, David Brownell wrote:
> 
> > But most importantly, that driver already uses SMBus calls.
> > So you shouldn't *stop* it from using them by adding wrapper
> > code like this:
> 
>  Well, I have to admit what I know in detail about I2C and how it is 
> handled by Linux is what I gathered over the last weekend.  The idea of 
> the change, given a change had to be done, was to make the driver a bit 
> more flexible.  Of all the I2C RTC drivers this seems to be one of the two 
> only that require *both* I2C *and* SMBus functionality in the driving 
> controller at the same time.  With this change in place only either of the 
> two is needed.  Please correct me if I am wrong.

It checks for "I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE_DATA" which is
admittedly odd.  Because if it can do I2C, it can assuredly do
that minor subset of SMBus.  (Modulo bugs in how drivers end up
reporting their capabilities.)


> > > +static int m41t80_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
> > > +{ 
> > > +	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > > +		return m41t80_i2c_transfer(client, 1, reg, 1, &val);
> > > +	else
> > > +		return i2c_smbus_write_byte_data(client, reg, val);
> > > +}
> > 
> > That's because if an I2C controller can execute that protocol,
> > it can do so through the SMBus calls too.  And the current code
> > already verifies that will work ... those wrappers are pointless,
> > as well as untested.
> 
>  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...)


> > The chip docs basically tell us that if the underlying SMBus
> > controller can issue i2c_smbus_read_i2c_block_data() and its
> > write-side sibling, it can *easily* talk to this chip.
> 
>  Well, I have obviously missed the existence of the SMBus block transfer
> and you have correctly inferred this means the controller used here does
> not support them.  The SOC implements a "subset of a standard SMBus
> controller with some extensions" -- the subset being send/receive byte,
> write/read byte and write/read word.

That's what I noticed in the current driver, yes.


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

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


> > So wouldn't it be a better idea to just replace the existing
> > calls to i2c_transfer() with those pseudo-SMBus block calls?
> 
>  I can do that, no problem.  It looks like a good way to eliminate code 
> duplication here.

And work on systems that don't support full I2C messaging, but
can support that minor SMBus extension.  (Which includes all
systems capable of full I2C, and a few others that this driver
currently won't support.)


> > And then *separately* add workarounds for i2c-sibyte, and
> > similar controllers that don't offer block transfers?  Maybe
> > using your existing structure ... just not replacing those
> > existing perfectly-fine SMBus calls.
> 
>  Given the driver uses a mixture of SMBus calls and raw I2C calls, I would
> assume the other controller currently used with the driver (that's the
> D-Link DNS-323 platform -- I cannot immediately figure out which I2C
> controller this one would use) does not support these SMBus block calls
> either.

If it can support the I2C version of those calls, it can support
the I2C version of those calls as made by the SMBus "emulation"
code.  Don't worry about that.


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


>     -- given how the I2C bus is specified, a device is 
> free to interpret a series of byte calls differently to seemingly
> equivalent block calls.  It may not support one or the other altogether as
> well.  Even this simple RTC device differs slightly in this respect, by
> latching or not the clock registers.

That's a different issue.  In that case you're switching to
different protocol operations, to cope with limitations of
the underlying controller.

Different protocol == different semantics.

So that's not the same as just using a different programming
interface to execute a given sequence of protocol messages.


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

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


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


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

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

- Dave




>  Thanks a lot for your review!
> 
>   Maciej
> 



_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

Hi David,

On Wed, 7 May 2008 16:25:20 -0700, David Brownell wrote:
> On Wednesday 07 May 2008, Maciej W. Rozycki wrote:
> >  Well, I have to admit what I know in detail about I2C and how it is 
> > handled by Linux is what I gathered over the last weekend.  The idea of 
> > the change, given a change had to be done, was to make the driver a bit 
> > more flexible.  Of all the I2C RTC drivers this seems to be one of the two 
> > only that require *both* I2C *and* SMBus functionality in the driving 
> > controller at the same time.  With this change in place only either of the 
> > two is needed.  Please correct me if I am wrong.
> 
> It checks for "I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE_DATA" which is
> admittedly odd.  Because if it can do I2C, it can assuredly do
> that minor subset of SMBus.  (Modulo bugs in how drivers end up
> reporting their capabilities.)

Not totally correct, see my reply somewhere else in this thread.

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

No, we don't have any function named that way. Not that I think that
these names would be appropriate, though. I'd rather have
i2c_smbus_read_i2c_block_emul() for example.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Thursday 08 May 2008, Jean Delvare wrote:
> > >  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.  ;)
> 
> No, we don't have any function named that way. Not that I think that
> these names would be appropriate, though. I'd rather have
> i2c_smbus_read_i2c_block_emul() for example.

I didn't pick up on the context switch Maciej stuck in, as a
not-quite-response to my comment about the functions that the
m41t80 driver could *portably* use:  the ones that have been
in the I2C core for some time now.  I thought he missed the
point that those operations were already present...

It's not at all clear what a "read i2c byte data" function
might do, for that matter.  I2C doesn't define semantics
even to the weak level SMBus does.

If I had noticed that some previously un-discussed primitive
had been introduced, I would have asked for a definition in
terms of protocol, and expected semantics; I can't guess.

- Dave


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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: [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: [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: [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: [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

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,

[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,

[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,

[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: [RFC][PATCH 4/4] RTC: SMBus support for the M41T80

[Jean Delvare]

Hi Maciej,

On Wed, 7 May 2008 23:28:25 +0100 (BST), Maciej W. Rozycki wrote:
> On Wed, 7 May 2008, David Brownell wrote:
> 
> > This patch is way too big for what it does.  Lots of whitespace
> > and similar noise (did those headers *need* rearranging?).
> 
>  I can submit the formatting fix for the register offsets separately, no 
> problem.  Similarly about the headers -- it is just that kind of the 
> changes that hardly deserve a separate patch, but if changes are made in 
> the area, it may well be a good time to tidy up.

If you think a change doesn't deserve a separate patch, I say it
doesn't deserve to be done at all. The golden rule is to have separate
patches for separate changes.

>  Personally I prefer to keep header inclusions sorted as it makes it
> easier to immediately see what is included and what is not, but if you 
> prefer them in a random order, then my preference is not strong enough for 
> me to resist.

The problem is that there is no such rule in Documentation/CodingStyle,
and in general developers don't care about sorting the header inclusions
alphabetically or otherwise, so it is unlikely that your sort will
survive the next changes that will happen in this area. So I join David
in saying that this change is just not needed.

> > But most importantly, that driver already uses SMBus calls.
> > So you shouldn't *stop* it from using them by adding wrapper
> > code like this:
> 
>  Well, I have to admit what I know in detail about I2C and how it is 
> handled by Linux is what I gathered over the last weekend.  The idea of 
> the change, given a change had to be done, was to make the driver a bit 
> more flexible.  Of all the I2C RTC drivers this seems to be one of the two 
> only that require *both* I2C *and* SMBus functionality in the driving 
> controller at the same time.  With this change in place only either of the 
> two is needed.  Please correct me if I am wrong.

You are correct, but it is important to understand that, for a Linux
chip driver, "requiring I2C and SMBus" and "requiring I2C" are almost
equivalent statements. So what you wrote above is almost equivalent to
writing:

"Of all the I2C RTC drivers this seems to be one of the two only that
require I2C functionality in the driving controller."

Hopefully, with this equivalence in mind you'll have an easier time
understanding what changes should be made to the driver to make it more
portable as you intended.

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

The driver rightly checks I2C_FUNC_SMBUS_BYTE_DATA because it calls
i2c_smbus_read_byte_data() and i2c_smbus_write_byte_data(). A driver
calling specific SMBus transaction functions should always check that
they are supported by the underlying bus driver first.

You are correct that, with what we said above, checking for
I2C_FUNC_I2C and checking for I2C_FUNC_I2C | I2C_FUNC_SMBUS_FOO
should be the same. However, this is only strictly equivalent if the
underlying bus driver supports I2C without any restriction. In practice
this is rarely the case, most I2C controllers have limitations. Many
don't support zero-byte messages, for example. Some don't support more
than 2 messages in a transaction, and some additionally have limits to
the maximum length of messages. We've even seen a controller lately
that would only support messages of 1, 8, 16 and 32 bytes.

For this reason, it is up to the bus driver to declare exactly what
part of SMBus they can emulate. Most of the time it's
I2C_FUNC_SMBUS_EMUL (which is a _subset_ of the SMBus transaction set
plus the I2C block transactions) but some drivers remove a few
functionality bits and some add a few ones. That's the reason why the
I2C chip drivers must still check for individual functionality bits if
they do direct calls to specific SMBus transaction functions.

> > The chip docs basically tell us that if the underlying SMBus
> > controller can issue i2c_smbus_read_i2c_block_data() and its
> > write-side sibling, it can *easily* talk to this chip.
> 
>  Well, I have obviously missed the existence of the SMBus block transfer

Important note: the chip supports _I2C_ block transactions, as David
properly wrote. These are different from _SMBus_ block transactions.

>  Given the driver uses a mixture of SMBus calls and raw I2C calls, I would
> assume the other controller currently used with the driver (that's the
> D-Link DNS-323 platform -- I cannot immediately figure out which I2C
> controller this one would use) does not support these SMBus block calls
> either.

Wrong assumption. All it means is that the original driver author took
the most direct path to get the driver working on _his_ hardware, which
apparently supported I2C. He left it to other developers (i.e. you) to
update the code if the driver was needed to work on top of less gifted
controllers.

>  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 -- given how the I2C bus is specified, a device is
> free to interpret a series of byte calls differently to seemingly
> equivalent block calls.

You are absolutely correct, and this is the reason why no such
emulation is done in i2c-core. However a number of chip drivers do that
emulation when they know that this is correct for their chip (e.g.
drivers/i2c/chips/eeprom.c). So you are free to do a similar emulation
in the rtc-m41t80 driver.

>                         It may not support one or the other altogether as
> well.  Even this simple RTC device differs slightly in this respect, by
> latching or not the clock registers.  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.
> 
>  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.  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?

This has been discussed recently on the lm-sensors list:
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-April/022908.html
(Search for "the smbus layer doesn't emulate it".)

The conclusion was that we may implement the emulation in i2c-core if
enough drivers needed it, and they agreed on the implementation. But
you shouldn't wait for it to happen, just do the emulation in your
driver for now. I'm really not sure if an i2c-core implementation will
ever happen, just look at the code in the eeprom driver, it's really
simple and I'm not sure we would save much by moving the emulation to
i2c-core. Plus, with the emulation done by each driver, the driver is
free to implement the emulation exactly the way it wants depending on
what the chip itself supports.

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

That's a trade-off... Bit-banged I2C is more CPU-intensive, but if you
can do block transactions that way, you might save some time. I'd say
it depends on which chip you're accessing and the size of the blocks.
If you're reading 10 bytes from an RTC chip once in a while, it's
probably not worth the effort. If you're reading a firmware from a large
EEPROM at boot time, you may consider it. But also keep in mind that
i2c-algo-bit can't run too fast, it is not necessarily stable over 100
kHz and simply cannot go above 250 kHz due to a design limitation. So
if your hardware controller does 400 kHz I2C, there's probably not much
to win by doing bit-banged I2C.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Maciej W. Rozycki]

Hi Jean,

 I am not dead anymore, so here it goes...

> If you think a change doesn't deserve a separate patch, I say it
> doesn't deserve to be done at all. The golden rule is to have separate
> patches for separate changes.

 I agree with the rule in general, although you always have to apply
common sense, as with everything in life -- complete inflexibility gets
you nowhere.  Originally patches comprising style correction only were not
allowed for Linux at all.  Based on my observation of the janitorial and
trivial monkey work over the last few years I think this is no longer the
case, but from various discussions at the relevant mailing lists I gather
small corrections of this kind are best to be included with substantive
changes within the area of interest.

 And last but not least I think the matter is not that a change that does
not deserve a separate patch, does not deserve to be done at all, but that
there are some changes that do not deserve extra people's effort to handle
them, but if they come for free with other changes, then this is not a big
deal.  I apply this rule to whatever changes I happen to be in charge to
ack and nobody has complained so far. :-)

> The problem is that there is no such rule in Documentation/CodingStyle,
> and in general developers don't care about sorting the header inclusions
> alphabetically or otherwise, so it is unlikely that your sort will
> survive the next changes that will happen in this area. So I join David
> in saying that this change is just not needed.

 Well, given no general rule any is valid.  I think anybody clever enough
to notice an obvious rule having been applied will follow it.  Personally
seeing no immediate rule within header inclusions I always have to invest
some brainpower in answering the question: "Is there any reason these
headers are in this particular order or not?  If so, where should I add a
new one -- would the beginning or the end be OK or should it come just
after <linux/fs.h>?"  This is because historically we used to have
ordering dependencies.  I think most of them have gone now, but few may
still remain.  If the alphabetical and functional (i.e. linux/ vs asm/,
etc.) order is kept, there is no need for such a question.  Besides, it
solves the problem of unnecessary multiple inclusions of the same file
popping out from time to time -- last seen yesterday with some report. ;-)

> The driver rightly checks I2C_FUNC_SMBUS_BYTE_DATA because it calls
> i2c_smbus_read_byte_data() and i2c_smbus_write_byte_data(). A driver
> calling specific SMBus transaction functions should always check that
> they are supported by the underlying bus driver first.
> 
> You are correct that, with what we said above, checking for
> I2C_FUNC_I2C and checking for I2C_FUNC_I2C | I2C_FUNC_SMBUS_FOO
> should be the same. However, this is only strictly equivalent if the
> underlying bus driver supports I2C without any restriction. In practice
> this is rarely the case, most I2C controllers have limitations. Many
> don't support zero-byte messages, for example. Some don't support more
> than 2 messages in a transaction, and some additionally have limits to
> the maximum length of messages. We've even seen a controller lately
> that would only support messages of 1, 8, 16 and 32 bytes.
> 
> For this reason, it is up to the bus driver to declare exactly what
> part of SMBus they can emulate. Most of the time it's
> I2C_FUNC_SMBUS_EMUL (which is a _subset_ of the SMBus transaction set
> plus the I2C block transactions) but some drivers remove a few
> functionality bits and some add a few ones. That's the reason why the
> I2C chip drivers must still check for individual functionality bits if
> they do direct calls to specific SMBus transaction functions.

 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
full-featured raw I2C controller -- say a bit-banger -- will set
I2C_FUNC_I2C | I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE |
[...] to let the core know these subsets of functionality can be achieved
somehow with this piece of silicon, even though there may be no dedicated
hardware support and all the layers of emulation may have to be involved 
-- am I correct?

 OK, I will adjust my changes accordingly.

> Important note: the chip supports _I2C_ block transactions, as David
> properly wrote. These are different from _SMBus_ block transactions.

 I have noticed that already, indeed, thanks.

> Wrong assumption. All it means is that the original driver author took
> the most direct path to get the driver working on _his_ hardware, which
> apparently supported I2C. He left it to other developers (i.e. you) to
> update the code if the driver was needed to work on top of less gifted
> controllers.

 Well, such an approach deserves a note somewhere around the piece of code
in question.

> You are absolutely correct, and this is the reason why no such
> emulation is done in i2c-core. However a number of chip drivers do that
> emulation when they know that this is correct for their chip (e.g.
> drivers/i2c/chips/eeprom.c). So you are free to do a similar emulation
> in the rtc-m41t80 driver.

 Code duplication is wrong and happens too often and it cannot be stressed
too much, so I will repeat again -- if there is anything to share, every
effort should be taken for it to be shared.  I agree it should be outside
the core, i.e. either in headers if trivial enough to be an inline
function or otherwise in a reasonable place like drivers/i2c/lib/.

> This has been discussed recently on the lm-sensors list:
> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-April/022908.html
> (Search for "the smbus layer doesn't emulate it".)

 Thanks for the reference.

> The conclusion was that we may implement the emulation in i2c-core if
> enough drivers needed it, and they agreed on the implementation. But
> you shouldn't wait for it to happen, just do the emulation in your
> driver for now. I'm really not sure if an i2c-core implementation will
> ever happen, just look at the code in the eeprom driver, it's really
> simple and I'm not sure we would save much by moving the emulation to
> i2c-core. Plus, with the emulation done by each driver, the driver is
> free to implement the emulation exactly the way it wants depending on
> what the chip itself supports.

 See my note above.  Small differences may be parametrised.  Large ones
may need a private copy, indeed, and this is the trade-off.

> That's a trade-off... Bit-banged I2C is more CPU-intensive, but if you
> can do block transactions that way, you might save some time. I'd say
> it depends on which chip you're accessing and the size of the blocks.
> If you're reading 10 bytes from an RTC chip once in a while, it's
> probably not worth the effort. If you're reading a firmware from a large
> EEPROM at boot time, you may consider it. But also keep in mind that
> i2c-algo-bit can't run too fast, it is not necessarily stable over 100
> kHz and simply cannot go above 250 kHz due to a design limitation. So
> if your hardware controller does 400 kHz I2C, there's probably not much
> to win by doing bit-banged I2C.

 The BCM1250A can go up to 12.5MHz on its I2C buses in hardware; I do not
know how many client devices if any at all can go beyond 400kHz by the
spec.  Besides, the SOC can drive I2C in the interrupt mode and now that
matters, doesn't it?  Our driver sets the bus #1 to 400kHz and the bus #0
to 100kHz -- I gather this is because SDRAM sockets are wired to the
latter.

  Maciej

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

Hi Maciej,

On Fri, 9 May 2008 20:18:22 +0100 (BST), Maciej W. Rozycki wrote:
>  I am not dead anymore, so here it goes...

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

> > If you think a change doesn't deserve a separate patch, I say it
> > doesn't deserve to be done at all. The golden rule is to have separate
> > patches for separate changes.
> 
>  I agree with the rule in general, although you always have to apply
> common sense, as with everything in life -- complete inflexibility gets
> you nowhere.  Originally patches comprising style correction only were not
> allowed for Linux at all.

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

>                            Based on my observation of the janitorial and
> trivial monkey work over the last few years I think this is no longer the
> case, but from various discussions at the relevant mailing lists I gather
> small corrections of this kind are best to be included with substantive
> changes within the area of interest.

We probably don't read the same mailing lists, all the ones I am reading
clearly carry the message that cleanups go to separate patches.

>  And last but not least I think the matter is not that a change that does
> not deserve a separate patch, does not deserve to be done at all, but that
> there are some changes that do not deserve extra people's effort to handle
> them, but if they come for free with other changes, then this is not a big
> deal.  I apply this rule to whatever changes I happen to be in charge to
> ack and nobody has complained so far. :-)

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.

> > The problem is that there is no such rule in Documentation/CodingStyle,
> > and in general developers don't care about sorting the header inclusions
> > alphabetically or otherwise, so it is unlikely that your sort will
> > survive the next changes that will happen in this area. So I join David
> > in saying that this change is just not needed.
> 
>  Well, given no general rule any is valid.

Definitely not :p

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

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.

That was for data for which being sorted has some value, and that is
clearly documented as being sorted. So I let you guess what will happen
to your header include list, which is not documented to be sorted and
for which being sorted has little interest.

>                                                                Personally
> seeing no immediate rule within header inclusions I always have to invest
> some brainpower in answering the question: "Is there any reason these
> headers are in this particular order or not?  If so, where should I add a
> new one -- would the beginning or the end be OK or should it come just
> after <linux/fs.h>?"  This is because historically we used to have
> ordering dependencies.  I think most of them have gone now, but few may

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

> still remain.  If the alphabetical and functional (i.e. linux/ vs asm/,
> etc.) order is kept, there is no need for such a question.  Besides, it
> solves the problem of unnecessary multiple inclusions of the same file
> popping out from time to time -- last seen yesterday with some report. ;-)

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.

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

> full-featured raw I2C controller -- say a bit-banger -- will set
> I2C_FUNC_I2C | I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE |
> [...] to let the core know these subsets of functionality can be achieved
> somehow with this piece of silicon, even though there may be no dedicated
> hardware support and all the layers of emulation may have to be involved 
> -- am I correct?

Yes, you are correct. In general, full-featured I2C bus drivers simply
declare I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL. I2C_FUNC_SMBUS_EMUL
includes all the SMBus transactions which can be emulated by i2c-core
without the cooperation of the bus driver, with the addition of the
I2C block transactions (because many SMBus controllers can handle them,
too.)

> > (...)
> > You are absolutely correct, and this is the reason why no such
> > emulation is done in i2c-core. However a number of chip drivers do that
> > emulation when they know that this is correct for their chip (e.g.
> > drivers/i2c/chips/eeprom.c). So you are free to do a similar emulation
> > in the rtc-m41t80 driver.
> 
>  Code duplication is wrong and happens too often and it cannot be stressed
> too much, so I will repeat again -- if there is anything to share, every
> effort should be taken for it to be shared.

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.

>                                             I agree it should be outside
> the core, i.e. either in headers if trivial enough to be an inline
> function or otherwise in a reasonable place like drivers/i2c/lib/.

It definitely won't be trivial enough to go in headers.

> > This has been discussed recently on the lm-sensors list:
> > http://lists.lm-sensors.org/pipermail/lm-sensors/2008-April/022908.html
> > (Search for "the smbus layer doesn't emulate it".)
> 
>  Thanks for the reference.
> 
> > The conclusion was that we may implement the emulation in i2c-core if
> > enough drivers needed it, and they agreed on the implementation. But
> > you shouldn't wait for it to happen, just do the emulation in your
> > driver for now. I'm really not sure if an i2c-core implementation will
> > ever happen, just look at the code in the eeprom driver, it's really
> > simple and I'm not sure we would save much by moving the emulation to
> > i2c-core. Plus, with the emulation done by each driver, the driver is
> > free to implement the emulation exactly the way it wants depending on
> > what the chip itself supports.
> 
>  See my note above.  Small differences may be parametrised.  Large ones
> may need a private copy, indeed, and this is the trade-off.

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.

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

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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: [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: [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