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 <programmingk...@gmail.com>
> >>>> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
> >>>> ---
> >>>> 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
