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

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 4707 bytes --]

On Sat, 19 Feb 2022, BALATON Zoltan wrote:
> On Sat, 19 Feb 2022, Liav Albani wrote:
>> 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.
>
> Maybe even the first if that's already contained in the default function 
> could be avoided with some reorganisation like
>
> if (size == 1) {
>    remaining switch cases to set val
> } else {
>    val = bmdma_default_read();
> }

On second thought this misses the cases where size == 1 and addr is those 
handeled in bmdma_default_read so one would still need the default case in 
the if part and then it's not much better than duplicating the if. Maybe 
calling the default first, then handling the remaining cases, like

val = bmdma_default_read();
if (size == 1) {
     remaining switch cases to set val
}
return val;

is the simplest and avoids the duplicated if. (Now we only have two trace 
messages instead of one but that's probably not a problem as it's only a 
debugging aid.

Regards.
BALATON Zoltan

> but I wasn't sure that won't change anything so may need a bit more thought.
>
>>>> 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?
>
> No, like the code you've moved had it. The split line should start after the 
> ( not on the same column. So:
>
> uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>                             unsigned size)
>
> but this line does not need to be split at all as it fits within 80 chars so 
> better to keep it one line and only split where needed.
>
>> 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?
>
> If checkpatch is happy then probably not a problem but just look at how code 
> is indented on other places and follow the same. The coding style doc may 
> have some words on it too. I don't know what setting Visual Studio might 
> need.
>
> Regards,
> BALATON Zoltan