[PATCH 00/17] spitz: fix hacks, fix CID 1421913, various cleanups

[PATCH 00/17] spitz: fix hacks, fix CID 1421913, various cleanups

[Peter Maydell]

This series of patches makes various cleanups to the spitz board
family code; the main driver here was fixing the minor Coverity
nit CID 1421913, which is a complaint that the call to
qemu_allocate_irqs() creates memory that is leaked.

We fix this by replacing the free-standing irq array and
callback function with a proper QOM device TYPE_SPITZ_MISC_GPIO,
which can have its own GPIO inputs and outputs which we can wire
up as appropriate. This also allows us to remove the ugly
file-scope variables that pointed to some of the devices on the
board so that the old callback function could prod them.

For this to work we need to add QOM properties and input GPIOs
to the max111x ADC devices so that we can control them that
way rather than by direct calls to max111x_set_input().
While we're in that bit of old code we take the opportunity to
get rid of its call to vmstate_register() and to give it a reset
method and a header file so we can document it a bit better.

The last few patches are unrelated cleanup that I noticed in
passing: we reduce the use of the zaurus_printf() macro in favour
of LOG_GUEST_ERROR logging for bad register accesess, and we
get rid of the old FROM_SSI_SLAVE which can be replaced with
QOM cast macros.

Patch 1 removes the hardcoded tabs in spitz.c, because they've
escaped our usual "fix as we touch a file" policy long enough,
and it's easier to do a wholesale detabify of the file before
starting.

As you review this series you might notice some other things in the
code that could also be cleaned up; so did I, but I felt that
17 patches was quite enough to be going on with :-)

thanks
-- PMM

Peter Maydell (17):
  hw/arm/spitz: Detabify
  hw/arm/spitz: Create SpitzMachineClass abstract base class
  hw/arm/spitz: Keep pointers to MPU and SSI devices in
    SpitzMachineState
  hw/arm/spitz: Keep pointers to scp0, scp1 in SpitzMachineState
  hw/arm/spitz: Implement inbound GPIO lines for bit5 and power signals
  hw/misc/max111x: provide QOM properties for setting initial values
  hw/misc/max111x: Don't use vmstate_register()
  ssi: Add ssi_realize_and_unref()
  hw/arm/spitz: Use max111x properties to set initial values
  hw/misc/max111x: Use GPIO lines rather than max111x_set_input()
  hw/misc/max111x: Create header file for documentation, TYPE_ macros
  hw/arm/spitz: Encapsulate misc GPIO handling in a device
  hw/gpio/zaurus.c: Use LOG_GUEST_ERROR for bad guest register accesses
  hw/arm/spitz: Use LOG_GUEST_ERROR for bad guest register accesses
  hw/arm/pxa2xx_pic: Use LOG_GUEST_ERROR for bad guest register accesses
  hw/arm/spitz: Provide usual QOM macros for corgi-ssp and spitz-lcdtg
  Replace uses of FROM_SSI_SLAVE() macro with QOM casts

 include/hw/arm/pxa.h      |   1 -
 include/hw/arm/sharpsl.h  |   3 -
 include/hw/misc/max111x.h |  57 +++++
 include/hw/ssi/ssi.h      |  31 ++-
 hw/arm/pxa2xx_pic.c       |   9 +-
 hw/arm/spitz.c            | 507 ++++++++++++++++++++++----------------
 hw/arm/z2.c               |  11 +-
 hw/display/ads7846.c      |   9 +-
 hw/display/ssd0323.c      |  10 +-
 hw/gpio/zaurus.c          |  12 +-
 hw/misc/max111x.c         |  87 ++++---
 hw/sd/ssi-sd.c            |   4 +-
 hw/ssi/ssi.c              |   7 +-
 MAINTAINERS               |   1 +
 14 files changed, 474 insertions(+), 275 deletions(-)
 create mode 100644 include/hw/misc/max111x.h

-- 
2.20.1

[PATCH 01/17] hw/arm/spitz: Detabify

[Peter Maydell]

The spitz board has been around a long time, and still has a fair number
of hard-coded tab characters in it. We're about to do some work on
this source file, so start out by expanding out the tabs.

This commit is a pure whitespace only change.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Couple of checkpatch errors due to the QUEUE_KEY macro which can
be ignored as this is just a detabify.
---
 hw/arm/spitz.c | 156 ++++++++++++++++++++++++-------------------------
 1 file changed, 78 insertions(+), 78 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index fc18212e686..9eaedab79b5 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -34,25 +34,25 @@
 #include "cpu.h"
 
 #undef REG_FMT
-#define REG_FMT			"0x%02lx"
+#define REG_FMT                         "0x%02lx"
 
 /* Spitz Flash */
-#define FLASH_BASE		0x0c000000
-#define FLASH_ECCLPLB		0x00	/* Line parity 7 - 0 bit */
-#define FLASH_ECCLPUB		0x04	/* Line parity 15 - 8 bit */
-#define FLASH_ECCCP		0x08	/* Column parity 5 - 0 bit */
-#define FLASH_ECCCNTR		0x0c	/* ECC byte counter */
-#define FLASH_ECCCLRR		0x10	/* Clear ECC */
-#define FLASH_FLASHIO		0x14	/* Flash I/O */
-#define FLASH_FLASHCTL		0x18	/* Flash Control */
+#define FLASH_BASE              0x0c000000
+#define FLASH_ECCLPLB           0x00    /* Line parity 7 - 0 bit */
+#define FLASH_ECCLPUB           0x04    /* Line parity 15 - 8 bit */
+#define FLASH_ECCCP             0x08    /* Column parity 5 - 0 bit */
+#define FLASH_ECCCNTR           0x0c    /* ECC byte counter */
+#define FLASH_ECCCLRR           0x10    /* Clear ECC */
+#define FLASH_FLASHIO           0x14    /* Flash I/O */
+#define FLASH_FLASHCTL          0x18    /* Flash Control */
 
-#define FLASHCTL_CE0		(1 << 0)
-#define FLASHCTL_CLE		(1 << 1)
-#define FLASHCTL_ALE		(1 << 2)
-#define FLASHCTL_WP		(1 << 3)
-#define FLASHCTL_CE1		(1 << 4)
-#define FLASHCTL_RYBY		(1 << 5)
-#define FLASHCTL_NCE		(FLASHCTL_CE0 | FLASHCTL_CE1)
+#define FLASHCTL_CE0            (1 << 0)
+#define FLASHCTL_CLE            (1 << 1)
+#define FLASHCTL_ALE            (1 << 2)
+#define FLASHCTL_WP             (1 << 3)
+#define FLASHCTL_CE1            (1 << 4)
+#define FLASHCTL_RYBY           (1 << 5)
+#define FLASHCTL_NCE            (FLASHCTL_CE0 | FLASHCTL_CE1)
 
 #define TYPE_SL_NAND "sl-nand"
 #define SL_NAND(obj) OBJECT_CHECK(SLNANDState, (obj), TYPE_SL_NAND)
@@ -74,12 +74,12 @@ static uint64_t sl_read(void *opaque, hwaddr addr, unsigned size)
     int ryby;
 
     switch (addr) {
-#define BSHR(byte, from, to)	((s->ecc.lp[byte] >> (from - to)) & (1 << to))
+#define BSHR(byte, from, to)    ((s->ecc.lp[byte] >> (from - to)) & (1 << to))
     case FLASH_ECCLPLB:
         return BSHR(0, 4, 0) | BSHR(0, 5, 2) | BSHR(0, 6, 4) | BSHR(0, 7, 6) |
                 BSHR(1, 4, 1) | BSHR(1, 5, 3) | BSHR(1, 6, 5) | BSHR(1, 7, 7);
 
-#define BSHL(byte, from, to)	((s->ecc.lp[byte] << (to - from)) & (1 << to))
+#define BSHL(byte, from, to)    ((s->ecc.lp[byte] << (to - from)) & (1 << to))
     case FLASH_ECCLPUB:
         return BSHL(0, 0, 0) | BSHL(0, 1, 2) | BSHL(0, 2, 4) | BSHL(0, 3, 6) |
                 BSHL(1, 0, 1) | BSHL(1, 1, 3) | BSHL(1, 2, 5) | BSHL(1, 3, 7);
@@ -191,8 +191,8 @@ static void sl_nand_realize(DeviceState *dev, Error **errp)
 
 /* Spitz Keyboard */
 
-#define SPITZ_KEY_STROBE_NUM	11
-#define SPITZ_KEY_SENSE_NUM	7
+#define SPITZ_KEY_STROBE_NUM    11
+#define SPITZ_KEY_SENSE_NUM     7
 
 static const int spitz_gpio_key_sense[SPITZ_KEY_SENSE_NUM] = {
     12, 17, 91, 34, 36, 38, 39
@@ -214,11 +214,11 @@ static int spitz_keymap[SPITZ_KEY_SENSE_NUM + 1][SPITZ_KEY_STROBE_NUM] = {
     { 0x52, 0x43, 0x01, 0x47, 0x49,  -1 ,  -1 ,  -1 ,  -1 ,  -1 ,  -1  },
 };
 
-#define SPITZ_GPIO_AK_INT	13	/* Remote control */
-#define SPITZ_GPIO_SYNC		16	/* Sync button */
-#define SPITZ_GPIO_ON_KEY	95	/* Power button */
-#define SPITZ_GPIO_SWA		97	/* Lid */
-#define SPITZ_GPIO_SWB		96	/* Tablet mode */
+#define SPITZ_GPIO_AK_INT       13      /* Remote control */
+#define SPITZ_GPIO_SYNC                 16      /* Sync button */
+#define SPITZ_GPIO_ON_KEY       95      /* Power button */
+#define SPITZ_GPIO_SWA          97      /* Lid */
+#define SPITZ_GPIO_SWB          96      /* Tablet mode */
 
 /* The special buttons are mapped to unused keys */
 static const int spitz_gpiomap[5] = {
@@ -300,7 +300,7 @@ static void spitz_keyboard_keydown(SpitzKeyboardState *s, int keycode)
 #define SPITZ_MOD_CTRL    (1 << 8)
 #define SPITZ_MOD_FN      (1 << 9)
 
-#define QUEUE_KEY(c)	s->fifo[(s->fifopos + s->fifolen ++) & 0xf] = c
+#define QUEUE_KEY(c)    s->fifo[(s->fifopos + s->fifolen ++) & 0xf] = c
 
 static void spitz_keyboard_handler(void *opaque, int keycode)
 {
@@ -308,25 +308,25 @@ static void spitz_keyboard_handler(void *opaque, int keycode)
     uint16_t code;
     int mapcode;
     switch (keycode) {
-    case 0x2a:	/* Left Shift */
+    case 0x2a:  /* Left Shift */
         s->modifiers |= 1;
         break;
     case 0xaa:
         s->modifiers &= ~1;
         break;
-    case 0x36:	/* Right Shift */
+    case 0x36:  /* Right Shift */
         s->modifiers |= 2;
         break;
     case 0xb6:
         s->modifiers &= ~2;
         break;
-    case 0x1d:	/* Control */
+    case 0x1d:  /* Control */
         s->modifiers |= 4;
         break;
     case 0x9d:
         s->modifiers &= ~4;
         break;
-    case 0x38:	/* Alt */
+    case 0x38:  /* Alt */
         s->modifiers |= 8;
         break;
     case 0xb8:
@@ -536,14 +536,14 @@ static void spitz_keyboard_realize(DeviceState *dev, Error **errp)
 
 /* LCD backlight controller */
 
-#define LCDTG_RESCTL	0x00
-#define LCDTG_PHACTRL	0x01
-#define LCDTG_DUTYCTRL	0x02
-#define LCDTG_POWERREG0	0x03
-#define LCDTG_POWERREG1	0x04
-#define LCDTG_GPOR3	0x05
-#define LCDTG_PICTRL	0x06
-#define LCDTG_POLCTRL	0x07
+#define LCDTG_RESCTL    0x00
+#define LCDTG_PHACTRL   0x01
+#define LCDTG_DUTYCTRL  0x02
+#define LCDTG_POWERREG0         0x03
+#define LCDTG_POWERREG1         0x04
+#define LCDTG_GPOR3     0x05
+#define LCDTG_PICTRL    0x06
+#define LCDTG_POLCTRL   0x07
 
 typedef struct {
     SSISlave ssidev;
@@ -623,12 +623,12 @@ static void spitz_lcdtg_realize(SSISlave *dev, Error **errp)
 
 /* SSP devices */
 
-#define CORGI_SSP_PORT		2
+#define CORGI_SSP_PORT          2
 
-#define SPITZ_GPIO_LCDCON_CS	53
-#define SPITZ_GPIO_ADS7846_CS	14
-#define SPITZ_GPIO_MAX1111_CS	20
-#define SPITZ_GPIO_TP_INT	11
+#define SPITZ_GPIO_LCDCON_CS    53
+#define SPITZ_GPIO_ADS7846_CS   14
+#define SPITZ_GPIO_MAX1111_CS   20
+#define SPITZ_GPIO_TP_INT       11
 
 static DeviceState *max1111;
 
@@ -659,13 +659,13 @@ static void corgi_ssp_gpio_cs(void *opaque, int line, int level)
     s->enable[line] = !level;
 }
 
-#define MAX1111_BATT_VOLT	1
-#define MAX1111_BATT_TEMP	2
-#define MAX1111_ACIN_VOLT	3
+#define MAX1111_BATT_VOLT       1
+#define MAX1111_BATT_TEMP       2
+#define MAX1111_ACIN_VOLT       3
 
-#define SPITZ_BATTERY_TEMP	0xe0	/* About 2.9V */
-#define SPITZ_BATTERY_VOLT	0xd0	/* About 4.0V */
-#define SPITZ_CHARGEON_ACIN	0x80	/* About 5.0V */
+#define SPITZ_BATTERY_TEMP      0xe0    /* About 2.9V */
+#define SPITZ_BATTERY_VOLT      0xd0    /* About 4.0V */
+#define SPITZ_CHARGEON_ACIN     0x80    /* About 5.0V */
 
 static void spitz_adc_temp_on(void *opaque, int line, int level)
 {
@@ -735,11 +735,11 @@ static void spitz_microdrive_attach(PXA2xxState *cpu, int slot)
 
 /* Wm8750 and Max7310 on I2C */
 
-#define AKITA_MAX_ADDR	0x18
-#define SPITZ_WM_ADDRL	0x1b
-#define SPITZ_WM_ADDRH	0x1a
+#define AKITA_MAX_ADDR  0x18
+#define SPITZ_WM_ADDRL  0x1b
+#define SPITZ_WM_ADDRH  0x1a
 
-#define SPITZ_GPIO_WM	5
+#define SPITZ_GPIO_WM   5
 
 static void spitz_wm8750_addr(void *opaque, int line, int level)
 {
@@ -806,20 +806,20 @@ static void spitz_out_switch(void *opaque, int line, int level)
     }
 }
 
-#define SPITZ_SCP_LED_GREEN		1
-#define SPITZ_SCP_JK_B			2
-#define SPITZ_SCP_CHRG_ON		3
-#define SPITZ_SCP_MUTE_L		4
-#define SPITZ_SCP_MUTE_R		5
-#define SPITZ_SCP_CF_POWER		6
-#define SPITZ_SCP_LED_ORANGE		7
-#define SPITZ_SCP_JK_A			8
-#define SPITZ_SCP_ADC_TEMP_ON		9
-#define SPITZ_SCP2_IR_ON		1
-#define SPITZ_SCP2_AKIN_PULLUP		2
-#define SPITZ_SCP2_BACKLIGHT_CONT	7
-#define SPITZ_SCP2_BACKLIGHT_ON		8
-#define SPITZ_SCP2_MIC_BIAS		9
+#define SPITZ_SCP_LED_GREEN             1
+#define SPITZ_SCP_JK_B                  2
+#define SPITZ_SCP_CHRG_ON               3
+#define SPITZ_SCP_MUTE_L                4
+#define SPITZ_SCP_MUTE_R                5
+#define SPITZ_SCP_CF_POWER              6
+#define SPITZ_SCP_LED_ORANGE            7
+#define SPITZ_SCP_JK_A                  8
+#define SPITZ_SCP_ADC_TEMP_ON           9
+#define SPITZ_SCP2_IR_ON                1
+#define SPITZ_SCP2_AKIN_PULLUP          2
+#define SPITZ_SCP2_BACKLIGHT_CONT       7
+#define SPITZ_SCP2_BACKLIGHT_ON                 8
+#define SPITZ_SCP2_MIC_BIAS             9
 
 static void spitz_scoop_gpio_setup(PXA2xxState *cpu,
                 DeviceState *scp0, DeviceState *scp1)
@@ -839,15 +839,15 @@ static void spitz_scoop_gpio_setup(PXA2xxState *cpu,
     qdev_connect_gpio_out(scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
 }
 
-#define SPITZ_GPIO_HSYNC		22
-#define SPITZ_GPIO_SD_DETECT		9
-#define SPITZ_GPIO_SD_WP		81
-#define SPITZ_GPIO_ON_RESET		89
-#define SPITZ_GPIO_BAT_COVER		90
-#define SPITZ_GPIO_CF1_IRQ		105
-#define SPITZ_GPIO_CF1_CD		94
-#define SPITZ_GPIO_CF2_IRQ		106
-#define SPITZ_GPIO_CF2_CD		93
+#define SPITZ_GPIO_HSYNC                22
+#define SPITZ_GPIO_SD_DETECT            9
+#define SPITZ_GPIO_SD_WP                81
+#define SPITZ_GPIO_ON_RESET             89
+#define SPITZ_GPIO_BAT_COVER            90
+#define SPITZ_GPIO_CF1_IRQ              105
+#define SPITZ_GPIO_CF1_CD               94
+#define SPITZ_GPIO_CF2_IRQ              106
+#define SPITZ_GPIO_CF2_CD               93
 
 static int spitz_hsync;
 
@@ -907,8 +907,8 @@ static void spitz_gpio_setup(PXA2xxState *cpu, int slots)
 /* Board init.  */
 enum spitz_model_e { spitz, akita, borzoi, terrier };
 
-#define SPITZ_RAM	0x04000000
-#define SPITZ_ROM	0x00800000
+#define SPITZ_RAM       0x04000000
+#define SPITZ_ROM       0x00800000
 
 static struct arm_boot_info spitz_binfo = {
     .loader_start = PXA2XX_SDRAM_BASE,
-- 
2.20.1

[PATCH 02/17] hw/arm/spitz: Create SpitzMachineClass abstract base class

[Peter Maydell]

For the four Spitz-family machines (akita, borzoi, spitz, terrier)
create a proper abstract class SpitzMachineClass which encapsulates
the common behaviour, rather than having them all derive directly
from TYPE_MACHINE:
 * instead of each machine class setting mc->init to a wrapper
   function which calls spitz_common_init() with parameters,
   put that data in the SpitzMachineClass and make spitz_common_init
   the SpitzMachineClass machine-init function
 * move the settings of mc->block_default_type and
   mc->ignore_memory_transaction_failures into the SpitzMachineClass
   class init rather than repeating them in each machine's class init

(The motivation is that we're going to want to keep some state in
the SpitzMachineState so we can connect GPIOs between devices created
in one sub-function of the machine init to devices created in a
different sub-function.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/spitz.c | 91 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 55 insertions(+), 36 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 9eaedab79b5..c70e912a33d 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -33,6 +33,26 @@
 #include "exec/address-spaces.h"
 #include "cpu.h"
 
+enum spitz_model_e { spitz, akita, borzoi, terrier };
+
+typedef struct {
+    MachineClass parent;
+    enum spitz_model_e model;
+    int arm_id;
+} SpitzMachineClass;
+
+typedef struct {
+    MachineState parent;
+} SpitzMachineState;
+
+#define TYPE_SPITZ_MACHINE "spitz-common"
+#define SPITZ_MACHINE(obj) \
+    OBJECT_CHECK(SpitzMachineState, obj, TYPE_SPITZ_MACHINE)
+#define SPITZ_MACHINE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(SpitzMachineClass, obj, TYPE_SPITZ_MACHINE)
+#define SPITZ_MACHINE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(SpitzMachineClass, klass, TYPE_SPITZ_MACHINE)
+
 #undef REG_FMT
 #define REG_FMT                         "0x%02lx"
 
@@ -905,8 +925,6 @@ static void spitz_gpio_setup(PXA2xxState *cpu, int slots)
 }
 
 /* Board init.  */
-enum spitz_model_e { spitz, akita, borzoi, terrier };
-
 #define SPITZ_RAM       0x04000000
 #define SPITZ_ROM       0x00800000
 
@@ -915,9 +933,10 @@ static struct arm_boot_info spitz_binfo = {
     .ram_size = 0x04000000,
 };
 
-static void spitz_common_init(MachineState *machine,
-                              enum spitz_model_e model, int arm_id)
+static void spitz_common_init(MachineState *machine)
 {
+    SpitzMachineClass *smc = SPITZ_MACHINE_GET_CLASS(machine);
+    enum spitz_model_e model = smc->model;
     PXA2xxState *mpu;
     DeviceState *scp0, *scp1 = NULL;
     MemoryRegion *address_space_mem = get_system_memory();
@@ -958,100 +977,100 @@ static void spitz_common_init(MachineState *machine,
         /* A 4.0 GB microdrive is permanently sitting in CF slot 0.  */
         spitz_microdrive_attach(mpu, 0);
 
-    spitz_binfo.board_id = arm_id;
+    spitz_binfo.board_id = smc->arm_id;
     arm_load_kernel(mpu->cpu, machine, &spitz_binfo);
     sl_bootparam_write(SL_PXA_PARAM_BASE);
 }
 
-static void spitz_init(MachineState *machine)
+static void spitz_common_class_init(ObjectClass *oc, void *data)
 {
-    spitz_common_init(machine, spitz, 0x2c9);
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->block_default_type = IF_IDE;
+    mc->ignore_memory_transaction_failures = true;
+    mc->init = spitz_common_init;
 }
 
-static void borzoi_init(MachineState *machine)
-{
-    spitz_common_init(machine, borzoi, 0x33f);
-}
-
-static void akita_init(MachineState *machine)
-{
-    spitz_common_init(machine, akita, 0x2e8);
-}
-
-static void terrier_init(MachineState *machine)
-{
-    spitz_common_init(machine, terrier, 0x33f);
-}
+static const TypeInfo spitz_common_info = {
+    .name = TYPE_SPITZ_MACHINE,
+    .parent = TYPE_MACHINE,
+    .abstract = true,
+    .instance_size = sizeof(SpitzMachineState),
+    .class_size = sizeof(SpitzMachineClass),
+    .class_init = spitz_common_class_init,
+};
 
 static void akitapda_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    SpitzMachineClass *smc = SPITZ_MACHINE_CLASS(oc);
 
     mc->desc = "Sharp SL-C1000 (Akita) PDA (PXA270)";
-    mc->init = akita_init;
-    mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("pxa270-c0");
+    smc->model = akita;
+    smc->arm_id = 0x2e8;
 }
 
 static const TypeInfo akitapda_type = {
     .name = MACHINE_TYPE_NAME("akita"),
-    .parent = TYPE_MACHINE,
+    .parent = TYPE_SPITZ_MACHINE,
     .class_init = akitapda_class_init,
 };
 
 static void spitzpda_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    SpitzMachineClass *smc = SPITZ_MACHINE_CLASS(oc);
 
     mc->desc = "Sharp SL-C3000 (Spitz) PDA (PXA270)";
-    mc->init = spitz_init;
-    mc->block_default_type = IF_IDE;
-    mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("pxa270-c0");
+    smc->model = spitz;
+    smc->arm_id = 0x2c9;
 }
 
 static const TypeInfo spitzpda_type = {
     .name = MACHINE_TYPE_NAME("spitz"),
-    .parent = TYPE_MACHINE,
+    .parent = TYPE_SPITZ_MACHINE,
     .class_init = spitzpda_class_init,
 };
 
 static void borzoipda_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    SpitzMachineClass *smc = SPITZ_MACHINE_CLASS(oc);
 
     mc->desc = "Sharp SL-C3100 (Borzoi) PDA (PXA270)";
-    mc->init = borzoi_init;
-    mc->block_default_type = IF_IDE;
-    mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("pxa270-c0");
+    smc->model = borzoi;
+    smc->arm_id = 0x33f;
 }
 
 static const TypeInfo borzoipda_type = {
     .name = MACHINE_TYPE_NAME("borzoi"),
-    .parent = TYPE_MACHINE,
+    .parent = TYPE_SPITZ_MACHINE,
     .class_init = borzoipda_class_init,
 };
 
 static void terrierpda_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    SpitzMachineClass *smc = SPITZ_MACHINE_CLASS(oc);
 
     mc->desc = "Sharp SL-C3200 (Terrier) PDA (PXA270)";
-    mc->init = terrier_init;
-    mc->block_default_type = IF_IDE;
-    mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("pxa270-c5");
+    smc->model = terrier;
+    smc->arm_id = 0x33f;
 }
 
 static const TypeInfo terrierpda_type = {
     .name = MACHINE_TYPE_NAME("terrier"),
-    .parent = TYPE_MACHINE,
+    .parent = TYPE_SPITZ_MACHINE,
     .class_init = terrierpda_class_init,
 };
 
 static void spitz_machine_init(void)
 {
+    type_register_static(&spitz_common_info);
     type_register_static(&akitapda_type);
     type_register_static(&spitzpda_type);
     type_register_static(&borzoipda_type);
-- 
2.20.1

[PATCH 03/17] hw/arm/spitz: Keep pointers to MPU and SSI devices in SpitzMachineState

[Peter Maydell]

Keep pointers to the MPU and the SSI devices in SpitzMachineState.
We're going to want to make GPIO connections between some of the
SSI devices and the SCPs, so we want to keep hold of a pointer to
those; putting the MPU into the struct allows us to pass just
one thing to spitz_ssp_attach() rather than two.

We have to retain the setting of the global "max1111" variable
for the moment as it is used in spitz_adc_temp_on(); later in
this series of commits we will be able to remove it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/spitz.c | 50 ++++++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index c70e912a33d..f48e966c047 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -43,6 +43,11 @@ typedef struct {
 
 typedef struct {
     MachineState parent;
+    PXA2xxState *mpu;
+    DeviceState *mux;
+    DeviceState *lcdtg;
+    DeviceState *ads7846;
+    DeviceState *max1111;
 } SpitzMachineState;
 
 #define TYPE_SPITZ_MACHINE "spitz-common"
@@ -709,34 +714,33 @@ static void corgi_ssp_realize(SSISlave *d, Error **errp)
     s->bus[2] = ssi_create_bus(dev, "ssi2");
 }
 
-static void spitz_ssp_attach(PXA2xxState *cpu)
+static void spitz_ssp_attach(SpitzMachineState *sms)
 {
-    DeviceState *mux;
-    DeviceState *dev;
     void *bus;
 
-    mux = ssi_create_slave(cpu->ssp[CORGI_SSP_PORT - 1], "corgi-ssp");
+    sms->mux = ssi_create_slave(sms->mpu->ssp[CORGI_SSP_PORT - 1], "corgi-ssp");
 
-    bus = qdev_get_child_bus(mux, "ssi0");
-    ssi_create_slave(bus, "spitz-lcdtg");
+    bus = qdev_get_child_bus(sms->mux, "ssi0");
+    sms->lcdtg = ssi_create_slave(bus, "spitz-lcdtg");
 
-    bus = qdev_get_child_bus(mux, "ssi1");
-    dev = ssi_create_slave(bus, "ads7846");
-    qdev_connect_gpio_out(dev, 0,
-                          qdev_get_gpio_in(cpu->gpio, SPITZ_GPIO_TP_INT));
+    bus = qdev_get_child_bus(sms->mux, "ssi1");
+    sms->ads7846 = ssi_create_slave(bus, "ads7846");
+    qdev_connect_gpio_out(sms->ads7846, 0,
+                          qdev_get_gpio_in(sms->mpu->gpio, SPITZ_GPIO_TP_INT));
 
-    bus = qdev_get_child_bus(mux, "ssi2");
-    max1111 = ssi_create_slave(bus, "max1111");
-    max111x_set_input(max1111, MAX1111_BATT_VOLT, SPITZ_BATTERY_VOLT);
-    max111x_set_input(max1111, MAX1111_BATT_TEMP, 0);
-    max111x_set_input(max1111, MAX1111_ACIN_VOLT, SPITZ_CHARGEON_ACIN);
+    bus = qdev_get_child_bus(sms->mux, "ssi2");
+    sms->max1111 = ssi_create_slave(bus, "max1111");
+    max1111 = sms->max1111;
+    max111x_set_input(sms->max1111, MAX1111_BATT_VOLT, SPITZ_BATTERY_VOLT);
+    max111x_set_input(sms->max1111, MAX1111_BATT_TEMP, 0);
+    max111x_set_input(sms->max1111, MAX1111_ACIN_VOLT, SPITZ_CHARGEON_ACIN);
 
-    qdev_connect_gpio_out(cpu->gpio, SPITZ_GPIO_LCDCON_CS,
-                        qdev_get_gpio_in(mux, 0));
-    qdev_connect_gpio_out(cpu->gpio, SPITZ_GPIO_ADS7846_CS,
-                        qdev_get_gpio_in(mux, 1));
-    qdev_connect_gpio_out(cpu->gpio, SPITZ_GPIO_MAX1111_CS,
-                        qdev_get_gpio_in(mux, 2));
+    qdev_connect_gpio_out(sms->mpu->gpio, SPITZ_GPIO_LCDCON_CS,
+                        qdev_get_gpio_in(sms->mux, 0));
+    qdev_connect_gpio_out(sms->mpu->gpio, SPITZ_GPIO_ADS7846_CS,
+                        qdev_get_gpio_in(sms->mux, 1));
+    qdev_connect_gpio_out(sms->mpu->gpio, SPITZ_GPIO_MAX1111_CS,
+                        qdev_get_gpio_in(sms->mux, 2));
 }
 
 /* CF Microdrive */
@@ -936,6 +940,7 @@ static struct arm_boot_info spitz_binfo = {
 static void spitz_common_init(MachineState *machine)
 {
     SpitzMachineClass *smc = SPITZ_MACHINE_GET_CLASS(machine);
+    SpitzMachineState *sms = SPITZ_MACHINE(machine);
     enum spitz_model_e model = smc->model;
     PXA2xxState *mpu;
     DeviceState *scp0, *scp1 = NULL;
@@ -945,6 +950,7 @@ static void spitz_common_init(MachineState *machine)
     /* Setup CPU & memory */
     mpu = pxa270_init(address_space_mem, spitz_binfo.ram_size,
                       machine->cpu_type);
+    sms->mpu = mpu;
 
     sl_flash_register(mpu, (model == spitz) ? FLASH_128M : FLASH_1024M);
 
@@ -954,7 +960,7 @@ static void spitz_common_init(MachineState *machine)
     /* Setup peripherals */
     spitz_keyboard_register(mpu);
 
-    spitz_ssp_attach(mpu);
+    spitz_ssp_attach(sms);
 
     scp0 = sysbus_create_simple("scoop", 0x10800000, NULL);
     if (model != akita) {
-- 
2.20.1

[PATCH 04/17] hw/arm/spitz: Keep pointers to scp0, scp1 in SpitzMachineState

[Peter Maydell]

Keep pointers to scp0, scp1 in SpitzMachineState, and just pass
that to spitz_scoop_gpio_setup().

(We'll want to use some of the other fields in SpitzMachineState
in that function in the next commit.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/spitz.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index f48e966c047..69bc2b3fa10 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -48,6 +48,8 @@ typedef struct {
     DeviceState *lcdtg;
     DeviceState *ads7846;
     DeviceState *max1111;
+    DeviceState *scp0;
+    DeviceState *scp1;
 } SpitzMachineState;
 
 #define TYPE_SPITZ_MACHINE "spitz-common"
@@ -845,22 +847,23 @@ static void spitz_out_switch(void *opaque, int line, int level)
 #define SPITZ_SCP2_BACKLIGHT_ON                 8
 #define SPITZ_SCP2_MIC_BIAS             9
 
-static void spitz_scoop_gpio_setup(PXA2xxState *cpu,
-                DeviceState *scp0, DeviceState *scp1)
+static void spitz_scoop_gpio_setup(SpitzMachineState *sms)
 {
-    qemu_irq *outsignals = qemu_allocate_irqs(spitz_out_switch, cpu, 8);
+    qemu_irq *outsignals = qemu_allocate_irqs(spitz_out_switch, sms->mpu, 8);
 
-    qdev_connect_gpio_out(scp0, SPITZ_SCP_CHRG_ON, outsignals[0]);
-    qdev_connect_gpio_out(scp0, SPITZ_SCP_JK_B, outsignals[1]);
-    qdev_connect_gpio_out(scp0, SPITZ_SCP_LED_GREEN, outsignals[2]);
-    qdev_connect_gpio_out(scp0, SPITZ_SCP_LED_ORANGE, outsignals[3]);
+    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_CHRG_ON, outsignals[0]);
+    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_JK_B, outsignals[1]);
+    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_GREEN, outsignals[2]);
+    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_ORANGE, outsignals[3]);
 
-    if (scp1) {
-        qdev_connect_gpio_out(scp1, SPITZ_SCP2_BACKLIGHT_CONT, outsignals[4]);
-        qdev_connect_gpio_out(scp1, SPITZ_SCP2_BACKLIGHT_ON, outsignals[5]);
+    if (sms->scp1) {
+        qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_CONT,
+                              outsignals[4]);
+        qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_ON,
+                              outsignals[5]);
     }
 
-    qdev_connect_gpio_out(scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
+    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
 }
 
 #define SPITZ_GPIO_HSYNC                22
@@ -943,7 +946,6 @@ static void spitz_common_init(MachineState *machine)
     SpitzMachineState *sms = SPITZ_MACHINE(machine);
     enum spitz_model_e model = smc->model;
     PXA2xxState *mpu;
-    DeviceState *scp0, *scp1 = NULL;
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *rom = g_new(MemoryRegion, 1);
 
@@ -962,12 +964,14 @@ static void spitz_common_init(MachineState *machine)
 
     spitz_ssp_attach(sms);
 
-    scp0 = sysbus_create_simple("scoop", 0x10800000, NULL);
+    sms->scp0 = sysbus_create_simple("scoop", 0x10800000, NULL);
     if (model != akita) {
-        scp1 = sysbus_create_simple("scoop", 0x08800040, NULL);
+        sms->scp1 = sysbus_create_simple("scoop", 0x08800040, NULL);
+    } else {
+        sms->scp1 = NULL;
     }
 
-    spitz_scoop_gpio_setup(mpu, scp0, scp1);
+    spitz_scoop_gpio_setup(sms);
 
     spitz_gpio_setup(mpu, (model == akita) ? 1 : 2);
 
-- 
2.20.1

[PATCH 05/17] hw/arm/spitz: Implement inbound GPIO lines for bit5 and power signals

[Peter Maydell]

Currently the Spitz board uses a nasty hack for the GPIO lines
that pass "bit5" and "power" information to the LCD controller:
the lcdtg realize function sets a global variable to point to
the instance it just realized, and then the functions spitz_bl_power()
and spitz_bl_bit5() use that to find the device they are changing
the internal state of. There is a comment reading:
 FIXME: Implement GPIO properly and remove this hack.
which was added in 2009.

Implement GPIO properly and remove this hack.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/spitz.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 69bc2b3fa10..11e413723f4 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -586,12 +586,9 @@ static void spitz_bl_update(SpitzLCDTG *s)
         zaurus_printf("LCD Backlight now off\n");
 }
 
-/* FIXME: Implement GPIO properly and remove this hack.  */
-static SpitzLCDTG *spitz_lcdtg;
-
 static inline void spitz_bl_bit5(void *opaque, int line, int level)
 {
-    SpitzLCDTG *s = spitz_lcdtg;
+    SpitzLCDTG *s = opaque;
     int prev = s->bl_intensity;
 
     if (level)
@@ -605,7 +602,7 @@ static inline void spitz_bl_bit5(void *opaque, int line, int level)
 
 static inline void spitz_bl_power(void *opaque, int line, int level)
 {
-    SpitzLCDTG *s = spitz_lcdtg;
+    SpitzLCDTG *s = opaque;
     s->bl_power = !!level;
     spitz_bl_update(s);
 }
@@ -639,13 +636,16 @@ static uint32_t spitz_lcdtg_transfer(SSISlave *dev, uint32_t value)
     return 0;
 }
 
-static void spitz_lcdtg_realize(SSISlave *dev, Error **errp)
+static void spitz_lcdtg_realize(SSISlave *ssi, Error **errp)
 {
-    SpitzLCDTG *s = FROM_SSI_SLAVE(SpitzLCDTG, dev);
+    SpitzLCDTG *s = FROM_SSI_SLAVE(SpitzLCDTG, ssi);
+    DeviceState *dev = DEVICE(s);
 
-    spitz_lcdtg = s;
     s->bl_power = 0;
     s->bl_intensity = 0x20;
+
+    qdev_init_gpio_in_named(dev, spitz_bl_bit5, "bl_bit5", 1);
+    qdev_init_gpio_in_named(dev, spitz_bl_power, "bl_power", 1);
 }
 
 /* SSP devices */
@@ -820,15 +820,11 @@ static void spitz_out_switch(void *opaque, int line, int level)
     case 3:
         zaurus_printf("Orange LED %s.\n", level ? "on" : "off");
         break;
-    case 4:
-        spitz_bl_bit5(opaque, line, level);
-        break;
-    case 5:
-        spitz_bl_power(opaque, line, level);
-        break;
     case 6:
         spitz_adc_temp_on(opaque, line, level);
         break;
+    default:
+        g_assert_not_reached();
     }
 }
 
@@ -858,9 +854,9 @@ static void spitz_scoop_gpio_setup(SpitzMachineState *sms)
 
     if (sms->scp1) {
         qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_CONT,
-                              outsignals[4]);
+                              qdev_get_gpio_in_named(sms->lcdtg, "bl_bit5", 0));
         qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_ON,
-                              outsignals[5]);
+                              qdev_get_gpio_in_named(sms->lcdtg, "bl_power", 0));
     }
 
     qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
