Re: [Qemu-devel] [PATCH 2/3] i2c: Add an SMBus vmstate structure

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 8/11/18 15:40, Peter Maydell wrote:
> On 8 November 2018 at 14:23, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Hi Corey,
>>
>>
>> On 7/11/18 16:54, minyard@acm.org wrote:
>>>
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> There is no vmstate handling for SMBus, so no device sitting on SMBus
>>> can have a state transfer that works reliable.  So add it.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> ---
>>>    hw/i2c/smbus.c         | 14 ++++++++++++++
>>>    include/hw/i2c/smbus.h | 18 +++++++++++++++---
>>>    2 files changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
>>> index 6ff77c582f..b0774d7683 100644
>>> --- a/hw/i2c/smbus.c
>>> +++ b/hw/i2c/smbus.c
>>> @@ -349,6 +349,20 @@ int smbus_write_block(I2CBus *bus, uint8_t addr,
>>> uint8_t command, uint8_t *data,
>>>        return 0;
>>>    }
>>>    +const VMStateDescription vmstate_smbus_device = {
>>> +    .name = TYPE_SMBUS_DEVICE,
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .fields      = (VMStateField[]) {
>>> +        VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
>>> +        VMSTATE_INT32(mode, SMBusDevice),
>>> +        VMSTATE_INT32(data_len, SMBusDevice),
>>> +        VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
>>> +        VMSTATE_UINT8(command, SMBusDevice),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>>    static void smbus_device_class_init(ObjectClass *klass, void *data)
>>>    {
>>>        I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
>>> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
>>> index d8b1b9ee81..7b52020121 100644
>>> --- a/include/hw/i2c/smbus.h
>>> +++ b/include/hw/i2c/smbus.h
>>> @@ -53,14 +53,16 @@ typedef struct SMBusDeviceClass
>>>        uint8_t (*read_data)(SMBusDevice *dev, uint8_t cmd, int n);
>>>    } SMBusDeviceClass;
>>>    +#define SMBUS_DATA_MAX_LEN 34  /* command + len + 32 bytes of data.  */
>>> +
>>>    struct SMBusDevice {
>>>        /* The SMBus protocol is implemented on top of I2C.  */
>>>        I2CSlave i2c;
>>>          /* Remaining fields for internal use only.  */
>>> -    int mode;
>>> -    int data_len;
>>> -    uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
>>> +    int32_t mode;
>>> +    int32_t data_len;
>>> +    uint8_t data_buf[SMBUS_DATA_MAX_LEN];
>>
>>
>> Those changes are not in your commit description.
>> Can you include them in a separate patch?
> 
> These are just the standard "promote types in the state struct
> to fixed-width ones required by the VMSTATE macros", aren't
> they? I think they're fine as part of the vmstate conversion.

Fine then!

> 
> thanks
> -- PMM
>