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

[Programmingkid]

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;
 - with added commit message that states "just a mechanical substitution, which
   a number of broken mappings left in."
 - with qemu_input_handler_register() call moved to the realized() function
 - code from power key patch merged with this patch

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.