Re: [Qemu-ppc] [PATCH v4 5/5] adb.c: add power key support

[David Gibson]

On Mon, Aug 15, 2016 at 03:53:02PM -0400, Programmingkid wrote:
> 
> On Aug 15, 2016, at 8:19 AM, David Gibson wrote:
> 
> > On Fri, Aug 12, 2016 at 08:10:03PM -0400, John Arbuckle wrote:
> >> Add support for the power key.
> >> 
> >> Signed-off-by: John Arbuckle <address@hidden>
> >> Reviewed-by: Peter Maydell <address@hidden>
> >> ---
> >> v4 changes
> >> Removed double-quote from end of comment.
> >> Removed debug printf statement.
> >> 
> >> v3 change
> >> Add several suggested comments.
> >> Moved the location of an else statement in the adb_keyboard_event() 
> >> function.
> >> 
> >> hw/input/adb.c | 43 +++++++++++++++++++++++++++++++++----------
> >> 1 file changed, 33 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/hw/input/adb.c b/hw/input/adb.c
> >> index 2042903..3998490 100644
> >> --- a/hw/input/adb.c
> >> +++ b/hw/input/adb.c
> >> @@ -340,10 +340,25 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
> >>         s->rptr = 0;
> >>     }
> >>     s->count--;
> >> -    obuf[0] = keycode;
> >> -    /* NOTE: could put a second keycode if needed */
> >> -    obuf[1] = 0xff;
> >> -    olen = 2;
> >> +    /*
> >> +     * 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;
> >> }
> >> @@ -421,14 +436,22 @@ static void adb_keyboard_event(DeviceState *dev, 
> >> QemuConsole *src,
> >>         return;
> >>     }
> >>     keycode = qcode_to_adb_keycode[qcode];
> >> -    if (keycode == NO_KEY) {  /* We don't want to send this to the guest 
> >> */
> >> +
> >> +    /* The power button is a special case because it is a 16-bit value */
> >> +    if (qcode == Q_KEY_CODE_POWER) {
> >> +        if (evt->u.key.data->down == true) { /* Power button pushed 
> >> keycode */
> >> +            adb_kbd_put_keycode(s, 0x7f);
> >> +        } else {                           /* Power button released 
> >> keycode */
> >> +            adb_kbd_put_keycode(s, 0xff);
> >> +        }
> >> +    } else if (keycode == NO_KEY) { /* NO_KEY shouldn't be sent to guest 
> >> */
> >>         return;
> >> +    } else {   /* For all non-power keys - safe for 8-bit keycodes */
> >> +        if (evt->u.key.data->down == false) { /* if key release event */
> >> +            keycode = keycode | 0x80;   /* create keyboard break code */
> >> +        }
> >> +        adb_kbd_put_keycode(s, keycode);
> > 
> > The logic in the CODE_POWER and else cases doesn't actually seem to be
> > different AFAICT.
> 
> The power key is a 16 bit value.

I assume you're meaning the adb code here, rather than the qkey code,
yes?

Oh.. btw.. your initial adb-keys.h commit has ADB_KEY_POWER commented
out, and I didn't see that removed in any of the intervening patches.

> It needs its own special case or the operating
> system would not see it. After disabling the "if (qcode == Q_KEY_CODE_POWER)" 
> condition
> so the power key would go thru the else statement, the guest stopped 
> detecting the
> power key.

Well.. I don't think your earlier patches actually put a mapping in
the table for Q_KEY_CODE_POWER, so I guess it would map to NO_KEY,
which would explain that.  But I don't see a reason you need special
logic here rather than just adding the mapping to the table.

The Q_KEY_CODE_POWER case here still only invokes a single
adb_kbd_put_keycode() with an 8-bit value.  So I'm not really seeing
where the 16-bit value comes into play.

> >>     }
> >> -    if (evt->u.key.data->down == false) { /* if key release event */
> >> -        keycode = keycode | 0x80;   /* create keyboard break code */
> >> -    }
> >> -
> >> -    adb_kbd_put_keycode(s, keycode);
> >> }
> >> 
> >> static const VMStateDescription vmstate_adb_kbd = {
> > 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature