| Message ID | 20181107155405.24013-3-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] i2c:pm_smbus: Fix state transfer | expand |
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? Thanks, Phil. > uint8_t command; > }; > > @@ -93,4 +95,14 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf); > void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, > const uint8_t *eeprom_spd, int size); > > +extern const VMStateDescription vmstate_smbus_device; > + > +#define VMSTATE_SMBUS_DEVICE(_field, _state) { \ > + .name = (stringify(_field)), \ > + .size = sizeof(SMBusDevice), \ > + .vmsd = &vmstate_smbus_device, \ > + .flags = VMS_STRUCT, \ > + .offset = vmstate_offset_value(_state, _field, SMBusDevice), \ > +} > + > #endif >
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. thanks -- PMM
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 >
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]; uint8_t command; }; @@ -93,4 +95,14 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf); void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, const uint8_t *eeprom_spd, int size); +extern const VMStateDescription vmstate_smbus_device; + +#define VMSTATE_SMBUS_DEVICE(_field, _state) { \ + .name = (stringify(_field)), \ + .size = sizeof(SMBusDevice), \ + .vmsd = &vmstate_smbus_device, \ + .flags = VMS_STRUCT, \ + .offset = vmstate_offset_value(_state, _field, SMBusDevice), \ +} + #endif