On 4/23/20 11:41 AM, Geert Uytterhoeven wrote:
> 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?
See CODING_STYLE.rst:
Include directives
------------------
Order include directives as follows:
.. code-block:: c
#include "qemu/osdep.h" /* Always first... */
#include <...> /* then system headers... */
#include "..." /* and finally QEMU headers. */
The "qemu/osdep.h" header contains preprocessor macros that affect the
behavior
of core system headers like <stdint.h>. It must be the first include so
that
core system headers included by external libraries get the preprocessor
macros
that QEMU depends on.
>
>>
>>> +#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’
Odd, because your backends/gpiodev.c already has:
#include "hw/qdev-core.h"
>
>>> +
>>> +void qemu_gpiodev_add(DeviceState *dev, const char *name, unsigned int maxgpio,
>>> + Error **errp);
>
> Gr{oetje,eeting}s,
>
> Geert
>
