All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hartmut Knaack <knaack.h@gmx.de>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	Jean Delvare <khali@linux-fr.org>,
	Jonathan Cameron <jic23@cam.ac.uk>,
	lm-sensors@lm-sensors.org, linux-iio@vger.kernel.org
Subject: Re: [PATCH v2 1/4] hwmon: (adt7410) Don't re-read non-volatile registers
Date: Tue, 19 Feb 2013 20:39:33 +0100	[thread overview]
Message-ID: <5123D4F5.5030001@gmx.de> (raw)
In-Reply-To: <20130219010229.GA25124@roeck-us.net>

Guenter Roeck schrieb:
> On Mon, Feb 18, 2013 at 09:22:18PM +0100, Hartmut Knaack wrote:
>> Lars-Peter Clausen schrieb:
>>> Currently each time the temperature register is read the driver also reads the
>>> threshold and hysteresis registers. This increases the amount of I2C traffic and
>>> time needed to read the temperature by a factor of ~5. Neither the threshold nor
>>> the hysteresis change on their own, so once we've read them, we should be able
>>> to just use the cached value of the registers. This patch modifies the code
>>> accordingly and only reads the threshold and hysteresis registers once during
>>> probe.
>> I have been thinking about this a lot, and I am concerned about data integrity. From what I know about I2C, there is no data integrity verification specified in the protocol. So, what the master sends is not necessarily what the slave receives (not to mention other devices on the bus, which could potentially mess around with the slaves, or even reset of the slave). Reading back just cached values makes it pretty hard to verify, if there are issues. I think it might be better to call a read-temperature function with a parameter that indicates, which temperature register is required.
> I am not concerned about that, unless there is a known issue with the chip
> and it is known to return bad data under some circumstances. I am doing the
> same in the PMBus drivers, since there are simply too many limit registers
> to read on some of the chips (there may literally be more than a hundred).
> That works fine most of the time; if it does not work, it is a chip problem,
> an i2c bus master problem, a hardware signal problem, or a combination of all.
> I actually think it is better if the problem is exposed by cached bad readings.
Could you please outline the last sentence? I'm having trouble to understand your intention with cached bad readings.
> Then there is a chance to do something about it. This would be much better
> than just re-reading the value next time and ignoring the problem.
>
> Something else - the commit window just opened (a week earlier than I guessed),
> so this part of the series is going to miss 3.9. Hartmut, if you plan to provide
> an Acked-by or Reviewed-by or Tested-by for the first part of the series, you'll
> have to do it soon, as I plan to send my push request to Linus around Thursday.
OK
>
> Thanks,
> Guenter
>


WARNING: multiple messages have this Message-ID (diff)
From: Hartmut Knaack <knaack.h@gmx.de>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	Jean Delvare <khali@linux-fr.org>,
	Jonathan Cameron <jic23@cam.ac.uk>,
	lm-sensors@lm-sensors.org, linux-iio@vger.kernel.org
Subject: Re: [lm-sensors] [PATCH v2 1/4] hwmon: (adt7410) Don't re-read non-volatile registers
Date: Tue, 19 Feb 2013 19:39:33 +0000	[thread overview]
Message-ID: <5123D4F5.5030001@gmx.de> (raw)
In-Reply-To: <20130219010229.GA25124@roeck-us.net>

