From owner-svn-src-stable-12@freebsd.org Sat Sep 14 20:26:50 2019 Return-Path: Delivered-To: svn-src-stable-12@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id EEEBAFE338; Sat, 14 Sep 2019 20:26:50 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 46W3vG62GZz4Psx; Sat, 14 Sep 2019 20:26:50 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id B23BA251B9; Sat, 14 Sep 2019 20:26:50 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id x8EKQodr085253; Sat, 14 Sep 2019 20:26:50 GMT (envelope-from ian@FreeBSD.org) Received: (from ian@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id x8EKQov2085252; Sat, 14 Sep 2019 20:26:50 GMT (envelope-from ian@FreeBSD.org) Message-Id: <201909142026.x8EKQov2085252@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: ian set sender to ian@FreeBSD.org using -f From: Ian Lepore Date: Sat, 14 Sep 2019 20:26:50 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r352339 - stable/12/sys/dev/iicbus X-SVN-Group: stable-12 X-SVN-Commit-Author: ian X-SVN-Commit-Paths: stable/12/sys/dev/iicbus X-SVN-Commit-Revision: 352339 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-stable-12@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for only the 12-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 14 Sep 2019 20:26:51 -0000 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)