svn commit: r351885 - head/sys/dev/iicbus

[Ian Lepore]

Author: ian
Date: Thu Sep  5 19:07:48 2019
New Revision: 351885
URL: https://svnweb.freebsd.org/changeset/base/351885

Log:
  Ensure a measurement is complete before reading the result in ads111x.
  Also, disable the comparator by default; it's not used for anything.
  
  The previous logic would start a measurement, and then pause_sbt() for the
  averaging time currently configured in the chip.  After waiting that long,
  the code would blindly read the measurement register and return its value.
  The problem is that the chip's idea of averaging time is based on its
  internal free-running 1MHz oscillator, which may be running at a wildly
  different rate than the kernel clock.  If the chip's internal timer was
  running slower than the kernel clock, we'd end up grabbing a stale result
  from an old measurement.
  
  The driver now still uses pause_sbt() to yield the cpu while waiting for
  the measurement to complete, but after sleeping it checks the chip's status
  register to ensure the measurement engine is idle.  If it's not, the driver
  uses a retry loop to wait a bit (5% of the original wait time) then check
  again for completion.

Modified:
  head/sys/dev/iicbus/ads111x.c

Modified: head/sys/dev/iicbus/ads111x.c
==============================================================================
--- head/sys/dev/iicbus/ads111x.c	Thu Sep  5 18:54:46 2019	(r351884)
+++ head/sys/dev/iicbus/ads111x.c	Thu Sep  5 19:07:48 2019	(r351885)
@@ -62,6 +62,7 @@ __FBSDID("$FreeBSD$");
 #define	  ADS111x_CONF_GAIN_SHIFT	 9	/* Programmable gain amp */
 #define	  ADS111x_CONF_MODE_SHIFT	 8	/* Operational mode */
 #define	  ADS111x_CONF_RATE_SHIFT	 5	/* Sample rate */
+#define	  ADS111x_CONF_COMP_DISABLE	 3	/* Comparator disable */
 
 #define	ADS111x_LOTHRESH		2	/* Compare lo threshold (rw) */
 
@@ -81,7 +82,8 @@ __FBSDID("$FreeBSD$");
  * comparator and we'll leave it alone if they do.  That allows them connect the
  * alert pin to something and use the feature without any help from this driver.
  */
-#define	ADS111x_CONF_DEFAULT	(1 << ADS111x_CONF_MODE_SHIFT)
+#define	ADS111x_CONF_DEFAULT    \
+    ((1 << ADS111x_CONF_MODE_SHIFT) | ADS111x_CONF_COMP_DISABLE)
 #define	ADS111x_CONF_USERMASK   0x001f
 
 /*
@@ -189,7 +191,7 @@ static int
 ads111x_sample_voltage(struct ads111x_softc *sc, int channum, int *voltage) 
 {
 	struct ads111x_channel *chan;
-	int err, cfgword, convword, rate, waitns;
+	int err, cfgword, convword, rate, retries, waitns;
 	int64_t fsrange;
 
 	chan = &sc->channels[channum];
@@ -205,7 +207,8 @@ ads111x_sample_voltage(struct ads111x_softc *sc, int c
 
 	/*
 	 * Calculate how long it will take to make the measurement at the
-	 * current sampling rate (round up), and sleep at least that long.
+	 * current sampling rate (round up).  The measurement averaging time
+	 * ranges from 300us to 125ms, so we yield the cpu while waiting.
 	 */
 	rate = sc->chipinfo->ratetab[chan->rateidx];
 	waitns = (1000000000 + rate - 1) / rate;
@@ -213,20 +216,27 @@ ads111x_sample_voltage(struct ads111x_softc *sc, int c
 	if (err != 0 && err != EWOULDBLOCK)
 		return (err);
 
-#if 0
 	/*
-	 * Sanity-check that the measurement is complete.  Not enabled by
-	 * default because checking wastes 200-800us just in moving the status
-	 * command and result across the i2c bus, which could double the time it
-	 * takes to get one measurement.  Unlike most i2c slaves, this device
-	 * does not auto-increment the register number on reads, so we can't
-	 * read both status and measurement in one operation.
+	 * In theory the measurement should be available now; we waited long
+	 * enough.  However, the chip times its averaging intervals using an
+	 * internal 1 MHz oscillator which likely isn't running at the same rate
+	 * as the system clock, so we have to double-check that the measurement
+	 * is complete before reading the result.  If it's not ready yet, yield
+	 * the cpu again for 5% of the time we originally calculated.
+	 *
+	 * Unlike most i2c slaves, this device does not auto-increment the
+	 * register number on reads, so we can't read both status and
+	 * measurement in one operation.
 	 */
-	if ((err = ads111x_read_2(sc, ADS111x_CONF, &cfgword)) != 0)
-		return (err);
-	if (!(cfgword & ADS111x_CONF_IDLE))
-		return (EIO);
-#endif
+	for (retries = 5; ; --retries) {
+		if (retries == 0)
+			return (EWOULDBLOCK);
+		if ((err = ads111x_read_2(sc, ADS111x_CONF, &cfgword)) != 0)
+			return (err);
+		if (cfgword & ADS111x_CONF_IDLE)
+			break;
+		pause_sbt("ads111x", nstosbt(waitns / 20), 0, C_PREL(2));
+	}
 
 	/* Retrieve the sample and convert it to microvolts. */
 	if ((err = ads111x_read_2(sc, ADS111x_CONV, &convword)) != 0)