Re: [PATCH v3 17/33] serial: make SerialIO a sysbus device

[Marc-André Lureau]

Hi

On Mon, Nov 18, 2019 at 7:17 PM Peter Maydell <address@hidden> wrote:
>
> On Wed, 23 Oct 2019 at 18:34, Marc-André Lureau
> <address@hidden> wrote:
> >
> > Make serial IO a proper sysbus device, similar to serial MM.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  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