Re: [PATCH][V2] intel-hid: support 5 array button

["Pali Rohár" <pali.rohar@gmail.com>]

On Wednesday 08 February 2017 17:05:13 Alex Hung wrote:
> On Wed, Feb 8, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Wednesday 08 February 2017 08:21:46 Michał Kępień wrote:
> >> > Hi!
> >> >
> >> > On Saturday 04 February 2017 02:26:05 Darren Hart wrote:
> >> > > Apologies, this time with Pali's correct email address (aliases fail).
> >> > >
> >> > ...
> >> > > >
> >> > > > Pali, would you care to offer a review or some testing to verify no unexpected
> >> > > > conflicts with the other dell drivers?
> >> >
> >> > I do not have Dell machine which uses intel-hid.ko so I cannot test this
> >> > patch. And obviously as it is not loaded it cannot break machines which
> >> > do not use intel-hid.ko.
> >> >
> >> > > > > +/* 5 button array notification value. */
> >> > > > > +static const struct key_entry intel_array_keymap[] = {
> >> > > > > +     { KE_KEY,    0xC2, { KEY_LEFTMETA} },                /* Press */
> >> > > > > +     { KE_IGNORE, 0xC3, { KEY_LEFTMETA} },                /* Release */
> >> > > > > +     { KE_KEY,    0xC4, { KEY_VOLUMEUP} },                /* Press */
> >> > > > > +     { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} },                /* Release */
> >> > > > > +     { KE_KEY,    0xC6, { KEY_VOLUMEDOWN} },              /* Press */
> >> > > > > +     { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} },              /* Release */
> >> > > > > +     { KE_SW,     0xC8, { .sw = {SW_ROTATE_LOCK, 1} } },   /* Press */
> >> > > > > +     { KE_SW,     0xC9, { .sw = {SW_ROTATE_LOCK, 0} } },   /* Release */
> >> > > > > +     { KE_KEY,    0xCE, { KEY_POWER} },                   /* Press */
> >> > > > > +     { KE_IGNORE, 0xCF, { KEY_POWER} },                   /* Release */
> >> > > > > +     { KE_END },
> >> > > > > +};
> >> >
> >> > This looks suspicious. Why are all release events ignored?
> >>
> >> I also do not have a Dell machine that makes use of this driver, but my
> >> understanding is that calling sparse_keymap_report_event() with
> >> autorelease set to true makes release events irrelevant and simplifies
> >> implementation.  Though each release event still needs a KE_IGNORE entry
> >> in the keymap so that superfluous KEY_UNKNOWN events are suppressed.
> 
> Thanks Michał, this was indeed my intention.
> 
> >
> > Right, but that means if ACPI/WMI/firmware provides correct information
> > when key was pressed and when released we should use it. It allow
> > userspace e.g. measure how long was key pressed or similar thing...
> 
> Pali,
> 
> The systems I tested generate a press event followed by a release
> event immediately, so the results will be the same for them.

Ok, in this case release events can be "simulated" by kernel as your
current code is doing it. That is fine.

> However, future ACPI implementation may not be the same and I will fix
> this in a following patch. Thanks for pointing this out.

If you think that this will be really changes in ACPI/firmware for new
machines (or upcoming BIOS/firmware update), then it make sense to use
release events from firmware.

-- 
Pali Rohár
pali.rohar@gmail.com