Re: [Qemu-devel] [PATCH 2/8] i2c: implement broadcast write.

[Peter Crosthwaite]

On Wed, May 13, 2015 at 12:12 PM,  <address@hidden> wrote:
> From: KONRAD Frederic <address@hidden>
>
> This does a write to every slaves when the I2C bus get a write to address 0.
>

"slave"

> Signed-off-by: KONRAD Frederic <address@hidden>
> ---
>  hw/i2c/core.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 5a64026..db1cbdd 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -15,6 +15,7 @@ struct I2CBus
>      I2CSlave *current_dev;
>      I2CSlave *dev;
>      uint8_t saved_address;
> +    bool broadcast;
>  };
>
>  static Property i2c_props[] = {
> @@ -67,6 +68,8 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
>
>      bus = I2C_BUS(qbus_create(TYPE_I2C_BUS, parent, name));
>      vmstate_register(NULL, -1, &vmstate_i2c_bus, bus);
> +
> +    bus->broadcast = false;

0 initialiser should not be needed for new QOM objects.

>      return bus;
>  }
>
> @@ -89,6 +92,21 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int 
> recv)
>      I2CSlave *slave = NULL;
>      I2CSlaveClass *sc;
>
> +    if (address == 0x00) {
> +        /*
> +         * This is a broadcast.
> +         */

one line comment.

> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {

qbus field is private to the QOM parent and cant be dereffed like
this. Use a BUS() cast.

> +            I2CSlave *dev = I2C_SLAVE(kid->child);
> +            sc = I2C_SLAVE_GET_CLASS(dev);
> +            bus->broadcast = true;

Move outside loop.

> +            if (sc->event) {
> +                sc->event(dev, recv ? I2C_START_RECV : I2C_START_SEND);
> +            }
> +        }
> +        return 0;
> +    }
> +

This leads to some duped code. The existing loop should be refactored
to contain the actual I2C transfer as a conditional, then the
broadcast flag can be ||d into that condition.

>      QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>          DeviceState *qdev = kid->child;
>          I2CSlave *candidate = I2C_SLAVE(qdev);
> @@ -114,9 +132,21 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int 
> recv)
>
>  void i2c_end_transfer(I2CBus *bus)
>  {
> +    BusChild *kid;
>      I2CSlave *dev = bus->current_dev;
>      I2CSlaveClass *sc;
>
> +    if (bus->broadcast) {
> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
> +            I2CSlave *dev = I2C_SLAVE(kid->child);
> +            sc = I2C_SLAVE_GET_CLASS(dev);
> +            if (sc->event) {
> +                sc->event(dev, I2C_FINISH);
> +            }
> +        }
> +        bus->broadcast = false;
> +    }
> +

Similar refactor would help here. Coreify

            sc = I2C_SLAVE_GET_CLASS(dev);
            if (sc->event) {
                sc->event(dev, I2C_FINISH);
            }

As a static inline, then i2c_end transfer ether calls for current_dev
or does a single-line bodied loop fr bcast.

Here and below.

Regards,
Peter

>      if (!dev) {
>          return;
>      }
> @@ -131,8 +161,22 @@ void i2c_end_transfer(I2CBus *bus)
>
>  int i2c_send(I2CBus *bus, uint8_t data)
>  {
> +    BusChild *kid;
>      I2CSlave *dev = bus->current_dev;
>      I2CSlaveClass *sc;
> +    int ret = 0;
> +
> +    if (bus->broadcast) {
> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
> +            I2CSlave *dev = I2C_SLAVE(kid->child);
> +            sc = I2C_SLAVE_GET_CLASS(dev);
> +            bus->broadcast = true;
> +            if (sc->send) {
> +                ret |= sc->send(dev, data);
> +            }
> +        }
> +        return ret;
> +    }
>
>      if (!dev) {
>          return -1;
> @@ -151,7 +195,7 @@ int i2c_recv(I2CBus *bus)
>      I2CSlave *dev = bus->current_dev;
>      I2CSlaveClass *sc;
>
> -    if (!dev) {
> +    if ((!dev) || (bus->broadcast)) {
>          return -1;
>      }
>
> --
> 1.9.0
>
>