From: Corey Minyard <cminy...@mvista.com>
Change 2293c27faddf (i2c: implement broadcast write) added broadcast
capability to the I2C bus, but it broke SMBus read transactions.
An SMBus read transaction does two i2c_start_transaction() calls
without an intervening i2c_end_transfer() call. This will
result in i2c_start_transfer() adding the same device to the
current_devs list twice, and then the ->event() for the same
device gets called twice in the second call to i2c_start_transfer(),
resulting in the smbus code getting confused.
This fix adds a third state to the i2c_start_transfer() recv
parameter, a read continued that will not scan for devices
and add them to current_devs. It also adds #defines for all
the values for the recv parameter.
This also deletes the empty check from the top of i2c_end_transfer().
It's unnecessary, and it prevents the broadcast variable from being
set to false at the end of the transaction if no devices were on
the bus.
Cc: KONRAD Frederic <fred.kon...@greensocs.com>
Cc: Alistair Francis <alistair.fran...@xilinx.com>
Cc: Peter Crosthwaite <crosthwaite.pe...@gmail.com>
Cc: Kwon <hyun.k...@xilinx.com>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Signed-off-by: Corey Minyard <cminy...@mvista.com>
---
hw/i2c/core.c | 24 +++++++++++-------------
hw/i2c/smbus.c | 22 +++++++++++-----------
include/hw/i2c/i2c.h | 9 +++++++++
3 files changed, 31 insertions(+), 24 deletions(-)
I considered a couple of ways to do this. I thought about adding a
separate function to do a "intermediate end" of the transaction, but
that seemed like too much work. I also thought about adding a
bool saing whether you are currently in a transaction and not rescan
the bus if you are. However, that would require that the bool be in
the vmstate, and that would be complicated.
On that note, the current_devs list is not in the vmstate. That means
that a migrate done in the middle of an I2C transaction will cause the
I2C transaction to fail, right? Maybe this whole broadcast thing is
a bad idea, or needs a different implementation?
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index abb3efb..53069dd 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -101,15 +101,17 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int
recv)
bus->broadcast = true;
}
- QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
- DeviceState *qdev = kid->child;
- I2CSlave *candidate = I2C_SLAVE(qdev);
- if ((candidate->address == address) || (bus->broadcast)) {
- node = g_malloc(sizeof(struct I2CNode));
- node->elt = candidate;
- QLIST_INSERT_HEAD(&bus->current_devs, node, next);
- if (!bus->broadcast) {
- break;
+ if (recv != I2C_START_CONTINUED_READ_TRANSFER) {
+ QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
+ DeviceState *qdev = kid->child;
+ I2CSlave *candidate = I2C_SLAVE(qdev);
+ if ((candidate->address == address) || (bus->broadcast)) {
+ node = g_malloc(sizeof(struct I2CNode));
+ node->elt = candidate;
+ QLIST_INSERT_HEAD(&bus->current_devs, node, next);
+ if (!bus->broadcast) {
+ break;
+ }
}
}
}
@@ -134,10 +136,6 @@ void i2c_end_transfer(I2CBus *bus)
I2CSlaveClass *sc;
I2CNode *node, *next;
- if (QLIST_EMPTY(&bus->current_devs)) {
- return;
- }
-
QLIST_FOREACH_SAFE(node, &bus->current_devs, next, next) {
sc = I2C_SLAVE_GET_CLASS(node->elt);
if (sc->event) {
diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 3979b3d..f63799d 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -222,7 +222,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr)
{
uint8_t data;
- if (i2c_start_transfer(bus, addr, 1)) {
+ if (i2c_start_transfer(bus, addr, I2C_START_READ_TRANSFER)) {
return -1;
}
data = i2c_recv(bus);
@@ -233,7 +233,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr)
int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
{
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
return -1;
}
i2c_send(bus, data);
@@ -244,11 +244,11 @@ int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t
data)
int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
{
uint8_t data;
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
return -1;
}
i2c_send(bus, command);
- i2c_start_transfer(bus, addr, 1);
+ i2c_start_transfer(bus, addr, I2C_START_CONTINUED_READ_TRANSFER);
data = i2c_recv(bus);
i2c_nack(bus);
i2c_end_transfer(bus);
@@ -257,7 +257,7 @@ int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t
command)
int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
{
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
return -1;
}
i2c_send(bus, command);
@@ -269,11 +269,11 @@ int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t
command, uint8_t data)
int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
{
uint16_t data;
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
return -1;
}
i2c_send(bus, command);
- i2c_start_transfer(bus, addr, 1);
+ i2c_start_transfer(bus, addr, I2C_START_CONTINUED_READ_TRANSFER);
data = i2c_recv(bus);
data |= i2c_recv(bus) << 8;
i2c_nack(bus);
@@ -283,7 +283,7 @@ int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t
command)
int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
{
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
return -1;
}
i2c_send(bus, command);
@@ -298,11 +298,11 @@ int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t
command, uint8_t *data)
int len;
int i;
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
return -1;
}
i2c_send(bus, command);
- i2c_start_transfer(bus, addr, 1);
+ i2c_start_transfer(bus, addr, I2C_START_CONTINUED_READ_TRANSFER);
len = i2c_recv(bus);
if (len > 32) {
len = 0;
@@ -323,7 +323,7 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t
command, uint8_t *data,
if (len > 32)
len = 32;
- if (i2c_start_transfer(bus, addr, 0)) {
+ if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
return -1;
}
i2c_send(bus, command);
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index c4085aa..16c910e 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -50,6 +50,15 @@ struct I2CSlave
uint8_t address;
};
+/* For the recv value in i2c_start_transfer. The first two values
+ correspond to false and true for the recv value. The third is a
+ special value that is used to tell i2c_start_transfer that this is
+ a continuation of the previous transfer, so don't rescan the bus
+ for devices to send to, continue with the current set of devices. */
+#define I2C_START_WRITE_TRANSFER 0
+#define I2C_START_READ_TRANSFER 1
+#define I2C_START_CONTINUED_READ_TRANSFER 2
+
I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
void i2c_set_slave_address(I2CSlave *dev, uint8_t address);
int i2c_bus_busy(I2CBus *bus);
--
2.7.4
