Re: [PATCH v3 13/33] serial: start making SerialMM a sysbus device

[Marc-André Lureau]

Hi

On Mon, Nov 18, 2019 at 6:43 PM Peter Maydell <address@hidden> wrote:
>
> On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau
> <address@hidden> wrote:
> >
> > Memory mapped serial device is in fact a sysbus device. The following
> > patches will make use of sysbus facilities for resource and
> > registration.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  hw/char/omap_uart.c      |  2 +-
> >  hw/char/serial.c         | 47 ++++++++++++++++++++++++++++------------
> >  hw/mips/boston.c         |  2 +-
> >  hw/mips/mips_malta.c     |  2 +-
> >  include/hw/char/serial.h | 20 ++++++++++++-----
> >  5 files changed, 51 insertions(+), 22 deletions(-)
>
>
> > -SerialState *serial_mm_init(MemoryRegion *address_space,
> > +SerialMM *serial_mm_init(MemoryRegion *address_space,
> >                              hwaddr base, int it_shift,
> >                              qemu_irq irq, int baudbase,
> >                              Chardev *chr, enum device_endian end)
> >  {
> > -    DeviceState *dev = DEVICE(object_new(TYPE_SERIAL));
> > -    SerialState *s = SERIAL(dev);
> > +    SerialMM *self = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM));
> > +    SerialState *s = &self->serial;
> >
> > -    s->it_shift = it_shift;
> > +    self->it_shift = it_shift;
> >      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_prop_set_uint32(DEVICE(s), "baudbase", baudbase);
> > +    qdev_prop_set_chr(DEVICE(s), "chardev", chr);
> > +    qdev_prop_set_int32(DEVICE(s), "instance-id", base);
> > +
> > +    qdev_init_nofail(DEVICE(s));
> > +    qdev_init_nofail(DEVICE(self));
>
> Something odd is going on here. This is a convenience
> wrapper around creating the SERIAL_MM device, so it's
> correct that it has to init DEVICE(self). But it should
> not be doing anything with the internals of 'self'.
> It's the instance_init/realize of the SERIAL_MM object that should
> instance_init/realize the 'self->serial' object. You have the
> code below to do the 'instance_init' in the serial_mm_instance_init
> function, but are missing the equivalent realize code.

This should be addressed later with "serial-mm: use sysbus
facilities". I'll add a comment.

>
> > -    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s,
> > +    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], self,
> >                            "serial", 8 << it_shift);
> >      memory_region_add_subregion(address_space, base, &s->io);
> > -    return s;
> > +
> > +    return self;
> > +}
> > +
> > +static void serial_mm_instance_init(Object *o)
> > +{
> > +    SerialMM *self = SERIAL_MM(o);
>
> 'self' is not idiomatic for the name of the variable containing
> the pointer to the object in QOM code ("git grep '\Wself\W' hw"
> shows no uses of it at all, which is quite unusual for us --
> usually the codebase has at least a few uses of any non-standard
> way of writing something ;-))
>
> Usually we use something approximating to the abbreviation
> of the type name, so here 'smm' would do.

I would prefer something more straightforward than having to come up
with various name mangling.

Imho, we loose some readability, consistency & semantic by not naming
the object argument of the method "self"

>
> > +
> > +    object_initialize_child(o, "serial", &self->serial, 
> > sizeof(self->serial),
> > +                            TYPE_SERIAL, &error_abort, NULL);
> >  }
>
> thanks
> -- PMM
>