Re: [Qemu-block] [Qemu-devel] [PATCH 10/10] hw/arm hw/xtensa: De-duplicate pflash creation code some

[Peter Maydell]

On Mon, 18 Feb 2019 at 13:08, Markus Armbruster <address@hidden> 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 <address@hidden>
> ---
>  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