qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH V2 2/7] i2c: implement broadcast write.
Date: Mon, 6 Jul 2015 09:54:45 -0700

On Mon, Jul 6, 2015 at 9:28 AM, Frederic Konrad
<address@hidden> wrote:
> On 24/06/2015 08:35, Peter Crosthwaite wrote:
>>
>> On Mon, Jun 15, 2015 at 8:15 AM,  <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.
>>>
>>> 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 object.
>>
>>>       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) {
>>> +            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;
>>> +    }
>>> +
>>>       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;
>>> +    }
>>> +
>>>       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);
>>> +            }
>>> +        }
>>
>> Still not sure about the duped core functionality of each of these
>> APIs. That is, the same code is needed in both a looped form and a 1
>> form. Can this be solved by listifying current_dev? That is, ->current
>> dev is turned into a list which in the normal case will be populated
>> with 1 element by start_transfer() for the current dev. In the
>> broadcast case, all qbus.children are added to the list. The broadcast
>> bool is then removed. start() send() and end_transfer() then just loop
>> through the list unconditionally.
>
>
> I think better keeping this broadcast as we need it for VMSD anyway?
>

Ok I see the VMSD issue and can keep the boolean, but I'm more
concerned about the duped code. It's hard to patch this reliably, if
the same core of code is duped.

Regards,
Peter

>
>> Regards,
>> Peter
>>
>>> +        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
>>>
>>>
>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]