-- 
2.20.1

[PATCH 06/17] hw/misc/max111x: provide QOM properties for setting initial values

[Peter Maydell]

Add some QOM properties to the max111x ADC device to allow the
initial values to be configured. Currently this is done by
board code calling max111x_set_input() after it creates the
device, which doesn't work on system reset.

This requires us to implement a reset method for this device,
so while we're doing that make sure we reset the other parts
of the device state.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/max111x.c | 57 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
index 2b87bdee5b7..d0e5534e4f5 100644
--- a/hw/misc/max111x.c
+++ b/hw/misc/max111x.c
@@ -15,11 +15,15 @@
 #include "hw/ssi/ssi.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;
 
@@ -135,16 +139,6 @@ static int max111x_init(SSISlave *d, int inputs)
     qdev_init_gpio_out(dev, &s->interrupt, 1);
 
     s->inputs = inputs;
-    /* TODO: add a user interface for setting these */
-    s->input[0] = 0xf0;
-    s->input[1] = 0xe0;
-    s->input[2] = 0xd0;
-    s->input[3] = 0xc0;
-    s->input[4] = 0xb0;
-    s->input[5] = 0xa0;
-    s->input[6] = 0x90;
-    s->input[7] = 0x80;
-    s->com = 0;
 
     vmstate_register(VMSTATE_IF(dev), VMSTATE_INSTANCE_ID_ANY,
                      &vmstate_max111x, s);
@@ -168,11 +162,50 @@ void max111x_set_input(DeviceState *dev, int line, uint8_t value)
     s->input[line] = value;
 }
 
