Hi Philippe,
Thanks for your comments!
On Thu, Apr 23, 2020 at 11:28 AM Philippe Mathieu-Daudé <f4...@amsat.org> 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+rene...@glider.be>
> > --- /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 -- ge...@linux-m68k.org
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
