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.