[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 10/10] hw/arm hw/xtensa: De-duplicate pflash cre
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 10/10] hw/arm hw/xtensa: De-duplicate pflash creation code some |
Date: |
Tue, 19 Feb 2019 16:46:39 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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.
- Re: [Qemu-devel] [PATCH 02/10] pflash: Macro PFLASH_BUG() is used just once, expand, (continued)
[Qemu-devel] [PATCH 10/10] hw/arm hw/xtensa: De-duplicate pflash creation code some, Markus Armbruster, 2019/02/18
[Qemu-devel] [PATCH 04/10] sam460ex: Don't size flash memory to match backing image, Markus Armbruster, 2019/02/18
[Qemu-devel] [PATCH 06/10] r2d: Flash memory creation is confused about size, mark FIXME, Markus Armbruster, 2019/02/18