+static void max111x_reset(DeviceState *dev)
+{
+    MAX111xState *s = MAX_111X(dev);
+    int i;
+
+    for (i = 0; i < s->inputs; i++) {
+        s->input[i] = s->reset_input[i];
+    }
+    s->com = 0;
+    s->tb1 = 0;
+    s->rb2 = 0;
+    s->rb3 = 0;
+    s->cycle = 0;
+}
+
+static Property max1110_properties[] = {
+    /* Reset values for ADC inputs */
+    DEFINE_PROP_UINT8("input0", MAX111xState, reset_input[0], 0xf0),
+    DEFINE_PROP_UINT8("input1", MAX111xState, reset_input[1], 0xe0),
+    DEFINE_PROP_UINT8("input2", MAX111xState, reset_input[2], 0xd0),
+    DEFINE_PROP_UINT8("input3", MAX111xState, reset_input[3], 0xc0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static Property max1111_properties[] = {
+    /* Reset values for ADC inputs */
+    DEFINE_PROP_UINT8("input0", MAX111xState, reset_input[0], 0xf0),
+    DEFINE_PROP_UINT8("input1", MAX111xState, reset_input[1], 0xe0),
+    DEFINE_PROP_UINT8("input2", MAX111xState, reset_input[2], 0xd0),
+    DEFINE_PROP_UINT8("input3", MAX111xState, reset_input[3], 0xc0),
+    DEFINE_PROP_UINT8("input4", MAX111xState, reset_input[4], 0xb0),
+    DEFINE_PROP_UINT8("input5", MAX111xState, reset_input[5], 0xa0),
+    DEFINE_PROP_UINT8("input6", MAX111xState, reset_input[6], 0x90),
+    DEFINE_PROP_UINT8("input7", MAX111xState, reset_input[7], 0x80),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void max111x_class_init(ObjectClass *klass, void *data)
 {
     SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
 
     k->transfer = max111x_transfer;
+    dc->reset = max111x_reset;
 }
 
 static const TypeInfo max111x_info = {
@@ -186,8 +219,10 @@ static const TypeInfo max111x_info = {
 static void max1110_class_init(ObjectClass *klass, void *data)
 {
     SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
 
     k->realize = max1110_realize;
+    device_class_set_props(dc, max1110_properties);
 }
 
 static const TypeInfo max1110_info = {
@@ -199,8 +234,10 @@ static const TypeInfo max1110_info = {
 static void max1111_class_init(ObjectClass *klass, void *data)
 {
     SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
 
     k->realize = max1111_realize;
+    device_class_set_props(dc, max1111_properties);
 }
 
 static const TypeInfo max1111_info = {
-- 
2.20.1

[PATCH 07/17] hw/misc/max111x: Don't use vmstate_register()

[Peter Maydell]

The max111x is a proper qdev device; we can use dc->vmsd rather than
directly calling vmstate_register().

It's possible that this is a migration compat break, but the only
boards that use this device are the spitz-family ('akita', 'borzoi',
'spitz', 'terrier').

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/max111x.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
index d0e5534e4f5..abddfa3c660 100644
--- a/hw/misc/max111x.c
+++ b/hw/misc/max111x.c
@@ -140,8 +140,6 @@ static int max111x_init(SSISlave *d, int inputs)
 
     s->inputs = inputs;
 
-    vmstate_register(VMSTATE_IF(dev), VMSTATE_INSTANCE_ID_ANY,
-                     &vmstate_max111x, s);
     return 0;
 }
 
@@ -206,6 +204,7 @@ static void max111x_class_init(ObjectClass *klass, void *data)
 
     k->transfer = max111x_transfer;
     dc->reset = max111x_reset;
+    dc->vmsd = &vmstate_max111x;
 }
 
 static const TypeInfo max111x_info = {
-- 
2.20.1

[PATCH 08/17] ssi: Add ssi_realize_and_unref()

[Peter Maydell]

Add an ssi_realize_and_unref(), for the benefit of callers
who want to be able to create an SSI device, set QOM properties
on it, and then do the realize-and-unref afterwards.

The API works on the same principle as the recently added
qdev_realize_and_undef(), sysbus_realize_and_undef(), etc.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/ssi/ssi.h | 26 ++++++++++++++++++++++++++
 hw/ssi/ssi.c         |  7 ++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index 93f2b8b0beb..4be5325e654 100644
--- a/include/hw/ssi/ssi.h
+++ b/include/hw/ssi/ssi.h
@@ -79,6 +79,32 @@ extern const VMStateDescription vmstate_ssi_slave;
 }
 
 DeviceState *ssi_create_slave(SSIBus *bus, const char *name);
+/**
+ * ssi_realize_and_unref: realize and unref an SSI slave device
+ * @dev: SSI slave device to realize
+ * @bus: SSI bus to put it on
+ * @errp: error pointer
+ *
+ * Call 'realize' on @dev, put it on the specified @bus, and drop the
+ * reference to it. Errors are reported via @errp and by returning
+ * false.
+ *
+ * This function is useful if you have created @dev via qdev_new()
+ * (which takes a reference to the device it returns to you), so that
+ * you can set properties on it before realizing it. If you don't need
+ * to set properties then ssi_create_slave() is probably better (as it
+ * does the create, init and realize in one step).
+ *
+ * If you are embedding the SSI slave into another QOM device and
+ * initialized it via some variant on object_initialize_child() then
+ * do not use this function, because that family of functions arrange
+ * for the only reference to the child device to be held by the parent
+ * via the child<> property, and so the reference-count-drop done here
+ * would be incorrect.  (Instead you would want ssi_realize(), which
+ * doesn't currently exist but would be trivial to create if we had
+ * any code that wanted it.)
+ */
+bool ssi_realize_and_unref(DeviceState *dev, SSIBus *bus, Error **errp);
 
 /* Master interface.  */
 SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index 67b48c31cd6..a35d7ebb266 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -90,11 +90,16 @@ static const TypeInfo ssi_slave_info = {
     .abstract = true,
 };
 
+bool ssi_realize_and_unref(DeviceState *dev, SSIBus *bus, Error **errp)
+{
+    return qdev_realize_and_unref(dev, &bus->parent_obj, errp);
+}
+
 DeviceState *ssi_create_slave(SSIBus *bus, const char *name)
 {
     DeviceState *dev = qdev_new(name);
 
-    qdev_realize_and_unref(dev, &bus->parent_obj, &error_fatal);
+    ssi_realize_and_unref(dev, bus, &error_fatal);
     return dev;
 }
 
-- 
2.20.1

[PATCH 09/17] hw/arm/spitz: Use max111x properties to set initial values

[Peter Maydell]

Use the new max111x qdev properties to set the initial input
values rather than calling max111x_set_input(); this means that
on system reset the inputs will correctly return to their initial
values.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/spitz.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 11e413723f4..93a25edcb5b 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -731,11 +731,14 @@ 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 = ssi_create_slave(bus, "max1111");
+    sms->max1111 = qdev_new("max1111");
     max1111 = sms->max1111;
-    max111x_set_input(sms->max1111, MAX1111_BATT_VOLT, SPITZ_BATTERY_VOLT);
-    max111x_set_input(sms->max1111, MAX1111_BATT_TEMP, 0);
-    max111x_set_input(sms->max1111, MAX1111_ACIN_VOLT, SPITZ_CHARGEON_ACIN);
+    qdev_prop_set_uint8(sms->max1111, "input1" /* BATT_VOLT */,
+                        SPITZ_BATTERY_VOLT);
+    qdev_prop_set_uint8(sms->max1111, "input2" /* BATT_TEMP */, 0);
+    qdev_prop_set_uint8(sms->max1111, "input3" /* ACIN_VOLT */,
+                        SPITZ_CHARGEON_ACIN);
+    ssi_realize_and_unref(sms->max1111, bus, &error_fatal);
 
     qdev_connect_gpio_out(sms->mpu->gpio, SPITZ_GPIO_LCDCON_CS,
                         qdev_get_gpio_in(sms->mux, 0));
-- 
2.20.1

[PATCH 10/17] hw/misc/max111x: Use GPIO lines rather than max111x_set_input()

[Peter Maydell]

The max111x ADC device model allows other code to set the level on
the 8 ADC inputs using the max111x_set_input() function.  Replace
this with generic qdev GPIO inputs, which also allow inputs to be set
to arbitrary values.

Using GPIO lines will make it easier for board code to wire things
up, so that if device A wants to set the ADC input it doesn't need to
have a direct pointer to the max111x but can just set that value on
its output GPIO, which is then wired up by the board to the
appropriate max111x input.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/ssi/ssi.h |  3 ---
 hw/arm/spitz.c       |  9 +++++----
 hw/misc/max111x.c    | 16 +++++++++-------
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index 4be5325e654..5fd411f2e4e 100644
--- a/include/hw/ssi/ssi.h
+++ b/include/hw/ssi/ssi.h
@@ -111,7 +111,4 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
 
 uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
 
-/* max111x.c */
-void max111x_set_input(DeviceState *dev, int line, uint8_t value);
-
 #endif
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 93a25edcb5b..fa592aad6d6 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -696,13 +696,14 @@ static void corgi_ssp_gpio_cs(void *opaque, int line, int level)
 
 static void spitz_adc_temp_on(void *opaque, int line, int level)
 {
+    int batt_temp;
+
     if (!max1111)
         return;
 
-    if (level)
-        max111x_set_input(max1111, MAX1111_BATT_TEMP, SPITZ_BATTERY_TEMP);
-    else
-        max111x_set_input(max1111, MAX1111_BATT_TEMP, 0);
+    batt_temp = level ? SPITZ_BATTERY_TEMP : 0;
+
+    qemu_set_irq(qdev_get_gpio_in(max1111, MAX1111_BATT_TEMP), batt_temp);
 }
 
 static void corgi_ssp_realize(SSISlave *d, Error **errp)
diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
index abddfa3c660..3a5cb838445 100644
--- a/hw/misc/max111x.c
+++ b/hw/misc/max111x.c
@@ -131,12 +131,21 @@ static const VMStateDescription vmstate_max111x = {
     }
 };
 
+static void max111x_input_set(void *opaque, int line, int value)
+{
+    MAX111xState *s = MAX_111X(opaque);
+
+    assert(line >= 0 && line < s->inputs);
+    s->input[line] = value;
+}
+
 static int max111x_init(SSISlave *d, int inputs)
 {
     DeviceState *dev = DEVICE(d);
     MAX111xState *s = MAX_111X(dev);
 
     qdev_init_gpio_out(dev, &s->interrupt, 1);
+    qdev_init_gpio_in(dev, max111x_input_set, inputs);
 
     s->inputs = inputs;
 
@@ -153,13 +162,6 @@ static void max1111_realize(SSISlave *dev, Error **errp)
     max111x_init(dev, 4);
 }
 
-void max111x_set_input(DeviceState *dev, int line, uint8_t value)
-{
-    MAX111xState *s = MAX_111X(dev);
-    assert(line >= 0 && line < s->inputs);
-    s->input[line] = value;
-}
-
 static void max111x_reset(DeviceState *dev)
 {
     MAX111xState *s = MAX_111X(dev);
-- 
2.20.1

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

[Peter Maydell]

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"
+
+/*
+ * 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)
+
+#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"
-#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
-- 
2.20.1

[PATCH 12/17] hw/arm/spitz: Encapsulate misc GPIO handling in a device

[Peter Maydell]

Currently we have a free-floating set of IRQs and a function
spitz_out_switch() which handle some miscellaneous GPIO lines for the
spitz board.  Encapsulate this behaviour in a simple QOM device.

At this point we can finally remove the 'max1111' global, because the
ADC battery-temperature value is now handled by the misc-gpio device
writing the value to its outbound "adc-temp" GPIO, which the board
code wires up to the appropriate inbound GPIO line on the max1111.

This commit also fixes Coverity issue CID 1421913 (which pointed out
that the 'outsignals' in spitz_scoop_gpio_setup() were leaked),
because it removes the use of the qemu_allocate_irqs() API from this
code entirely.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/spitz.c | 129 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 87 insertions(+), 42 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 1400a56729d..bab9968ccee 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -51,6 +51,7 @@ typedef struct {
     DeviceState *max1111;
     DeviceState *scp0;
     DeviceState *scp1;
+    DeviceState *misc_gpio;
 } SpitzMachineState;
 
 #define TYPE_SPITZ_MACHINE "spitz-common"
@@ -658,8 +659,6 @@ static void spitz_lcdtg_realize(SSISlave *ssi, Error **errp)
 #define SPITZ_GPIO_MAX1111_CS   20
 #define SPITZ_GPIO_TP_INT       11
 
-static DeviceState *max1111;
-
 /* "Demux" the signal based on current chipselect */
 typedef struct {
     SSISlave ssidev;
@@ -695,18 +694,6 @@ static void corgi_ssp_gpio_cs(void *opaque, int line, int level)
 #define SPITZ_BATTERY_VOLT      0xd0    /* About 4.0V */
 #define SPITZ_CHARGEON_ACIN     0x80    /* About 5.0V */
 
-static void spitz_adc_temp_on(void *opaque, int line, int level)
-{
-    int batt_temp;
-
-    if (!max1111)
-        return;
-
-    batt_temp = level ? SPITZ_BATTERY_TEMP : 0;
-
-    qemu_set_irq(qdev_get_gpio_in(max1111, MAX1111_BATT_TEMP), batt_temp);
-}
-
 static void corgi_ssp_realize(SSISlave *d, Error **errp)
 {
     DeviceState *dev = DEVICE(d);
@@ -734,7 +721,6 @@ static void spitz_ssp_attach(SpitzMachineState *sms)
 
     bus = qdev_get_child_bus(sms->mux, "ssi2");
     sms->max1111 = qdev_new(TYPE_MAX_1111);
-    max1111 = sms->max1111;
     qdev_prop_set_uint8(sms->max1111, "input1" /* BATT_VOLT */,
                         SPITZ_BATTERY_VOLT);
     qdev_prop_set_uint8(sms->max1111, "input2" /* BATT_TEMP */, 0);
@@ -810,27 +796,66 @@ static void spitz_akita_i2c_setup(PXA2xxState *cpu)
 
 /* Other peripherals */
 
-static void spitz_out_switch(void *opaque, int line, int level)
+/*
+ * Encapsulation of some miscellaneous GPIO line behaviour for the Spitz boards.
+ *
+ * QEMU interface:
+ *  + named GPIO inputs "green-led", "orange-led", "charging", "discharging":
+ *    these currently just print messages that the line has been signalled
+ *  + named GPIO input "adc-temp-on": set to cause the battery-temperature
+ *    value to be passed to the max111x ADC
+ *  + named GPIO output "adc-temp": the ADC value, to be wired up to the max111x
+ */
+#define TYPE_SPITZ_MISC_GPIO "spitz-misc-gpio"
+#define SPITZ_MISC_GPIO(obj) \
+    OBJECT_CHECK(SpitzMiscGPIOState, (obj), TYPE_SPITZ_MISC_GPIO)
+
+typedef struct SpitzMiscGPIOState {
+    SysBusDevice parent_obj;
+
+    qemu_irq adc_value;
+} SpitzMiscGPIOState;
+
+static void spitz_misc_charging(void *opaque, int n, int level)
 {
-    switch (line) {
-    case 0:
-        zaurus_printf("Charging %s.\n", level ? "off" : "on");
-        break;
-    case 1:
-        zaurus_printf("Discharging %s.\n", level ? "on" : "off");
-        break;
-    case 2:
-        zaurus_printf("Green LED %s.\n", level ? "on" : "off");
-        break;
-    case 3:
-        zaurus_printf("Orange LED %s.\n", level ? "on" : "off");
-        break;
-    case 6:
-        spitz_adc_temp_on(opaque, line, level);
-        break;
-    default:
-        g_assert_not_reached();
-    }
+    zaurus_printf("Charging %s.\n", level ? "off" : "on");
+}
+
+static void spitz_misc_discharging(void *opaque, int n, int level)
+{
+    zaurus_printf("Discharging %s.\n", level ? "off" : "on");
+}
+
+static void spitz_misc_green_led(void *opaque, int n, int level)
+{
+    zaurus_printf("Green LED %s.\n", level ? "off" : "on");
+}
+
+static void spitz_misc_orange_led(void *opaque, int n, int level)
+{
+    zaurus_printf("Orange LED %s.\n", level ? "off" : "on");
+}
+
+static void spitz_misc_adc_temp(void *opaque, int n, int level)
+{
+    SpitzMiscGPIOState *s = SPITZ_MISC_GPIO(opaque);
+    int batt_temp = level ? SPITZ_BATTERY_TEMP : 0;
+
+    qemu_set_irq(s->adc_value, batt_temp);
+}
+
+static void spitz_misc_gpio_init(Object *obj)
+{
+    SpitzMiscGPIOState *s = SPITZ_MISC_GPIO(obj);
+    DeviceState *dev = DEVICE(obj);
+
+    qdev_init_gpio_in_named(dev, spitz_misc_charging, "charging", 1);
+    qdev_init_gpio_in_named(dev, spitz_misc_discharging, "discharging", 1);
+    qdev_init_gpio_in_named(dev, spitz_misc_green_led, "green-led", 1);
+    qdev_init_gpio_in_named(dev, spitz_misc_orange_led, "orange-led", 1);
+    qdev_init_gpio_in_named(dev, spitz_misc_adc_temp, "adc-temp-on", 1);
+
+    qdev_init_gpio_out_named(dev, &s->adc_value, "adc-temp", 1);
 }
 
 #define SPITZ_SCP_LED_GREEN             1
@@ -850,12 +875,22 @@ static void spitz_out_switch(void *opaque, int line, int level)
 
 static void spitz_scoop_gpio_setup(SpitzMachineState *sms)
 {
-    qemu_irq *outsignals = qemu_allocate_irqs(spitz_out_switch, sms->mpu, 8);
+    DeviceState *miscdev = sysbus_create_simple(TYPE_SPITZ_MISC_GPIO, -1, NULL);
 
-    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_CHRG_ON, outsignals[0]);
-    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_JK_B, outsignals[1]);
-    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_GREEN, outsignals[2]);
-    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_ORANGE, outsignals[3]);
+    sms->misc_gpio = miscdev;
+
+    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_CHRG_ON,
+                          qdev_get_gpio_in_named(miscdev, "charging", 0));
+    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_JK_B,
+                          qdev_get_gpio_in_named(miscdev, "discharging", 0));
+    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_GREEN,
+                          qdev_get_gpio_in_named(miscdev, "green-led", 0));
+    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_ORANGE,
+                          qdev_get_gpio_in_named(miscdev, "orange-led", 0));
+    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_ADC_TEMP_ON,
+                          qdev_get_gpio_in_named(miscdev, "adc-temp-on", 0));
+    qdev_connect_gpio_out_named(miscdev, "adc-temp", 0,
+                                qdev_get_gpio_in(sms->max1111, MAX1111_BATT_TEMP));
 
     if (sms->scp1) {
         qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_CONT,
@@ -863,8 +898,6 @@ static void spitz_scoop_gpio_setup(SpitzMachineState *sms)
         qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_ON,
                               qdev_get_gpio_in_named(sms->lcdtg, "bl_power", 0));
     }
-
-    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
 }
 
 #define SPITZ_GPIO_HSYNC                22
@@ -1217,12 +1250,24 @@ static const TypeInfo spitz_lcdtg_info = {
     .class_init    = spitz_lcdtg_class_init,
 };
 
+static const TypeInfo spitz_misc_gpio_info = {
+    .name = TYPE_SPITZ_MISC_GPIO,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(SpitzMiscGPIOState),
+    .instance_init = spitz_misc_gpio_init,
+    /*
+     * No class_init required: device has no internal state so does not
+     * need to set up reset or vmstate, and does not have a realize method.
+     */
+};
+
 static void spitz_register_types(void)
 {
     type_register_static(&corgi_ssp_info);
     type_register_static(&spitz_lcdtg_info);
     type_register_static(&spitz_keyboard_info);
     type_register_static(&sl_nand_info);
+    type_register_static(&spitz_misc_gpio_info);
 }
 
 type_init(spitz_register_types)
-- 
2.20.1

[PATCH 13/17] hw/gpio/zaurus.c: Use LOG_GUEST_ERROR for bad guest register accesses

[Peter Maydell]

Instead of logging guest accesses to invalid register offsets in this
device using zaurus_printf() (which just prints to stderr), use the
usual qemu_log_mask(LOG_GUEST_ERROR,...).

Since this was the only use of the zaurus_printf() macro outside
spitz.c, we can move the definition of that macro from sharpsl.h
to spitz.c.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/sharpsl.h |  3 ---
 hw/arm/spitz.c           |  3 +++
 hw/gpio/zaurus.c         | 12 +++++++-----
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/hw/arm/sharpsl.h b/include/hw/arm/sharpsl.h
index 89e168fbff3..e986b28c527 100644
--- a/include/hw/arm/sharpsl.h
+++ b/include/hw/arm/sharpsl.h
@@ -9,9 +9,6 @@
 
 #include "exec/hwaddr.h"
 
-#define zaurus_printf(format, ...)	\
-    fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
-
 /* zaurus.c */
 
 #define SL_PXA_PARAM_BASE	0xa0000a00
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index bab9968ccee..6eb46869157 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -62,6 +62,9 @@ typedef struct {
 #define SPITZ_MACHINE_CLASS(klass) \
     OBJECT_CLASS_CHECK(SpitzMachineClass, klass, TYPE_SPITZ_MACHINE)
 
+#define zaurus_printf(format, ...)                              \
+    fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
+
 #undef REG_FMT
 #define REG_FMT                         "0x%02lx"
 
diff --git a/hw/gpio/zaurus.c b/hw/gpio/zaurus.c
index 9a12c683420..258e9264930 100644
--- a/hw/gpio/zaurus.c
+++ b/hw/gpio/zaurus.c
@@ -22,9 +22,7 @@
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
-
-#undef REG_FMT
-#define REG_FMT			"0x%02lx"
+#include "qemu/log.h"
 
 /* SCOOP devices */
 
@@ -104,7 +102,9 @@ static uint64_t scoop_read(void *opaque, hwaddr addr,
     case SCOOP_GPRR:
         return s->gpio_level;
     default:
-        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "scoop_read: bad register offset 0x%02" HWADDR_PRIx "\n",
+                      addr);
     }
 
     return 0;
@@ -150,7 +150,9 @@ static void scoop_write(void *opaque, hwaddr addr,
         scoop_gpio_handler_update(s);
         break;
     default:
-        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "scoop_write: bad register offset 0x%02" HWADDR_PRIx "\n",
+                      addr);
     }
 }
 
-- 
2.20.1

[PATCH 14/17] hw/arm/spitz: Use LOG_GUEST_ERROR for bad guest register accesses

[Peter Maydell]

Instead of logging guest accesses to invalid register offsets in the
Spitz flash device with zaurus_printf() (which just prints to stderr),
use the usual qemu_log_mask(LOG_GUEST_ERROR,...).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/spitz.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 6eb46869157..49eae3fce4e 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -23,6 +23,7 @@
 #include "hw/ssi/ssi.h"
 #include "hw/block/flash.h"
 #include "qemu/timer.h"
+#include "qemu/log.h"
 #include "hw/arm/sharpsl.h"
 #include "ui/console.h"
 #include "hw/audio/wm8750.h"
@@ -65,9 +66,6 @@ typedef struct {
 #define zaurus_printf(format, ...)                              \
     fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
 
-#undef REG_FMT
-#define REG_FMT                         "0x%02lx"
-
 /* Spitz Flash */
 #define FLASH_BASE              0x0c000000
 #define FLASH_ECCLPLB           0x00    /* Line parity 7 - 0 bit */
@@ -137,7 +135,9 @@ static uint64_t sl_read(void *opaque, hwaddr addr, unsigned size)
         return ecc_digest(&s->ecc, nand_getio(s->nand));
 
     default:
-        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "sl_read: bad register offset 0x%02" HWADDR_PRIx "\n",
+                      addr);
     }
     return 0;
 }
@@ -168,7 +168,9 @@ static void sl_write(void *opaque, hwaddr addr,
         break;
 
     default:
-        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "sl_write: bad register offset 0x%02" HWADDR_PRIx "\n",
+                      addr);
     }
 }
 
-- 
2.20.1

[PATCH 15/17] hw/arm/pxa2xx_pic: Use LOG_GUEST_ERROR for bad guest register accesses

[Peter Maydell]

Instead of using printf() for logging guest accesses to invalid
register offsets in the pxa2xx PIC device, use the usual
qemu_log_mask(LOG_GUEST_ERROR,...).

This was the only user of the REG_FMT macro in pxa.h, so we can
remove that.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/pxa.h | 1 -
 hw/arm/pxa2xx_pic.c  | 9 +++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h
index f6dfb5c0cf0..8843e5f9107 100644
--- a/include/hw/arm/pxa.h
+++ b/include/hw/arm/pxa.h
@@ -184,7 +184,6 @@ struct PXA2xxI2SState {
 };
 
 # define PA_FMT			"0x%08lx"
-# define REG_FMT		"0x" TARGET_FMT_plx
 
 PXA2xxState *pxa270_init(MemoryRegion *address_space, unsigned int sdram_size,
                          const char *revision);
diff --git a/hw/arm/pxa2xx_pic.c b/hw/arm/pxa2xx_pic.c
index 105c5e63f2f..ceee6aa48db 100644
--- a/hw/arm/pxa2xx_pic.c
+++ b/hw/arm/pxa2xx_pic.c
@@ -11,6 +11,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "qemu/log.h"
 #include "cpu.h"
 #include "hw/arm/pxa.h"
 #include "hw/sysbus.h"
@@ -166,7 +167,9 @@ static uint64_t pxa2xx_pic_mem_read(void *opaque, hwaddr offset,
     case ICHP:	/* Highest Priority register */
         return pxa2xx_pic_highest(s);
     default:
-        printf("%s: Bad register offset " REG_FMT "\n", __func__, offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pxa2xx_pic_mem_read: bad register offset 0x%" HWADDR_PRIx
+                      "\n", offset);
         return 0;
     }
 }
@@ -199,7 +202,9 @@ static void pxa2xx_pic_mem_write(void *opaque, hwaddr offset,
         s->priority[32 + ((offset - IPR32) >> 2)] = value & 0x8000003f;
         break;
     default:
-        printf("%s: Bad register offset " REG_FMT "\n", __func__, offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pxa2xx_pic_mem_write: bad register offset 0x%"
+                      HWADDR_PRIx "\n", offset);
         return;
     }
     pxa2xx_pic_update(opaque);
-- 
2.20.1

[PATCH 16/17] hw/arm/spitz: Provide usual QOM macros for corgi-ssp and spitz-lcdtg

[Peter Maydell]

The QOM types "spitz-lcdtg" and "corgi-ssp" are missing the
usual QOM TYPE and casting macros; provide and use them.

In particular, we can safely use the QOM cast macros instead of
FROM_SSI_SLAVE() because in both cases the 'ssidev' field of
the instance state struct is the first field in it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/spitz.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 49eae3fce4e..f020aff9747 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -579,6 +579,9 @@ static void spitz_keyboard_realize(DeviceState *dev, Error **errp)
 #define LCDTG_PICTRL    0x06
 #define LCDTG_POLCTRL   0x07
 
