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.)
thanks
-- PMM
