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

[Mark Cave-Ayland]

On 10/06/2022 08:17, Mark Cave-Ayland wrote:

> 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.

Looking at this again, the gpio being defined here actually is the (only) lasips2 
output IRQ, and so should be left unnamed.

The reason for adding LASIPS2_IRQ was so that the gpio connection process 
looked like:

    qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq);

instead of:

    qdev_connect_gpio_out(dev, 0, irq);

Would you still prefer for me to simply hardcode 0 here and drop the LASIPS2_IRQ 
define in this case since there is only one output IRQ?

> > 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.