[lm-sensors] PEC support in i2c/lm_sensors CVS

[lm-sensors] PEC support in i2c/lm_sensors CVS

[Jean Delvare]

Hi all,

I am almost done backporting many of the i2c and hwmon changes which
made it into Linux 2.6.14-git1 to i2c/lm_sensors CVS. As these are
backports, I am quite confident that these changes are all safe, but
you never know. So everyone still using a 2.4 kernel is welcome to give
a try to i2c and lm_sensors CVS, and report any problem.

One set of changes I did not backport yet is the software PEC support
rewrite and all the related changes. Usually we avoid large changes to
i2c CVS, but for this one time I would like to make an exception, and
backport the new PEC code so that i2c/lm_sensors CVS and Linux 2.6
behave the same way.

My arguments in favor of doing so are as follow:

* PEC support has a user-space interface part (i2c-dev.h). The new
interface is not incompatible with the old one, but still slightly
different. It's probably better for application writers to have a
common interface to deal with.

* The old PEC code was buggy. Hideki Iwamoto reported several problems,
some of which are already fixed in i2c CVS, but some others are still
not, and I also know of at least one more which wasn't reported. If we
don't replace the old implementation with the new one, we will at least
need to fix it - or delete it altogether.

* The fact that the old code was not working and nobody reported before
Hideki did proves that PEC is not widely used, if at all. So, there is
very little risk of upsetting anyone by changing the PEC code now.
Having a common implementation in CVS and Linux 2.6 will make it easier
for future driver authors. We have seen that people are still writing
drivers for Linux 2.4, which will be ported to 2.6 in the long run.

* The old PEC support was not even in Linux 2.4, so we don't break any
kind of compatibility at this level if changing the PEC code in
i2c/lm_sensors CVS.

* As a maintainer, I would very much prefer to have a single
implementation to support.

So, unless someone comes up with good reasons for not backporting the
new PEC code to i2c/lm_sensors CVS, I will probably do so. I would
appreciate if someone could take a look at the new Linux 2.6 code
(2.6.14-git1 and later) and confirm that I got it right. I don't think
it got any form of review so far, except for the fact that It Works For
Me (TM).

I would like to publicly thank Hideki for his work on the old PEC code.
Arigato, Hideki-san! Don't go thinking that your investigations and
reports were in vain. Even if later versions of i2c/lm_sensors CVS use
a completely different PEC code, you still much helped me understand
the old implementation and its weaknesses, so I could design something
(hopefully) better. And this is also very valuable to know the bugs
that existed in older versions of our software, as people are likely to
continue to use it for a long time.

Thanks,
-- 
Jean Delvare

[lm-sensors] PEC support in i2c/lm_sensors CVS

[Mark M. Hoffman]

Hi Jean:

* Jean Delvare <khali@linux-fr.org> [2005-10-30 17:13:35 +0100]:
(snip)
> So, unless someone comes up with good reasons for not backporting the
> new PEC code to i2c/lm_sensors CVS, I will probably do so. I would
> appreciate if someone could take a look at the new Linux 2.6 code
> (2.6.14-git1 and later) and confirm that I got it right. I don't think
> it got any form of review so far, except for the fact that It Works For
> Me (TM).

I finally reviewed those patches; one suggestion, to apply if you like...

This patch tweaks i2c-i801.c so that the driver always sets the SMBAUXCTL
register (which enables/disables PEC) explicitly before each transaction.

Signed-off-by: Mark M. Hoffman <mhoffman@lightlink.com>

--- linux-2.6.14-git5.orig/drivers/i2c/busses/i2c-i801.c
+++ linux-2.6.14-git5/drivers/i2c/busses/i2c-i801.c
@@ -468,8 +468,7 @@ static s32 i801_access(struct i2c_adapte
 		return -1;
 	}
 
-	if (hwpec)
-		outb_p(1, SMBAUXCTL);	/* enable hardware PEC */
+	outb_p(hwpec, SMBAUXCTL);	/* enable/disable hardware PEC */
 
 	if(block)
 		ret = i801_block_transaction(data, read_write, size, hwpec);
@@ -478,9 +477,6 @@ static s32 i801_access(struct i2c_adapte
 		ret = i801_transaction();
 	}
 
-	if (hwpec)
-		outb_p(0, SMBAUXCTL);	/* disable hardware PEC */
-
 	if(block)
 		return ret;
 	if(ret)
-- 
Mark M. Hoffman
mhoffman@lightlink.com

[lm-sensors] PEC support in i2c/lm_sensors CVS

[Jean Delvare]

Hi Mark,

On 2005-11-03, Mark M. Hoffman wrote:
> I finally reviewed those patches; one suggestion, to apply if you like...
>
> This patch tweaks i2c-i801.c so that the driver always sets the SMBAUXCTL
> register (which enables/disables PEC) explicitly before each transaction.

I agree, I prefer it that way too. I'll apply that patch, and I'll test
it on my ICH3M too. My testing possibilities are limited though, as I
only have EEPROMs on that bus.

I was expecting more comments from your review, about the PEC core code
rewrite itself. Inidivual SMBus drivers can always be tweaked, it isn't
really related to the core rework. Do you have any kind of objection to
the rewrite itself? My implementation is completely different from what
we had before. It also has slightly different expectations from both bus
and chip drivers. Just because I think it is better doesn't mean
everyone has to agree. I might have missed something. I would hate it if
people will come and complain one year from now that this rewrite was a
bad idea, for a reason I failed to think of.

