Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 13, 2012 at 08:06:47PM +0200, Maxim Mikityanskiy wrote:
> 2012/12/13 Anisse Astier <anisse@xxxxxxxxx>:
> > On Wed, 12 Dec 2012 10:58:56 -0800, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote :
> >
> >> On Wed, Dec 12, 2012 at 10:58:29AM +0100, Anisse Astier wrote:
> >> > On Tue, 11 Dec 2012 20:27:26 +0200, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote :
> >> >
> >> > > 2012/12/11 Anisse Astier <anisse@xxxxxxxxx>:
> >> > > > On Tue, 11 Dec 2012 19:07:51 +0200, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote :
> >> > > >
> >> > > >> >> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
> >> > > >> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
> >> > > >> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
> >> > > >> >>                               eventcode);
> >> > > >> >> -             if (key) {
> >> > > >> >> +             if (!key) {
> >> > > >> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
> >> > > >> >> +                     goto msi_wmi_notify_exit;
> >> > > >> >> +             }
> >> > > >> >> +             if (quirk_last_pressed) {
> >> > > >> >> +                     size_t key_index = key - msi_wmi_keymap;
> >> > > >> > Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
> >> > > >> > intent here otherwise.
> >> > > >>
> >> > > >> msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
> >> > > >> first item. key is a pointer to some array's item. So 'key -
> >> > > >> msi_wmi_keymap' is a difference between pointers, i.e. index of key in
> >> > > >> msi_wmi_keymap.
> >> > > >>
> >> > > >> I do pointer arithmetic here because in patch 12 I add some new
> >> > > >> scancodes, and holes appear in scancode sequence, so we can't just use
> >> > > >> 'key->code - MSI_SCANCODE_BASE' to get item index in array.
> >> > > >
> >> > > > Oh, I see. This is very clever, but a bit too clever. You have no
> >> > > > guarantee, that sparse_keymap_entry_from_scancode will give you a pointer
> >> > > > to *your* key_entry. In fact, it doesn't.
> >> > > >
> >> > > > In sparse_keymap_setup (drivers/input/sparse-keymap.c), the keymap
> >> > > > array(msi_wmi_keymap) is mempcy-ed, at line 187 (kernel ~3.7). So if you
> >> > > > want to use this method, you might need to re-compute the index by
> >> > > > iterating over the elements and comparing key->code for each.
> >> > >
> >> > > Oops, I'm sorry, I was looking only into
> >> > > sparse_keymap_entry_from_scancode() and I didn't discover the fact
> >> > > that dev->keycode is not equal to msi_wmi_keymap. I just wanted to
> >> > > avoid iterating through the array second time. Also, can I use
> >> > > msi_wmi_input_dev->keycode instead of msi_wmi_keymap as array base?
> >> > > input_dev::keycode field is documented at include/linux/input.h:62.
> >> >
> >> > As I said, you could iterate over msi_wmi_keymap and compare key->code to
> >> > each keycode to compute key_index each time.
> >> >
> >> > >
> >> > > It's strange that I didn't get a crash or data corruption on my system
> >> > > when I forced usage of last_pressed for my laptop, loaded this module
> >> > > and tried to press keys.
> >> > >
> >> > > It's very unconvenient that struct key_entry does not have some field
> >> > > for driver-specific extra data. In such field we could store index in
> >> > > last_pressed or event last press time. But we haven't such field.
> >> >
> >> > If that's really needed, such field could be added. Cc-ing Dmitry
> >> > Torokhov to ask what he thinks about altering the sparse keymap API for
> >> > this use case, ie having per key_entry variables to store information,
> >> > here used for debouncing.
> >> > Previously the keymap was contiguous, but we're adding new keys, so it'll
> >> > really be sparse, so we can't use the same trick substraction trick.
> >>
> >> A different question - do you really need to store times for all
> >> possible keys or you actually need to know just the last key that was
> >> pressed?
> >
> > Not at all. This isn't a gamepad, so I think this quirk could be
> > simplified by just storing the last trigger time overall, not per-key
> > trigger time. 1 key every 50ms means you can still input 20 keys per
> > second, which isn't bad.
> >
> > Maxim, if you make this modification it would simplify your patch even
> > further.
> 
> I agree, it would simplify things a lot. But I think it could cause
> problems when a user presses two different keys simultaneously. We
> would receive lots of events from both keys in some undefined order. A
> variable holding last key code would change its value several times so
> we would send more key press events than user really made (more than
> two). I know, this case is very unlikely to happen (nobody presses two
> special keys at a time), maybe it's even impossible due to some
> hardware restrictions (I don't know is it true).
> 
> We could ignore this rare case at all, or we could filter out all
> events within 50ms, not only the same ones. What do you think on this?

I think for the kind of keys the device provides simply ignoring events
coming within 50 msecs would work well enough.

But you have the hardware so try experimenting and see what kind of
response you can get from it and what strategy is the best.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux