On Fri, Dec 18, 2015 at 7:01 AM, Darren Hart <dvhart@xxxxxxxxxxxxx> wrote:
> On Thu, Dec 17, 2015 at 02:50:39PM -0800, Andy Lutomirski wrote:
>> On Thu, Dec 17, 2015 at 2:15 PM, Darren Hart <dvhart@xxxxxxxxxxxxx> wrote:
>> > On Thu, Dec 17, 2015 at 03:30:02PM +0800, Alex Hung wrote:
>> >> This driver supports various hid events including hotkeys.
>> >> Dell XPS 13 9350 requires it for wireless hotkey.
>> >>
>> >> Andy Lutomirski contributed greatly to this driver.
>> >>
>> >> Signed-off-by: Alex Hung <alex.hung@xxxxxxxxxxxxx>
>> >
>> > Queued for testing.
>> >
>> > Andy, please provide your Reviewed-by if you're happy with it.
>>
>> Reviewed-and-tested-by: Andy Lutomirski <luto@xxxxxxxxxx>
>>
>> Minor nits, though:
>>
>> + This driver provides supports for Intel HID event. Some laptops
>> + require this driver for hotkey supports.
>>
>> Should that be "This driver provides support for the Intel HID Event
>> hotkey interface. Some laptops require this driver for hotkey
>> support."?
>>
>> Also, can we remove the word "Filter" a couple lines up in the Kconfig
>> file? I'd guess the the Windows equivalent is a "filter' driver in
>> the Windows model, but it's not logically filtering anything, and
>> there's nothing filter-like about the Linux driver.
>>
>> (I think it's slightly sad that HID is in the name, too, but that's
>> indeed what it seems to be called.)
>
> These are minor, but I had similar thoughts. I'd be happy to see these included
> and respun.
That sounds good. I am removing "filter" in various places and will
resend the patch shortly.
>
>>
>> >
>> > Alex, just one question maybe you can answer quickly and save me some time
>> > below:
>> >
>> >
>> >> +static int intel_hid_pl_suspend_handler(struct device *device)
>> >> +{
>> >> + intel_hid_set_enable(device, 0);
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int intel_hid_pl_resume_handler(struct device *device)
>> >> +{
>> >> + intel_hid_set_enable(device, 1);
>> >> + return 0;
>> >> +}
>> >
>> > Why not propagate the intel_hid_set_enable() return code? Is it because it just
>> > doesn't really impact suspend/resume, even if we do get an error?
>>
>> That was me, actually. I definitely don't want to cause suspect to
>> fail if the ACPI call fails. At worst we'll report a spurious key
>> event when we resume.
>>
>
> OK, thanks for the clarification (reminder :-)
>
> --
> Darren Hart
> Intel Open Source Technology Center
--
Cheers,
Alex Hung
--
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
