Re: [PATCH 06/13] hw/ide: Extract bmdma_init_ops()

[Bernhard Beschow]

Am 23. April 2023 17:43:22 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>On 22/4/23 17:07, Bernhard Beschow wrote:
>> There are three private copies of bmdma_setup_bar() with small adaptions.
>> Consolidate them into one public implementation.
>> 
>> While at it rename the function to bmdma_init_ops() to reflect that the 
>> memory
>> regions being initialized represent BMDMA operations. The actual mapping as a
>> PCI BAR is still performed separately in each device.
>> 
>> Note that the bmdma_bar attribute will be renamed in a separate commit.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/hw/ide/pci.h |  1 +
>>   hw/ide/cmd646.c      | 20 +-------------------
>>   hw/ide/pci.c         | 16 ++++++++++++++++
>>   hw/ide/piix.c        | 19 +------------------
>>   hw/ide/via.c         | 19 +------------------
>>   5 files changed, 20 insertions(+), 55 deletions(-)
>
>Nice.
>
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

I'd rework this patch in the next iteration. I think that most of the memory 
region initialization can be moved to pci_ide_init(). Unlike realize methods, 
the nice thing about these instance_init() methods is that the parent 
implementation is called implicitly rather than being overridden by the child 
implementation, similar to C++ constructors. This allows for code reuse without 
much gymnastics.

Best regards,
Bernhard