Re: [PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c

[Liav Albani <liavalb@gmail.com>]

On 2/19/22 13:19, BALATON Zoltan wrote:
> On Sat, 19 Feb 2022, Liav Albani wrote:
>> Instead of letting each implementation to duplicate this code, we can
>> share these functions between IDE PIIX3/4 and VIA implementations.
>
> OK but there's a way to take this even further as cmd646 also uses 
> similar functions just with more cases so you could remove the cases 
> handled by these functions and only leave the difference and call the 
> default function from the default case. E.g. (untested, just to show 
> the idea):
>
> hw/ide/cmd646.c:
> static uint64_t bmdma_read(void *opaque, hwaddr addr,
>                            unsigned size)
> {
>     BMDMAState *bm = opaque;
>     PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
>     uint32_t val;
>
>     if (size != 1) {
>         return ((uint64_t)1 << (size * 8)) - 1;
>     }
>
>     switch(addr & 3) {
>     case 1:
>         val = pci_dev->config[MRDMODE];
>         break;
>     case 3:
>         if (bm == &bm->pci_dev->bmdma[0]) {
>             val = pci_dev->config[UDIDETCR0];
>         } else {
>             val = pci_dev->config[UDIDETCR1];
>         }
>         break;
>     default:
>         val = bmdma_default_read(opaque, addr, size);
>         break;
>     }
>
>     trace_bmdma_read_cmd646(addr, val);
>     return val;
> }
>
Yeah, I see how I can do that for both bmdma write and read of cmd646. 
I'll send a v2 right away with a fix.
>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>> ---
>> hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
>> hw/ide/piix.c        | 50 ++-----------------------------------------
>> hw/ide/via.c         | 51 ++------------------------------------------
>> include/hw/ide/pci.h |  4 ++++
>> 4 files changed, 55 insertions(+), 97 deletions(-)
>>
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index 84ba733548..c8b867659a 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
>>     .reset = bmdma_reset,
>> };
>>
>> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>> +                           unsigned size)
>
> Indentation off? Also everywhere else, usually we indent not with the 
> parenthesis but with the list within. (Auto indent in most editors 
> does that probably.)
>
I guess you mean that it should be:

+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+                                                unsigned size)

Like this?

I'm using Visual Studio Code, so I might not have the correct settings 
for this editor with QEMU.
The checkpatch script doesn't complain on style issues, so what can I do 
to make this correct?

Best regards,
Liav

> Regards,
> BALATON Zoltan