| Message ID | 1436020450-1748-1-git-send-email-jcd@tribudubois.net |
|---|---|
| State | New |
| Headers | show |
On Sat, Jul 4, 2015 at 7:34 AM, Jean-Christophe Dubois <jcd@tribudubois.net> wrote: > Move constructor to DEVICE_CLASS methods > * imx_serial_init A nit, but this is not a DEVICE_CLASS thing. > * imx_serial_realize > > Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> > --- > > Changes since v1: > * not present on v1 > > Changes since v2: > * not present on v2 > > Changes since v3: > * not present on v3 > > Changes since v4: > * not present on v4 > > Changes since v5: > * not present on v5 > > Changes since v6: > * not present on v6 > > Changes since v7: > * not present on v7 > > Changes since v8: > * Remove Qdev construction helper > > hw/char/imx_serial.c | 72 +++++++++++++++------------------------------------- > include/hw/arm/imx.h | 2 -- > 2 files changed, 21 insertions(+), 53 deletions(-) > > diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c > index 1dcb325..8059e0f 100644 > --- a/hw/char/imx_serial.c > +++ b/hw/char/imx_serial.c > @@ -21,7 +21,6 @@ > #include "hw/char/imx_serial.h" > #include "sysemu/sysemu.h" > #include "sysemu/char.h" > -#include "hw/arm/imx.h" > > //#define DEBUG_SERIAL 1 > #ifdef DEBUG_SERIAL > @@ -38,13 +37,13 @@ do { printf("imx_serial: " fmt , ##args); } while (0) > //#define DEBUG_IMPLEMENTATION 1 > #ifdef DEBUG_IMPLEMENTATION > # define IPRINTF(fmt, args...) \ > - do { fprintf(stderr, "imx_serial: " fmt, ##args); } while (0) > + do { fprintf(stderr, "%s: " fmt, TYPE_IMX_SERIAL, ##args); } while (0) > #else > # define IPRINTF(fmt, args...) do {} while (0) > #endif > > static const VMStateDescription vmstate_imx_serial = { > - .name = "imx-serial", > + .name = TYPE_IMX_SERIAL, > .version_id = 1, > .minimum_version_id = 1, > .fields = (VMStateField[]) { > @@ -299,22 +298,18 @@ static void imx_event(void *opaque, int event) > } > } > > - > static const struct MemoryRegionOps imx_serial_ops = { > .read = imx_serial_read, > .write = imx_serial_write, > .endianness = DEVICE_NATIVE_ENDIAN, > }; > > -static int imx_serial_init(SysBusDevice *dev) > +static void imx_serial_realize(DeviceState *dev, Error **errp) > { > IMXSerialState *s = IMX_SERIAL(dev); > > - > - memory_region_init_io(&s->iomem, OBJECT(s), &imx_serial_ops, s, > - "imx-serial", 0x1000); > - sysbus_init_mmio(dev, &s->iomem); > - sysbus_init_irq(dev, &s->irq); > + /* FIXME use a qdev chardev prop instead of qemu_char_get_next_serial() */ > + s->chr = qemu_char_get_next_serial(); Is it the case that this prop should exist and _create should set it? (see comment below about _create change). > > if (s->chr) { > qemu_chr_add_handlers(s->chr, imx_can_receive, imx_receive, > @@ -323,45 +318,20 @@ static int imx_serial_init(SysBusDevice *dev) > DPRINTF("No char dev for uart at 0x%lx\n", > (unsigned long)s->iomem.ram_addr); > } > - > - return 0; > } > > -void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq) > +static void imx_serial_init(Object *obj) > { > - DeviceState *dev; > - SysBusDevice *bus; > - CharDriverState *chr; > - const char chr_name[] = "serial"; > - char label[ARRAY_SIZE(chr_name) + 1]; > - > - dev = qdev_create(NULL, TYPE_IMX_SERIAL); > - > - if (uart >= MAX_SERIAL_PORTS) { > - hw_error("Cannot assign uart %d: QEMU supports only %d ports\n", > - uart, MAX_SERIAL_PORTS); > - } > - chr = serial_hds[uart]; > - if (!chr) { > - snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, uart); > - chr = qemu_chr_new(label, "null", NULL); > - if (!(chr)) { > - hw_error("Can't assign serial port to imx-uart%d.\n", uart); > - } > - } > - > - qdev_prop_set_chr(dev, "chardev", chr); > - bus = SYS_BUS_DEVICE(dev); > - qdev_init_nofail(dev); > - if (addr != (hwaddr)-1) { > - sysbus_mmio_map(bus, 0, addr); > - } > - sysbus_connect_irq(bus, 0, irq); > + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > + IMXSerialState *s = IMX_SERIAL(obj); > > + memory_region_init_io(&s->iomem, obj, &imx_serial_ops, s, > + TYPE_IMX_SERIAL, 0x1000); > + sysbus_init_mmio(sbd, &s->iomem); > + sysbus_init_irq(sbd, &s->irq); > } You are trying to do two things here that are reasonably independent. 1: init/realize conversion. 2: remove the _create functions Both are correct. However the two are sequential. Your change for #1 looks good and could be split off and good to go as-is, but you remove the _create function without replacing the logic inline, causing a bisectability issue (is it fixed later for kzm.c)? The removal of _create may have to come later, or split to a sep patch that handles kzm.c change. > > - > -static Property imx32_serial_properties[] = { > +static Property imx_serial_properties[] = { explain change of name in commit message. > DEFINE_PROP_CHR("chardev", IMXSerialState, chr), > DEFINE_PROP_END_OF_LIST(), > }; > @@ -369,21 +339,21 @@ static Property imx32_serial_properties[] = { > static void imx_serial_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > - SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); > > - k->init = imx_serial_init; > + dc->realize = imx_serial_realize; > dc->vmsd = &vmstate_imx_serial; > dc->reset = imx_serial_reset_at_boot; > - set_bit(DEVICE_CATEGORY_INPUT, dc->categories); > dc->desc = "i.MX series UART"; > - dc->props = imx32_serial_properties; > + dc->props = imx_serial_properties; > + set_bit(DEVICE_CATEGORY_INPUT, dc->categories); do you need to reorder statements? > } > > static const TypeInfo imx_serial_info = { > - .name = TYPE_IMX_SERIAL, > - .parent = TYPE_SYS_BUS_DEVICE, > - .instance_size = sizeof(IMXSerialState), > - .class_init = imx_serial_class_init, > + .name = TYPE_IMX_SERIAL, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(IMXSerialState), > + .instance_init = imx_serial_init, > + .class_init = imx_serial_class_init, > }; > > static void imx_serial_register_types(void) > diff --git a/include/hw/arm/imx.h b/include/hw/arm/imx.h > index ea9e093..c39f112 100644 > --- a/include/hw/arm/imx.h > +++ b/include/hw/arm/imx.h > @@ -11,8 +11,6 @@ > #ifndef IMX_H > #define IMX_H > > -void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq); > - This would also go in the later patch. Regards, Peter > typedef enum { > NOCLK, > MCU, > -- > 2.1.4 > >
diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c index 1dcb325..8059e0f 100644 --- a/hw/char/imx_serial.c +++ b/hw/char/imx_serial.c @@ -21,7 +21,6 @@ #include "hw/char/imx_serial.h" #include "sysemu/sysemu.h" #include "sysemu/char.h" -#include "hw/arm/imx.h" //#define DEBUG_SERIAL 1 #ifdef DEBUG_SERIAL @@ -38,13 +37,13 @@ do { printf("imx_serial: " fmt , ##args); } while (0) //#define DEBUG_IMPLEMENTATION 1 #ifdef DEBUG_IMPLEMENTATION # define IPRINTF(fmt, args...) \ - do { fprintf(stderr, "imx_serial: " fmt, ##args); } while (0) + do { fprintf(stderr, "%s: " fmt, TYPE_IMX_SERIAL, ##args); } while (0) #else # define IPRINTF(fmt, args...) do {} while (0) #endif static const VMStateDescription vmstate_imx_serial = { - .name = "imx-serial", + .name = TYPE_IMX_SERIAL, .version_id = 1, .minimum_version_id = 1, .fields = (VMStateField[]) { @@ -299,22 +298,18 @@ static void imx_event(void *opaque, int event) } } - static const struct MemoryRegionOps imx_serial_ops = { .read = imx_serial_read, .write = imx_serial_write, .endianness = DEVICE_NATIVE_ENDIAN, }; -static int imx_serial_init(SysBusDevice *dev) +static void imx_serial_realize(DeviceState *dev, Error **errp) { IMXSerialState *s = IMX_SERIAL(dev); - - memory_region_init_io(&s->iomem, OBJECT(s), &imx_serial_ops, s, - "imx-serial", 0x1000); - sysbus_init_mmio(dev, &s->iomem); - sysbus_init_irq(dev, &s->irq); + /* FIXME use a qdev chardev prop instead of qemu_char_get_next_serial() */ + s->chr = qemu_char_get_next_serial(); if (s->chr) { qemu_chr_add_handlers(s->chr, imx_can_receive, imx_receive, @@ -323,45 +318,20 @@ static int imx_serial_init(SysBusDevice *dev) DPRINTF("No char dev for uart at 0x%lx\n", (unsigned long)s->iomem.ram_addr); } - - return 0; } -void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq) +static void imx_serial_init(Object *obj) { - DeviceState *dev; - SysBusDevice *bus; - CharDriverState *chr; - const char chr_name[] = "serial"; - char label[ARRAY_SIZE(chr_name) + 1]; - - dev = qdev_create(NULL, TYPE_IMX_SERIAL); - - if (uart >= MAX_SERIAL_PORTS) { - hw_error("Cannot assign uart %d: QEMU supports only %d ports\n", - uart, MAX_SERIAL_PORTS); - } - chr = serial_hds[uart]; - if (!chr) { - snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, uart); - chr = qemu_chr_new(label, "null", NULL); - if (!(chr)) { - hw_error("Can't assign serial port to imx-uart%d.\n", uart); - } - } - - qdev_prop_set_chr(dev, "chardev", chr); - bus = SYS_BUS_DEVICE(dev); - qdev_init_nofail(dev); - if (addr != (hwaddr)-1) { - sysbus_mmio_map(bus, 0, addr); - } - sysbus_connect_irq(bus, 0, irq); + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); + IMXSerialState *s = IMX_SERIAL(obj); + memory_region_init_io(&s->iomem, obj, &imx_serial_ops, s, + TYPE_IMX_SERIAL, 0x1000); + sysbus_init_mmio(sbd, &s->iomem); + sysbus_init_irq(sbd, &s->irq); } - -static Property imx32_serial_properties[] = { +static Property imx_serial_properties[] = { DEFINE_PROP_CHR("chardev", IMXSerialState, chr), DEFINE_PROP_END_OF_LIST(), }; @@ -369,21 +339,21 @@ static Property imx32_serial_properties[] = { static void imx_serial_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); - SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); - k->init = imx_serial_init; + dc->realize = imx_serial_realize; dc->vmsd = &vmstate_imx_serial; dc->reset = imx_serial_reset_at_boot; - set_bit(DEVICE_CATEGORY_INPUT, dc->categories); dc->desc = "i.MX series UART"; - dc->props = imx32_serial_properties; + dc->props = imx_serial_properties; + set_bit(DEVICE_CATEGORY_INPUT, dc->categories); } static const TypeInfo imx_serial_info = { - .name = TYPE_IMX_SERIAL, - .parent = TYPE_SYS_BUS_DEVICE, - .instance_size = sizeof(IMXSerialState), - .class_init = imx_serial_class_init, + .name = TYPE_IMX_SERIAL, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(IMXSerialState), + .instance_init = imx_serial_init, + .class_init = imx_serial_class_init, }; static void imx_serial_register_types(void) diff --git a/include/hw/arm/imx.h b/include/hw/arm/imx.h index ea9e093..c39f112 100644 --- a/include/hw/arm/imx.h +++ b/include/hw/arm/imx.h @@ -11,8 +11,6 @@ #ifndef IMX_H #define IMX_H -void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq); - typedef enum { NOCLK, MCU,
Move constructor to DEVICE_CLASS methods * imx_serial_init * imx_serial_realize Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> --- Changes since v1: * not present on v1 Changes since v2: * not present on v2 Changes since v3: * not present on v3 Changes since v4: * not present on v4 Changes since v5: * not present on v5 Changes since v6: * not present on v6 Changes since v7: * not present on v7 Changes since v8: * Remove Qdev construction helper hw/char/imx_serial.c | 72 +++++++++++++++------------------------------------- include/hw/arm/imx.h | 2 -- 2 files changed, 21 insertions(+), 53 deletions(-)