Peter Maydell <peter.mayd...@linaro.org> writes: > On Mon, 18 Feb 2019 at 13:08, Markus Armbruster <arm...@redhat.com> wrote: >> >> pflash_cfi01_register() creates a TYPE_CFI_PFLASH01 device, sets >> properties, realizes, and wires up. >> >> We have three modified copies of it, because their users need to set >> additional properties, or have the wiring done differently. >> >> Factor out their common part into pflash_cfi01_create(). >> >> Signed-off-by: Markus Armbruster <arm...@redhat.com> >> --- >> hw/arm/vexpress.c | 22 +++++----------------- >> hw/arm/virt.c | 26 +++++++++----------------- >> hw/block/pflash_cfi01.c | 39 +++++++++++++++++++++++++++------------ >> hw/xtensa/xtfpga.c | 18 +++++++----------- >> include/hw/block/flash.h | 8 ++++++++ >> 5 files changed, 56 insertions(+), 57 deletions(-) >> >> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c >> index 00913f2655..b23c63ed24 100644 >> --- a/hw/arm/vexpress.c >> +++ b/hw/arm/vexpress.c >> @@ -515,26 +515,14 @@ static void vexpress_modify_dtb(const struct >> arm_boot_info *info, void *fdt) >> static PFlashCFI01 *ve_pflash_cfi01_register(hwaddr base, const char *name, >> DriveInfo *di) >> { >> - DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01); >> + DeviceState *dev; >> >> - if (di) { >> - qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di), >> - &error_abort); >> - } >> - >> - qdev_prop_set_uint32(dev, "num-blocks", >> - VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE); >> - qdev_prop_set_uint64(dev, "sector-length", VEXPRESS_FLASH_SECT_SIZE); >> - qdev_prop_set_uint8(dev, "width", 4); >> + dev = DEVICE(pflash_cfi01_create(name, VEXPRESS_FLASH_SIZE, >> + di ? blk_by_legacy_dinfo(di) : NULL, >> + VEXPRESS_FLASH_SECT_SIZE, >> + 4, 0x89, 0x18, 0x00, 0x00, false)); >> qdev_prop_set_uint8(dev, "device-width", 2); >> - qdev_prop_set_bit(dev, "big-endian", false); >> - qdev_prop_set_uint16(dev, "id0", 0x89); >> - qdev_prop_set_uint16(dev, "id1", 0x18); >> - qdev_prop_set_uint16(dev, "id2", 0x00); >> - qdev_prop_set_uint16(dev, "id3", 0x00); >> - qdev_prop_set_string(dev, "name", name); >> qdev_init_nofail(dev); >> - >> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); >> return CFI_PFLASH01(dev); >> } > > I prefer this code the way it stands. In particular the > "call another function but then set the 'device-width' > property here" looks dubious. But broadly speaking the > "do all the property setting directly rather than calling > a helper function" is the style choice I think we should > be aiming for. (The prevalence of the other approach is > due to a mix of (1) older code we haven't updated and > (2) property-setting being annoyingly heavyweight > syntax.)
Okay, no problem.