On Sat, 19 Feb 2022, Liav Albani wrote:
On 2/19/22 17:57, BALATON Zoltan wrote:
On Sat, 19 Feb 2022, BALATON Zoltan wrote:
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.
Hmm, is it OK though to have two trace messages? I'm not against it, but if
someone tries to use the debug messages to see what's going on, it's better
to not have two of the same message as it will confuse people. We definitely
don't want that to happen.
I don't think this is a problem. One message is from the bmdma default
function and one is from the cmd646 specific function overriding it so
both may have some usefulness and one can enable/disable them separately
so for cmd646 one can only enable the specific one to avoid duplicated
messages.
So, let's keep it simple (and remove code duplications) but also let's do
that correctly, to ensure that in the view of the developer that uses the
debug messages, it all seem clear and neat :)
I don't see an easy way to avoid that otherwise because we can't move the
trace out from the default functions because some controllers only use
that so the simplest is to not care about this.
but I wasn't sure that won't change anything so may need a bit more
thought.
Signed-off-by: Liav Albani <liav...@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.
OK. I'll align it to the character after the start of the parenthesis. I'll
take a look into other code snippets in QEMU, but at least in the IDE code,
there are lots of code style issues (the checkpatch script says so) so I'll
probably look into other parts of QEMU to see how it goes.
Some parts of QEMU has some old code not yet following the latest coding
style. Checkpatch usually tells you and if it's a few lines it's OK to
correct it in the part you touch to silence checkpatch. Otherwise a
separate code style fix patch before the modifications could also be
included to avoid such warnings during later changes. I think this is
explained in the patch submission guidelines.
I'll take this a bit slow, as I wanted to send v2 today. This is probably not
a good idea as we should let people to see this and maybe to let the
maintainer (John) to look into this and put his comments on this too. I'll
give this a couple of days - no rush here, although I'd be very happy to see
things going forward as soon as possible, so we merge this patch and then
going back to the ich6-ide patch, and then hopefully more patches to the IDE
code to add more features and fixes in this part of QEMU codebase.
The only constraint is if you want this in the next release or it's ok to
postpone it further. See the planning page about the schedule:
https://wiki.qemu.org/Planning/7.0 which means if it's not in a pull
request until March 8 then it will miss 7.0 and you'll have plenty of time
to get the remaining changes polished until development opens again in
second half of April.
Regards,
BALATON Zoltan