[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, P
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02 |
Date: |
Tue, 19 Feb 2019 15:33:46 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.0 |
On 2/19/19 2:41 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <address@hidden> writes:
>
>> On 2/18/19 1:56 PM, Markus Armbruster wrote:
>>> flash.h's incomplete struct pflash_t is completed both in
>>> pflash_cfi01.c and in pflash_cfi02.c. The complete types are
>>> incompatible. This can hide type errors, such as passing a pflash_t
>>> created with pflash_cfi02_register() to pflash_cfi01_get_memory().
>>>
>>> Furthermore, POSIX reserves typedef names ending with _t.
Worth adding in CODING_STYLE 'Naming' section :)
>>>
>>> Rename the two structs to PFlashCFI01 and PFlashCFI02.
>>
>> Why not ParallelFlashCFIxx?
>
> Feels a bit long, and we abbreviate to pflash pretty consistently. That
> said, I'm not particularly enamored with my choice of name :)
>
>> Ideally ParallelFlashCFI would be an InterfaceInfo...
>
> You mean TYPE_CFI_PFLASH0{1,2} should be children of an abstract parent?
I'd use "TYPE_PFLASH_CFI0[12]".
---
The "Common Flash memory Interface" as stated is definitively an
interface :)
QEMU models the 2 most famous industry implementations:
- vendor 0x01: Intel (and Sharp)
- vendor 0x02: AMD (and Fujistu)
---
My first refactor attempt was to have both implementations inheritate
from an abstract parent, but since the migration structures are
different, it looked easier to me to extract an InterfaceInfo.
>From here my idea was to add an NorFlash abstract parent where to share
the block devices and some of the reset:
+-------------------+
| |
| NOR Flash |
| |
+-------------------+
| Parent |
| |
| |
+------------v---+ +---v------------+
| | | |
| PFlash01 | | PFlash02 |
| | | |
+----------------+ +----------------+
Child | |
| |
| Implements|
+---v-----------v---+
| |
| CFI Flash |
| |
+-------------------+
But since there is no consumer of the CFI InterfaceInfo, we can simply
go the way you suggested:
+-------------------+
| |
| CFI NOR Flash |
| |
+-------------------+
| Parent |
| |
+------------v---+ +---v------------+
| | | |
| PFlash01 | | PFlash02 |
| | | |
+----------------+ +----------------+
Child Child
- Re: [Qemu-devel] [PATCH 08/10] pflash: Clean up after commit 368a354f02b part 1, (continued)
- [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02, Markus Armbruster, 2019/02/18
- Re: [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02, Laszlo Ersek, 2019/02/18
- Re: [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02, Philippe Mathieu-Daudé, 2019/02/19
- Re: [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02, Markus Armbruster, 2019/02/19
- Re: [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02,
Philippe Mathieu-Daudé <=
- Re: [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02, Markus Armbruster, 2019/02/21
- Re: [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02, Philippe Mathieu-Daudé, 2019/02/21
Re: [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02, Alex Bennée, 2019/02/21
Re: [Qemu-devel] [PATCH 00/10] pflash: Fixes and cleanups, no-reply, 2019/02/18
Re: [Qemu-devel] [PATCH 00/10] pflash: Fixes and cleanups, no-reply, 2019/02/18
Re: [Qemu-devel] [PATCH 00/10] pflash: Fixes and cleanups, no-reply, 2019/02/18
Re: [Qemu-devel] [PATCH 00/10] pflash: Fixes and cleanups, no-reply, 2019/02/18
Re: [Qemu-devel] [PATCH 00/10] pflash: Fixes and cleanups, no-reply, 2019/02/18
Re: [Qemu-devel] [PATCH 00/10] pflash: Fixes and cleanups, no-reply, 2019/02/19
Re: [Qemu-devel] [PATCH 00/10] pflash: Fixes and cleanups, no-reply, 2019/02/19