Hi
On Mon, Nov 18, 2019 at 6:43 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau
> <marcandre.lur...@redhat.com> 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 <marcandre.lur...@redhat.com>
> > ---
> > 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
>
