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

[Philippe Mathieu-Daudé]

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é <address@hidden> 
> 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 <address@hidden>
> 
>>> --- /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
>