Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 2/4] adb.c: add support for QKeyCode

[BALATON Zoltan]

Hello,

Not a review just some style comments.

On Wed, 17 Aug 2016, John Arbuckle wrote:
> static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
> {
> -    static int ext_keycode;
>     KBDState *s = ADB_KEYBOARD(d);
> -    int adb_keycode, keycode;
> +    int keycode;
>     int olen;
>
>     olen = 0;
> -    for(;;) {
> -        if (s->count == 0)
> -            break;
> -        keycode = s->data[s->rptr];
> -        if (++s->rptr == sizeof(s->data))
> -            s->rptr = 0;
> -        s->count--;
> -
> -        if (keycode == 0xe0) {
> -            ext_keycode = 1;
> -        } else {
> -            if (ext_keycode)
> -                adb_keycode =  pc_to_adb_keycode[keycode | 0x80];
> -            else
> -                adb_keycode =  pc_to_adb_keycode[keycode & 0x7f];
> -            obuf[0] = adb_keycode | (keycode & 0x80);
> -            /* NOTE: could put a second keycode if needed */
> -            obuf[1] = 0xff;
> -            olen = 2;
> -            ext_keycode = 0;
> -            break;
> -        }
> +    if (s->count == 0) {
> +        return 0;
> +    }
> +    keycode = s->data[s->rptr];
> +    s->rptr++;
> +    if (s->rptr == sizeof(s->data)) {
> +        s->rptr = 0;
>     }
> +    s->count--;

It's more ususal to use the postfix increment/decrement in the same line 
where the value is used than its own line, unless that's needed to be 
separate (that's why there are two variants: post and prefix). So you 
could do the ++ in the if or where keycode is assigned and you may be able 
to move the -- also the other if when the value of count can go one less 
(or at least move the s->count-- line closer to the if which looks clearer 
to me but that may be my taste only.

Regards,
BALATON Zoltan

> +    /*
> +     * The power key is the only two byte value key, so it is a special case.
> +     * Since 0x7f is not a used keycode for ADB we overload it to indicate the
> +     * power button when we're storing keycodes in our internal buffer, and
> +     * expand it out to two bytes when we send to the guest.
> +     */
> +    if (keycode == 0x7f) {
> +        obuf[0] = 0x7f;
> +        obuf[1] = 0x7f;
> +        olen = 2;
> +    } else {
> +        obuf[0] = keycode;
> +        /* NOTE: the power key key-up is the two byte sequence 0xff 0xff;
> +         * otherwise we could in theory send a second keycode in the second
> +         * byte, but choose not to bother.
> +         */
> +        obuf[1] = 0xff;
> +        olen = 2;
> +    }
> +
>     return olen;
> }
>
> @@ -313,6 +434,26 @@ static int adb_kbd_request(ADBDevice *d, uint8_t *obuf,
>     return olen;
> }