Hi
On Mon, Nov 18, 2019 at 7:17 PM Peter Maydell <[email protected]> wrote:
>
> On Wed, 23 Oct 2019 at 18:34, Marc-André Lureau
> <[email protected]> wrote:
> >
> > Make serial IO a proper sysbus device, similar to serial MM.
> >
> > Signed-off-by: Marc-André Lureau <[email protected]>
> > ---
> > hw/char/serial.c | 58 ++++++++++++++++++++++++++++++++--------
> > include/hw/char/serial.h | 13 +++++++--
> > 2 files changed, 58 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> > index a40bc3ab81..84de2740a7 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -986,22 +986,57 @@ const MemoryRegionOps serial_io_ops = {
> > .endianness = DEVICE_LITTLE_ENDIAN,
> > };
> >
> > -SerialState *serial_init(int base, qemu_irq irq, int baudbase,
> > - Chardev *chr, MemoryRegion *system_io)
> > +static void serial_io_realize(DeviceState *dev, Error **errp)
> > {
> > - DeviceState *dev = DEVICE(object_new(TYPE_SERIAL));
> > - SerialState *s = SERIAL(dev);
> > + SerialIO *self = SERIAL_IO(dev);
>
> "sio" or something rather than "self".
"sio", or something, "s", "si", "serialio"... "self"? Isn't it more
clear that we are talking about the instance itself? No ambiguity,
less weird naming. Isn't this more familar to any OO programmer? Let's
keep the discussion in "serial: start making SerialMM a sysbus
device".
>
> > + SerialState *s = &self->serial;
> >
> > - s->irq = irq;
> > - qdev_prop_set_uint32(dev, "baudbase", baudbase);
> > - qdev_prop_set_chr(dev, "chardev", chr);
> > - qdev_prop_set_int32(dev, "instance-id", base);
> > - qdev_init_nofail(dev);
> > + qdev_init_nofail(DEVICE(s));
> >
> > memory_region_init_io(&s->io, NULL, &serial_io_ops, s, "serial", 8);
> > - memory_region_add_subregion(system_io, base, &s->io);
> > + sysbus_init_irq(SYS_BUS_DEVICE(self), &self->serial.irq);
>
> You could say '&s->irq' here, since you have the local variable.
right, fixed
>
> > +}
> > +
> > +static void serial_io_class_init(ObjectClass *klass, void* data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > + dc->realize = serial_io_realize;
>
> For class methods where the class has no data that needs
> to be migrated it's helpful to have a comment
> /* No dc->vmsd: class has no migratable state */
> (which lets us know that it's intentional and not a forgotten
> thing). Some day I will get round to writing a patch so you
> can say "dc->vmsd = no_migratable_state;" ...
>
added
thanks
> > +}
> > +
> > +static void serial_io_instance_init(Object *o)
> > +{
> > + SerialIO *self = SERIAL_IO(o);
> > +
> > + object_initialize_child(o, "serial", &self->serial,
> > sizeof(self->serial),
> > + TYPE_SERIAL, &error_abort, NULL);
> > +
> > + qdev_alias_all_properties(DEVICE(&self->serial), o);
> > +}
> > +
> > +
> > +static const TypeInfo serial_io_info = {
> > + .name = TYPE_SERIAL_IO,
> > + .parent = TYPE_SYS_BUS_DEVICE,
> > + .instance_size = sizeof(SerialIO),
> > + .instance_init = serial_io_instance_init,
> > + .class_init = serial_io_class_init,
> > +};
> > +
> > +SerialIO *serial_init(int base, qemu_irq irq, int baudbase,
> > + Chardev *chr, MemoryRegion *system_io)
> > +{
> > + SerialIO *self = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO));
> >
> > - return s;
> > + qdev_prop_set_uint32(DEVICE(self), "baudbase", baudbase);
> > + qdev_prop_set_chr(DEVICE(self), "chardev", chr);
> > + qdev_prop_set_int32(DEVICE(self), "instance-id", base);
> > + qdev_init_nofail(DEVICE(self));
> > +
> > + sysbus_connect_irq(SYS_BUS_DEVICE(self), 0, irq);
> > + memory_region_add_subregion(system_io, base, &self->serial.io);
> > +
> > + return self;
> > }
>
> thanks
> -- PMM
>
--
Marc-André Lureau
