Re: [PATCH QEMU v2 3/5] Add a GPIO backend using libgpiod

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Philippe,

Thanks for your comments!

On Thu, Apr 23, 2020 at 11:28 AM Philippe Mathieu-Daudé <f4bug@xxxxxxxxx> wrote:
> On 4/23/20 11:01 AM, Geert Uytterhoeven wrote:
> > Add a GPIO controller backend, to connect virtual GPIOs on the guest to
> > physical GPIOs on the host.  This allows the guest to control any
> > external device connected to the physical GPIOs.
> >
> > Features and limitations:
> >   - The backend uses libgpiod on Linux,
> >   - For now only GPIO outputs are supported,
> >   - The number of GPIO lines mapped is limited to the number of GPIO
> >     lines available on the virtual GPIO controller.
> >
> > Future work:
> >   - GPIO inputs,
> >   - GPIO line configuration,
> >   - Optimizations for controlling multiple GPIO lines at once,
> >   - ...
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

> > --- /dev/null
> > +++ b/backends/gpiodev.c
> > @@ -0,0 +1,94 @@
> > +/*
> > + * QEMU GPIO Backend
> > + *
> > + * Copyright (C) 2018-2020 Glider bv
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include <errno.h>
>
> <errno.h> probably not needed.

It is indeed included by one of the other header files.
What is the QEMU policy w.r.t. that?

>
> > +#include <gpiod.h>
>
> Please move this one...
>
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/config-file.h"
> > +#include "qemu/cutils.h"

I forgot to remove the two above...

> > +#include "qemu/error-report.h"
> > +#include "qemu/module.h"
> > +#include "qemu/option.h"

... and the two above.

> > +#include "qapi/error.h"
> > +
> > +#include "sysemu/gpiodev.h"
> > +
> > +#include "hw/irq.h"
> > +#include "hw/qdev-core.h"
>
> ... here:
>
> #include <gpiod.h>

OK.

> > --- a/configure
> > +++ b/configure
> > @@ -509,6 +509,7 @@ libpmem=""
> >  default_devices="yes"
> >  plugins="no"
> >  fuzzing="no"
> > +gpio=""
>
> Maybe name this feature 'libgpiod'?

Makes sense.

> >
> >  supported_cpu="no"
> >  supported_os="no"
> > @@ -1601,6 +1602,10 @@ for opt do
> >    ;;
> >    --gdb=*) gdb_bin="$optarg"
> >    ;;
> > +  --disable-gpio) gpio="no"
> > +  ;;
> > +  --enable-gpio) gpio="yes"
>
> Ditto: --enable-libgpiod, because else it seems rather confusing.

OK.

> > --- /dev/null
> > +++ b/include/sysemu/gpiodev.h
> > @@ -0,0 +1,12 @@
> > +/*
> > + * QEMU GPIO Backend
> > + *
> > + * Copyright (C) 2018-2020 Glider bv
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include "qemu/typedefs.h"
>
> "qemu/typedefs.h" not needed in includes.

While removing that works, it does mean the header file is no longer
self-contained:

include/sysemu/gpiodev.h:10:23: error: unknown type name ‘DeviceState’

> > +
> > +void qemu_gpiodev_add(DeviceState *dev, const char *name, unsigned int maxgpio,
> > +                      Error **errp);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds