On 14/06/2020 18:16, Philippe Mathieu-Daudé wrote:
> Hi Mark,
>
> On 6/14/20 4:28 PM, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
>> ---
>> hw/input/adb.c | 23 ++++++++++++++++++++++-
>> hw/input/trace-events | 7 +++++++
>> 2 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/input/adb.c b/hw/input/adb.c
>> index fe0f6c7ef3..4976f52c36 100644
>> --- a/hw/input/adb.c
>> +++ b/hw/input/adb.c
>> @@ -29,10 +29,18 @@
>> #include "qemu/module.h"
>> #include "qemu/timer.h"
>> #include "adb-internal.h"
>> +#include "trace.h"
>>
>> /* error codes */
>> #define ADB_RET_NOTPRESENT (-2)
>>
>> +static const char *adb_commands[] = {
>> + "RESET", "FLUSH", "(Reserved 0x2)", "(Reserved 0x3)",
>> + "Reserved (0x4)", "(Reserved 0x5)", "(Reserved 0x6)", "(Reserved 0x7)",
>> + "LISTEN r0", "LISTEN r1", "LISTEN r2", "LISTEN r3",
>> + "TALK r0", "TALK r1", "TALK r2", "TALK r3",
>> +};
>> +
>> static void adb_device_reset(ADBDevice *d)
>> {
>> qdev_reset_all(DEVICE(d));
>> @@ -86,9 +94,16 @@ static int do_adb_request(ADBBusState *s, uint8_t *obuf,
>> const uint8_t *buf,
>>
>> int adb_request(ADBBusState *s, uint8_t *obuf, const uint8_t *buf, int len)
>> {
>> + int ret;
>> +
>> + trace_adb_bus_request(buf[0] >> 4, adb_commands[buf[0] & 0xf], len);
>> +
>> assert(s->autopoll_blocked);
>>
>> - return do_adb_request(s, obuf, buf, len);
>> + ret = do_adb_request(s, obuf, buf, len);
>> +
>> + trace_adb_bus_request_done(buf[0] >> 4, adb_commands[buf[0] & 0xf],
>> ret);
>> + return ret;
>> }
>>
>> int adb_poll(ADBBusState *s, uint8_t *obuf, uint16_t poll_mask)
>> @@ -160,6 +175,8 @@ void adb_set_autopoll_mask(ADBBusState *s, uint16_t mask)
>>
>> void adb_autopoll_block(ADBBusState *s)
>> {
>> + trace_adb_bus_autopoll_block("autopoll BLOCKED");
>
> Regarding how trace backends work, in this case it is better
> to use a boolean value and let the backend do the formatting:
>
> trace_adb_bus_autopoll_block(true);
>
> The rationale is it is easier for backends to filter on a
> bool (register) arg rather than fetching memory for strcmp.
>
> So format can be:
>
> adb_bus_autopoll_block(bool state) "autopoll is_blocked:%u"
>
> Anyway if you want to keep as it, it is cleaner to change the
> format as "autopoll %s".
>
>> +
>> s->autopoll_blocked = true;
>
> This can also be:
>
> trace_adb_bus_autopoll_block(s->autopoll_blocked);
>
>>
>> if (s->autopoll_enabled) {
>> @@ -169,6 +186,8 @@ void adb_autopoll_block(ADBBusState *s)
>>
>> void adb_autopoll_unblock(ADBBusState *s)
>> {
>> + trace_adb_bus_autopoll_block("autopoll UNBLOCKED");
>> +
>> s->autopoll_blocked = false;
>
> Ditto:
>
> trace_adb_bus_autopoll_block(s->autopoll_blocked);
>
>>
>> if (s->autopoll_enabled) {
>> @@ -183,7 +202,9 @@ static void adb_autopoll(void *opaque)
>> ADBBusState *s = opaque;
>>
>> if (!s->autopoll_blocked) {
>> + trace_adb_bus_autopoll_cb(s->autopoll_mask);
>> s->autopoll_cb(s->autopoll_cb_opaque);
>> + trace_adb_bus_autopoll_cb_done(s->autopoll_mask);
>> }
>>
>> timer_mod(s->autopoll_timer,
>> diff --git a/hw/input/trace-events b/hw/input/trace-events
>> index 6f0d78241c..119d1ce2bd 100644
>> --- a/hw/input/trace-events
>> +++ b/hw/input/trace-events
>> @@ -14,6 +14,13 @@ adb_device_mouse_readreg(int reg, uint8_t val0, uint8_t
>> val1) "reg %d obuf[0] 0x
>> adb_device_mouse_request_change_addr(int devaddr) "change addr to 0x%x"
>> adb_device_mouse_request_change_addr_and_handler(int devaddr, int handler)
>> "change addr and handler to 0x%x, 0x%x"
>>
>> +# adb.c
>> +adb_bus_request(uint8_t addr, const char *cmd, int size) "device 0x%x %s
>> cmdsize=%d"
>> +adb_bus_request_done(uint8_t addr, const char *cmd, int size) "device 0x%x
>> %s replysize=%d"
>> +adb_bus_autopoll_block(const char *s) "%s"
>> +adb_bus_autopoll_cb(uint16_t mask) "executing autopoll_cb with autopoll
>> mask 0x%x"
>> +adb_bus_autopoll_cb_done(uint16_t mask) "done executing autopoll_cb with
>> autopoll mask 0x%x"
>> +
>> # pckbd.c
>> pckbd_kbd_read_data(uint32_t val) "0x%02x"
>> pckbd_kbd_read_status(int status) "0x%02x"
>>
>
> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Thanks for the review: I think you're right in that the boolean makes things
easier
for other trace backends, so I'll implement your changes in a v2.
ATB,
Mark.
