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

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

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

Okay, no problem.