All of lore.kernel.org
 help / color / mirror / Atom feed
From: Darren Hart <dvhart@infradead.org>
To: Andy Lutomirski <luto@amacapital.net>
Cc: Alex Hung <alex.hung@canonical.com>, platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH] intel-hid: new hid event driver for hotkeys
Date: Thu, 17 Dec 2015 15:01:07 -0800	[thread overview]
Message-ID: <20151217230107.GB23048@malice.jf.intel.com> (raw)
In-Reply-To: <CALCETrUoEyVKxcUJU76=5yZzCuihxtFJsxu2vAUxLr38cR+e2Q@mail.gmail.com>

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@infradead.org> 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@canonical.com>
> >
> > Queued for testing.
> >
> > Andy, please provide your Reviewed-by if you're happy with it.
> 
> Reviewed-and-tested-by: Andy Lutomirski <luto@kernel.org>
> 
> 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.

> 
> >
> > 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

  reply	other threads:[~2015-12-17 23:01 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-17  7:30 [PATCH] intel-hid: new hid event driver for hotkeys Alex Hung
2015-12-17 22:15 ` Darren Hart
2015-12-17 22:50   ` Andy Lutomirski
2015-12-17 23:01     ` Darren Hart [this message]
2015-12-18 15:28       ` Alex Hung
2016-09-29 16:02 ` Dmitry Torokhov
2016-09-29 17:29   ` Andy Lutomirski
2016-09-30 22:44     ` Dmitry Torokhov
2016-09-30 23:06       ` Andy Lutomirski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20151217230107.GB23048@malice.jf.intel.com \
    --to=dvhart@infradead.org \
    --cc=alex.hung@canonical.com \
    --cc=luto@amacapital.net \
    --cc=platform-driver-x86@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.