On 23/10/2018 07:49, Thomas Huth wrote: > On 2018-10-18 19:28, Mark Cave-Ayland wrote: >> From: Laurent Vivier <laur...@vivier.eu> >> >> Co-developed-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> >> Signed-off-by: Laurent Vivier <laur...@vivier.eu> >> --- >> hw/input/adb.c | 2 + >> hw/misc/mac_via.c | 166 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/misc/mac_via.h | 7 ++ >> 3 files changed, 175 insertions(+) >> >> diff --git a/hw/input/adb.c b/hw/input/adb.c >> index bbb40aeef1..d69ca74364 100644 >> --- a/hw/input/adb.c >> +++ b/hw/input/adb.c >> @@ -25,6 +25,8 @@ >> #include "hw/input/adb.h" >> #include "adb-internal.h" >> >> +#define ADB_POLL_FREQ 50 > > A single define without a user in a .c file? Looks suspicious... > > As far as I can see, this has been replace by VIA_ADB_POLL_FREQ which > has been introduced in the previous patch already, so you can remove > this define here.
Ooops yes, this should have been removed from my previous refactoring - I've now fixed this. >> /* error codes */ >> #define ADB_RET_NOTPRESENT (-2) >> >> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c >> index 084974a24d..1ec563a707 100644 >> --- a/hw/misc/mac_via.c >> +++ b/hw/misc/mac_via.c > [...] >> +static int adb_via_send(MacVIAState *s, int state, uint8_t data) >> +{ >> + switch (state) { >> + case ADB_STATE_NEW: >> + s->adb_data_out_index = 0; >> + break; >> + case ADB_STATE_EVEN: >> + if ((s->adb_data_out_index & 1) == 0) { >> + return 0; >> + } >> + break; >> + case ADB_STATE_ODD: >> + if (s->adb_data_out_index & 1) { >> + return 0; >> + } >> + break; >> + case ADB_STATE_IDLE: >> + return 0; >> + } > > Could you please add a > > assert(s->adb_data_out_index < sizeof(s->adb_data_out) -1); > > here, just in case? Done. >> + s->adb_data_out[s->adb_data_out_index++] = data; >> + qemu_irq_raise(s->adb_data_ready); >> + return 1; >> +} >> + >> +static int adb_via_receive(MacVIAState *s, int state, uint8_t *data) >> +{ >> + switch (state) { >> + case ADB_STATE_NEW: >> + return 0; >> + case ADB_STATE_EVEN: >> + if (s->adb_data_in_size <= 0) { >> + qemu_irq_raise(s->adb_data_ready); >> + return 0; >> + } >> + if (s->adb_data_in_index >= s->adb_data_in_size) { >> + *data = 0; >> + qemu_irq_raise(s->adb_data_ready); >> + return 1; >> + } >> + if ((s->adb_data_in_index & 1) == 0) { >> + return 0; >> + } >> + break; >> + case ADB_STATE_ODD: >> + if (s->adb_data_in_size <= 0) { >> + qemu_irq_raise(s->adb_data_ready); >> + return 0; >> + } >> + if (s->adb_data_in_index >= s->adb_data_in_size) { >> + *data = 0; >> + qemu_irq_raise(s->adb_data_ready); >> + return 1; >> + } >> + if (s->adb_data_in_index & 1) { >> + return 0; >> + } >> + break; >> + case ADB_STATE_IDLE: >> + if (s->adb_data_out_index == 0) { >> + return 0; >> + } >> + s->adb_data_in_size = adb_request(&s->adb_bus, s->adb_data_in, >> + s->adb_data_out, >> + s->adb_data_out_index); >> + s->adb_data_out_index = 0; >> + s->adb_data_in_index = 0; >> + if (s->adb_data_in_size < 0) { >> + *data = 0xff; >> + qemu_irq_raise(s->adb_data_ready); >> + return -1; >> + } >> + if (s->adb_data_in_size == 0) { >> + return 0; >> + } >> + break; >> + } > > Please also add an assert before the next line here - just in case... And also done here. >> + *data = s->adb_data_in[s->adb_data_in_index++]; >> + qemu_irq_raise(s->adb_data_ready); >> + if (*data == 0xff || *data == 0) { >> + return 0; >> + } >> + return 1; >> +} ATB, Mark.