Re: [PATCH 45/50] lasips2: use qdev gpio for output IRQ

[Mark Cave-Ayland]

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

> On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
> > This enables the IRQ to be wired up using qdev_connect_gpio_out() in
> > lasips2_initfn().
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hw/input/lasips2.c         | 8 ++++----
> >   include/hw/input/lasips2.h | 2 ++
> >   2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
> > index 6849b71e5c..644cf70955 100644
> > --- a/hw/input/lasips2.c
> > +++ b/hw/input/lasips2.c
> > @@ -247,16 +247,14 @@ static void lasips2_port_set_irq(void *opaque, int level)
> >
> >   LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq)
> >   {
> > -    LASIPS2State *s;
> >       DeviceState *dev;
> >
> >       dev = qdev_new(TYPE_LASIPS2);
> >       qdev_prop_set_uint64(dev, "base", base);
> >       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > -    s = LASIPS2(dev);
> > +    qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq);
> >
> > -    s->irq = irq;
> > -    return s;
> > +    return LASIPS2(dev);
> >   }
> >
> >   static void lasips2_realize(DeviceState *dev, Error **errp)
> > @@ -285,6 +283,8 @@ static void lasips2_init(Object *obj)
> >
> >       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->kbd.reg);
> >       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
> > +
> > +    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
> >   }
> >
> >   static Property lasips2_properties[] = {
> > diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h
> > index 7e4437b925..d3e9719d65 100644
> > --- a/include/hw/input/lasips2.h
> > +++ b/include/hw/input/lasips2.h
> > @@ -22,6 +22,8 @@ typedef struct LASIPS2Port {
> >       bool irq;
> >   } LASIPS2Port;
> >
> > +#define LASIPS2_IRQ    0
>
> If you find yourself #defining names for IRQ lines then this is
> probably a sign you should be using named GPIO lines :-)

Yeah that's something I've done a few times here, mainly to have just one "set IRQ" 
function rather a separate one for both keyboard and mouse. It takes a bit more work, 
but I can certainly separate them out.

> Alternatively, maybe use sysbus_init_irq()? By convention the
> only sysbus IRQ on a device is generally "its IRQ line".

Thinking longer term about sysbus, I can see that sysbus_init_irq() would be one of 
the top entries on my list of things to go. For that reason I'd like to stick to 
using gpios here :)


ATB,

Mark.