Re: [PATCH 46/50] lasips2: switch over from update_irq() function to PS2 device gpio

[Mark Cave-Ayland]

On 09/06/2022 12:21, Peter Maydell wrote:

> On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
> > Add a qdev gpio input in lasips2_init() by taking the existing 
> > lasips2_port_set_irq()
> > function, updating it accordingly and then renaming to lasips2_set_irq(). Use 
> > these
> > new qdev gpio inputs to wire up the PS2 keyboard and mouse devices.
> >
> > At the same time set update_irq() and update_arg to NULL in ps2_kbd_init() and
> > ps2_mouse_init() to ensure that any accidental attempt to use the legacy 
> > update_irq()
> > function will cause a NULL pointer dereference.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hw/input/lasips2.c | 17 +++++++++++++----
> >   1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
> > index 644cf70955..12ff95a05f 100644
> > --- a/hw/input/lasips2.c
> > +++ b/hw/input/lasips2.c
> > @@ -117,6 +117,9 @@ static const char *lasips2_write_reg_name(uint64_t addr)
> >       }
> >   }
> >
> > +#define LASIPS2_KBD_INPUT_IRQ      0
> > +#define LASIPS2_MOUSE_INPUT_IRQ    1
> > +
> >   static void lasips2_update_irq(LASIPS2State *s)
> >   {
> >       trace_lasips2_intr(s->kbd.irq | s->mouse.irq);
> > @@ -237,9 +240,10 @@ static const MemoryRegionOps lasips2_reg_ops = {
> >       .endianness = DEVICE_NATIVE_ENDIAN,
> >   };
> >
> > -static void lasips2_port_set_irq(void *opaque, int level)
> > +static void lasips2_set_irq(void *opaque, int n, int level)
> >   {
> > -    LASIPS2Port *port = opaque;
> > +    LASIPS2State *s = LASIPS2(opaque);
> > +    LASIPS2Port *port = (n ? &s->mouse : &s->kbd);
> >
> >       port->irq = level;
> >       lasips2_update_irq(port->parent);
> > @@ -263,8 +267,12 @@ static void lasips2_realize(DeviceState *dev, Error **errp)
> >
> >       vmstate_register(NULL, s->base, &vmstate_lasips2, s);
> >
> > -    s->kbd.dev = ps2_kbd_init(lasips2_port_set_irq, &s->kbd);
> > -    s->mouse.dev = ps2_mouse_init(lasips2_port_set_irq, &s->mouse);
> > +    s->kbd.dev = ps2_kbd_init(NULL, NULL);
> > +    qdev_connect_gpio_out(DEVICE(s->kbd.dev), PS2_DEVICE_IRQ,
> > +                          qdev_get_gpio_in(dev, LASIPS2_KBD_INPUT_IRQ));
> > +    s->mouse.dev = ps2_mouse_init(NULL, NULL);
> > +    qdev_connect_gpio_out(DEVICE(s->mouse.dev), PS2_DEVICE_IRQ,
> > +                          qdev_get_gpio_in(dev, LASIPS2_MOUSE_INPUT_IRQ));
> >   }
> >
> >   static void lasips2_init(Object *obj)
> > @@ -285,6 +293,7 @@ static void lasips2_init(Object *obj)
> >       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
> >
> >       qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
> > +    qdev_init_gpio_in(DEVICE(obj), lasips2_set_irq, 2);
> >   }
>
> These definitely should be named GPIO inputs, as with the pl050.
>
> Aside, if you felt like adding "QEMU interface" comments to these
> devices that summarize what the various sysbus IRQs, MMIOs,
> qdev GPIOs, etc do, that would be nice, and then gives you a
> place to document that these GPIO lines are part of the internal
> implementation rather than externally-facing. include/hw/intc/ppc-uic.h
> is one example but you can find lots by grepping for "QEMU interface"
> in include/hw/.

Oh interesting, I wasn't aware of this. I'll see if I can add a summary of the 
modelling in v2.


ATB,

Mark.