+#define TYPE_SPITZ_LCDTG "spitz-lcdtg"
+#define SPITZ_LCDTG(obj) OBJECT_CHECK(SpitzLCDTG, (obj), TYPE_SPITZ_LCDTG)
+
 typedef struct {
     SSISlave ssidev;
     uint32_t bl_intensity;
@@ -616,7 +619,7 @@ static inline void spitz_bl_power(void *opaque, int line, int level)
 
 static uint32_t spitz_lcdtg_transfer(SSISlave *dev, uint32_t value)
 {
-    SpitzLCDTG *s = FROM_SSI_SLAVE(SpitzLCDTG, dev);
+    SpitzLCDTG *s = SPITZ_LCDTG(dev);
     int addr;
     addr = value >> 5;
     value &= 0x1f;
@@ -645,7 +648,7 @@ static uint32_t spitz_lcdtg_transfer(SSISlave *dev, uint32_t value)
 
 static void spitz_lcdtg_realize(SSISlave *ssi, Error **errp)
 {
-    SpitzLCDTG *s = FROM_SSI_SLAVE(SpitzLCDTG, ssi);
+    SpitzLCDTG *s = SPITZ_LCDTG(ssi);
     DeviceState *dev = DEVICE(s);
 
     s->bl_power = 0;
@@ -664,6 +667,9 @@ static void spitz_lcdtg_realize(SSISlave *ssi, Error **errp)
 #define SPITZ_GPIO_MAX1111_CS   20
 #define SPITZ_GPIO_TP_INT       11
 
+#define TYPE_CORGI_SSP "corgi-ssp"
+#define CORGI_SSP(obj) OBJECT_CHECK(CorgiSSPState, (obj), TYPE_CORGI_SSP)
+
 /* "Demux" the signal based on current chipselect */
 typedef struct {
     SSISlave ssidev;
@@ -673,7 +679,7 @@ typedef struct {
 
 static uint32_t corgi_ssp_transfer(SSISlave *dev, uint32_t value)
 {
-    CorgiSSPState *s = FROM_SSI_SLAVE(CorgiSSPState, dev);
+    CorgiSSPState *s = CORGI_SSP(dev);
     int i;
 
     for (i = 0; i < 3; i++) {
@@ -702,7 +708,7 @@ static void corgi_ssp_gpio_cs(void *opaque, int line, int level)
 static void corgi_ssp_realize(SSISlave *d, Error **errp)
 {
     DeviceState *dev = DEVICE(d);
-    CorgiSSPState *s = FROM_SSI_SLAVE(CorgiSSPState, d);
+    CorgiSSPState *s = CORGI_SSP(d);
 
     qdev_init_gpio_in(dev, corgi_ssp_gpio_cs, 3);
     s->bus[0] = ssi_create_bus(dev, "ssi0");
@@ -714,10 +720,11 @@ static void spitz_ssp_attach(SpitzMachineState *sms)
 {
     void *bus;
 
-    sms->mux = ssi_create_slave(sms->mpu->ssp[CORGI_SSP_PORT - 1], "corgi-ssp");
+    sms->mux = ssi_create_slave(sms->mpu->ssp[CORGI_SSP_PORT - 1],
+                                TYPE_CORGI_SSP);
 
     bus = qdev_get_child_bus(sms->mux, "ssi0");
-    sms->lcdtg = ssi_create_slave(bus, "spitz-lcdtg");
+    sms->lcdtg = ssi_create_slave(bus, TYPE_SPITZ_LCDTG);
 
     bus = qdev_get_child_bus(sms->mux, "ssi1");
     sms->ads7846 = ssi_create_slave(bus, "ads7846");
@@ -1220,7 +1227,7 @@ static void corgi_ssp_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo corgi_ssp_info = {
-    .name          = "corgi-ssp",
+    .name          = TYPE_CORGI_SSP,
     .parent        = TYPE_SSI_SLAVE,
     .instance_size = sizeof(CorgiSSPState),
     .class_init    = corgi_ssp_class_init,
@@ -1249,7 +1256,7 @@ static void spitz_lcdtg_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo spitz_lcdtg_info = {
-    .name          = "spitz-lcdtg",
+    .name          = TYPE_SPITZ_LCDTG,
     .parent        = TYPE_SSI_SLAVE,
     .instance_size = sizeof(SpitzLCDTG),
     .class_init    = spitz_lcdtg_class_init,
-- 
2.20.1

[PATCH 17/17] Replace uses of FROM_SSI_SLAVE() macro with QOM casts

[Peter Maydell]

The FROM_SSI_SLAVE() macro predates QOM and is used as a typesafe way
to cast from an SSISlave* to the instance struct of a subtype of
TYPE_SSI_SLAVE.  Switch to using the QOM cast macros instead, which
have the same effect (by writing the QOM macros if the types were
previously missing them.)

(The FROM_SSI_SLAVE() macro allows the SSISlave member of the
subtype's struct to be anywhere as long as it is named "ssidev",
whereas a QOM cast macro insists that it is the first thing in the
subtype's struct.  This is true for all the types we convert here.)

This removes all the uses of FROM_SSI_SLAVE() so we can delete the
definition.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/ssi/ssi.h |  2 --
 hw/arm/z2.c          | 11 +++++++----
 hw/display/ads7846.c |  9 ++++++---
 hw/display/ssd0323.c | 10 +++++++---
 hw/sd/ssi-sd.c       |  4 ++--
 5 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index 5fd411f2e4e..eac168aa1db 100644
--- a/include/hw/ssi/ssi.h
+++ b/include/hw/ssi/ssi.h
@@ -66,8 +66,6 @@ struct SSISlave {
     bool cs;
 };
 
-#define FROM_SSI_SLAVE(type, dev) DO_UPCAST(type, ssidev, dev)
-
 extern const VMStateDescription vmstate_ssi_slave;
 
 #define VMSTATE_SSI_SLAVE(_field, _state) {                          \
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index a0f40959904..e1f22f58681 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -111,9 +111,12 @@ typedef struct {
     int pos;
 } ZipitLCD;
 
+#define TYPE_ZIPIT_LCD "zipit-lcd"
+#define ZIPIT_LCD(obj) OBJECT_CHECK(ZipitLCD, (obj), TYPE_ZIPIT_LCD)
+
 static uint32_t zipit_lcd_transfer(SSISlave *dev, uint32_t value)
 {
-    ZipitLCD *z = FROM_SSI_SLAVE(ZipitLCD, dev);
+    ZipitLCD *z = ZIPIT_LCD(dev);
     uint16_t val;
     if (z->selected) {
         z->buf[z->pos] = value & 0xff;
@@ -153,7 +156,7 @@ static void z2_lcd_cs(void *opaque, int line, int level)
 
 static void zipit_lcd_realize(SSISlave *dev, Error **errp)
 {
-    ZipitLCD *z = FROM_SSI_SLAVE(ZipitLCD, dev);
+    ZipitLCD *z = ZIPIT_LCD(dev);
     z->selected = 0;
     z->enabled = 0;
     z->pos = 0;
@@ -185,7 +188,7 @@ static void zipit_lcd_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo zipit_lcd_info = {
-    .name          = "zipit-lcd",
+    .name          = TYPE_ZIPIT_LCD,
     .parent        = TYPE_SSI_SLAVE,
     .instance_size = sizeof(ZipitLCD),
     .class_init    = zipit_lcd_class_init,
@@ -325,7 +328,7 @@ static void z2_init(MachineState *machine)
 
     type_register_static(&zipit_lcd_info);
     type_register_static(&aer915_info);
-    z2_lcd = ssi_create_slave(mpu->ssp[1], "zipit-lcd");
+    z2_lcd = ssi_create_slave(mpu->ssp[1], TYPE_ZIPIT_LCD);
     bus = pxa2xx_i2c_bus(mpu->i2c[0]);
     i2c_create_slave(bus, TYPE_AER915, 0x55);
     wm = i2c_create_slave(bus, TYPE_WM8750, 0x1b);
diff --git a/hw/display/ads7846.c b/hw/display/ads7846.c
index 9228b40b1af..56bf82fe079 100644
--- a/hw/display/ads7846.c
+++ b/hw/display/ads7846.c
@@ -29,6 +29,9 @@ typedef struct {
     int output;
 } ADS7846State;
 
+#define TYPE_ADS7846 "ads7846"
+#define ADS7846(obj) OBJECT_CHECK(ADS7846State, (obj), TYPE_ADS7846)
+
 /* Control-byte bitfields */
 #define CB_PD0		(1 << 0)
 #define CB_PD1		(1 << 1)
@@ -61,7 +64,7 @@ static void ads7846_int_update(ADS7846State *s)
 
 static uint32_t ads7846_transfer(SSISlave *dev, uint32_t value)
 {
-    ADS7846State *s = FROM_SSI_SLAVE(ADS7846State, dev);
+    ADS7846State *s = ADS7846(dev);
 
     switch (s->cycle ++) {
     case 0:
@@ -139,7 +142,7 @@ static const VMStateDescription vmstate_ads7846 = {
 static void ads7846_realize(SSISlave *d, Error **errp)
 {
     DeviceState *dev = DEVICE(d);
-    ADS7846State *s = FROM_SSI_SLAVE(ADS7846State, d);
+    ADS7846State *s = ADS7846(d);
 
     qdev_init_gpio_out(dev, &s->interrupt, 1);
 
@@ -166,7 +169,7 @@ static void ads7846_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo ads7846_info = {
-    .name          = "ads7846",
+    .name          = TYPE_ADS7846,
     .parent        = TYPE_SSI_SLAVE,
     .instance_size = sizeof(ADS7846State),
     .class_init    = ads7846_class_init,
diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
index c3bdb18742c..32d27f008ae 100644
--- a/hw/display/ssd0323.c
+++ b/hw/display/ssd0323.c
@@ -66,9 +66,13 @@ typedef struct {
     uint8_t framebuffer[128 * 80 / 2];
 } ssd0323_state;
 
+#define TYPE_SSD0323 "ssd0323"
+#define SSD0323(obj) OBJECT_CHECK(ssd0323_state, (obj), TYPE_SSD0323)
+
+
 static uint32_t ssd0323_transfer(SSISlave *dev, uint32_t data)
 {
-    ssd0323_state *s = FROM_SSI_SLAVE(ssd0323_state, dev);
+    ssd0323_state *s = SSD0323(dev);
 
     switch (s->mode) {
     case SSD0323_DATA:
@@ -346,7 +350,7 @@ static const GraphicHwOps ssd0323_ops = {
 static void ssd0323_realize(SSISlave *d, Error **errp)
 {
     DeviceState *dev = DEVICE(d);
-    ssd0323_state *s = FROM_SSI_SLAVE(ssd0323_state, d);
+    ssd0323_state *s = SSD0323(d);
 
     s->col_end = 63;
     s->row_end = 79;
@@ -368,7 +372,7 @@ static void ssd0323_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo ssd0323_info = {
-    .name          = "ssd0323",
+    .name          = TYPE_SSD0323,
     .parent        = TYPE_SSI_SLAVE,
     .instance_size = sizeof(ssd0323_state),
     .class_init    = ssd0323_class_init,
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 25cec2ddeaa..25cdf4c966d 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -74,7 +74,7 @@ typedef struct {
 
 static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
 {
-    ssi_sd_state *s = FROM_SSI_SLAVE(ssi_sd_state, dev);
+    ssi_sd_state *s = SSI_SD(dev);
 
     /* Special case: allow CMD12 (STOP TRANSMISSION) while reading data.  */
     if (s->mode == SSI_SD_DATA_READ && val == 0x4d) {
@@ -241,7 +241,7 @@ static const VMStateDescription vmstate_ssi_sd = {
 
 static void ssi_sd_realize(SSISlave *d, Error **errp)
 {
-    ssi_sd_state *s = FROM_SSI_SLAVE(ssi_sd_state, d);
+    ssi_sd_state *s = SSI_SD(d);
     DeviceState *carddev;
     DriveInfo *dinfo;
     Error *err = NULL;
-- 
2.20.1

Re: [PATCH 00/17] spitz: fix hacks, fix CID 1421913, various cleanups

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200628142429.17111-1-peter.maydell@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 00/17] spitz: fix hacks, fix CID 1421913, various cleanups
Type: series
Message-id: 20200628142429.17111-1-peter.maydell@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
   553cf5d..e765115  master     -> master
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200628142429.17111-1-peter.maydell@linaro.org -> patchew/20200628142429.17111-1-peter.maydell@linaro.org
Switched to a new branch 'test'
108665c Replace uses of FROM_SSI_SLAVE() macro with QOM casts
6c99757 hw/arm/spitz: Provide usual QOM macros for corgi-ssp and spitz-lcdtg
98348e5 hw/arm/pxa2xx_pic: Use LOG_GUEST_ERROR for bad guest register accesses
0c5e2a2 hw/arm/spitz: Use LOG_GUEST_ERROR for bad guest register accesses
7856c52 hw/gpio/zaurus.c: Use LOG_GUEST_ERROR for bad guest register accesses
45b5766 hw/arm/spitz: Encapsulate misc GPIO handling in a device
7c67922 hw/misc/max111x: Create header file for documentation, TYPE_ macros
952c610 hw/misc/max111x: Use GPIO lines rather than max111x_set_input()
8febdff hw/arm/spitz: Use max111x properties to set initial values
9a1ed3c ssi: Add ssi_realize_and_unref()
efa4918 hw/misc/max111x: Don't use vmstate_register()
ccc835d hw/misc/max111x: provide QOM properties for setting initial values
9e5c852 hw/arm/spitz: Implement inbound GPIO lines for bit5 and power signals
0ec3ef7 hw/arm/spitz: Keep pointers to scp0, scp1 in SpitzMachineState
7be6379 hw/arm/spitz: Keep pointers to MPU and SSI devices in SpitzMachineState
8059a4e hw/arm/spitz: Create SpitzMachineClass abstract base class
736d97f hw/arm/spitz: Detabify

=== OUTPUT BEGIN ===
1/17 Checking commit 736d97fd84fb (hw/arm/spitz: Detabify)
ERROR: space prohibited before that '++' (ctx:WxB)
#110: FILE: hw/arm/spitz.c:303:
+#define QUEUE_KEY(c)    s->fifo[(s->fifopos + s->fifolen ++) & 0xf] = c
                                                          ^

ERROR: Macros with complex values should be enclosed in parenthesis
#110: FILE: hw/arm/spitz.c:303:
+#define QUEUE_KEY(c)    s->fifo[(s->fifopos + s->fifolen ++) & 0xf] = c

total: 2 errors, 0 warnings, 259 lines checked

Patch 1/17 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/17 Checking commit 8059a4e54d2b (hw/arm/spitz: Create SpitzMachineClass abstract base class)
3/17 Checking commit 7be6379c8f45 (hw/arm/spitz: Keep pointers to MPU and SSI devices in SpitzMachineState)
4/17 Checking commit 0ec3ef7a8701 (hw/arm/spitz: Keep pointers to scp0, scp1 in SpitzMachineState)
5/17 Checking commit 9e5c852ff9ad (hw/arm/spitz: Implement inbound GPIO lines for bit5 and power signals)
WARNING: line over 80 characters
#94: FILE: hw/arm/spitz.c:859:
+                              qdev_get_gpio_in_named(sms->lcdtg, "bl_power", 0));

total: 0 errors, 1 warnings, 68 lines checked

Patch 5/17 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/17 Checking commit ccc835da8b9e (hw/misc/max111x: provide QOM properties for setting initial values)
7/17 Checking commit efa4918480f8 (hw/misc/max111x: Don't use vmstate_register())
8/17 Checking commit 9a1ed3c4a634 (ssi: Add ssi_realize_and_unref())
9/17 Checking commit 8febdffc3c42 (hw/arm/spitz: Use max111x properties to set initial values)
WARNING: Block comments use a leading /* on a separate line
#29: FILE: hw/arm/spitz.c:736:
+    qdev_prop_set_uint8(sms->max1111, "input1" /* BATT_VOLT */,

WARNING: Block comments use a leading /* on a separate line
#31: FILE: hw/arm/spitz.c:738:
+    qdev_prop_set_uint8(sms->max1111, "input2" /* BATT_TEMP */, 0);

WARNING: Block comments use a leading /* on a separate line
#32: FILE: hw/arm/spitz.c:739:
+    qdev_prop_set_uint8(sms->max1111, "input3" /* ACIN_VOLT */,

total: 0 errors, 3 warnings, 18 lines checked

Patch 9/17 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
10/17 Checking commit 952c610248da (hw/misc/max111x: Use GPIO lines rather than max111x_set_input())
11/17 Checking commit 7c6792207add (hw/misc/max111x: Create header file for documentation, TYPE_ macros)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#94: 
new file mode 100644

total: 0 errors, 1 warnings, 114 lines checked

Patch 11/17 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/17 Checking commit 45b57669dce5 (hw/arm/spitz: Encapsulate misc GPIO handling in a device)
WARNING: line over 80 characters
#183: FILE: hw/arm/spitz.c:893:
+                                qdev_get_gpio_in(sms->max1111, MAX1111_BATT_TEMP));

total: 0 errors, 1 warnings, 185 lines checked

Patch 12/17 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
13/17 Checking commit 7856c52c4f9e (hw/gpio/zaurus.c: Use LOG_GUEST_ERROR for bad guest register accesses)
14/17 Checking commit 0c5e2a25d7ce (hw/arm/spitz: Use LOG_GUEST_ERROR for bad guest register accesses)
15/17 Checking commit 98348e514804 (hw/arm/pxa2xx_pic: Use LOG_GUEST_ERROR for bad guest register accesses)
16/17 Checking commit 6c9975774bb7 (hw/arm/spitz: Provide usual QOM macros for corgi-ssp and spitz-lcdtg)
17/17 Checking commit 108665c37c6d (Replace uses of FROM_SSI_SLAVE() macro with QOM casts)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200628142429.17111-1-peter.maydell@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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
> 

Re: [PATCH 01/17] hw/arm/spitz: Detabify

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> The spitz board has been around a long time, and still has a fair number
> of hard-coded tab characters in it. We're about to do some work on
> this source file, so start out by expanding out the tabs.
> 
> This commit is a pure whitespace only change.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Couple of checkpatch errors due to the QUEUE_KEY macro which can
> be ignored as this is just a detabify.
> ---
>  hw/arm/spitz.c | 156 ++++++++++++++++++++++++-------------------------
>  1 file changed, 78 insertions(+), 78 deletions(-)

'git-diff -w' -> no change.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 02/17] hw/arm/spitz: Create SpitzMachineClass abstract base class

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> For the four Spitz-family machines (akita, borzoi, spitz, terrier)
> create a proper abstract class SpitzMachineClass which encapsulates
> the common behaviour, rather than having them all derive directly
> from TYPE_MACHINE:
>  * instead of each machine class setting mc->init to a wrapper
>    function which calls spitz_common_init() with parameters,
>    put that data in the SpitzMachineClass and make spitz_common_init
>    the SpitzMachineClass machine-init function
>  * move the settings of mc->block_default_type and
>    mc->ignore_memory_transaction_failures into the SpitzMachineClass
>    class init rather than repeating them in each machine's class init
> 
> (The motivation is that we're going to want to keep some state in
> the SpitzMachineState so we can connect GPIOs between devices created
> in one sub-function of the machine init to devices created in a
> different sub-function.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/spitz.c | 91 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 55 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 9eaedab79b5..c70e912a33d 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -33,6 +33,26 @@
>  #include "exec/address-spaces.h"
>  #include "cpu.h"
>  
> +enum spitz_model_e { spitz, akita, borzoi, terrier };
> +
> +typedef struct {
> +    MachineClass parent;
> +    enum spitz_model_e model;

Nitpick, I'd drop the not very useful typedef and use
directly ...:

       enum { spitz, akita, borzoi, terrier } model

> +    int arm_id;
> +} SpitzMachineClass;
> +
> +typedef struct {
> +    MachineState parent;
> +} SpitzMachineState;
> +
> +#define TYPE_SPITZ_MACHINE "spitz-common"
> +#define SPITZ_MACHINE(obj) \
> +    OBJECT_CHECK(SpitzMachineState, obj, TYPE_SPITZ_MACHINE)
> +#define SPITZ_MACHINE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(SpitzMachineClass, obj, TYPE_SPITZ_MACHINE)
> +#define SPITZ_MACHINE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(SpitzMachineClass, klass, TYPE_SPITZ_MACHINE)
> +
>  #undef REG_FMT
>  #define REG_FMT                         "0x%02lx"
>  
> @@ -905,8 +925,6 @@ static void spitz_gpio_setup(PXA2xxState *cpu, int slots)
>  }
>  
>  /* Board init.  */
> -enum spitz_model_e { spitz, akita, borzoi, terrier };
> -
>  #define SPITZ_RAM       0x04000000
>  #define SPITZ_ROM       0x00800000
>  
> @@ -915,9 +933,10 @@ static struct arm_boot_info spitz_binfo = {
>      .ram_size = 0x04000000,
>  };
>  
> -static void spitz_common_init(MachineState *machine,
> -                              enum spitz_model_e model, int arm_id)
> +static void spitz_common_init(MachineState *machine)
>  {
> +    SpitzMachineClass *smc = SPITZ_MACHINE_GET_CLASS(machine);
> +    enum spitz_model_e model = smc->model;

... and use 'smc->model' in place.

Patch easier to review with 'git-diff -W' [*].

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[*] Content of my .gitattributes:

$ cat .gitattributes
*.c         diff=c
*.cpp       diff=cpp
*.m         text diff=objc
*.h         diff=c
*.py        diff=python
*.json      text
*.pl        text diff=perl
*.sh        text eol=lf
*.xml       text
*.yml       text
*.bz2       binary

Re: [PATCH 06/17] hw/misc/max111x: provide QOM properties for setting initial values

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Add some QOM properties to the max111x ADC device to allow the
> initial values to be configured. Currently this is done by
> board code calling max111x_set_input() after it creates the
> device, which doesn't work on system reset.
> 
> This requires us to implement a reset method for this device,
> so while we're doing that make sure we reset the other parts
> of the device state.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/misc/max111x.c | 57 ++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
> index 2b87bdee5b7..d0e5534e4f5 100644
> --- a/hw/misc/max111x.c
> +++ b/hw/misc/max111x.c
> @@ -15,11 +15,15 @@
>  #include "hw/ssi/ssi.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;
>  

An eventual improvement is to make 'inputs' a class property:

  MAX111xClass {
      SSISlaveClass parent_obj;

      unsigned input_count;
  }

"eventual" because from a QOM point it is cleaner, but we'd have
to add more boiler-plate casts, so code less clear.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 07/17] hw/misc/max111x: Don't use vmstate_register()

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> The max111x is a proper qdev device; we can use dc->vmsd rather than
> directly calling vmstate_register().
> 
> It's possible that this is a migration compat break, but the only
> boards that use this device are the spitz-family ('akita', 'borzoi',
> 'spitz', 'terrier').
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/misc/max111x.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
> index d0e5534e4f5..abddfa3c660 100644
> --- a/hw/misc/max111x.c
> +++ b/hw/misc/max111x.c
> @@ -140,8 +140,6 @@ static int max111x_init(SSISlave *d, int inputs)
>  
>      s->inputs = inputs;
>  
> -    vmstate_register(VMSTATE_IF(dev), VMSTATE_INSTANCE_ID_ANY,
> -                     &vmstate_max111x, s);
>      return 0;
>  }
>  
> @@ -206,6 +204,7 @@ static void max111x_class_init(ObjectClass *klass, void *data)
>  
>      k->transfer = max111x_transfer;
>      dc->reset = max111x_reset;
> +    dc->vmsd = &vmstate_max111x;
>  }
>  
>  static const TypeInfo max111x_info = {
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 08/17] ssi: Add ssi_realize_and_unref()

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Add an ssi_realize_and_unref(), for the benefit of callers
> who want to be able to create an SSI device, set QOM properties
> on it, and then do the realize-and-unref afterwards.
> 
> The API works on the same principle as the recently added
> qdev_realize_and_undef(), sysbus_realize_and_undef(), etc.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/ssi/ssi.h | 26 ++++++++++++++++++++++++++
>  hw/ssi/ssi.c         |  7 ++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 93f2b8b0beb..4be5325e654 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -79,6 +79,32 @@ extern const VMStateDescription vmstate_ssi_slave;
>  }
>  
>  DeviceState *ssi_create_slave(SSIBus *bus, const char *name);
> +/**
> + * ssi_realize_and_unref: realize and unref an SSI slave device
> + * @dev: SSI slave device to realize
> + * @bus: SSI bus to put it on
> + * @errp: error pointer
> + *
> + * Call 'realize' on @dev, put it on the specified @bus, and drop the
> + * reference to it. Errors are reported via @errp and by returning
> + * false.
> + *
> + * This function is useful if you have created @dev via qdev_new()
> + * (which takes a reference to the device it returns to you), so that
> + * you can set properties on it before realizing it. If you don't need
> + * to set properties then ssi_create_slave() is probably better (as it
> + * does the create, init and realize in one step).
> + *
> + * If you are embedding the SSI slave into another QOM device and
> + * initialized it via some variant on object_initialize_child() then
> + * do not use this function, because that family of functions arrange
> + * for the only reference to the child device to be held by the parent
> + * via the child<> property, and so the reference-count-drop done here
> + * would be incorrect.  (Instead you would want ssi_realize(), which
> + * doesn't currently exist but would be trivial to create if we had
> + * any code that wanted it.)
> + */
> +bool ssi_realize_and_unref(DeviceState *dev, SSIBus *bus, Error **errp);
>  
>  /* Master interface.  */
>  SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index 67b48c31cd6..a35d7ebb266 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -90,11 +90,16 @@ static const TypeInfo ssi_slave_info = {
>      .abstract = true,
>  };
>  
> +bool ssi_realize_and_unref(DeviceState *dev, SSIBus *bus, Error **errp)
> +{
> +    return qdev_realize_and_unref(dev, &bus->parent_obj, errp);
> +}
> +
>  DeviceState *ssi_create_slave(SSIBus *bus, const char *name)
>  {
>      DeviceState *dev = qdev_new(name);
>  
> -    qdev_realize_and_unref(dev, &bus->parent_obj, &error_fatal);
> +    ssi_realize_and_unref(dev, bus, &error_fatal);
>      return dev;
>  }
>  

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 09/17] hw/arm/spitz: Use max111x properties to set initial values

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Use the new max111x qdev properties to set the initial input
> values rather than calling max111x_set_input(); this means that
> on system reset the inputs will correctly return to their initial
> values.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/spitz.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 11e413723f4..93a25edcb5b 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -731,11 +731,14 @@ 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 = ssi_create_slave(bus, "max1111");
> +    sms->max1111 = qdev_new("max1111");
>      max1111 = sms->max1111;
> -    max111x_set_input(sms->max1111, MAX1111_BATT_VOLT, SPITZ_BATTERY_VOLT);
> -    max111x_set_input(sms->max1111, MAX1111_BATT_TEMP, 0);
> -    max111x_set_input(sms->max1111, MAX1111_ACIN_VOLT, SPITZ_CHARGEON_ACIN);
> +    qdev_prop_set_uint8(sms->max1111, "input1" /* BATT_VOLT */,
> +                        SPITZ_BATTERY_VOLT);
> +    qdev_prop_set_uint8(sms->max1111, "input2" /* BATT_TEMP */, 0);
> +    qdev_prop_set_uint8(sms->max1111, "input3" /* ACIN_VOLT */,
> +                        SPITZ_CHARGEON_ACIN);

Actually for arrays it would be nice to use:

    DEFINE_PROP_ARRAY("input", MAX111xState, nr_inputs, reset_input,
                      qdev_prop_uint8, uint8_t),

Then something like:

qdev_prop_set_uint8_indexed(sms->max1111, "input", 2 /*BATT_TEMP*/, 0);

Anyway,
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +    ssi_realize_and_unref(sms->max1111, bus, &error_fatal);
>  
>      qdev_connect_gpio_out(sms->mpu->gpio, SPITZ_GPIO_LCDCON_CS,
>                          qdev_get_gpio_in(sms->mux, 0));
> 

Re: [PATCH 12/17] hw/arm/spitz: Encapsulate misc GPIO handling in a device

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Currently we have a free-floating set of IRQs and a function
> spitz_out_switch() which handle some miscellaneous GPIO lines for the
> spitz board.  Encapsulate this behaviour in a simple QOM device.
> 
> At this point we can finally remove the 'max1111' global, because the
> ADC battery-temperature value is now handled by the misc-gpio device
> writing the value to its outbound "adc-temp" GPIO, which the board
> code wires up to the appropriate inbound GPIO line on the max1111.
> 
> This commit also fixes Coverity issue CID 1421913 (which pointed out
> that the 'outsignals' in spitz_scoop_gpio_setup() were leaked),
> because it removes the use of the qemu_allocate_irqs() API from this
> code entirely.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/arm/spitz.c | 129 +++++++++++++++++++++++++++++++++----------------
>  1 file changed, 87 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 1400a56729d..bab9968ccee 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -51,6 +51,7 @@ typedef struct {
>      DeviceState *max1111;
>      DeviceState *scp0;
>      DeviceState *scp1;
> +    DeviceState *misc_gpio;
>  } SpitzMachineState;
>  
>  #define TYPE_SPITZ_MACHINE "spitz-common"
> @@ -658,8 +659,6 @@ static void spitz_lcdtg_realize(SSISlave *ssi, Error **errp)
>  #define SPITZ_GPIO_MAX1111_CS   20
>  #define SPITZ_GPIO_TP_INT       11
>  
> -static DeviceState *max1111;
> -
>  /* "Demux" the signal based on current chipselect */
>  typedef struct {
>      SSISlave ssidev;
> @@ -695,18 +694,6 @@ static void corgi_ssp_gpio_cs(void *opaque, int line, int level)
>  #define SPITZ_BATTERY_VOLT      0xd0    /* About 4.0V */
>  #define SPITZ_CHARGEON_ACIN     0x80    /* About 5.0V */
>  
> -static void spitz_adc_temp_on(void *opaque, int line, int level)
> -{
> -    int batt_temp;
> -
> -    if (!max1111)
> -        return;
> -
> -    batt_temp = level ? SPITZ_BATTERY_TEMP : 0;
> -
> -    qemu_set_irq(qdev_get_gpio_in(max1111, MAX1111_BATT_TEMP), batt_temp);
> -}
> -
>  static void corgi_ssp_realize(SSISlave *d, Error **errp)
>  {
>      DeviceState *dev = DEVICE(d);
> @@ -734,7 +721,6 @@ static void spitz_ssp_attach(SpitzMachineState *sms)
>  
>      bus = qdev_get_child_bus(sms->mux, "ssi2");
>      sms->max1111 = qdev_new(TYPE_MAX_1111);
> -    max1111 = sms->max1111;
>      qdev_prop_set_uint8(sms->max1111, "input1" /* BATT_VOLT */,
>                          SPITZ_BATTERY_VOLT);
>      qdev_prop_set_uint8(sms->max1111, "input2" /* BATT_TEMP */, 0);
> @@ -810,27 +796,66 @@ static void spitz_akita_i2c_setup(PXA2xxState *cpu)
>  
>  /* Other peripherals */
>  
> -static void spitz_out_switch(void *opaque, int line, int level)
> +/*
> + * Encapsulation of some miscellaneous GPIO line behaviour for the Spitz boards.
> + *
> + * QEMU interface:
> + *  + named GPIO inputs "green-led", "orange-led", "charging", "discharging":
> + *    these currently just print messages that the line has been signalled
> + *  + named GPIO input "adc-temp-on": set to cause the battery-temperature
> + *    value to be passed to the max111x ADC
> + *  + named GPIO output "adc-temp": the ADC value, to be wired up to the max111x
> + */
> +#define TYPE_SPITZ_MISC_GPIO "spitz-misc-gpio"
> +#define SPITZ_MISC_GPIO(obj) \
> +    OBJECT_CHECK(SpitzMiscGPIOState, (obj), TYPE_SPITZ_MISC_GPIO)
> +
> +typedef struct SpitzMiscGPIOState {
> +    SysBusDevice parent_obj;
> +
> +    qemu_irq adc_value;
> +} SpitzMiscGPIOState;
> +
> +static void spitz_misc_charging(void *opaque, int n, int level)
>  {
> -    switch (line) {
> -    case 0:
> -        zaurus_printf("Charging %s.\n", level ? "off" : "on");
> -        break;
> -    case 1:
> -        zaurus_printf("Discharging %s.\n", level ? "on" : "off");
> -        break;
> -    case 2:
> -        zaurus_printf("Green LED %s.\n", level ? "on" : "off");
> -        break;
> -    case 3:
> -        zaurus_printf("Orange LED %s.\n", level ? "on" : "off");
> -        break;
> -    case 6:
> -        spitz_adc_temp_on(opaque, line, level);
> -        break;
> -    default:
> -        g_assert_not_reached();
> -    }
> +    zaurus_printf("Charging %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_discharging(void *opaque, int n, int level)
> +{
> +    zaurus_printf("Discharging %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_green_led(void *opaque, int n, int level)
> +{
> +    zaurus_printf("Green LED %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_orange_led(void *opaque, int n, int level)
> +{
> +    zaurus_printf("Orange LED %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_adc_temp(void *opaque, int n, int level)
> +{
> +    SpitzMiscGPIOState *s = SPITZ_MISC_GPIO(opaque);
> +    int batt_temp = level ? SPITZ_BATTERY_TEMP : 0;
> +
> +    qemu_set_irq(s->adc_value, batt_temp);
> +}
> +
> +static void spitz_misc_gpio_init(Object *obj)
> +{
> +    SpitzMiscGPIOState *s = SPITZ_MISC_GPIO(obj);
> +    DeviceState *dev = DEVICE(obj);
> +
> +    qdev_init_gpio_in_named(dev, spitz_misc_charging, "charging", 1);
> +    qdev_init_gpio_in_named(dev, spitz_misc_discharging, "discharging", 1);
> +    qdev_init_gpio_in_named(dev, spitz_misc_green_led, "green-led", 1);
> +    qdev_init_gpio_in_named(dev, spitz_misc_orange_led, "orange-led", 1);
> +    qdev_init_gpio_in_named(dev, spitz_misc_adc_temp, "adc-temp-on", 1);
> +
> +    qdev_init_gpio_out_named(dev, &s->adc_value, "adc-temp", 1);
>  }
>  
>  #define SPITZ_SCP_LED_GREEN             1
> @@ -850,12 +875,22 @@ static void spitz_out_switch(void *opaque, int line, int level)
>  
>  static void spitz_scoop_gpio_setup(SpitzMachineState *sms)
>  {
> -    qemu_irq *outsignals = qemu_allocate_irqs(spitz_out_switch, sms->mpu, 8);
> +    DeviceState *miscdev = sysbus_create_simple(TYPE_SPITZ_MISC_GPIO, -1, NULL);
>  
> -    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_CHRG_ON, outsignals[0]);
> -    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_JK_B, outsignals[1]);
> -    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_GREEN, outsignals[2]);
> -    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_ORANGE, outsignals[3]);
> +    sms->misc_gpio = miscdev;
> +
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_CHRG_ON,
> +                          qdev_get_gpio_in_named(miscdev, "charging", 0));
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_JK_B,
> +                          qdev_get_gpio_in_named(miscdev, "discharging", 0));
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_GREEN,
> +                          qdev_get_gpio_in_named(miscdev, "green-led", 0));
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_ORANGE,
> +                          qdev_get_gpio_in_named(miscdev, "orange-led", 0));
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_ADC_TEMP_ON,
> +                          qdev_get_gpio_in_named(miscdev, "adc-temp-on", 0));
> +    qdev_connect_gpio_out_named(miscdev, "adc-temp", 0,
> +                                qdev_get_gpio_in(sms->max1111, MAX1111_BATT_TEMP));
>  
>      if (sms->scp1) {
>          qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_CONT,
> @@ -863,8 +898,6 @@ static void spitz_scoop_gpio_setup(SpitzMachineState *sms)
>          qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_ON,
>                                qdev_get_gpio_in_named(sms->lcdtg, "bl_power", 0));
>      }
> -
> -    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
>  }
>  
>  #define SPITZ_GPIO_HSYNC                22
> @@ -1217,12 +1250,24 @@ static const TypeInfo spitz_lcdtg_info = {
>      .class_init    = spitz_lcdtg_class_init,
>  };
>  
> +static const TypeInfo spitz_misc_gpio_info = {
> +    .name = TYPE_SPITZ_MISC_GPIO,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(SpitzMiscGPIOState),
> +    .instance_init = spitz_misc_gpio_init,
> +    /*
> +     * No class_init required: device has no internal state so does not
> +     * need to set up reset or vmstate, and does not have a realize method.
> +     */
> +};
> +
>  static void spitz_register_types(void)
>  {
>      type_register_static(&corgi_ssp_info);
>      type_register_static(&spitz_lcdtg_info);
>      type_register_static(&spitz_keyboard_info);
>      type_register_static(&sl_nand_info);
> +    type_register_static(&spitz_misc_gpio_info);
>  }
>  
>  type_init(spitz_register_types)
> 

Re: [PATCH 13/17] hw/gpio/zaurus.c: Use LOG_GUEST_ERROR for bad guest register accesses

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Instead of logging guest accesses to invalid register offsets in this
> device using zaurus_printf() (which just prints to stderr), use the
> usual qemu_log_mask(LOG_GUEST_ERROR,...).
> 
> Since this was the only use of the zaurus_printf() macro outside
> spitz.c, we can move the definition of that macro from sharpsl.h
> to spitz.c.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  include/hw/arm/sharpsl.h |  3 ---
>  hw/arm/spitz.c           |  3 +++
>  hw/gpio/zaurus.c         | 12 +++++++-----
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/arm/sharpsl.h b/include/hw/arm/sharpsl.h
> index 89e168fbff3..e986b28c527 100644
> --- a/include/hw/arm/sharpsl.h
> +++ b/include/hw/arm/sharpsl.h
> @@ -9,9 +9,6 @@
>  
>  #include "exec/hwaddr.h"
>  
> -#define zaurus_printf(format, ...)	\
> -    fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
> -
>  /* zaurus.c */
>  
>  #define SL_PXA_PARAM_BASE	0xa0000a00
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index bab9968ccee..6eb46869157 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -62,6 +62,9 @@ typedef struct {
>  #define SPITZ_MACHINE_CLASS(klass) \
>      OBJECT_CLASS_CHECK(SpitzMachineClass, klass, TYPE_SPITZ_MACHINE)
>  
> +#define zaurus_printf(format, ...)                              \
> +    fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
> +
>  #undef REG_FMT
>  #define REG_FMT                         "0x%02lx"
>  
> diff --git a/hw/gpio/zaurus.c b/hw/gpio/zaurus.c
> index 9a12c683420..258e9264930 100644
> --- a/hw/gpio/zaurus.c
> +++ b/hw/gpio/zaurus.c
> @@ -22,9 +22,7 @@
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
>  #include "qemu/module.h"
> -
> -#undef REG_FMT
> -#define REG_FMT			"0x%02lx"
> +#include "qemu/log.h"
>  
>  /* SCOOP devices */
>  
> @@ -104,7 +102,9 @@ static uint64_t scoop_read(void *opaque, hwaddr addr,
>      case SCOOP_GPRR:
>          return s->gpio_level;
>      default:
> -        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "scoop_read: bad register offset 0x%02" HWADDR_PRIx "\n",
> +                      addr);
>      }
>  
>      return 0;
> @@ -150,7 +150,9 @@ static void scoop_write(void *opaque, hwaddr addr,
>          scoop_gpio_handler_update(s);
>          break;
>      default:
> -        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "scoop_write: bad register offset 0x%02" HWADDR_PRIx "\n",
> +                      addr);
>      }
>  }
>  
> 

Re: [PATCH 14/17] hw/arm/spitz: Use LOG_GUEST_ERROR for bad guest register accesses

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Instead of logging guest accesses to invalid register offsets in the
> Spitz flash device with zaurus_printf() (which just prints to stderr),
> use the usual qemu_log_mask(LOG_GUEST_ERROR,...).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/arm/spitz.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 6eb46869157..49eae3fce4e 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -23,6 +23,7 @@
>  #include "hw/ssi/ssi.h"
>  #include "hw/block/flash.h"
>  #include "qemu/timer.h"
> +#include "qemu/log.h"
>  #include "hw/arm/sharpsl.h"
>  #include "ui/console.h"
>  #include "hw/audio/wm8750.h"
> @@ -65,9 +66,6 @@ typedef struct {
>  #define zaurus_printf(format, ...)                              \
>      fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
>  
> -#undef REG_FMT
> -#define REG_FMT                         "0x%02lx"
> -
>  /* Spitz Flash */
>  #define FLASH_BASE              0x0c000000
>  #define FLASH_ECCLPLB           0x00    /* Line parity 7 - 0 bit */
> @@ -137,7 +135,9 @@ static uint64_t sl_read(void *opaque, hwaddr addr, unsigned size)
>          return ecc_digest(&s->ecc, nand_getio(s->nand));
>  
>      default:
> -        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "sl_read: bad register offset 0x%02" HWADDR_PRIx "\n",
> +                      addr);
>      }
>      return 0;
>  }
> @@ -168,7 +168,9 @@ static void sl_write(void *opaque, hwaddr addr,
>          break;
>  
>      default:
> -        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "sl_write: bad register offset 0x%02" HWADDR_PRIx "\n",
> +                      addr);
>      }
>  }
>  
> 

Re: [PATCH 15/17] hw/arm/pxa2xx_pic: Use LOG_GUEST_ERROR for bad guest register accesses

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Instead of using printf() for logging guest accesses to invalid
> register offsets in the pxa2xx PIC device, use the usual
> qemu_log_mask(LOG_GUEST_ERROR,...).
> 
> This was the only user of the REG_FMT macro in pxa.h, so we can
> remove that.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  include/hw/arm/pxa.h | 1 -
>  hw/arm/pxa2xx_pic.c  | 9 +++++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h
> index f6dfb5c0cf0..8843e5f9107 100644
> --- a/include/hw/arm/pxa.h
> +++ b/include/hw/arm/pxa.h
> @@ -184,7 +184,6 @@ struct PXA2xxI2SState {
>  };
>  
>  # define PA_FMT			"0x%08lx"
> -# define REG_FMT		"0x" TARGET_FMT_plx
>  
>  PXA2xxState *pxa270_init(MemoryRegion *address_space, unsigned int sdram_size,
>                           const char *revision);
> diff --git a/hw/arm/pxa2xx_pic.c b/hw/arm/pxa2xx_pic.c
> index 105c5e63f2f..ceee6aa48db 100644
> --- a/hw/arm/pxa2xx_pic.c
> +++ b/hw/arm/pxa2xx_pic.c
> @@ -11,6 +11,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
> +#include "qemu/log.h"
>  #include "cpu.h"
>  #include "hw/arm/pxa.h"
>  #include "hw/sysbus.h"
> @@ -166,7 +167,9 @@ static uint64_t pxa2xx_pic_mem_read(void *opaque, hwaddr offset,
>      case ICHP:	/* Highest Priority register */
>          return pxa2xx_pic_highest(s);
>      default:
> -        printf("%s: Bad register offset " REG_FMT "\n", __func__, offset);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "pxa2xx_pic_mem_read: bad register offset 0x%" HWADDR_PRIx
> +                      "\n", offset);
>          return 0;
>      }
>  }
> @@ -199,7 +202,9 @@ static void pxa2xx_pic_mem_write(void *opaque, hwaddr offset,
>          s->priority[32 + ((offset - IPR32) >> 2)] = value & 0x8000003f;
>          break;
>      default:
> -        printf("%s: Bad register offset " REG_FMT "\n", __func__, offset);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "pxa2xx_pic_mem_write: bad register offset 0x%"
> +                      HWADDR_PRIx "\n", offset);
>          return;
>      }
>      pxa2xx_pic_update(opaque);
> 

Re: [PATCH 16/17] hw/arm/spitz: Provide usual QOM macros for corgi-ssp and spitz-lcdtg

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> The QOM types "spitz-lcdtg" and "corgi-ssp" are missing the
> usual QOM TYPE and casting macros; provide and use them.
> 
> In particular, we can safely use the QOM cast macros instead of
> FROM_SSI_SLAVE() because in both cases the 'ssidev' field of
> the instance state struct is the first field in it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/spitz.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 17/17] Replace uses of FROM_SSI_SLAVE() macro with QOM casts

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> The FROM_SSI_SLAVE() macro predates QOM and is used as a typesafe way
> to cast from an SSISlave* to the instance struct of a subtype of
> TYPE_SSI_SLAVE.  Switch to using the QOM cast macros instead, which
> have the same effect (by writing the QOM macros if the types were
> previously missing them.)
> 
> (The FROM_SSI_SLAVE() macro allows the SSISlave member of the
> subtype's struct to be anywhere as long as it is named "ssidev",
> whereas a QOM cast macro insists that it is the first thing in the
> subtype's struct.  This is true for all the types we convert here.)
> 
> This removes all the uses of FROM_SSI_SLAVE() so we can delete the
> definition.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/ssi/ssi.h |  2 --
>  hw/arm/z2.c          | 11 +++++++----
>  hw/display/ads7846.c |  9 ++++++---
>  hw/display/ssd0323.c | 10 +++++++---
>  hw/sd/ssi-sd.c       |  4 ++--
>  5 files changed, 22 insertions(+), 14 deletions(-)
> 
[...]
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 25cec2ddeaa..25cdf4c966d 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -74,7 +74,7 @@ typedef struct {
>  
>  static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
>  {
> -    ssi_sd_state *s = FROM_SSI_SLAVE(ssi_sd_state, dev);
> +    ssi_sd_state *s = SSI_SD(dev);
>  
>      /* Special case: allow CMD12 (STOP TRANSMISSION) while reading data.  */
>      if (s->mode == SSI_SD_DATA_READ && val == 0x4d) {
> @@ -241,7 +241,7 @@ static const VMStateDescription vmstate_ssi_sd = {
>  
>  static void ssi_sd_realize(SSISlave *d, Error **errp)
>  {
> -    ssi_sd_state *s = FROM_SSI_SLAVE(ssi_sd_state, d);
> +    ssi_sd_state *s = SSI_SD(d);
>      DeviceState *carddev;
>      DriveInfo *dinfo;
>      Error *err = NULL;
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

[Peter Maydell]

On Mon, 29 Jun 2020 at 09:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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>
> > ---

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

This is deliberate, as noted in the commit message.

thanks
-- PMM

Re: [PATCH 02/17] hw/arm/spitz: Create SpitzMachineClass abstract base class

[Peter Maydell]

On Mon, 29 Jun 2020 at 09:55, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 6/28/20 4:24 PM, Peter Maydell wrote:
> > For the four Spitz-family machines (akita, borzoi, spitz, terrier)
> > create a proper abstract class SpitzMachineClass which encapsulates
> > the common behaviour, rather than having them all derive directly
> > from TYPE_MACHINE:
> >  * instead of each machine class setting mc->init to a wrapper
> >    function which calls spitz_common_init() with parameters,
> >    put that data in the SpitzMachineClass and make spitz_common_init
> >    the SpitzMachineClass machine-init function
> >  * move the settings of mc->block_default_type and
> >    mc->ignore_memory_transaction_failures into the SpitzMachineClass
> >    class init rather than repeating them in each machine's class init
> >
> > (The motivation is that we're going to want to keep some state in
> > the SpitzMachineState so we can connect GPIOs between devices created
> > in one sub-function of the machine init to devices created in a
> > different sub-function.)
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/arm/spitz.c | 91 ++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 55 insertions(+), 36 deletions(-)
> >
> > diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> > index 9eaedab79b5..c70e912a33d 100644
> > --- a/hw/arm/spitz.c
> > +++ b/hw/arm/spitz.c
> > @@ -33,6 +33,26 @@
> >  #include "exec/address-spaces.h"
> >  #include "cpu.h"
> >
> > +enum spitz_model_e { spitz, akita, borzoi, terrier };
> > +
> > +typedef struct {
> > +    MachineClass parent;
> > +    enum spitz_model_e model;
>
> Nitpick, I'd drop the not very useful typedef and use
> directly ...:
>
>        enum { spitz, akita, borzoi, terrier } model

This is just code motion, I didn't want to mess with the
existing way this enum was defined.


> > -static void spitz_common_init(MachineState *machine,
> > -                              enum spitz_model_e model, int arm_id)
> > +static void spitz_common_init(MachineState *machine)
> >  {
> > +    SpitzMachineClass *smc = SPITZ_MACHINE_GET_CLASS(machine);
> > +    enum spitz_model_e model = smc->model;
>
> ... and use 'smc->model' in place.

Similarly here I used smc->arm_id in-place because there
was only one user, but went for model = smc->model because
there were multiple users and it would have put extra
changed lines into the patch that I didn't think were
necessary.

thanks
-- PMM

Re: [PATCH 09/17] hw/arm/spitz: Use max111x properties to set initial values

[Peter Maydell]

On Mon, 29 Jun 2020 at 10:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 6/28/20 4:24 PM, Peter Maydell wrote:
> > Use the new max111x qdev properties to set the initial input
> > values rather than calling max111x_set_input(); this means that
> > on system reset the inputs will correctly return to their initial
> > values.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---

> Actually for arrays it would be nice to use:
>
>     DEFINE_PROP_ARRAY("input", MAX111xState, nr_inputs, reset_input,
>                       qdev_prop_uint8, uint8_t),

DEFINE_PROP_ARRAY defines a variable length property array;
what we want for max111x is fixed-length.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 6/29/20 2:07 PM, Peter Maydell wrote:
> On Mon, 29 Jun 2020 at 09:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> 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

Ah, fine.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>>  * 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>
>>> ---
> 
>> Nitpick, we can keep MAX_111X() + MAX111xState in "hw/misc/max111x.c"
>> until we get a consumer.
> 
> This is deliberate, as noted in the commit message.
> 
> thanks
> -- PMM
> 

Re: [PATCH 01/17] hw/arm/spitz: Detabify

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:24 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The spitz board has been around a long time, and still has a fair number
> of hard-coded tab characters in it. We're about to do some work on
> this source file, so start out by expanding out the tabs.
>
> This commit is a pure whitespace only change.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Couple of checkpatch errors due to the QUEUE_KEY macro which can
> be ignored as this is just a detabify.
> ---
>  hw/arm/spitz.c | 156 ++++++++++++++++++++++++-------------------------
>  1 file changed, 78 insertions(+), 78 deletions(-)
>
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index fc18212e686..9eaedab79b5 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -34,25 +34,25 @@
>  #include "cpu.h"
>
>  #undef REG_FMT
> -#define REG_FMT                        "0x%02lx"
> +#define REG_FMT                         "0x%02lx"
>
>  /* Spitz Flash */
> -#define FLASH_BASE             0x0c000000
> -#define FLASH_ECCLPLB          0x00    /* Line parity 7 - 0 bit */
> -#define FLASH_ECCLPUB          0x04    /* Line parity 15 - 8 bit */
> -#define FLASH_ECCCP            0x08    /* Column parity 5 - 0 bit */
> -#define FLASH_ECCCNTR          0x0c    /* ECC byte counter */
> -#define FLASH_ECCCLRR          0x10    /* Clear ECC */
> -#define FLASH_FLASHIO          0x14    /* Flash I/O */
> -#define FLASH_FLASHCTL         0x18    /* Flash Control */
> +#define FLASH_BASE              0x0c000000
> +#define FLASH_ECCLPLB           0x00    /* Line parity 7 - 0 bit */
> +#define FLASH_ECCLPUB           0x04    /* Line parity 15 - 8 bit */
> +#define FLASH_ECCCP             0x08    /* Column parity 5 - 0 bit */
> +#define FLASH_ECCCNTR           0x0c    /* ECC byte counter */
> +#define FLASH_ECCCLRR           0x10    /* Clear ECC */
> +#define FLASH_FLASHIO           0x14    /* Flash I/O */
> +#define FLASH_FLASHCTL          0x18    /* Flash Control */
>
> -#define FLASHCTL_CE0           (1 << 0)
> -#define FLASHCTL_CLE           (1 << 1)
> -#define FLASHCTL_ALE           (1 << 2)
> -#define FLASHCTL_WP            (1 << 3)
> -#define FLASHCTL_CE1           (1 << 4)
> -#define FLASHCTL_RYBY          (1 << 5)
> -#define FLASHCTL_NCE           (FLASHCTL_CE0 | FLASHCTL_CE1)
> +#define FLASHCTL_CE0            (1 << 0)
> +#define FLASHCTL_CLE            (1 << 1)
> +#define FLASHCTL_ALE            (1 << 2)
> +#define FLASHCTL_WP             (1 << 3)
> +#define FLASHCTL_CE1            (1 << 4)
> +#define FLASHCTL_RYBY           (1 << 5)
> +#define FLASHCTL_NCE            (FLASHCTL_CE0 | FLASHCTL_CE1)
>
>  #define TYPE_SL_NAND "sl-nand"
>  #define SL_NAND(obj) OBJECT_CHECK(SLNANDState, (obj), TYPE_SL_NAND)
> @@ -74,12 +74,12 @@ static uint64_t sl_read(void *opaque, hwaddr addr, unsigned size)
>      int ryby;
>
>      switch (addr) {
> -#define BSHR(byte, from, to)   ((s->ecc.lp[byte] >> (from - to)) & (1 << to))
> +#define BSHR(byte, from, to)    ((s->ecc.lp[byte] >> (from - to)) & (1 << to))
>      case FLASH_ECCLPLB:
>          return BSHR(0, 4, 0) | BSHR(0, 5, 2) | BSHR(0, 6, 4) | BSHR(0, 7, 6) |
>                  BSHR(1, 4, 1) | BSHR(1, 5, 3) | BSHR(1, 6, 5) | BSHR(1, 7, 7);
>
> -#define BSHL(byte, from, to)   ((s->ecc.lp[byte] << (to - from)) & (1 << to))
> +#define BSHL(byte, from, to)    ((s->ecc.lp[byte] << (to - from)) & (1 << to))
>      case FLASH_ECCLPUB:
>          return BSHL(0, 0, 0) | BSHL(0, 1, 2) | BSHL(0, 2, 4) | BSHL(0, 3, 6) |
>                  BSHL(1, 0, 1) | BSHL(1, 1, 3) | BSHL(1, 2, 5) | BSHL(1, 3, 7);
> @@ -191,8 +191,8 @@ static void sl_nand_realize(DeviceState *dev, Error **errp)
>
>  /* Spitz Keyboard */
>
> -#define SPITZ_KEY_STROBE_NUM   11
> -#define SPITZ_KEY_SENSE_NUM    7
> +#define SPITZ_KEY_STROBE_NUM    11
> +#define SPITZ_KEY_SENSE_NUM     7
>
>  static const int spitz_gpio_key_sense[SPITZ_KEY_SENSE_NUM] = {
>      12, 17, 91, 34, 36, 38, 39
> @@ -214,11 +214,11 @@ static int spitz_keymap[SPITZ_KEY_SENSE_NUM + 1][SPITZ_KEY_STROBE_NUM] = {
>      { 0x52, 0x43, 0x01, 0x47, 0x49,  -1 ,  -1 ,  -1 ,  -1 ,  -1 ,  -1  },
>  };
>
> -#define SPITZ_GPIO_AK_INT      13      /* Remote control */
> -#define SPITZ_GPIO_SYNC                16      /* Sync button */
> -#define SPITZ_GPIO_ON_KEY      95      /* Power button */
> -#define SPITZ_GPIO_SWA         97      /* Lid */
> -#define SPITZ_GPIO_SWB         96      /* Tablet mode */
> +#define SPITZ_GPIO_AK_INT       13      /* Remote control */
> +#define SPITZ_GPIO_SYNC                 16      /* Sync button */
> +#define SPITZ_GPIO_ON_KEY       95      /* Power button */
> +#define SPITZ_GPIO_SWA          97      /* Lid */
> +#define SPITZ_GPIO_SWB          96      /* Tablet mode */
>
>  /* The special buttons are mapped to unused keys */
>  static const int spitz_gpiomap[5] = {
> @@ -300,7 +300,7 @@ static void spitz_keyboard_keydown(SpitzKeyboardState *s, int keycode)
>  #define SPITZ_MOD_CTRL    (1 << 8)
>  #define SPITZ_MOD_FN      (1 << 9)
>
> -#define QUEUE_KEY(c)   s->fifo[(s->fifopos + s->fifolen ++) & 0xf] = c
> +#define QUEUE_KEY(c)    s->fifo[(s->fifopos + s->fifolen ++) & 0xf] = c
>
>  static void spitz_keyboard_handler(void *opaque, int keycode)
>  {
> @@ -308,25 +308,25 @@ static void spitz_keyboard_handler(void *opaque, int keycode)
>      uint16_t code;
>      int mapcode;
>      switch (keycode) {
> -    case 0x2a: /* Left Shift */
> +    case 0x2a:  /* Left Shift */
>          s->modifiers |= 1;
>          break;
>      case 0xaa:
>          s->modifiers &= ~1;
>          break;
> -    case 0x36: /* Right Shift */
> +    case 0x36:  /* Right Shift */
>          s->modifiers |= 2;
>          break;
>      case 0xb6:
>          s->modifiers &= ~2;
>          break;
> -    case 0x1d: /* Control */
> +    case 0x1d:  /* Control */
>          s->modifiers |= 4;
>          break;
>      case 0x9d:
>          s->modifiers &= ~4;
>          break;
> -    case 0x38: /* Alt */
> +    case 0x38:  /* Alt */
>          s->modifiers |= 8;
>          break;
>      case 0xb8:
> @@ -536,14 +536,14 @@ static void spitz_keyboard_realize(DeviceState *dev, Error **errp)
>
>  /* LCD backlight controller */
>
> -#define LCDTG_RESCTL   0x00
> -#define LCDTG_PHACTRL  0x01
> -#define LCDTG_DUTYCTRL 0x02
> -#define LCDTG_POWERREG0        0x03
> -#define LCDTG_POWERREG1        0x04
> -#define LCDTG_GPOR3    0x05
> -#define LCDTG_PICTRL   0x06
> -#define LCDTG_POLCTRL  0x07
> +#define LCDTG_RESCTL    0x00
> +#define LCDTG_PHACTRL   0x01
> +#define LCDTG_DUTYCTRL  0x02
> +#define LCDTG_POWERREG0         0x03
> +#define LCDTG_POWERREG1         0x04
> +#define LCDTG_GPOR3     0x05
> +#define LCDTG_PICTRL    0x06
> +#define LCDTG_POLCTRL   0x07
>
>  typedef struct {
>      SSISlave ssidev;
> @@ -623,12 +623,12 @@ static void spitz_lcdtg_realize(SSISlave *dev, Error **errp)
>
>  /* SSP devices */
>
> -#define CORGI_SSP_PORT         2
> +#define CORGI_SSP_PORT          2
>
> -#define SPITZ_GPIO_LCDCON_CS   53
> -#define SPITZ_GPIO_ADS7846_CS  14
> -#define SPITZ_GPIO_MAX1111_CS  20
> -#define SPITZ_GPIO_TP_INT      11
> +#define SPITZ_GPIO_LCDCON_CS    53
> +#define SPITZ_GPIO_ADS7846_CS   14
> +#define SPITZ_GPIO_MAX1111_CS   20
> +#define SPITZ_GPIO_TP_INT       11
>
>  static DeviceState *max1111;
>
> @@ -659,13 +659,13 @@ static void corgi_ssp_gpio_cs(void *opaque, int line, int level)
>      s->enable[line] = !level;
>  }
>
> -#define MAX1111_BATT_VOLT      1
> -#define MAX1111_BATT_TEMP      2
> -#define MAX1111_ACIN_VOLT      3
> +#define MAX1111_BATT_VOLT       1
> +#define MAX1111_BATT_TEMP       2
> +#define MAX1111_ACIN_VOLT       3
>
> -#define SPITZ_BATTERY_TEMP     0xe0    /* About 2.9V */
> -#define SPITZ_BATTERY_VOLT     0xd0    /* About 4.0V */
> -#define SPITZ_CHARGEON_ACIN    0x80    /* About 5.0V */
> +#define SPITZ_BATTERY_TEMP      0xe0    /* About 2.9V */
> +#define SPITZ_BATTERY_VOLT      0xd0    /* About 4.0V */
> +#define SPITZ_CHARGEON_ACIN     0x80    /* About 5.0V */
>
>  static void spitz_adc_temp_on(void *opaque, int line, int level)
>  {
> @@ -735,11 +735,11 @@ static void spitz_microdrive_attach(PXA2xxState *cpu, int slot)
>
>  /* Wm8750 and Max7310 on I2C */
>
> -#define AKITA_MAX_ADDR 0x18
> -#define SPITZ_WM_ADDRL 0x1b
> -#define SPITZ_WM_ADDRH 0x1a
> +#define AKITA_MAX_ADDR  0x18
> +#define SPITZ_WM_ADDRL  0x1b
> +#define SPITZ_WM_ADDRH  0x1a
>
> -#define SPITZ_GPIO_WM  5
> +#define SPITZ_GPIO_WM   5
>
>  static void spitz_wm8750_addr(void *opaque, int line, int level)
>  {
> @@ -806,20 +806,20 @@ static void spitz_out_switch(void *opaque, int line, int level)
>      }
>  }
>
> -#define SPITZ_SCP_LED_GREEN            1
> -#define SPITZ_SCP_JK_B                 2
> -#define SPITZ_SCP_CHRG_ON              3
> -#define SPITZ_SCP_MUTE_L               4
> -#define SPITZ_SCP_MUTE_R               5
> -#define SPITZ_SCP_CF_POWER             6
> -#define SPITZ_SCP_LED_ORANGE           7
> -#define SPITZ_SCP_JK_A                 8
> -#define SPITZ_SCP_ADC_TEMP_ON          9
> -#define SPITZ_SCP2_IR_ON               1
> -#define SPITZ_SCP2_AKIN_PULLUP         2
> -#define SPITZ_SCP2_BACKLIGHT_CONT      7
> -#define SPITZ_SCP2_BACKLIGHT_ON                8
> -#define SPITZ_SCP2_MIC_BIAS            9
> +#define SPITZ_SCP_LED_GREEN             1
> +#define SPITZ_SCP_JK_B                  2
> +#define SPITZ_SCP_CHRG_ON               3
> +#define SPITZ_SCP_MUTE_L                4
> +#define SPITZ_SCP_MUTE_R                5
> +#define SPITZ_SCP_CF_POWER              6
> +#define SPITZ_SCP_LED_ORANGE            7
> +#define SPITZ_SCP_JK_A                  8
> +#define SPITZ_SCP_ADC_TEMP_ON           9
> +#define SPITZ_SCP2_IR_ON                1
> +#define SPITZ_SCP2_AKIN_PULLUP          2
> +#define SPITZ_SCP2_BACKLIGHT_CONT       7
> +#define SPITZ_SCP2_BACKLIGHT_ON                 8
> +#define SPITZ_SCP2_MIC_BIAS             9
>
>  static void spitz_scoop_gpio_setup(PXA2xxState *cpu,
>                  DeviceState *scp0, DeviceState *scp1)
> @@ -839,15 +839,15 @@ static void spitz_scoop_gpio_setup(PXA2xxState *cpu,
>      qdev_connect_gpio_out(scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
>  }
>
> -#define SPITZ_GPIO_HSYNC               22
> -#define SPITZ_GPIO_SD_DETECT           9
> -#define SPITZ_GPIO_SD_WP               81
> -#define SPITZ_GPIO_ON_RESET            89
> -#define SPITZ_GPIO_BAT_COVER           90
> -#define SPITZ_GPIO_CF1_IRQ             105
> -#define SPITZ_GPIO_CF1_CD              94
> -#define SPITZ_GPIO_CF2_IRQ             106
> -#define SPITZ_GPIO_CF2_CD              93
> +#define SPITZ_GPIO_HSYNC                22
> +#define SPITZ_GPIO_SD_DETECT            9
> +#define SPITZ_GPIO_SD_WP                81
> +#define SPITZ_GPIO_ON_RESET             89
> +#define SPITZ_GPIO_BAT_COVER            90
> +#define SPITZ_GPIO_CF1_IRQ              105
> +#define SPITZ_GPIO_CF1_CD               94
> +#define SPITZ_GPIO_CF2_IRQ              106
> +#define SPITZ_GPIO_CF2_CD               93
>
>  static int spitz_hsync;
>
> @@ -907,8 +907,8 @@ static void spitz_gpio_setup(PXA2xxState *cpu, int slots)
>  /* Board init.  */
>  enum spitz_model_e { spitz, akita, borzoi, terrier };
>
> -#define SPITZ_RAM      0x04000000
> -#define SPITZ_ROM      0x00800000
> +#define SPITZ_RAM       0x04000000
> +#define SPITZ_ROM       0x00800000
>
>  static struct arm_boot_info spitz_binfo = {
>      .loader_start = PXA2XX_SDRAM_BASE,
> --
> 2.20.1
>
>

Re: [PATCH 03/17] hw/arm/spitz: Keep pointers to MPU and SSI devices in SpitzMachineState

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:24 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Keep pointers to the MPU and the SSI devices in SpitzMachineState.
> We're going to want to make GPIO connections between some of the
> SSI devices and the SCPs, so we want to keep hold of a pointer to
> those; putting the MPU into the struct allows us to pass just
> one thing to spitz_ssp_attach() rather than two.
>
> We have to retain the setting of the global "max1111" variable
> for the moment as it is used in spitz_adc_temp_on(); later in
> this series of commits we will be able to remove it.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/spitz.c | 50 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index c70e912a33d..f48e966c047 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -43,6 +43,11 @@ typedef struct {
>
>  typedef struct {
>      MachineState parent;
> +    PXA2xxState *mpu;
> +    DeviceState *mux;
> +    DeviceState *lcdtg;
> +    DeviceState *ads7846;
> +    DeviceState *max1111;
>  } SpitzMachineState;
>
>  #define TYPE_SPITZ_MACHINE "spitz-common"
> @@ -709,34 +714,33 @@ static void corgi_ssp_realize(SSISlave *d, Error **errp)
>      s->bus[2] = ssi_create_bus(dev, "ssi2");
>  }
>
> -static void spitz_ssp_attach(PXA2xxState *cpu)
> +static void spitz_ssp_attach(SpitzMachineState *sms)
>  {
> -    DeviceState *mux;
> -    DeviceState *dev;
>      void *bus;
>
> -    mux = ssi_create_slave(cpu->ssp[CORGI_SSP_PORT - 1], "corgi-ssp");
> +    sms->mux = ssi_create_slave(sms->mpu->ssp[CORGI_SSP_PORT - 1], "corgi-ssp");
>
> -    bus = qdev_get_child_bus(mux, "ssi0");
> -    ssi_create_slave(bus, "spitz-lcdtg");
> +    bus = qdev_get_child_bus(sms->mux, "ssi0");
> +    sms->lcdtg = ssi_create_slave(bus, "spitz-lcdtg");
>
> -    bus = qdev_get_child_bus(mux, "ssi1");
> -    dev = ssi_create_slave(bus, "ads7846");
> -    qdev_connect_gpio_out(dev, 0,
> -                          qdev_get_gpio_in(cpu->gpio, SPITZ_GPIO_TP_INT));
> +    bus = qdev_get_child_bus(sms->mux, "ssi1");
> +    sms->ads7846 = ssi_create_slave(bus, "ads7846");
> +    qdev_connect_gpio_out(sms->ads7846, 0,
> +                          qdev_get_gpio_in(sms->mpu->gpio, SPITZ_GPIO_TP_INT));
>
> -    bus = qdev_get_child_bus(mux, "ssi2");
> -    max1111 = ssi_create_slave(bus, "max1111");
> -    max111x_set_input(max1111, MAX1111_BATT_VOLT, SPITZ_BATTERY_VOLT);
> -    max111x_set_input(max1111, MAX1111_BATT_TEMP, 0);
> -    max111x_set_input(max1111, MAX1111_ACIN_VOLT, SPITZ_CHARGEON_ACIN);
> +    bus = qdev_get_child_bus(sms->mux, "ssi2");
> +    sms->max1111 = ssi_create_slave(bus, "max1111");
> +    max1111 = sms->max1111;
> +    max111x_set_input(sms->max1111, MAX1111_BATT_VOLT, SPITZ_BATTERY_VOLT);
> +    max111x_set_input(sms->max1111, MAX1111_BATT_TEMP, 0);
> +    max111x_set_input(sms->max1111, MAX1111_ACIN_VOLT, SPITZ_CHARGEON_ACIN);
>
> -    qdev_connect_gpio_out(cpu->gpio, SPITZ_GPIO_LCDCON_CS,
> -                        qdev_get_gpio_in(mux, 0));
> -    qdev_connect_gpio_out(cpu->gpio, SPITZ_GPIO_ADS7846_CS,
> -                        qdev_get_gpio_in(mux, 1));
> -    qdev_connect_gpio_out(cpu->gpio, SPITZ_GPIO_MAX1111_CS,
> -                        qdev_get_gpio_in(mux, 2));
> +    qdev_connect_gpio_out(sms->mpu->gpio, SPITZ_GPIO_LCDCON_CS,
> +                        qdev_get_gpio_in(sms->mux, 0));
> +    qdev_connect_gpio_out(sms->mpu->gpio, SPITZ_GPIO_ADS7846_CS,
> +                        qdev_get_gpio_in(sms->mux, 1));
> +    qdev_connect_gpio_out(sms->mpu->gpio, SPITZ_GPIO_MAX1111_CS,
> +                        qdev_get_gpio_in(sms->mux, 2));
>  }
>
>  /* CF Microdrive */
> @@ -936,6 +940,7 @@ static struct arm_boot_info spitz_binfo = {
>  static void spitz_common_init(MachineState *machine)
>  {
>      SpitzMachineClass *smc = SPITZ_MACHINE_GET_CLASS(machine);
> +    SpitzMachineState *sms = SPITZ_MACHINE(machine);
>      enum spitz_model_e model = smc->model;
>      PXA2xxState *mpu;
>      DeviceState *scp0, *scp1 = NULL;
> @@ -945,6 +950,7 @@ static void spitz_common_init(MachineState *machine)
>      /* Setup CPU & memory */
>      mpu = pxa270_init(address_space_mem, spitz_binfo.ram_size,
>                        machine->cpu_type);
> +    sms->mpu = mpu;
>
>      sl_flash_register(mpu, (model == spitz) ? FLASH_128M : FLASH_1024M);
>
> @@ -954,7 +960,7 @@ static void spitz_common_init(MachineState *machine)
>      /* Setup peripherals */
>      spitz_keyboard_register(mpu);
>
> -    spitz_ssp_attach(mpu);
> +    spitz_ssp_attach(sms);
>
>      scp0 = sysbus_create_simple("scoop", 0x10800000, NULL);
>      if (model != akita) {
> --
> 2.20.1
>
>

Re: [PATCH 04/17] hw/arm/spitz: Keep pointers to scp0, scp1 in SpitzMachineState

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:27 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Keep pointers to scp0, scp1 in SpitzMachineState, and just pass
> that to spitz_scoop_gpio_setup().
>
> (We'll want to use some of the other fields in SpitzMachineState
> in that function in the next commit.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/spitz.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index f48e966c047..69bc2b3fa10 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -48,6 +48,8 @@ typedef struct {
>      DeviceState *lcdtg;
>      DeviceState *ads7846;
>      DeviceState *max1111;
> +    DeviceState *scp0;
> +    DeviceState *scp1;
>  } SpitzMachineState;
>
>  #define TYPE_SPITZ_MACHINE "spitz-common"
> @@ -845,22 +847,23 @@ static void spitz_out_switch(void *opaque, int line, int level)
>  #define SPITZ_SCP2_BACKLIGHT_ON                 8
>  #define SPITZ_SCP2_MIC_BIAS             9
>
> -static void spitz_scoop_gpio_setup(PXA2xxState *cpu,
> -                DeviceState *scp0, DeviceState *scp1)
> +static void spitz_scoop_gpio_setup(SpitzMachineState *sms)
>  {
> -    qemu_irq *outsignals = qemu_allocate_irqs(spitz_out_switch, cpu, 8);
> +    qemu_irq *outsignals = qemu_allocate_irqs(spitz_out_switch, sms->mpu, 8);
>
> -    qdev_connect_gpio_out(scp0, SPITZ_SCP_CHRG_ON, outsignals[0]);
> -    qdev_connect_gpio_out(scp0, SPITZ_SCP_JK_B, outsignals[1]);
> -    qdev_connect_gpio_out(scp0, SPITZ_SCP_LED_GREEN, outsignals[2]);
> -    qdev_connect_gpio_out(scp0, SPITZ_SCP_LED_ORANGE, outsignals[3]);
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_CHRG_ON, outsignals[0]);
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_JK_B, outsignals[1]);
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_GREEN, outsignals[2]);
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_ORANGE, outsignals[3]);
>
> -    if (scp1) {
> -        qdev_connect_gpio_out(scp1, SPITZ_SCP2_BACKLIGHT_CONT, outsignals[4]);
> -        qdev_connect_gpio_out(scp1, SPITZ_SCP2_BACKLIGHT_ON, outsignals[5]);
> +    if (sms->scp1) {
> +        qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_CONT,
> +                              outsignals[4]);
> +        qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_ON,
> +                              outsignals[5]);
>      }
>
> -    qdev_connect_gpio_out(scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
>  }
>
>  #define SPITZ_GPIO_HSYNC                22
> @@ -943,7 +946,6 @@ static void spitz_common_init(MachineState *machine)
>      SpitzMachineState *sms = SPITZ_MACHINE(machine);
>      enum spitz_model_e model = smc->model;
>      PXA2xxState *mpu;
> -    DeviceState *scp0, *scp1 = NULL;
>      MemoryRegion *address_space_mem = get_system_memory();
>      MemoryRegion *rom = g_new(MemoryRegion, 1);
>
> @@ -962,12 +964,14 @@ static void spitz_common_init(MachineState *machine)
>
>      spitz_ssp_attach(sms);
>
> -    scp0 = sysbus_create_simple("scoop", 0x10800000, NULL);
> +    sms->scp0 = sysbus_create_simple("scoop", 0x10800000, NULL);
>      if (model != akita) {
> -        scp1 = sysbus_create_simple("scoop", 0x08800040, NULL);
> +        sms->scp1 = sysbus_create_simple("scoop", 0x08800040, NULL);
> +    } else {
> +        sms->scp1 = NULL;
>      }
>
> -    spitz_scoop_gpio_setup(mpu, scp0, scp1);
> +    spitz_scoop_gpio_setup(sms);
>
>      spitz_gpio_setup(mpu, (model == akita) ? 1 : 2);
>
> --
> 2.20.1
>
>

Re: [PATCH 05/17] hw/arm/spitz: Implement inbound GPIO lines for bit5 and power signals

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:29 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Currently the Spitz board uses a nasty hack for the GPIO lines
> that pass "bit5" and "power" information to the LCD controller:
> the lcdtg realize function sets a global variable to point to
> the instance it just realized, and then the functions spitz_bl_power()
> and spitz_bl_bit5() use that to find the device they are changing
> the internal state of. There is a comment reading:
>  FIXME: Implement GPIO properly and remove this hack.
> which was added in 2009.
>
> Implement GPIO properly and remove this hack.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/spitz.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 69bc2b3fa10..11e413723f4 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -586,12 +586,9 @@ static void spitz_bl_update(SpitzLCDTG *s)
>          zaurus_printf("LCD Backlight now off\n");
>  }
>
> -/* FIXME: Implement GPIO properly and remove this hack.  */
> -static SpitzLCDTG *spitz_lcdtg;
> -
>  static inline void spitz_bl_bit5(void *opaque, int line, int level)
>  {
> -    SpitzLCDTG *s = spitz_lcdtg;
> +    SpitzLCDTG *s = opaque;
>      int prev = s->bl_intensity;
>
>      if (level)
> @@ -605,7 +602,7 @@ static inline void spitz_bl_bit5(void *opaque, int line, int level)
>
>  static inline void spitz_bl_power(void *opaque, int line, int level)
>  {
> -    SpitzLCDTG *s = spitz_lcdtg;
> +    SpitzLCDTG *s = opaque;
>      s->bl_power = !!level;
>      spitz_bl_update(s);
>  }
> @@ -639,13 +636,16 @@ static uint32_t spitz_lcdtg_transfer(SSISlave *dev, uint32_t value)
>      return 0;
>  }
>
> -static void spitz_lcdtg_realize(SSISlave *dev, Error **errp)
> +static void spitz_lcdtg_realize(SSISlave *ssi, Error **errp)
>  {
> -    SpitzLCDTG *s = FROM_SSI_SLAVE(SpitzLCDTG, dev);
> +    SpitzLCDTG *s = FROM_SSI_SLAVE(SpitzLCDTG, ssi);
> +    DeviceState *dev = DEVICE(s);
>
> -    spitz_lcdtg = s;
>      s->bl_power = 0;
>      s->bl_intensity = 0x20;
> +
> +    qdev_init_gpio_in_named(dev, spitz_bl_bit5, "bl_bit5", 1);
> +    qdev_init_gpio_in_named(dev, spitz_bl_power, "bl_power", 1);
>  }
>
>  /* SSP devices */
> @@ -820,15 +820,11 @@ static void spitz_out_switch(void *opaque, int line, int level)
>      case 3:
>          zaurus_printf("Orange LED %s.\n", level ? "on" : "off");
>          break;
> -    case 4:
> -        spitz_bl_bit5(opaque, line, level);
> -        break;
> -    case 5:
> -        spitz_bl_power(opaque, line, level);
> -        break;
>      case 6:
>          spitz_adc_temp_on(opaque, line, level);
>          break;
> +    default:
> +        g_assert_not_reached();
>      }
>  }
>
> @@ -858,9 +854,9 @@ static void spitz_scoop_gpio_setup(SpitzMachineState *sms)
>
>      if (sms->scp1) {
>          qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_CONT,
> -                              outsignals[4]);
> +                              qdev_get_gpio_in_named(sms->lcdtg, "bl_bit5", 0));
>          qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_ON,
> -                              outsignals[5]);
> +                              qdev_get_gpio_in_named(sms->lcdtg, "bl_power", 0));
>      }
>
>      qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
> --
> 2.20.1
>
>

Re: [PATCH 06/17] hw/misc/max111x: provide QOM properties for setting initial values

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:29 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Add some QOM properties to the max111x ADC device to allow the
> initial values to be configured. Currently this is done by
> board code calling max111x_set_input() after it creates the
> device, which doesn't work on system reset.
>
> This requires us to implement a reset method for this device,
> so while we're doing that make sure we reset the other parts
> of the device state.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/misc/max111x.c | 57 ++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
> index 2b87bdee5b7..d0e5534e4f5 100644
> --- a/hw/misc/max111x.c
> +++ b/hw/misc/max111x.c
> @@ -15,11 +15,15 @@
>  #include "hw/ssi/ssi.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;
>
> @@ -135,16 +139,6 @@ static int max111x_init(SSISlave *d, int inputs)
>      qdev_init_gpio_out(dev, &s->interrupt, 1);
>
>      s->inputs = inputs;
> -    /* TODO: add a user interface for setting these */
> -    s->input[0] = 0xf0;
> -    s->input[1] = 0xe0;
> -    s->input[2] = 0xd0;
> -    s->input[3] = 0xc0;
> -    s->input[4] = 0xb0;
> -    s->input[5] = 0xa0;
> -    s->input[6] = 0x90;
> -    s->input[7] = 0x80;
> -    s->com = 0;
>
>      vmstate_register(VMSTATE_IF(dev), VMSTATE_INSTANCE_ID_ANY,
>                       &vmstate_max111x, s);
> @@ -168,11 +162,50 @@ void max111x_set_input(DeviceState *dev, int line, uint8_t value)
>      s->input[line] = value;
>  }
>
> +static void max111x_reset(DeviceState *dev)
> +{
> +    MAX111xState *s = MAX_111X(dev);
> +    int i;
> +
> +    for (i = 0; i < s->inputs; i++) {
> +        s->input[i] = s->reset_input[i];
> +    }
> +    s->com = 0;
> +    s->tb1 = 0;
> +    s->rb2 = 0;
> +    s->rb3 = 0;
> +    s->cycle = 0;
> +}
> +
> +static Property max1110_properties[] = {
> +    /* Reset values for ADC inputs */
> +    DEFINE_PROP_UINT8("input0", MAX111xState, reset_input[0], 0xf0),
> +    DEFINE_PROP_UINT8("input1", MAX111xState, reset_input[1], 0xe0),
> +    DEFINE_PROP_UINT8("input2", MAX111xState, reset_input[2], 0xd0),
> +    DEFINE_PROP_UINT8("input3", MAX111xState, reset_input[3], 0xc0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static Property max1111_properties[] = {
> +    /* Reset values for ADC inputs */
> +    DEFINE_PROP_UINT8("input0", MAX111xState, reset_input[0], 0xf0),
> +    DEFINE_PROP_UINT8("input1", MAX111xState, reset_input[1], 0xe0),
> +    DEFINE_PROP_UINT8("input2", MAX111xState, reset_input[2], 0xd0),
> +    DEFINE_PROP_UINT8("input3", MAX111xState, reset_input[3], 0xc0),
> +    DEFINE_PROP_UINT8("input4", MAX111xState, reset_input[4], 0xb0),
> +    DEFINE_PROP_UINT8("input5", MAX111xState, reset_input[5], 0xa0),
> +    DEFINE_PROP_UINT8("input6", MAX111xState, reset_input[6], 0x90),
> +    DEFINE_PROP_UINT8("input7", MAX111xState, reset_input[7], 0x80),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void max111x_class_init(ObjectClass *klass, void *data)
>  {
>      SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>
>      k->transfer = max111x_transfer;
> +    dc->reset = max111x_reset;
>  }
>
>  static const TypeInfo max111x_info = {
> @@ -186,8 +219,10 @@ static const TypeInfo max111x_info = {
>  static void max1110_class_init(ObjectClass *klass, void *data)
>  {
>      SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>
>      k->realize = max1110_realize;
> +    device_class_set_props(dc, max1110_properties);
>  }
>
>  static const TypeInfo max1110_info = {
> @@ -199,8 +234,10 @@ static const TypeInfo max1110_info = {
>  static void max1111_class_init(ObjectClass *klass, void *data)
>  {
>      SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>
>      k->realize = max1111_realize;
> +    device_class_set_props(dc, max1111_properties);
>  }
>
>  static const TypeInfo max1111_info = {
> --
> 2.20.1
>
>

Re: [PATCH 07/17] hw/misc/max111x: Don't use vmstate_register()

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:25 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The max111x is a proper qdev device; we can use dc->vmsd rather than
> directly calling vmstate_register().
>
> It's possible that this is a migration compat break, but the only
> boards that use this device are the spitz-family ('akita', 'borzoi',
> 'spitz', 'terrier').
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/misc/max111x.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
> index d0e5534e4f5..abddfa3c660 100644
> --- a/hw/misc/max111x.c
> +++ b/hw/misc/max111x.c
> @@ -140,8 +140,6 @@ static int max111x_init(SSISlave *d, int inputs)
>
>      s->inputs = inputs;
>
> -    vmstate_register(VMSTATE_IF(dev), VMSTATE_INSTANCE_ID_ANY,
> -                     &vmstate_max111x, s);
>      return 0;
>  }
>
> @@ -206,6 +204,7 @@ static void max111x_class_init(ObjectClass *klass, void *data)
>
>      k->transfer = max111x_transfer;
>      dc->reset = max111x_reset;
> +    dc->vmsd = &vmstate_max111x;
>  }
>
>  static const TypeInfo max111x_info = {
> --
> 2.20.1
>
>

Re: [PATCH 08/17] ssi: Add ssi_realize_and_unref()

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:27 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Add an ssi_realize_and_unref(), for the benefit of callers
> who want to be able to create an SSI device, set QOM properties
> on it, and then do the realize-and-unref afterwards.
>
> The API works on the same principle as the recently added
> qdev_realize_and_undef(), sysbus_realize_and_undef(), etc.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/ssi/ssi.h | 26 ++++++++++++++++++++++++++
>  hw/ssi/ssi.c         |  7 ++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 93f2b8b0beb..4be5325e654 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -79,6 +79,32 @@ extern const VMStateDescription vmstate_ssi_slave;
>  }
>
>  DeviceState *ssi_create_slave(SSIBus *bus, const char *name);
> +/**
> + * ssi_realize_and_unref: realize and unref an SSI slave device
> + * @dev: SSI slave device to realize
> + * @bus: SSI bus to put it on
> + * @errp: error pointer
> + *
> + * Call 'realize' on @dev, put it on the specified @bus, and drop the
> + * reference to it. Errors are reported via @errp and by returning
> + * false.
> + *
> + * This function is useful if you have created @dev via qdev_new()
> + * (which takes a reference to the device it returns to you), so that
> + * you can set properties on it before realizing it. If you don't need
> + * to set properties then ssi_create_slave() is probably better (as it
> + * does the create, init and realize in one step).
> + *
> + * If you are embedding the SSI slave into another QOM device and
> + * initialized it via some variant on object_initialize_child() then
> + * do not use this function, because that family of functions arrange
> + * for the only reference to the child device to be held by the parent
> + * via the child<> property, and so the reference-count-drop done here
> + * would be incorrect.  (Instead you would want ssi_realize(), which
> + * doesn't currently exist but would be trivial to create if we had
> + * any code that wanted it.)
> + */
> +bool ssi_realize_and_unref(DeviceState *dev, SSIBus *bus, Error **errp);
>
>  /* Master interface.  */
>  SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index 67b48c31cd6..a35d7ebb266 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -90,11 +90,16 @@ static const TypeInfo ssi_slave_info = {
>      .abstract = true,
>  };
>
> +bool ssi_realize_and_unref(DeviceState *dev, SSIBus *bus, Error **errp)
> +{
> +    return qdev_realize_and_unref(dev, &bus->parent_obj, errp);
> +}
> +
>  DeviceState *ssi_create_slave(SSIBus *bus, const char *name)
>  {
>      DeviceState *dev = qdev_new(name);
>
> -    qdev_realize_and_unref(dev, &bus->parent_obj, &error_fatal);
> +    ssi_realize_and_unref(dev, bus, &error_fatal);
>      return dev;
>  }
>
> --
> 2.20.1
>
>

Re: [PATCH 10/17] hw/misc/max111x: Use GPIO lines rather than max111x_set_input()

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:28 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The max111x ADC device model allows other code to set the level on
> the 8 ADC inputs using the max111x_set_input() function.  Replace
> this with generic qdev GPIO inputs, which also allow inputs to be set
> to arbitrary values.
>
> Using GPIO lines will make it easier for board code to wire things
> up, so that if device A wants to set the ADC input it doesn't need to
> have a direct pointer to the max111x but can just set that value on
> its output GPIO, which is then wired up by the board to the
> appropriate max111x input.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/ssi/ssi.h |  3 ---
>  hw/arm/spitz.c       |  9 +++++----
>  hw/misc/max111x.c    | 16 +++++++++-------
>  3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 4be5325e654..5fd411f2e4e 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -111,7 +111,4 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>
>  uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>
> -/* max111x.c */
> -void max111x_set_input(DeviceState *dev, int line, uint8_t value);
> -
>  #endif
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 93a25edcb5b..fa592aad6d6 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -696,13 +696,14 @@ static void corgi_ssp_gpio_cs(void *opaque, int line, int level)
>
>  static void spitz_adc_temp_on(void *opaque, int line, int level)
>  {
> +    int batt_temp;
> +
>      if (!max1111)
>          return;
>
> -    if (level)
> -        max111x_set_input(max1111, MAX1111_BATT_TEMP, SPITZ_BATTERY_TEMP);
> -    else
> -        max111x_set_input(max1111, MAX1111_BATT_TEMP, 0);
> +    batt_temp = level ? SPITZ_BATTERY_TEMP : 0;
> +
> +    qemu_set_irq(qdev_get_gpio_in(max1111, MAX1111_BATT_TEMP), batt_temp);
>  }
>
>  static void corgi_ssp_realize(SSISlave *d, Error **errp)
> diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
> index abddfa3c660..3a5cb838445 100644
> --- a/hw/misc/max111x.c
> +++ b/hw/misc/max111x.c
> @@ -131,12 +131,21 @@ static const VMStateDescription vmstate_max111x = {
>      }
>  };
>
> +static void max111x_input_set(void *opaque, int line, int value)
> +{
> +    MAX111xState *s = MAX_111X(opaque);
> +
> +    assert(line >= 0 && line < s->inputs);
> +    s->input[line] = value;
> +}
> +
>  static int max111x_init(SSISlave *d, int inputs)
>  {
>      DeviceState *dev = DEVICE(d);
>      MAX111xState *s = MAX_111X(dev);
>
>      qdev_init_gpio_out(dev, &s->interrupt, 1);
> +    qdev_init_gpio_in(dev, max111x_input_set, inputs);
>
>      s->inputs = inputs;
>
> @@ -153,13 +162,6 @@ static void max1111_realize(SSISlave *dev, Error **errp)
>      max111x_init(dev, 4);
>  }
>
> -void max111x_set_input(DeviceState *dev, int line, uint8_t value)
> -{
> -    MAX111xState *s = MAX_111X(dev);
> -    assert(line >= 0 && line < s->inputs);
> -    s->input[line] = value;
> -}
> -
>  static void max111x_reset(DeviceState *dev)
>  {
>      MAX111xState *s = MAX_111X(dev);
> --
> 2.20.1
>
>

Re: [PATCH 13/17] hw/gpio/zaurus.c: Use LOG_GUEST_ERROR for bad guest register accesses

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:35 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Instead of logging guest accesses to invalid register offsets in this
> device using zaurus_printf() (which just prints to stderr), use the
> usual qemu_log_mask(LOG_GUEST_ERROR,...).
>
> Since this was the only use of the zaurus_printf() macro outside
> spitz.c, we can move the definition of that macro from sharpsl.h
> to spitz.c.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/arm/sharpsl.h |  3 ---
>  hw/arm/spitz.c           |  3 +++
>  hw/gpio/zaurus.c         | 12 +++++++-----
>  3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/arm/sharpsl.h b/include/hw/arm/sharpsl.h
> index 89e168fbff3..e986b28c527 100644
> --- a/include/hw/arm/sharpsl.h
> +++ b/include/hw/arm/sharpsl.h
> @@ -9,9 +9,6 @@
>
>  #include "exec/hwaddr.h"
>
> -#define zaurus_printf(format, ...)     \
> -    fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
> -
>  /* zaurus.c */
>
>  #define SL_PXA_PARAM_BASE      0xa0000a00
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index bab9968ccee..6eb46869157 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -62,6 +62,9 @@ typedef struct {
>  #define SPITZ_MACHINE_CLASS(klass) \
>      OBJECT_CLASS_CHECK(SpitzMachineClass, klass, TYPE_SPITZ_MACHINE)
>
> +#define zaurus_printf(format, ...)                              \
> +    fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
> +
>  #undef REG_FMT
>  #define REG_FMT                         "0x%02lx"
>
> diff --git a/hw/gpio/zaurus.c b/hw/gpio/zaurus.c
> index 9a12c683420..258e9264930 100644
> --- a/hw/gpio/zaurus.c
> +++ b/hw/gpio/zaurus.c
> @@ -22,9 +22,7 @@
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
>  #include "qemu/module.h"
> -
> -#undef REG_FMT
> -#define REG_FMT                        "0x%02lx"
> +#include "qemu/log.h"
>
>  /* SCOOP devices */
>
> @@ -104,7 +102,9 @@ static uint64_t scoop_read(void *opaque, hwaddr addr,
>      case SCOOP_GPRR:
>          return s->gpio_level;
>      default:
> -        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "scoop_read: bad register offset 0x%02" HWADDR_PRIx "\n",
> +                      addr);
>      }
>
>      return 0;
> @@ -150,7 +150,9 @@ static void scoop_write(void *opaque, hwaddr addr,
>          scoop_gpio_handler_update(s);
>          break;
>      default:
> -        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "scoop_write: bad register offset 0x%02" HWADDR_PRIx "\n",
> +                      addr);
>      }
>  }
>
> --
> 2.20.1
>
>

Re: [PATCH 14/17] hw/arm/spitz: Use LOG_GUEST_ERROR for bad guest register accesses

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:33 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Instead of logging guest accesses to invalid register offsets in the
> Spitz flash device with zaurus_printf() (which just prints to stderr),
> use the usual qemu_log_mask(LOG_GUEST_ERROR,...).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/spitz.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 6eb46869157..49eae3fce4e 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -23,6 +23,7 @@
>  #include "hw/ssi/ssi.h"
>  #include "hw/block/flash.h"
>  #include "qemu/timer.h"
> +#include "qemu/log.h"
>  #include "hw/arm/sharpsl.h"
>  #include "ui/console.h"
>  #include "hw/audio/wm8750.h"
> @@ -65,9 +66,6 @@ typedef struct {
>  #define zaurus_printf(format, ...)                              \
>      fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
>
> -#undef REG_FMT
> -#define REG_FMT                         "0x%02lx"
> -
>  /* Spitz Flash */
>  #define FLASH_BASE              0x0c000000
>  #define FLASH_ECCLPLB           0x00    /* Line parity 7 - 0 bit */
> @@ -137,7 +135,9 @@ static uint64_t sl_read(void *opaque, hwaddr addr, unsigned size)
>          return ecc_digest(&s->ecc, nand_getio(s->nand));
>
>      default:
> -        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "sl_read: bad register offset 0x%02" HWADDR_PRIx "\n",
> +                      addr);
>      }
>      return 0;
>  }
> @@ -168,7 +168,9 @@ static void sl_write(void *opaque, hwaddr addr,
>          break;
>
>      default:
> -        zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned long)addr);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "sl_write: bad register offset 0x%02" HWADDR_PRIx "\n",
> +                      addr);
>      }
>  }
>
> --
> 2.20.1
>
>

Re: [PATCH 15/17] hw/arm/pxa2xx_pic: Use LOG_GUEST_ERROR for bad guest register accesses

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:29 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Instead of using printf() for logging guest accesses to invalid
> register offsets in the pxa2xx PIC device, use the usual
> qemu_log_mask(LOG_GUEST_ERROR,...).
>
> This was the only user of the REG_FMT macro in pxa.h, so we can
> remove that.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/arm/pxa.h | 1 -
>  hw/arm/pxa2xx_pic.c  | 9 +++++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h
> index f6dfb5c0cf0..8843e5f9107 100644
> --- a/include/hw/arm/pxa.h
> +++ b/include/hw/arm/pxa.h
> @@ -184,7 +184,6 @@ struct PXA2xxI2SState {
>  };
>
>  # define PA_FMT                        "0x%08lx"
> -# define REG_FMT               "0x" TARGET_FMT_plx
>
>  PXA2xxState *pxa270_init(MemoryRegion *address_space, unsigned int sdram_size,
>                           const char *revision);
> diff --git a/hw/arm/pxa2xx_pic.c b/hw/arm/pxa2xx_pic.c
> index 105c5e63f2f..ceee6aa48db 100644
> --- a/hw/arm/pxa2xx_pic.c
> +++ b/hw/arm/pxa2xx_pic.c
> @@ -11,6 +11,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
> +#include "qemu/log.h"
>  #include "cpu.h"
>  #include "hw/arm/pxa.h"
>  #include "hw/sysbus.h"
> @@ -166,7 +167,9 @@ static uint64_t pxa2xx_pic_mem_read(void *opaque, hwaddr offset,
>      case ICHP: /* Highest Priority register */
>          return pxa2xx_pic_highest(s);
>      default:
> -        printf("%s: Bad register offset " REG_FMT "\n", __func__, offset);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "pxa2xx_pic_mem_read: bad register offset 0x%" HWADDR_PRIx
> +                      "\n", offset);
>          return 0;
>      }
>  }
> @@ -199,7 +202,9 @@ static void pxa2xx_pic_mem_write(void *opaque, hwaddr offset,
>          s->priority[32 + ((offset - IPR32) >> 2)] = value & 0x8000003f;
>          break;
>      default:
> -        printf("%s: Bad register offset " REG_FMT "\n", __func__, offset);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "pxa2xx_pic_mem_write: bad register offset 0x%"
> +                      HWADDR_PRIx "\n", offset);
>          return;
>      }
>      pxa2xx_pic_update(opaque);
> --
> 2.20.1
>
>

Re: [PATCH 12/17] hw/arm/spitz: Encapsulate misc GPIO handling in a device

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:33 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Currently we have a free-floating set of IRQs and a function
> spitz_out_switch() which handle some miscellaneous GPIO lines for the
> spitz board.  Encapsulate this behaviour in a simple QOM device.
>
> At this point we can finally remove the 'max1111' global, because the
> ADC battery-temperature value is now handled by the misc-gpio device
> writing the value to its outbound "adc-temp" GPIO, which the board
> code wires up to the appropriate inbound GPIO line on the max1111.
>
> This commit also fixes Coverity issue CID 1421913 (which pointed out
> that the 'outsignals' in spitz_scoop_gpio_setup() were leaked),
> because it removes the use of the qemu_allocate_irqs() API from this
> code entirely.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/spitz.c | 129 +++++++++++++++++++++++++++++++++----------------
>  1 file changed, 87 insertions(+), 42 deletions(-)
>
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 1400a56729d..bab9968ccee 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -51,6 +51,7 @@ typedef struct {
>      DeviceState *max1111;
>      DeviceState *scp0;
>      DeviceState *scp1;
> +    DeviceState *misc_gpio;
>  } SpitzMachineState;
>
>  #define TYPE_SPITZ_MACHINE "spitz-common"
> @@ -658,8 +659,6 @@ static void spitz_lcdtg_realize(SSISlave *ssi, Error **errp)
>  #define SPITZ_GPIO_MAX1111_CS   20
>  #define SPITZ_GPIO_TP_INT       11
>
> -static DeviceState *max1111;
> -
>  /* "Demux" the signal based on current chipselect */
>  typedef struct {
>      SSISlave ssidev;
> @@ -695,18 +694,6 @@ static void corgi_ssp_gpio_cs(void *opaque, int line, int level)
>  #define SPITZ_BATTERY_VOLT      0xd0    /* About 4.0V */
>  #define SPITZ_CHARGEON_ACIN     0x80    /* About 5.0V */
>
> -static void spitz_adc_temp_on(void *opaque, int line, int level)
> -{
> -    int batt_temp;
> -
> -    if (!max1111)
> -        return;
> -
> -    batt_temp = level ? SPITZ_BATTERY_TEMP : 0;
> -
> -    qemu_set_irq(qdev_get_gpio_in(max1111, MAX1111_BATT_TEMP), batt_temp);
> -}
> -
>  static void corgi_ssp_realize(SSISlave *d, Error **errp)
>  {
>      DeviceState *dev = DEVICE(d);
> @@ -734,7 +721,6 @@ static void spitz_ssp_attach(SpitzMachineState *sms)
>
>      bus = qdev_get_child_bus(sms->mux, "ssi2");
>      sms->max1111 = qdev_new(TYPE_MAX_1111);
> -    max1111 = sms->max1111;
>      qdev_prop_set_uint8(sms->max1111, "input1" /* BATT_VOLT */,
>                          SPITZ_BATTERY_VOLT);
>      qdev_prop_set_uint8(sms->max1111, "input2" /* BATT_TEMP */, 0);
> @@ -810,27 +796,66 @@ static void spitz_akita_i2c_setup(PXA2xxState *cpu)
>
>  /* Other peripherals */
>
> -static void spitz_out_switch(void *opaque, int line, int level)
> +/*
> + * Encapsulation of some miscellaneous GPIO line behaviour for the Spitz boards.
> + *
> + * QEMU interface:
> + *  + named GPIO inputs "green-led", "orange-led", "charging", "discharging":
> + *    these currently just print messages that the line has been signalled
> + *  + named GPIO input "adc-temp-on": set to cause the battery-temperature
> + *    value to be passed to the max111x ADC
> + *  + named GPIO output "adc-temp": the ADC value, to be wired up to the max111x
> + */
> +#define TYPE_SPITZ_MISC_GPIO "spitz-misc-gpio"
> +#define SPITZ_MISC_GPIO(obj) \
> +    OBJECT_CHECK(SpitzMiscGPIOState, (obj), TYPE_SPITZ_MISC_GPIO)
> +
> +typedef struct SpitzMiscGPIOState {
> +    SysBusDevice parent_obj;
> +
> +    qemu_irq adc_value;
> +} SpitzMiscGPIOState;
> +
> +static void spitz_misc_charging(void *opaque, int n, int level)
>  {
> -    switch (line) {
> -    case 0:
> -        zaurus_printf("Charging %s.\n", level ? "off" : "on");
> -        break;
> -    case 1:
> -        zaurus_printf("Discharging %s.\n", level ? "on" : "off");
> -        break;
> -    case 2:
> -        zaurus_printf("Green LED %s.\n", level ? "on" : "off");
> -        break;
> -    case 3:
> -        zaurus_printf("Orange LED %s.\n", level ? "on" : "off");
> -        break;
> -    case 6:
> -        spitz_adc_temp_on(opaque, line, level);
> -        break;
> -    default:
> -        g_assert_not_reached();
> -    }
> +    zaurus_printf("Charging %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_discharging(void *opaque, int n, int level)
> +{
> +    zaurus_printf("Discharging %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_green_led(void *opaque, int n, int level)
> +{
> +    zaurus_printf("Green LED %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_orange_led(void *opaque, int n, int level)
> +{
> +    zaurus_printf("Orange LED %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_adc_temp(void *opaque, int n, int level)
> +{
> +    SpitzMiscGPIOState *s = SPITZ_MISC_GPIO(opaque);
> +    int batt_temp = level ? SPITZ_BATTERY_TEMP : 0;
> +
> +    qemu_set_irq(s->adc_value, batt_temp);
> +}
> +
> +static void spitz_misc_gpio_init(Object *obj)
> +{
> +    SpitzMiscGPIOState *s = SPITZ_MISC_GPIO(obj);
> +    DeviceState *dev = DEVICE(obj);
> +
> +    qdev_init_gpio_in_named(dev, spitz_misc_charging, "charging", 1);
> +    qdev_init_gpio_in_named(dev, spitz_misc_discharging, "discharging", 1);
> +    qdev_init_gpio_in_named(dev, spitz_misc_green_led, "green-led", 1);
> +    qdev_init_gpio_in_named(dev, spitz_misc_orange_led, "orange-led", 1);
> +    qdev_init_gpio_in_named(dev, spitz_misc_adc_temp, "adc-temp-on", 1);
> +
> +    qdev_init_gpio_out_named(dev, &s->adc_value, "adc-temp", 1);
>  }
>
>  #define SPITZ_SCP_LED_GREEN             1
> @@ -850,12 +875,22 @@ static void spitz_out_switch(void *opaque, int line, int level)
>
>  static void spitz_scoop_gpio_setup(SpitzMachineState *sms)
>  {
> -    qemu_irq *outsignals = qemu_allocate_irqs(spitz_out_switch, sms->mpu, 8);
> +    DeviceState *miscdev = sysbus_create_simple(TYPE_SPITZ_MISC_GPIO, -1, NULL);
>
> -    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_CHRG_ON, outsignals[0]);
> -    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_JK_B, outsignals[1]);
> -    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_GREEN, outsignals[2]);
> -    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_ORANGE, outsignals[3]);
> +    sms->misc_gpio = miscdev;
> +
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_CHRG_ON,
> +                          qdev_get_gpio_in_named(miscdev, "charging", 0));
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_JK_B,
> +                          qdev_get_gpio_in_named(miscdev, "discharging", 0));
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_GREEN,
> +                          qdev_get_gpio_in_named(miscdev, "green-led", 0));
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_LED_ORANGE,
> +                          qdev_get_gpio_in_named(miscdev, "orange-led", 0));
> +    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_ADC_TEMP_ON,
> +                          qdev_get_gpio_in_named(miscdev, "adc-temp-on", 0));
> +    qdev_connect_gpio_out_named(miscdev, "adc-temp", 0,
> +                                qdev_get_gpio_in(sms->max1111, MAX1111_BATT_TEMP));
>
>      if (sms->scp1) {
>          qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_CONT,
> @@ -863,8 +898,6 @@ static void spitz_scoop_gpio_setup(SpitzMachineState *sms)
>          qdev_connect_gpio_out(sms->scp1, SPITZ_SCP2_BACKLIGHT_ON,
>                                qdev_get_gpio_in_named(sms->lcdtg, "bl_power", 0));
>      }
> -
> -    qdev_connect_gpio_out(sms->scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
>  }
>
>  #define SPITZ_GPIO_HSYNC                22
> @@ -1217,12 +1250,24 @@ static const TypeInfo spitz_lcdtg_info = {
>      .class_init    = spitz_lcdtg_class_init,
>  };
>
> +static const TypeInfo spitz_misc_gpio_info = {
> +    .name = TYPE_SPITZ_MISC_GPIO,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(SpitzMiscGPIOState),
> +    .instance_init = spitz_misc_gpio_init,
> +    /*
> +     * No class_init required: device has no internal state so does not
> +     * need to set up reset or vmstate, and does not have a realize method.
> +     */
> +};
> +
>  static void spitz_register_types(void)
>  {
>      type_register_static(&corgi_ssp_info);
>      type_register_static(&spitz_lcdtg_info);
>      type_register_static(&spitz_keyboard_info);
>      type_register_static(&sl_nand_info);
> +    type_register_static(&spitz_misc_gpio_info);
>  }
>
>  type_init(spitz_register_types)
> --
> 2.20.1
>
>

Re: [PATCH 16/17] hw/arm/spitz: Provide usual QOM macros for corgi-ssp and spitz-lcdtg

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:35 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The QOM types "spitz-lcdtg" and "corgi-ssp" are missing the
> usual QOM TYPE and casting macros; provide and use them.
>
> In particular, we can safely use the QOM cast macros instead of
> FROM_SSI_SLAVE() because in both cases the 'ssidev' field of
> the instance state struct is the first field in it.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/spitz.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 49eae3fce4e..f020aff9747 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -579,6 +579,9 @@ static void spitz_keyboard_realize(DeviceState *dev, Error **errp)
>  #define LCDTG_PICTRL    0x06
>  #define LCDTG_POLCTRL   0x07
>
> +#define TYPE_SPITZ_LCDTG "spitz-lcdtg"
> +#define SPITZ_LCDTG(obj) OBJECT_CHECK(SpitzLCDTG, (obj), TYPE_SPITZ_LCDTG)
> +
>  typedef struct {
>      SSISlave ssidev;
>      uint32_t bl_intensity;
> @@ -616,7 +619,7 @@ static inline void spitz_bl_power(void *opaque, int line, int level)
>
>  static uint32_t spitz_lcdtg_transfer(SSISlave *dev, uint32_t value)
>  {
> -    SpitzLCDTG *s = FROM_SSI_SLAVE(SpitzLCDTG, dev);
> +    SpitzLCDTG *s = SPITZ_LCDTG(dev);
>      int addr;
>      addr = value >> 5;
>      value &= 0x1f;
> @@ -645,7 +648,7 @@ static uint32_t spitz_lcdtg_transfer(SSISlave *dev, uint32_t value)
>
>  static void spitz_lcdtg_realize(SSISlave *ssi, Error **errp)
>  {
> -    SpitzLCDTG *s = FROM_SSI_SLAVE(SpitzLCDTG, ssi);
> +    SpitzLCDTG *s = SPITZ_LCDTG(ssi);
>      DeviceState *dev = DEVICE(s);
>
>      s->bl_power = 0;
> @@ -664,6 +667,9 @@ static void spitz_lcdtg_realize(SSISlave *ssi, Error **errp)
>  #define SPITZ_GPIO_MAX1111_CS   20
>  #define SPITZ_GPIO_TP_INT       11
>
> +#define TYPE_CORGI_SSP "corgi-ssp"
> +#define CORGI_SSP(obj) OBJECT_CHECK(CorgiSSPState, (obj), TYPE_CORGI_SSP)
> +
>  /* "Demux" the signal based on current chipselect */
>  typedef struct {
>      SSISlave ssidev;
> @@ -673,7 +679,7 @@ typedef struct {
>
>  static uint32_t corgi_ssp_transfer(SSISlave *dev, uint32_t value)
>  {
> -    CorgiSSPState *s = FROM_SSI_SLAVE(CorgiSSPState, dev);
> +    CorgiSSPState *s = CORGI_SSP(dev);
>      int i;
>
>      for (i = 0; i < 3; i++) {
> @@ -702,7 +708,7 @@ static void corgi_ssp_gpio_cs(void *opaque, int line, int level)
>  static void corgi_ssp_realize(SSISlave *d, Error **errp)
>  {
>      DeviceState *dev = DEVICE(d);
> -    CorgiSSPState *s = FROM_SSI_SLAVE(CorgiSSPState, d);
> +    CorgiSSPState *s = CORGI_SSP(d);
>
>      qdev_init_gpio_in(dev, corgi_ssp_gpio_cs, 3);
>      s->bus[0] = ssi_create_bus(dev, "ssi0");
> @@ -714,10 +720,11 @@ static void spitz_ssp_attach(SpitzMachineState *sms)
>  {
>      void *bus;
>
> -    sms->mux = ssi_create_slave(sms->mpu->ssp[CORGI_SSP_PORT - 1], "corgi-ssp");
> +    sms->mux = ssi_create_slave(sms->mpu->ssp[CORGI_SSP_PORT - 1],
> +                                TYPE_CORGI_SSP);
>
>      bus = qdev_get_child_bus(sms->mux, "ssi0");
> -    sms->lcdtg = ssi_create_slave(bus, "spitz-lcdtg");
> +    sms->lcdtg = ssi_create_slave(bus, TYPE_SPITZ_LCDTG);
>
>      bus = qdev_get_child_bus(sms->mux, "ssi1");
>      sms->ads7846 = ssi_create_slave(bus, "ads7846");
> @@ -1220,7 +1227,7 @@ static void corgi_ssp_class_init(ObjectClass *klass, void *data)
>  }
>
>  static const TypeInfo corgi_ssp_info = {
> -    .name          = "corgi-ssp",
> +    .name          = TYPE_CORGI_SSP,
>      .parent        = TYPE_SSI_SLAVE,
>      .instance_size = sizeof(CorgiSSPState),
>      .class_init    = corgi_ssp_class_init,
> @@ -1249,7 +1256,7 @@ static void spitz_lcdtg_class_init(ObjectClass *klass, void *data)
>  }
>
>  static const TypeInfo spitz_lcdtg_info = {
> -    .name          = "spitz-lcdtg",
> +    .name          = TYPE_SPITZ_LCDTG,
>      .parent        = TYPE_SSI_SLAVE,
>      .instance_size = sizeof(SpitzLCDTG),
>      .class_init    = spitz_lcdtg_class_init,
> --
> 2.20.1
>
>

Re: [PATCH 17/17] Replace uses of FROM_SSI_SLAVE() macro with QOM casts

[Alistair Francis]

On Sun, Jun 28, 2020 at 7:37 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The FROM_SSI_SLAVE() macro predates QOM and is used as a typesafe way
> to cast from an SSISlave* to the instance struct of a subtype of
> TYPE_SSI_SLAVE.  Switch to using the QOM cast macros instead, which
> have the same effect (by writing the QOM macros if the types were
> previously missing them.)
>
> (The FROM_SSI_SLAVE() macro allows the SSISlave member of the
> subtype's struct to be anywhere as long as it is named "ssidev",
> whereas a QOM cast macro insists that it is the first thing in the
> subtype's struct.  This is true for all the types we convert here.)
>
> This removes all the uses of FROM_SSI_SLAVE() so we can delete the
> definition.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/ssi/ssi.h |  2 --
>  hw/arm/z2.c          | 11 +++++++----
>  hw/display/ads7846.c |  9 ++++++---
>  hw/display/ssd0323.c | 10 +++++++---
>  hw/sd/ssi-sd.c       |  4 ++--
>  5 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 5fd411f2e4e..eac168aa1db 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -66,8 +66,6 @@ struct SSISlave {
>      bool cs;
>  };
>
> -#define FROM_SSI_SLAVE(type, dev) DO_UPCAST(type, ssidev, dev)
> -
>  extern const VMStateDescription vmstate_ssi_slave;
>
>  #define VMSTATE_SSI_SLAVE(_field, _state) {                          \
> diff --git a/hw/arm/z2.c b/hw/arm/z2.c
> index a0f40959904..e1f22f58681 100644
> --- a/hw/arm/z2.c
> +++ b/hw/arm/z2.c
> @@ -111,9 +111,12 @@ typedef struct {
>      int pos;
>  } ZipitLCD;
>
> +#define TYPE_ZIPIT_LCD "zipit-lcd"
> +#define ZIPIT_LCD(obj) OBJECT_CHECK(ZipitLCD, (obj), TYPE_ZIPIT_LCD)
> +
>  static uint32_t zipit_lcd_transfer(SSISlave *dev, uint32_t value)
>  {
> -    ZipitLCD *z = FROM_SSI_SLAVE(ZipitLCD, dev);
> +    ZipitLCD *z = ZIPIT_LCD(dev);
>      uint16_t val;
>      if (z->selected) {
>          z->buf[z->pos] = value & 0xff;
> @@ -153,7 +156,7 @@ static void z2_lcd_cs(void *opaque, int line, int level)
>
>  static void zipit_lcd_realize(SSISlave *dev, Error **errp)
>  {
> -    ZipitLCD *z = FROM_SSI_SLAVE(ZipitLCD, dev);
> +    ZipitLCD *z = ZIPIT_LCD(dev);
>      z->selected = 0;
>      z->enabled = 0;
>      z->pos = 0;
> @@ -185,7 +188,7 @@ static void zipit_lcd_class_init(ObjectClass *klass, void *data)
>  }
>
>  static const TypeInfo zipit_lcd_info = {
> -    .name          = "zipit-lcd",
> +    .name          = TYPE_ZIPIT_LCD,
>      .parent        = TYPE_SSI_SLAVE,
>      .instance_size = sizeof(ZipitLCD),
>      .class_init    = zipit_lcd_class_init,
> @@ -325,7 +328,7 @@ static void z2_init(MachineState *machine)
>
>      type_register_static(&zipit_lcd_info);
>      type_register_static(&aer915_info);
> -    z2_lcd = ssi_create_slave(mpu->ssp[1], "zipit-lcd");
> +    z2_lcd = ssi_create_slave(mpu->ssp[1], TYPE_ZIPIT_LCD);
>      bus = pxa2xx_i2c_bus(mpu->i2c[0]);
>      i2c_create_slave(bus, TYPE_AER915, 0x55);
>      wm = i2c_create_slave(bus, TYPE_WM8750, 0x1b);
> diff --git a/hw/display/ads7846.c b/hw/display/ads7846.c
> index 9228b40b1af..56bf82fe079 100644
> --- a/hw/display/ads7846.c
> +++ b/hw/display/ads7846.c
> @@ -29,6 +29,9 @@ typedef struct {
>      int output;
>  } ADS7846State;
>
> +#define TYPE_ADS7846 "ads7846"
> +#define ADS7846(obj) OBJECT_CHECK(ADS7846State, (obj), TYPE_ADS7846)
> +
>  /* Control-byte bitfields */
>  #define CB_PD0         (1 << 0)
>  #define CB_PD1         (1 << 1)
> @@ -61,7 +64,7 @@ static void ads7846_int_update(ADS7846State *s)
>
>  static uint32_t ads7846_transfer(SSISlave *dev, uint32_t value)
>  {
> -    ADS7846State *s = FROM_SSI_SLAVE(ADS7846State, dev);
> +    ADS7846State *s = ADS7846(dev);
>
>      switch (s->cycle ++) {
>      case 0:
> @@ -139,7 +142,7 @@ static const VMStateDescription vmstate_ads7846 = {
>  static void ads7846_realize(SSISlave *d, Error **errp)
>  {
>      DeviceState *dev = DEVICE(d);
> -    ADS7846State *s = FROM_SSI_SLAVE(ADS7846State, d);
> +    ADS7846State *s = ADS7846(d);
>
>      qdev_init_gpio_out(dev, &s->interrupt, 1);
>
> @@ -166,7 +169,7 @@ static void ads7846_class_init(ObjectClass *klass, void *data)
>  }
>
>  static const TypeInfo ads7846_info = {
> -    .name          = "ads7846",
> +    .name          = TYPE_ADS7846,
>      .parent        = TYPE_SSI_SLAVE,
>      .instance_size = sizeof(ADS7846State),
>      .class_init    = ads7846_class_init,
> diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
> index c3bdb18742c..32d27f008ae 100644
> --- a/hw/display/ssd0323.c
> +++ b/hw/display/ssd0323.c
> @@ -66,9 +66,13 @@ typedef struct {
>      uint8_t framebuffer[128 * 80 / 2];
>  } ssd0323_state;
>
> +#define TYPE_SSD0323 "ssd0323"
> +#define SSD0323(obj) OBJECT_CHECK(ssd0323_state, (obj), TYPE_SSD0323)
> +
> +
>  static uint32_t ssd0323_transfer(SSISlave *dev, uint32_t data)
>  {
> -    ssd0323_state *s = FROM_SSI_SLAVE(ssd0323_state, dev);
> +    ssd0323_state *s = SSD0323(dev);
>
>      switch (s->mode) {
>      case SSD0323_DATA:
> @@ -346,7 +350,7 @@ static const GraphicHwOps ssd0323_ops = {
>  static void ssd0323_realize(SSISlave *d, Error **errp)
>  {
>      DeviceState *dev = DEVICE(d);
> -    ssd0323_state *s = FROM_SSI_SLAVE(ssd0323_state, d);
> +    ssd0323_state *s = SSD0323(d);
>
>      s->col_end = 63;
>      s->row_end = 79;
> @@ -368,7 +372,7 @@ static void ssd0323_class_init(ObjectClass *klass, void *data)
>  }
>
>  static const TypeInfo ssd0323_info = {
> -    .name          = "ssd0323",
> +    .name          = TYPE_SSD0323,
>      .parent        = TYPE_SSI_SLAVE,
>      .instance_size = sizeof(ssd0323_state),
>      .class_init    = ssd0323_class_init,
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 25cec2ddeaa..25cdf4c966d 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -74,7 +74,7 @@ typedef struct {
>
>  static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
>  {
> -    ssi_sd_state *s = FROM_SSI_SLAVE(ssi_sd_state, dev);
> +    ssi_sd_state *s = SSI_SD(dev);
>
>      /* Special case: allow CMD12 (STOP TRANSMISSION) while reading data.  */
>      if (s->mode == SSI_SD_DATA_READ && val == 0x4d) {
> @@ -241,7 +241,7 @@ static const VMStateDescription vmstate_ssi_sd = {
>
>  static void ssi_sd_realize(SSISlave *d, Error **errp)
>  {
> -    ssi_sd_state *s = FROM_SSI_SLAVE(ssi_sd_state, d);
> +    ssi_sd_state *s = SSI_SD(d);
>      DeviceState *carddev;
>      DriveInfo *dinfo;
>      Error *err = NULL;
> --
> 2.20.1
>
>