| Message ID | 20191120152442.26657-32-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On Wed, Nov 20, 2019 at 07:24:36PM +0400, Marc-André Lureau wrote: > Instead, set the initial data field directly. > > (the initial data is an array of 256 bytes. As I don't know if it may > change over time, I keep the pointer to original buffer as is, but it > might be worth to consider to copy it instead) This looks ok to me, I can add it to my tree. I think we are in hard freeze now, so this will have to wait until that's done. -corey > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/i2c/smbus_eeprom.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > index 54c86a0112..533c728b3b 100644 > --- a/hw/i2c/smbus_eeprom.c > +++ b/hw/i2c/smbus_eeprom.c > @@ -44,7 +44,7 @@ > typedef struct SMBusEEPROMDevice { > SMBusDevice smbusdev; > uint8_t data[SMBUS_EEPROM_SIZE]; > - void *init_data; > + uint8_t *init_data; > uint8_t offset; > bool accessed; > } SMBusEEPROMDevice; > @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev) > > static void smbus_eeprom_realize(DeviceState *dev, Error **errp) > { > + SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); > + > smbus_eeprom_reset(dev); > + if (eeprom->init_data == NULL) { > + error_setg(errp, "init_data cannot be NULL"); > + } > } > > -static Property smbus_eeprom_properties[] = { > - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), > - DEFINE_PROP_END_OF_LIST(), > -}; > - > static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > dc->reset = smbus_eeprom_reset; > sc->receive_byte = eeprom_receive_byte; > sc->write_data = eeprom_write_data; > - dc->props = smbus_eeprom_properties; > dc->vmsd = &vmstate_smbus_eeprom; > - /* Reason: pointer property "data" */ > + /* Reason: init_data */ > dc->user_creatable = false; > } > > @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) > > dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM); > qdev_prop_set_uint8(dev, "address", address); > - qdev_prop_set_ptr(dev, "data", eeprom_buf); > + SMBUS_EEPROM(dev)->init_data = eeprom_buf; > qdev_init_nofail(dev); > } > > -- > 2.24.0 >
On 11/20/19 4:24 PM, Marc-André Lureau wrote: > Instead, set the initial data field directly. > > (the initial data is an array of 256 bytes. As I don't know if it may > change over time, I keep the pointer to original buffer as is, but it > might be worth to consider to copy it instead) > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/i2c/smbus_eeprom.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > index 54c86a0112..533c728b3b 100644 > --- a/hw/i2c/smbus_eeprom.c > +++ b/hw/i2c/smbus_eeprom.c > @@ -44,7 +44,7 @@ > typedef struct SMBusEEPROMDevice { > SMBusDevice smbusdev; > uint8_t data[SMBUS_EEPROM_SIZE]; > - void *init_data; > + uint8_t *init_data; > uint8_t offset; > bool accessed; > } SMBusEEPROMDevice; > @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev) > > static void smbus_eeprom_realize(DeviceState *dev, Error **errp) > { > + SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); > + > smbus_eeprom_reset(dev); > + if (eeprom->init_data == NULL) { > + error_setg(errp, "init_data cannot be NULL"); > + } > } > > -static Property smbus_eeprom_properties[] = { > - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), > - DEFINE_PROP_END_OF_LIST(), > -}; > - > static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > dc->reset = smbus_eeprom_reset; > sc->receive_byte = eeprom_receive_byte; > sc->write_data = eeprom_write_data; > - dc->props = smbus_eeprom_properties; > dc->vmsd = &vmstate_smbus_eeprom; > - /* Reason: pointer property "data" */ > + /* Reason: init_data */ > dc->user_creatable = false; > } > > @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) > > dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM); > qdev_prop_set_uint8(dev, "address", address); > - qdev_prop_set_ptr(dev, "data", eeprom_buf); > + SMBUS_EEPROM(dev)->init_data = eeprom_buf; Isn't this a QOM anti-pattern? > qdev_init_nofail(dev); > } > >
On Fri, Nov 22, 2019 at 3:07 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 11/20/19 4:24 PM, Marc-André Lureau wrote: > > Instead, set the initial data field directly. > > > > (the initial data is an array of 256 bytes. As I don't know if it may > > change over time, I keep the pointer to original buffer as is, but it > > might be worth to consider to copy it instead) > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/i2c/smbus_eeprom.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > > index 54c86a0112..533c728b3b 100644 > > --- a/hw/i2c/smbus_eeprom.c > > +++ b/hw/i2c/smbus_eeprom.c > > @@ -44,7 +44,7 @@ > > typedef struct SMBusEEPROMDevice { > > SMBusDevice smbusdev; > > uint8_t data[SMBUS_EEPROM_SIZE]; > > - void *init_data; > > + uint8_t *init_data; > > uint8_t offset; > > bool accessed; > > } SMBusEEPROMDevice; > > @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev) > > > > static void smbus_eeprom_realize(DeviceState *dev, Error **errp) > > { > > + SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); > > + > > smbus_eeprom_reset(dev); > > + if (eeprom->init_data == NULL) { > > + error_setg(errp, "init_data cannot be NULL"); > > + } > > } > > > > -static Property smbus_eeprom_properties[] = { > > - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), > > - DEFINE_PROP_END_OF_LIST(), > > -}; > > - > > static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > > dc->reset = smbus_eeprom_reset; > > sc->receive_byte = eeprom_receive_byte; > > sc->write_data = eeprom_write_data; > > - dc->props = smbus_eeprom_properties; > > dc->vmsd = &vmstate_smbus_eeprom; > > - /* Reason: pointer property "data" */ > > + /* Reason: init_data */ > > dc->user_creatable = false; > > } > > > > @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) > > > > dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM); > > qdev_prop_set_uint8(dev, "address", address); > > - qdev_prop_set_ptr(dev, "data", eeprom_buf); > > + SMBUS_EEPROM(dev)->init_data = eeprom_buf; > > Isn't this a QOM anti-pattern? Sort of, but using prop_ptr in the first place is an anti-pattern. So I am not improving the situation much here, but just getting rid of prop_ptr usage. Using an array of bytes instead could be done in a follow-up patch. I can leave a fixme if that helps. > > > qdev_init_nofail(dev); > > } > > > > >
On 11/22/19 12:12 PM, Marc-André Lureau wrote: > On Fri, Nov 22, 2019 at 3:07 PM Philippe Mathieu-Daudé > <philmd@redhat.com> wrote: >> >> On 11/20/19 4:24 PM, Marc-André Lureau wrote: >>> Instead, set the initial data field directly. >>> >>> (the initial data is an array of 256 bytes. As I don't know if it may >>> change over time, I keep the pointer to original buffer as is, but it >>> might be worth to consider to copy it instead) >>> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> --- >>> hw/i2c/smbus_eeprom.c | 17 ++++++++--------- >>> 1 file changed, 8 insertions(+), 9 deletions(-) >>> >>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >>> index 54c86a0112..533c728b3b 100644 >>> --- a/hw/i2c/smbus_eeprom.c >>> +++ b/hw/i2c/smbus_eeprom.c >>> @@ -44,7 +44,7 @@ >>> typedef struct SMBusEEPROMDevice { >>> SMBusDevice smbusdev; >>> uint8_t data[SMBUS_EEPROM_SIZE]; >>> - void *init_data; >>> + uint8_t *init_data; >>> uint8_t offset; >>> bool accessed; >>> } SMBusEEPROMDevice; >>> @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev) >>> >>> static void smbus_eeprom_realize(DeviceState *dev, Error **errp) >>> { >>> + SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); >>> + >>> smbus_eeprom_reset(dev); >>> + if (eeprom->init_data == NULL) { >>> + error_setg(errp, "init_data cannot be NULL"); >>> + } >>> } >>> >>> -static Property smbus_eeprom_properties[] = { >>> - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), >>> - DEFINE_PROP_END_OF_LIST(), >>> -}; >>> - >>> static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) >>> { >>> DeviceClass *dc = DEVICE_CLASS(klass); >>> @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) >>> dc->reset = smbus_eeprom_reset; >>> sc->receive_byte = eeprom_receive_byte; >>> sc->write_data = eeprom_write_data; >>> - dc->props = smbus_eeprom_properties; >>> dc->vmsd = &vmstate_smbus_eeprom; >>> - /* Reason: pointer property "data" */ >>> + /* Reason: init_data */ >>> dc->user_creatable = false; >>> } >>> >>> @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) >>> >>> dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM); >>> qdev_prop_set_uint8(dev, "address", address); >>> - qdev_prop_set_ptr(dev, "data", eeprom_buf); >>> + SMBUS_EEPROM(dev)->init_data = eeprom_buf; >> >> Isn't this a QOM anti-pattern? > > Sort of, but using prop_ptr in the first place is an anti-pattern. So > I am not improving the situation much here, but just getting rid of > prop_ptr usage. Alright. > Using an array of bytes instead could be done in a follow-up patch. I > can leave a fixme if that helps. IIRC there was a discussion earlier this year about using block backend for this device. >> >>> qdev_init_nofail(dev); >>> } >>> >>> >> >
Hi Corey On Fri, Nov 22, 2019 at 2:44 AM Corey Minyard <cminyard@mvista.com> wrote: > > On Wed, Nov 20, 2019 at 07:24:36PM +0400, Marc-André Lureau wrote: > > Instead, set the initial data field directly. > > > > (the initial data is an array of 256 bytes. As I don't know if it may > > change over time, I keep the pointer to original buffer as is, but it > > might be worth to consider to copy it instead) > > This looks ok to me, I can add it to my tree. I think we are in hard > freeze now, so this will have to wait until that's done. Thanks, I think I would rather keep it in this series, as the goal is to remove PROP_PTR, and tracking various subsystems will be tedious. Instead, we can hopefully do it in one go. Does it get your reviewed-by then? > > -corey > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/i2c/smbus_eeprom.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > > index 54c86a0112..533c728b3b 100644 > > --- a/hw/i2c/smbus_eeprom.c > > +++ b/hw/i2c/smbus_eeprom.c > > @@ -44,7 +44,7 @@ > > typedef struct SMBusEEPROMDevice { > > SMBusDevice smbusdev; > > uint8_t data[SMBUS_EEPROM_SIZE]; > > - void *init_data; > > + uint8_t *init_data; > > uint8_t offset; > > bool accessed; > > } SMBusEEPROMDevice; > > @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev) > > > > static void smbus_eeprom_realize(DeviceState *dev, Error **errp) > > { > > + SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); > > + > > smbus_eeprom_reset(dev); > > + if (eeprom->init_data == NULL) { > > + error_setg(errp, "init_data cannot be NULL"); > > + } > > } > > > > -static Property smbus_eeprom_properties[] = { > > - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), > > - DEFINE_PROP_END_OF_LIST(), > > -}; > > - > > static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > > dc->reset = smbus_eeprom_reset; > > sc->receive_byte = eeprom_receive_byte; > > sc->write_data = eeprom_write_data; > > - dc->props = smbus_eeprom_properties; > > dc->vmsd = &vmstate_smbus_eeprom; > > - /* Reason: pointer property "data" */ > > + /* Reason: init_data */ > > dc->user_creatable = false; > > } > > > > @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) > > > > dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM); > > qdev_prop_set_uint8(dev, "address", address); > > - qdev_prop_set_ptr(dev, "data", eeprom_buf); > > + SMBUS_EEPROM(dev)->init_data = eeprom_buf; > > qdev_init_nofail(dev); > > } > > > > -- > > 2.24.0 > > >
On Fri, Nov 29, 2019 at 01:06:41PM +0400, Marc-André Lureau wrote: > Hi Corey > > On Fri, Nov 22, 2019 at 2:44 AM Corey Minyard <cminyard@mvista.com> wrote: > > > > On Wed, Nov 20, 2019 at 07:24:36PM +0400, Marc-André Lureau wrote: > > > Instead, set the initial data field directly. > > > > > > (the initial data is an array of 256 bytes. As I don't know if it may > > > change over time, I keep the pointer to original buffer as is, but it > > > might be worth to consider to copy it instead) > > > > This looks ok to me, I can add it to my tree. I think we are in hard > > freeze now, so this will have to wait until that's done. > > Thanks, I think I would rather keep it in this series, as the goal is > to remove PROP_PTR, and tracking various subsystems will be tedious. > Instead, we can hopefully do it in one go. > > Does it get your reviewed-by then? Yes, I thought I had done that. Reviewed-by: Corey Minyard <cminyard@mvista.com> > > > > > -corey > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > --- > > > hw/i2c/smbus_eeprom.c | 17 ++++++++--------- > > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > > > index 54c86a0112..533c728b3b 100644 > > > --- a/hw/i2c/smbus_eeprom.c > > > +++ b/hw/i2c/smbus_eeprom.c > > > @@ -44,7 +44,7 @@ > > > typedef struct SMBusEEPROMDevice { > > > SMBusDevice smbusdev; > > > uint8_t data[SMBUS_EEPROM_SIZE]; > > > - void *init_data; > > > + uint8_t *init_data; > > > uint8_t offset; > > > bool accessed; > > > } SMBusEEPROMDevice; > > > @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev) > > > > > > static void smbus_eeprom_realize(DeviceState *dev, Error **errp) > > > { > > > + SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); > > > + > > > smbus_eeprom_reset(dev); > > > + if (eeprom->init_data == NULL) { > > > + error_setg(errp, "init_data cannot be NULL"); > > > + } > > > } > > > > > > -static Property smbus_eeprom_properties[] = { > > > - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), > > > - DEFINE_PROP_END_OF_LIST(), > > > -}; > > > - > > > static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > > > { > > > DeviceClass *dc = DEVICE_CLASS(klass); > > > @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > > > dc->reset = smbus_eeprom_reset; > > > sc->receive_byte = eeprom_receive_byte; > > > sc->write_data = eeprom_write_data; > > > - dc->props = smbus_eeprom_properties; > > > dc->vmsd = &vmstate_smbus_eeprom; > > > - /* Reason: pointer property "data" */ > > > + /* Reason: init_data */ > > > dc->user_creatable = false; > > > } > > > > > > @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) > > > > > > dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM); > > > qdev_prop_set_uint8(dev, "address", address); > > > - qdev_prop_set_ptr(dev, "data", eeprom_buf); > > > + SMBUS_EEPROM(dev)->init_data = eeprom_buf; > > > qdev_init_nofail(dev); > > > } > > > > > > -- > > > 2.24.0 > > > > > > > > -- > Marc-André Lureau
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index 54c86a0112..533c728b3b 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -44,7 +44,7 @@ typedef struct SMBusEEPROMDevice { SMBusDevice smbusdev; uint8_t data[SMBUS_EEPROM_SIZE]; - void *init_data; + uint8_t *init_data; uint8_t offset; bool accessed; } SMBusEEPROMDevice; @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev) static void smbus_eeprom_realize(DeviceState *dev, Error **errp) { + SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); + smbus_eeprom_reset(dev); + if (eeprom->init_data == NULL) { + error_setg(errp, "init_data cannot be NULL"); + } } -static Property smbus_eeprom_properties[] = { - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), - DEFINE_PROP_END_OF_LIST(), -}; - static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) dc->reset = smbus_eeprom_reset; sc->receive_byte = eeprom_receive_byte; sc->write_data = eeprom_write_data; - dc->props = smbus_eeprom_properties; dc->vmsd = &vmstate_smbus_eeprom; - /* Reason: pointer property "data" */ + /* Reason: init_data */ dc->user_creatable = false; } @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf) dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM); qdev_prop_set_uint8(dev, "address", address); - qdev_prop_set_ptr(dev, "data", eeprom_buf); + SMBUS_EEPROM(dev)->init_data = eeprom_buf; qdev_init_nofail(dev); }
Instead, set the initial data field directly. (the initial data is an array of 256 bytes. As I don't know if it may change over time, I keep the pointer to original buffer as is, but it might be worth to consider to copy it instead) Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/i2c/smbus_eeprom.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)