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

[David Gibson]

On Tue, Aug 16, 2016 at 10:06:14AM -0400, Programmingkid wrote:
> 
> On Aug 15, 2016, at 11:55 PM, David Gibson wrote:
> 
> > 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?
> 
> 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.
> 
> I completely forgot about that.
> 
> It does look like I can use the else case in the adb_keyboard_event() for the 
> Power key.
> 
> > 
> >> 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 = {
> 
> Ok I think I have a plan for my next set of patches:
> 
> patch 1: adb-keys.h initial commit 
>  - with Power key uncommented
> 
> patch 2: add support for QKeyCode
>  - minus the Power key if-condition in adb_keyboard_event()
>  - with NO_KEY mapping added to beginning of the qcode_to_adb_keycode array
>  - with NO_KEY if-condition added to the adb_keyboard_event() function
>  - with the ADB_DPRINTF() code added to the adb_keyboard_event() (see below)
>    if (keycode == NO_KEY) { /* NO_KEY shouldn't be sent to guest */
>         ADB_DPRINTF("Ignoring NO_KEY\n");
>         return;

Actually I think the NO_KEY handling would still be clearer in a
separate patch.

>  - with added commit message that states "just a mechanical substitution, 
> which
>    a number of broken mappings left in."

This comment wouldn't be entirely accurate if you include the NO_KEY
changes, which is another reason to keep it separate.  Something like
this should definitely go in the message though - this is basically a
signal to reviewers that they should just be checking the new code has
the same effect as the old, not that it makes sense in absolute terms.

>  - with qemu_input_handler_register() call moved to the realized() function
>  - code from power key patch merged with this patch

That change should definitely be folded in though.

> 
> patch 3: correct several key assignments
>  - Will still fix the broken mappings left in
> 
> If this looks right I will make my next set of patches.

-- 
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