svn commit: r352339 - stable/12/sys/dev/iicbus
Ian Lepore
ian at FreeBSD.org
Sat Sep 14 20:26:51 UTC 2019
Author: ian
Date: Sat Sep 14 20:26:50 2019
New Revision: 352339
URL: https://svnweb.freebsd.org/changeset/base/352339
Log:
MFC r351885, r351887
r351885:
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.
r351887:
Use a single write of 3 bytes instead of iicdev_writeto() in ads111x.
The iicdev_writeto() function basically does scatter-gather IO by filling
in a pair of iic_msg structs to write the register address then the data
from different locations but with a single bus START/xfer/STOP sequence.
It turns out several low-level i2c controller drivers do not honor the
IIC_NOSTART flag, so the second piece of the write gets a new START on
the bus, and that confuses the ads111x chips which expect a continuous
write of 3 bytes to set a register.
A proper fix for this is to track down all the misbehaving controllers
drivers and fix them. For now this change makes this driver work again.
Modified:
stable/12/sys/dev/iicbus/ads111x.c
Directory Properties:
stable/12/ (props changed)
Modified: stable/12/sys/dev/iicbus/ads111x.c
==============================================================================
--- stable/12/sys/dev/iicbus/ads111x.c Sat Sep 14 19:33:36 2019 (r352338)
+++ stable/12/sys/dev/iicbus/ads111x.c Sat Sep 14 20:26:50 2019 (r352339)
@@ -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
/*
@@ -165,11 +167,21 @@ struct ads111x_softc {
static int
ads111x_write_2(struct ads111x_softc *sc, int reg, int val)
{
- uint8_t data[2];
+ uint8_t data[3];
+ struct iic_msg msgs[1];
+ uint8_t slaveaddr;
- be16enc(data, val);
+ slaveaddr = iicbus_get_addr(sc->dev);
- return (iic2errno(iicdev_writeto(sc->dev, reg, data, 2, IIC_WAIT)));
+ data[0] = reg;
+ be16enc(&data[1], val);
+
+ msgs[0].slave = slaveaddr;
+ msgs[0].flags = IIC_M_WR;
+ msgs[0].len = sizeof(data);
+ msgs[0].buf = data;
+
+ return (iicbus_transfer_excl(sc->dev, msgs, nitems(msgs), IIC_WAIT));
}
static int
@@ -189,7 +201,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 +217,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 +226,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)
More information about the svn-src-all
mailing list