diff options
| author | Andriy Gapon <avg@FreeBSD.org> | 2021-11-26 08:07:27 +0000 |
|---|---|---|
| committer | Andriy Gapon <avg@FreeBSD.org> | 2021-12-17 07:31:21 +0000 |
| commit | 074248b948b6fab3ec3a84e2573201fe82190cf3 (patch) | |
| tree | d8ca57bdcd163241c4b4b0bd101cf8e027ba8379 | |
| parent | 194be9b71121ba21eb6150a8c7873a2ed0e5e74e (diff) | |
| download | src-074248b948b6fab3ec3a84e2573201fe82190cf3.tar.gz src-074248b948b6fab3ec3a84e2573201fe82190cf3.zip | |
twsi: move handling of TWSI_CONTROL_ACK into the state machine
Previously the code set TWSI_CONTROL_ACK in twsi_transfer() based on
whether the first message had a length of one. That was done regardless
of whether the message was a read or write and what kind of messages
followed it.
Now the bit is set or cleared while handling TWSI_STATUS_ADDR_R_ACK
state transition based on the current (read) message.
The old code did not correctly work in a scenario where a single byte
was read from an EEPROM device with two byte addressing.
For example:
i2c -m tr -a 0x50 -d r -w 16 -o 0 -c 1 -v
The reason is that the first message (a write) has two bytes, so
TWSI_CONTROL_ACK was set and never cleared.
Since the controller did not send NACK the EEPROM sent more data resulting
in a buffer overrun.
While working on TWSI_STATUS_ADDR_R_ACK I also added support for
the zero-length read access and then I did the same for zero-length write
access.
While rare, those types of I2C transactions are completely valid and are
used by some devices.
PR: 258994
(cherry picked from commit 04622a7f21157827fe75276a4520a52d3a571a86)
| -rw-r--r-- | sys/dev/iicbus/twsi/twsi.c | 40 |
1 files changed, 26 insertions, 14 deletions
diff --git a/sys/dev/iicbus/twsi/twsi.c b/sys/dev/iicbus/twsi/twsi.c index ecadfade04d9..4a20d7ea35b6 100644 --- a/sys/dev/iicbus/twsi/twsi.c +++ b/sys/dev/iicbus/twsi/twsi.c @@ -516,11 +516,9 @@ twsi_transfer(device_t dev, struct iic_msg *msgs, uint32_t nmsgs) for (int i = 0; i < nmsgs; i++) debugf(sc, "msg %d is %d bytes long\n", i, msgs[i].len); #endif + /* Send start and re-enable interrupts */ - sc->control_val = TWSI_CONTROL_TWSIEN | - TWSI_CONTROL_INTEN | TWSI_CONTROL_ACK; - if (sc->msgs[0].len == 1) - sc->control_val &= ~TWSI_CONTROL_ACK; + sc->control_val = TWSI_CONTROL_TWSIEN | TWSI_CONTROL_INTEN; TWSI_WRITE(sc, sc->reg_control, sc->control_val | TWSI_CONTROL_START); msleep_sbt(sc, &sc->mutex, 0, "twsi", 3000 * SBT_1MS, SBT_1MS, 0); debugf(sc, "pause finish\n"); @@ -597,22 +595,36 @@ twsi_intr(void *arg) break; case TWSI_STATUS_ADDR_W_ACK: - debugf(sc, "Ack received after transmitting the address (write)\n"); - /* Directly send the first byte */ - sc->sent_bytes = 1; - debugf(sc, "Sending byte 0 (of %d) = %x\n", - sc->msgs[sc->msg_idx].len, - sc->msgs[sc->msg_idx].buf[0]); - TWSI_WRITE(sc, sc->reg_data, sc->msgs[sc->msg_idx].buf[0]); + debugf(sc, "Address ACK-ed (write)\n"); - TWSI_WRITE(sc, sc->reg_control, sc->control_val); + if (sc->msgs[sc->msg_idx].len > 0) { + /* Directly send the first byte */ + sc->sent_bytes = 1; + debugf(sc, "Sending byte 0 (of %d) = %x\n", + sc->msgs[sc->msg_idx].len, + sc->msgs[sc->msg_idx].buf[0]); + TWSI_WRITE(sc, sc->reg_data, + sc->msgs[sc->msg_idx].buf[0]); + } else { + debugf(sc, "Zero-length write, sending STOP\n"); + TWSI_WRITE(sc, sc->reg_control, + sc->control_val | TWSI_CONTROL_STOP); + } break; case TWSI_STATUS_ADDR_R_ACK: - debugf(sc, "Ack received after transmitting the address (read)\n"); + debugf(sc, "Address ACK-ed (read)\n"); sc->recv_bytes = 0; - TWSI_WRITE(sc, sc->reg_control, sc->control_val); + if (sc->msgs[sc->msg_idx].len == 0) { + debugf(sc, "Zero-length read, sending STOP\n"); + TWSI_WRITE(sc, sc->reg_control, + sc->control_val | TWSI_CONTROL_STOP); + } else if (sc->msgs[sc->msg_idx].len == 1) { + sc->control_val &= ~TWSI_CONTROL_ACK; + } else { + sc->control_val |= TWSI_CONTROL_ACK; + } break; case TWSI_STATUS_ADDR_W_NACK: |