Given that the new implementation is already in Linus' tree for Linux
2.6, it's there to stay unless someone complains. The point about
i2c/lm_sensors CVS is still open though. I will have to interpret the
lack of objection as a green light for me to merge my rewrite in there.
Much like everything else I do anyway, but this time the effects are
much broader.

Thanks,
--
Jean Delvare

[lm-sensors] PEC support in i2c/lm_sensors CVS

[Mark M. Hoffman]

Hi Jean:

> On 2005-11-03, Mark M. Hoffman wrote:
> > I finally reviewed those patches; one suggestion, to apply if you like...
> >
> > This patch tweaks i2c-i801.c so that the driver always sets the SMBAUXCTL
> > register (which enables/disables PEC) explicitly before each transaction.

* Jean Delvare <khali@linux-fr.org> [2005-11-03 11:15:20 +0100]:
> I agree, I prefer it that way too. I'll apply that patch, and I'll test
> it on my ICH3M too. My testing possibilities are limited though, as I
> only have EEPROMs on that bus.
> 
> I was expecting more comments from your review, about the PEC core code
> rewrite itself. Inidivual SMBus drivers can always be tweaked, it isn't
> really related to the core rework. Do you have any kind of objection to
> the rewrite itself? My implementation is completely different from what
> we had before. It also has slightly different expectations from both bus
> and chip drivers. Just because I think it is better doesn't mean

Since you insist, heh... there was this bit from lm90.c for ADM1032 PEC:

+/* The ADM1032 supports PEC but not on write byte transactions, so we need
+   to explicitely ask for a transaction without PEC. */
+static inline s32 adm1032_write_byte(struct i2c_client *client, u8 value)
+{
+	return i2c_smbus_xfer(client->adapter, client->addr,
+			      client->flags & ~I2C_CLIENT_PEC,
+			      I2C_SMBUS_WRITE, value, I2C_SMBUS_BYTE, NULL);
+}

It's a bit of a hack, but in practice I hope it won't be necessary very
often.  Although, here we have the very first PEC capable device requiring
that.  But I don't have a better idea, so I wasn't going to mention it.

> everyone has to agree. I might have missed something. I would hate it if
> people will come and complain one year from now that this rewrite was a
> bad idea, for a reason I failed to think of.

No worries - I don't see anything glaring.  And there's one user for which
it works, right?  Not that I think it will, but if it did need to be reworked
again later, so be it.

> Given that the new implementation is already in Linus' tree for Linux
> 2.6, it's there to stay unless someone complains. The point about
> i2c/lm_sensors CVS is still open though. I will have to interpret the
> lack of objection as a green light for me to merge my rewrite in there.
> Much like everything else I do anyway, but this time the effects are
> much broader.

As for backporting this to 2.4: I have no opinion about that anymore.

(Except that, by this time next year, maybe we should deep-freeze the I2C
CVS tree, delete all the kernel 2.4 bits in lm_sensors2, and advance the
version to 3.0.0)

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

[lm-sensors] PEC support in i2c/lm_sensors CVS

[Jean Delvare]

Hi Mark,

On 2005-11-04, Mark M. Hoffman wrote:
> Since you insist, heh... there was this bit from lm90.c for ADM1032 PEC:
>
> +/* The ADM1032 supports PEC but not on write byte transactions, so we need
> +   to explicitely ask for a transaction without PEC. */
> +static inline s32 adm1032_write_byte(struct i2c_client *client, u8 value)
> +{
> +	return i2c_smbus_xfer(client->adapter, client->addr,
> +			      client->flags & ~I2C_CLIENT_PEC,
> +			      I2C_SMBUS_WRITE, value, I2C_SMBUS_BYTE, NULL);
> +}
>
> It's a bit of a hack, but in practice I hope it won't be necessary very
> often.  Although, here we have the very first PEC capable device requiring
> that.  But I don't have a better idea, so I wasn't going to mention it.

I have to agree it's not exactly elegant, and I totally share your
analysis. I am waiting to see more chips implementing PEC to decide what
to do with this case and similar ones.

I fear that all chips will have the problem with this one transaction
type, because SMBus Send Byte with PEC is the same as SMBus Write Byte
without PEC. I consider this a specification weakness. Due to the fact
that PEC must always be optional as per the same specification, any chip
supporting SMBus Write Byte and SMBus Send Byte with the same
"command" value can obviously not support the latter with PEC.

Anyway, this is a side problem. The old PEC implementation would not have
handled this case better as far as I can see, so this is no regression.

> No worries - I don't see anything glaring.  And there's one user for which
> it works, right?  Not that I think it will, but if it did need to be
> reworked again later, so be it.

That one user is me, so I'm not sure if it counts ;) I am still waiting
for a "works for me too" type of report.

> (Except that, by this time next year, maybe we should deep-freeze the I2C
> CVS tree, delete all the kernel 2.4 bits in lm_sensors2, and advance the
> version to 3.0.0)

This implies that, by that time, the sysfs interface in Linux 2.6 will
have been fully stabilized and completed in a totally chip-independant
way, and we will have a brand new, essentially chip-independant library
to interface with it. I wish it really happens, but it doesn't sound
too realistic with the little manpower we have available at the moment.

Thanks,
--
Jean Delvare