On Wed, Feb 8, 2017 at 4:17 PM, Pali Rohár <pali.rohar@xxxxxxxxx> 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.
However, future ACPI implementation may not be the same and I will fix
this in a following patch. Thanks for pointing this out.
>
> --
> Pali Rohár
> pali.rohar@xxxxxxxxx
--
Cheers,
Alex Hung