Guenter Roeck schrieb:
> On Mon, Feb 18, 2013 at 09:22:18PM +0100, Hartmut Knaack wrote:
>> Lars-Peter Clausen schrieb:
>>> Currently each time the temperature register is read the driver also reads the
>>> threshold and hysteresis registers. This increases the amount of I2C traffic and
>>> time needed to read the temperature by a factor of ~5. Neither the threshold nor
>>> the hysteresis change on their own, so once we've read them, we should be able
>>> to just use the cached value of the registers. This patch modifies the code
>>> accordingly and only reads the threshold and hysteresis registers once during
>>> probe.
>> I have been thinking about this a lot, and I am concerned about data integrity. From what I know about I2C, there is no data integrity verification specified in the protocol. So, what the master sends is not necessarily what the slave receives (not to mention other devices on the bus, which could potentially mess around with the slaves, or even reset of the slave). Reading back just cached values makes it pretty hard to verify, if there are issues. I think it might be better to call a read-temperature function with a parameter that indicates, which temperature register is required.
> I am not concerned about that, unless there is a known issue with the chip
> and it is known to return bad data under some circumstances. I am doing the
> same in the PMBus drivers, since there are simply too many limit registers
> to read on some of the chips (there may literally be more than a hundred).
> That works fine most of the time; if it does not work, it is a chip problem,
> an i2c bus master problem, a hardware signal problem, or a combination of all.
> I actually think it is better if the problem is exposed by cached bad readings.
Could you please outline the last sentence? I'm having trouble to understand your intention with cached bad readings.
> Then there is a chance to do something about it. This would be much better
> than just re-reading the value next time and ignoring the problem.
>
> Something else - the commit window just opened (a week earlier than I guessed),
> so this part of the series is going to miss 3.9. Hartmut, if you plan to provide
> an Acked-by or Reviewed-by or Tested-by for the first part of the series, you'll
> have to do it soon, as I plan to send my push request to Linus around Thursday.
OK
>
> Thanks,
> Guenter
>


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

  reply	other threads:[~2013-02-19 19:39 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-18 13:38 [PATCH v2 1/4] hwmon: (adt7410) Don't re-read non-volatile registers Lars-Peter Clausen
2013-02-18 13:38 ` [lm-sensors] " Lars-Peter Clausen
2013-02-18 13:38 ` [PATCH v2 2/4] hwmon: (adt7410) Add support for the adt7310/adt7320 Lars-Peter Clausen
2013-02-18 13:38   ` [lm-sensors] [PATCH v2 2/4] hwmon: (adt7410) =?utf-8?q?Add_support_for_the_ad Lars-Peter Clausen
2013-02-19  1:30   ` [PATCH v2 2/4] hwmon: (adt7410) Add support for the adt7310/adt7320 Guenter Roeck
2013-02-19  1:30     ` [lm-sensors] " Guenter Roeck
2013-02-19 11:57     ` Lars-Peter Clausen
2013-02-19 11:57       ` [lm-sensors] " Lars-Peter Clausen
2013-02-19 16:52       ` Guenter Roeck
2013-02-19 16:52         ` [lm-sensors] " Guenter Roeck
2013-02-18 13:38 ` [PATCH v2 3/4] hwmon: (adt7x10) Add alarm interrupt support Lars-Peter Clausen
2013-02-18 13:38   ` [lm-sensors] " Lars-Peter Clausen
2013-02-19  1:39   ` Guenter Roeck
2013-02-19  1:39     ` [lm-sensors] " Guenter Roeck
2013-02-19 12:05     ` Lars-Peter Clausen
2013-02-19 12:05       ` [lm-sensors] " Lars-Peter Clausen
2013-02-19 17:10       ` Guenter Roeck
2013-02-19 17:10         ` [lm-sensors] " Guenter Roeck
2013-02-18 13:38 ` [PATCH v2 4/4] staging:iio: Remove adt7410 driver Lars-Peter Clausen
2013-02-18 13:38   ` [lm-sensors] " Lars-Peter Clausen
2013-03-02 16:45   ` Jonathan Cameron
2013-03-02 16:45     ` [lm-sensors] " Jonathan Cameron
2013-03-02 17:10     ` Guenter Roeck
2013-03-02 17:10       ` [lm-sensors] " Guenter Roeck
2013-02-18 20:22 ` [PATCH v2 1/4] hwmon: (adt7410) Don't re-read non-volatile registers Hartmut Knaack
2013-02-18 20:22   ` [lm-sensors] " Hartmut Knaack
2013-02-19  1:02   ` Guenter Roeck
2013-02-19  1:02     ` [lm-sensors] " Guenter Roeck
2013-02-19 19:39     ` Hartmut Knaack [this message]
2013-02-19 19:39       ` Hartmut Knaack
2013-02-20  1:22       ` Guenter Roeck
2013-02-20  1:22         ` [lm-sensors] " Guenter Roeck
2013-02-19  1:32 ` Guenter Roeck
2013-02-19  1:32   ` [lm-sensors] " Guenter Roeck
2013-02-23  0:45 ` Hartmut Knaack
2013-02-23 20:18   ` Guenter Roeck
2013-02-23 20:18     ` [lm-sensors] " Guenter Roeck
2013-02-25 22:03     ` Hartmut Knaack
2013-02-25 22:03       ` [lm-sensors] " Hartmut Knaack
2013-02-26  1:40       ` Guenter Roeck
2013-02-26  1:40         ` [lm-sensors] " Guenter Roeck
2013-02-25  9:54   ` Lars-Peter Clausen
2013-02-25  9:54     ` [lm-sensors] " Lars-Peter Clausen
2013-02-25 21:30     ` Hartmut Knaack
2013-02-25 21:30       ` [lm-sensors] " Hartmut Knaack
2013-02-26  1:39       ` Guenter Roeck
2013-02-26  1:39         ` [lm-sensors] " Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5123D4F5.5030001@gmx.de \
    --to=knaack.h@gmx.de \
    --cc=jic23@cam.ac.uk \
    --cc=khali@linux-fr.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=lm-sensors@lm-sensors.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.