Re: [PATCH 11/17] hw/misc/max111x: Create header file for documentation, TYPE_ macros

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Create a header file for the hw/misc/max111x device, in the
> usual modern style for QOM devices:
>  * definition of the TYPE_ constants and macros
>  * definition of the device's state struct so that it can
>    be embedded in other structs if desired
>  * documentation of the interface
> 
> This allows us to use TYPE_MAX_1111 in the spitz.c code rather
> than the string "max1111".
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> To be honest the main driver here is that I wanted a header
> file to put the documentation comment in :-)
> ---
>  include/hw/misc/max111x.h | 57 +++++++++++++++++++++++++++++++++++++++
>  hw/arm/spitz.c            |  3 ++-
>  hw/misc/max111x.c         | 25 +----------------
>  MAINTAINERS               |  1 +
>  4 files changed, 61 insertions(+), 25 deletions(-)
>  create mode 100644 include/hw/misc/max111x.h
> 
> diff --git a/include/hw/misc/max111x.h b/include/hw/misc/max111x.h
> new file mode 100644
> index 00000000000..337ba63d588
> --- /dev/null
> +++ b/include/hw/misc/max111x.h
> @@ -0,0 +1,57 @@
> +/*
> + * Maxim MAX1110/1111 ADC chip emulation.
> + *
> + * Copyright (c) 2006 Openedhand Ltd.
> + * Written by Andrzej Zaborowski <balrog@zabor.org>
> + *
> + * This code is licensed under the GNU GPLv2.
> + *
> + * Contributions after 2012-01-13 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + */
> +
> +#ifndef HW_MISC_MAX111X_H
> +#define HW_MISC_MAX111X_H
> +
> +#include "hw/ssi/ssi.h"
> +#include "hw/irq.h"

"hw/irq.h" not needed as qemu_irq is forward-declared in
"qemu/typedefs.h".

> +
> +/*
> + * This is a model of the Maxim MAX1110/1111 ADC chip, which for QEMU
> + * is an SSI slave device. It has either 4 (max1110) or 8 (max1111)
> + * 8-bit ADC channels.
> + *
> + * QEMU interface:
> + *  + GPIO inputs 0..3 (for max1110) or 0..7 (for max1111): set the value
> + *    of each ADC input, as an unsigned 8-bit value
> + *  + GPIO output 0: interrupt line
> + *  + Properties "input0" to "input3" (max1110) or "input0" to "input7"
> + *    (max1111): initial reset values for ADC inputs.
> + *
> + * Known bugs:
> + *  + the interrupt line is not correctly implemented, and will never
> + *    be lowered once it has been asserted.
> + */
> +typedef struct {
> +    SSISlave parent_obj;
> +
> +    qemu_irq interrupt;
> +    /* Values of inputs at system reset (settable by QOM property) */
> +    uint8_t reset_input[8];
> +
> +    uint8_t tb1, rb2, rb3;
> +    int cycle;
> +
> +    uint8_t input[8];
> +    int inputs, com;
> +} MAX111xState;
> +
> +#define TYPE_MAX_111X "max111x"
> +
> +#define MAX_111X(obj) \
> +    OBJECT_CHECK(MAX111xState, (obj), TYPE_MAX_111X)

Nitpick, we can keep MAX_111X() + MAX111xState in "hw/misc/max111x.c"
until we get a consumer.

> +
> +#define TYPE_MAX_1110 "max1110"
> +#define TYPE_MAX_1111 "max1111"
> +
> +#endif
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index fa592aad6d6..1400a56729d 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -29,6 +29,7 @@
>  #include "audio/audio.h"
>  #include "hw/boards.h"
>  #include "hw/sysbus.h"
> +#include "hw/misc/max111x.h"
>  #include "migration/vmstate.h"
>  #include "exec/address-spaces.h"
>  #include "cpu.h"
> @@ -732,7 +733,7 @@ static void spitz_ssp_attach(SpitzMachineState *sms)
>                            qdev_get_gpio_in(sms->mpu->gpio, 
> SPITZ_GPIO_TP_INT));
>  
>      bus = qdev_get_child_bus(sms->mux, "ssi2");
> -    sms->max1111 = qdev_new("max1111");
> +    sms->max1111 = qdev_new(TYPE_MAX_1111);
>      max1111 = sms->max1111;
>      qdev_prop_set_uint8(sms->max1111, "input1" /* BATT_VOLT */,
>                          SPITZ_BATTERY_VOLT);
> diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
> index 3a5cb838445..d41cfd92a8d 100644
> --- a/hw/misc/max111x.c
> +++ b/hw/misc/max111x.c
> @@ -11,34 +11,11 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "hw/irq.h"

You still need "hw/irq.h" for qemu_irq_raise().

> -#include "hw/ssi/ssi.h"
> +#include "hw/misc/max111x.h"
>  #include "migration/vmstate.h"
>  #include "qemu/module.h"
>  #include "hw/qdev-properties.h"
>  
> -typedef struct {
> -    SSISlave parent_obj;
> -
> -    qemu_irq interrupt;
> -    /* Values of inputs at system reset (settable by QOM property) */
> -    uint8_t reset_input[8];
> -
> -    uint8_t tb1, rb2, rb3;
> -    int cycle;
> -
> -    uint8_t input[8];
> -    int inputs, com;
> -} MAX111xState;
> -
> -#define TYPE_MAX_111X "max111x"
> -
> -#define MAX_111X(obj) \
> -    OBJECT_CHECK(MAX111xState, (obj), TYPE_MAX_111X)
> -
> -#define TYPE_MAX_1110 "max1110"
> -#define TYPE_MAX_1111 "max1111"
> -
>  /* Control-byte bitfields */
>  #define CB_PD0               (1 << 0)
>  #define CB_PD1               (1 << 1)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1b40446c739..550844d1514 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -787,6 +787,7 @@ F: hw/gpio/max7310.c
>  F: hw/gpio/zaurus.c
>  F: hw/misc/mst_fpga.c
>  F: hw/misc/max111x.c
> +F: include/hw/misc/max111x.h
>  F: include/hw/arm/pxa.h
>  F: include/hw/arm/sharpsl.h
>  F: include/hw/display/tc6393xb.h
>