Le 19/12/2017 à 16:26, Philippe Mathieu-Daudé a écrit :
> Hi Laurent,
>
> On Tue, Dec 19, 2017 at 8:54 AM, Laurent Vivier <laur...@vivier.eu> wrote:
>> It makes the code clearer to separate the bus implementation
>> from the devices one.
>>
>> Remove ADB_DPRINTF() from adb.c instead of adding it to new files.
>> Some minor changes to make checkpatch.pl happy.
>>
>> Signed-off-by: Laurent Vivier <laur...@vivier.eu>
>> ---
>> hw/input/Makefile.objs | 2 +-
>> hw/input/adb-kbd.c | 395 +++++++++++++++++++++++++++++++
>> hw/input/adb-mouse.c | 250 ++++++++++++++++++++
>> hw/input/adb.c | 621
>> +------------------------------------------------
>> include/hw/input/adb.h | 26 +++
>> 5 files changed, 673 insertions(+), 621 deletions(-)
>> create mode 100644 hw/input/adb-kbd.c
>> create mode 100644 hw/input/adb-mouse.c
>>
...
>> +
>> +/* ADB commands */
>> +
>> +#define ADB_BUSRESET 0x00
>> +#define ADB_FLUSH 0x01
>> +#define ADB_WRITEREG 0x08
>> +#define ADB_READREG 0x0c
>> +
>> +/* ADB device commands */
>> +
>> +#define ADB_CMD_SELF_TEST 0xff
>> +#define ADB_CMD_CHANGE_ID 0xfe
>> +#define ADB_CMD_CHANGE_ID_AND_ACT 0xfd
>> +#define ADB_CMD_CHANGE_ID_AND_ENABLE 0x00
>> +
>> +/* ADB default device IDs (upper 4 bits of ADB command byte) */
>> +
>> +#define ADB_DEVID_DONGLE 1
>> +#define ADB_DEVID_KEYBOARD 2
>> +#define ADB_DEVID_MOUSE 3
>> +#define ADB_DEVID_TABLET 4
>> +#define ADB_DEVID_MODEM 5
>> +#define ADB_DEVID_MISC 7
>
> Is ADB the protocol used out of adb*.c?
> Personally I prefer not expose those protocol defines if not needed,
> so I'd add them to hw/input/adb-protocol.h (or to match the other
> includes, hw/input/adb-internal.h).
>
> regardless this comment:
> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>
>> +
>> typedef struct ADBDevice ADBDevice;
>>
>> /* buf = NULL means polling */
>> @@ -77,6 +101,8 @@ struct ADBBusState {
>> int poll_index;
>> };
>>
>> +extern const VMStateDescription vmstate_adb_device;
>
> If you choose to add hw/input/adb-internal.h, this extern can go there.
>
>> +
>> int adb_request(ADBBusState *s, uint8_t *buf_out,
>> const uint8_t *buf, int len);
>> int adb_poll(ADBBusState *s, uint8_t *buf_out, uint16_t poll_mask);
>> --
>> 2.14.3
>>
I agree with you, I'm going to add an new include, hw/input/adb-internal.h
Thanks,
Laurent
