Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
On Mon, Apr 15, 2024 at 04:28:19PM -0400, Mark Pearson wrote: > Hi > > On Mon, Apr 15, 2024, at 3:58 PM, Dmitry Torokhov wrote: > > On Mon, Apr 15, 2024 at 09:50:37PM +0200, Hans de Goede wrote: > >> Hi, > >> > >> On 4/15/24 9:40 PM, Dmitry Torokhov wrote: > >> > On Wed, Apr 10, 2024 at 10:48:10PM -0400, Mark Pearson wrote: > >> >> > >> >> I have a stronger preference to keep the KEY_DOUBLECLICK - that one > >> >> seems less controversial as a genuine new input event. > >> > > >> > Please see my response to Peter's letter. I think it very much depends > >> > on how it will be used (associated with the pointer or standalone as it > >> > is now). > >> > > >> > For standalone application, recalling your statement that on Win you > >> > have this gesture invoke configuration utility I would argue for > >> > KEY_CONFIG for it. > >> > >> KEY_CONFIG is already generated by Fn + F# on some ThinkPads to launch > >> the GNOME/KDE control center/panel and I believe that at least GNOME > >> comes with a default binding to map KEY_CONFIG to the control-center. > > > > Not KEY_CONTROLPANEL? > > > > Are there devices that use both Fn+# and the doubleclick? Would it be an > > acceptable behavior for the users to have them behave the same? > > > Catching up with the thread, thanks for all the comments. > > For FN+N (originally KEY_DEBUG_SYS_INFO) the proposal was to now use > KEY_VENDOR there. My conclusion was that this is targeting vendor > specific functionality, and that was the closest fit, if a new keycode > was not preferred. Fn+N -> KEY_VENDOR mapping sounds good to me. > > For the doubletap (which is a unique input event - not related to the > pointer) I would like to keep it as a new unique event. > > I think it's most likely use would be for control panel, but I don't > think it should be limited to that. I can see it being useful if users > are able to reconfigure it to launch something different (browser or > music player maybe?), hence it would be best if it did not conflict > with an existing keycode function. I also can't confirm it doesn't > clash on existing or future systems - it's possible. So here is the problem. Keycodes in linux input are not mere placeholders for something that will be decided later how it is to be used, they are supposed to communicate intent and userspace ideally does not need to have any additional knowledge about where the event is coming from. A keyboard either internal or external sends KEY_SCREENLOCK and the system should lock the screen. It should not be aware that one device was a generic USB external keyboard while another had an internal sensor that recognized hovering palm making swiping motion to the right because a vendor decided to make it. Otherwise you have millions of input devices all generating unique codes and you need userspace to decide how to interpret data coming from each device individually. If you truly do not have a defined use case for it you have a couple options: - assign it KEY_RESERVED, ensure your driver allows remapping to an arbitrary keycode, let user or distro assign desired keycode to it - assign KEY_PROG1 .. KEY_PROG4 - pretty much the same - leave it in the hand of the user to define a shortcut in their DE to make it useful > > FWIW - I wouldn't be surprised with some of the new gaming systems > we're seeing (Steamdeck, Legion-Go, etc), that a doubletap event on a > joystick might be useful to have, if the HW supports it? What would it do exactly? Once we have this answer we can define key or button code (although I do agree that game controller buttons are different from "normal" keys because they map to the geometry of the controller which in turn defines their commonly understood function). But in any case you would not reuse the same keycode for something that is supposed to invoke a configuration utility and also to let's say drop a flash grenade in a game. Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
On Mon, Apr 15, 2024 at 09:50:37PM +0200, Hans de Goede wrote: > Hi, > > On 4/15/24 9:40 PM, Dmitry Torokhov wrote: > > On Wed, Apr 10, 2024 at 10:48:10PM -0400, Mark Pearson wrote: > >> > >> I have a stronger preference to keep the KEY_DOUBLECLICK - that one seems > >> less controversial as a genuine new input event. > > > > Please see my response to Peter's letter. I think it very much depends > > on how it will be used (associated with the pointer or standalone as it > > is now). > > > > For standalone application, recalling your statement that on Win you > > have this gesture invoke configuration utility I would argue for > > KEY_CONFIG for it. > > KEY_CONFIG is already generated by Fn + F# on some ThinkPads to launch > the GNOME/KDE control center/panel and I believe that at least GNOME > comes with a default binding to map KEY_CONFIG to the control-center. Not KEY_CONTROLPANEL? Are there devices that use both Fn+# and the doubleclick? Would it be an acceptable behavior for the users to have them behave the same? Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
On Mon, Apr 15, 2024 at 09:47:10PM +0200, Hans de Goede wrote: > Hi, > > On 4/15/24 9:35 PM, Dmitry Torokhov wrote: > > On Thu, Apr 11, 2024 at 02:30:35PM +0200, Hans de Goede wrote: > >> Hi Dmitry, > >> > >> On 4/11/24 2:02 AM, Dmitry Torokhov wrote: > >>> On Tue, Apr 09, 2024 at 10:17:05PM -0400, Mark Pearson wrote: > >>>> Hi Dmitry > >>>> > >>>> On Tue, Apr 9, 2024, at 9:20 PM, Dmitry Torokhov wrote: > >>>>> On Tue, Apr 09, 2024 at 02:47:05PM -0700, Dmitry Torokhov wrote: > >>>>>> On Tue, Apr 09, 2024 at 03:23:52PM +1000, Peter Hutterer wrote: > >>>>>>> On 09/04/2024 09:31, Dmitry Torokhov wrote: > >>>>>>>> Hi Mark, > >>>>>>>> > >>>>>>>> On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote: > >>>>>>>>> Add support for new input events on Lenovo laptops that need > >>>>>>>>> exporting to > >>>>>>>>> user space. > >>>>>>>>> > >>>>>>>>> Lenovo trackpoints are adding the ability to generate a doubletap > >>>>>>>>> event. > >>>>>>>>> Add a new keycode to allow this to be used by userspace. > >>>>>>>> > >>>>>>>> What is the intended meaning of this keycode? How does it differ from > >>>>>>>> the driver sending BTN_LEFT press/release twice? > >>>>>>>>> > >>>>>>>>> Lenovo support is using FN+N with Windows to collect needed details > >>>>>>>>> for > >>>>>>>>> support cases. Add a keycode so that we'll be able to provide > >>>>>>>>> similar > >>>>>>>>> support on Linux. > >>>>>>>> > >>>>>>>> Is there a userspace consumer for this? > >>>>>>> > >>>>>>> Funnily enough XKB has had a keysym for this for decades but it's not > >>>>>>> hooked up anywhere due to the way it's pointer keys accessibility > >>>>>>> feature was implemented. Theory is that most of userspace just needs > >>>>>>> to patch the various pieces together for the new evdev code + keysym, > >>>>>>> it's not really any different to handling a volume key (except this > >>>>>>> one needs to be assignable). > >>>>>> > >>>>>> What is the keysym? If we can make them relatable to each other that > >>>>>> would be good. Or maybe we could find a matching usage from HID usage > >>>>>> tables... > >>>>> > >>>>> I was looking through the existing codes and I see: > >>>>> > >>>>> #define KEY_INFO0x166 /* AL OEM > >>>>> Features/Tips/Tutorial */ > >>>>> > >>>>> We also have KEY_VENDOR used in a few drivers/plafrom/x86, including > >>>>> thinkkpad_acpi.c and I wonder if it would be suitable for this vendor > >>>>> specific debug info collection application (which I honestly doubt will > >>>>> materialize). > >>>>> > >>>> > >>>> That's a somewhat disappointing note on your doubts, is that based on > >>>> anything? Just wondering what we've done to deserve that criticism. > >>> > >>> Sorry, this was not meant as a criticism really, but you mentioned > >>> yourself that there isn't anything in the works yet, you just have some > >>> plans. > >>> > >>> For such a project to succeed Lenovo needs to invest into selling > >>> devices with Linux as a primary operating system, and it has to be > >>> consumer segment (or small business, because for corporate they > >>> typically roll their own support channels). The case of retrofitting > >>> Linux onto a that device originally came with Windows OS rarely gets > >>> much if any response from the normal support channels. > >>> > >>> Is this something that is actually happening? > >> > >> Yes, Lenovo is actually offering Fedora as an OS choice when > >> ordering ThinkPads directly from their website in many countries > >> including when ordering as a consum
Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
On Wed, Apr 10, 2024 at 10:48:10PM -0400, Mark Pearson wrote: > > I have a stronger preference to keep the KEY_DOUBLECLICK - that one seems > less controversial as a genuine new input event. Please see my response to Peter's letter. I think it very much depends on how it will be used (associated with the pointer or standalone as it is now). For standalone application, recalling your statement that on Win you have this gesture invoke configuration utility I would argue for KEY_CONFIG for it. Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
On Thu, Apr 11, 2024 at 02:30:35PM +0200, Hans de Goede wrote: > Hi Dmitry, > > On 4/11/24 2:02 AM, Dmitry Torokhov wrote: > > On Tue, Apr 09, 2024 at 10:17:05PM -0400, Mark Pearson wrote: > >> Hi Dmitry > >> > >> On Tue, Apr 9, 2024, at 9:20 PM, Dmitry Torokhov wrote: > >>> On Tue, Apr 09, 2024 at 02:47:05PM -0700, Dmitry Torokhov wrote: > >>>> On Tue, Apr 09, 2024 at 03:23:52PM +1000, Peter Hutterer wrote: > >>>>> On 09/04/2024 09:31, Dmitry Torokhov wrote: > >>>>>> Hi Mark, > >>>>>> > >>>>>> On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote: > >>>>>>> Add support for new input events on Lenovo laptops that need > >>>>>>> exporting to > >>>>>>> user space. > >>>>>>> > >>>>>>> Lenovo trackpoints are adding the ability to generate a doubletap > >>>>>>> event. > >>>>>>> Add a new keycode to allow this to be used by userspace. > >>>>>> > >>>>>> What is the intended meaning of this keycode? How does it differ from > >>>>>> the driver sending BTN_LEFT press/release twice? > >>>>>>> > >>>>>>> Lenovo support is using FN+N with Windows to collect needed details > >>>>>>> for > >>>>>>> support cases. Add a keycode so that we'll be able to provide similar > >>>>>>> support on Linux. > >>>>>> > >>>>>> Is there a userspace consumer for this? > >>>>> > >>>>> Funnily enough XKB has had a keysym for this for decades but it's not > >>>>> hooked up anywhere due to the way it's pointer keys accessibility > >>>>> feature was implemented. Theory is that most of userspace just needs > >>>>> to patch the various pieces together for the new evdev code + keysym, > >>>>> it's not really any different to handling a volume key (except this > >>>>> one needs to be assignable). > >>>> > >>>> What is the keysym? If we can make them relatable to each other that > >>>> would be good. Or maybe we could find a matching usage from HID usage > >>>> tables... > >>> > >>> I was looking through the existing codes and I see: > >>> > >>> #define KEY_INFO 0x166 /* AL OEM Features/Tips/Tutorial */ > >>> > >>> We also have KEY_VENDOR used in a few drivers/plafrom/x86, including > >>> thinkkpad_acpi.c and I wonder if it would be suitable for this vendor > >>> specific debug info collection application (which I honestly doubt will > >>> materialize). > >>> > >> > >> That's a somewhat disappointing note on your doubts, is that based on > >> anything? Just wondering what we've done to deserve that criticism. > > > > Sorry, this was not meant as a criticism really, but you mentioned > > yourself that there isn't anything in the works yet, you just have some > > plans. > > > > For such a project to succeed Lenovo needs to invest into selling > > devices with Linux as a primary operating system, and it has to be > > consumer segment (or small business, because for corporate they > > typically roll their own support channels). The case of retrofitting > > Linux onto a that device originally came with Windows OS rarely gets > > much if any response from the normal support channels. > > > > Is this something that is actually happening? > > Yes, Lenovo is actually offering Fedora as an OS choice when > ordering ThinkPads directly from their website in many countries > including when ordering as a consumer. Ah, very nice, I was not aware of this. > > And unlike other vendor's Linux preloads which often use a kernel > with downstream laptop specific changes these laptops are running > unmodified Fedora kernels, which themselves are almost pristine > upstream kernels. > > Lenovo (Mark) has been really good the last couple of years in > making sure that their hw just works with mainline kernels without > any downstream vendor specific patches. > > >> That aside, I guess KEY_INFO or KEY_VENDOR could be a good fit (I > >> personally don't think KEY_CONFIG matches well), but I would be > >> worried about clashing with existing functionality. > > Using KEY_INFO / KEY_VENDOR works for me too. So maybe we should > just go with one of those 2 ? It looks like Mark's preference is KEY_VENDOR, so let's go with it? Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
On Wed, Apr 10, 2024 at 02:32:56PM +1000, Peter Hutterer wrote: > On 10/04/2024 11:20, Dmitry Torokhov wrote: > > On Tue, Apr 09, 2024 at 02:47:05PM -0700, Dmitry Torokhov wrote: > > > On Tue, Apr 09, 2024 at 03:23:52PM +1000, Peter Hutterer wrote: > > > > On 09/04/2024 09:31, Dmitry Torokhov wrote: > > > > > Hi Mark, > > > > > > > > > > On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote: > > > > > > Add support for new input events on Lenovo laptops that need > > > > > > exporting to > > > > > > user space. > > > > > > > > > > > > Lenovo trackpoints are adding the ability to generate a doubletap > > > > > > event. > > > > > > Add a new keycode to allow this to be used by userspace. > > > > > > > > > > What is the intended meaning of this keycode? How does it differ from > > > > > the driver sending BTN_LEFT press/release twice? > > > > > > > > > > > > Lenovo support is using FN+N with Windows to collect needed details > > > > > > for > > > > > > support cases. Add a keycode so that we'll be able to provide > > > > > > similar > > > > > > support on Linux. > > > > > > > > > > Is there a userspace consumer for this? > > > > > > > > Funnily enough XKB has had a keysym for this for decades but it's not > > > > hooked up anywhere due to the way it's pointer keys accessibility > > > > feature was implemented. Theory is that most of userspace just needs > > > > to patch the various pieces together for the new evdev code + keysym, > > > > it's not really any different to handling a volume key (except this > > > > one needs to be assignable). > > > > > > What is the keysym? If we can make them relatable to each other that > > > would be good. Or maybe we could find a matching usage from HID usage > > > tables... > > There's a set of XK_Pointer_ keysyms defined in X11/keysym.h, > including XK_Pointer_DblClick1 and XK_Pointer_DblClickDefault. > Unfortunately they're not hooked up to anything atm, see this draft > MR: > https://gitlab.freedesktop.org/xkeyboard-config/xkeyboard-config/-/merge_requests/659 > Because they're not hooked up anywhere they'll also need to be hooked > into the client space, same as the various XF86FooBar symbols we've > added over the years. > > If we were to add KEY_DOUBLECLICK we can patch xkeyboard-config to > bind that to the existing XK_Pointer_DblClickDefault symbol (it would > get XF86DoubleClick assigned by the current automated scripts), so in > theory that key would work like any other key with that symbol > assigned. > > > I was looking through the existing codes and I see: > > > > #define KEY_INFO0x166 /* AL OEM Features/Tips/Tutorial */ > > > > We also have KEY_VENDOR used in a few drivers/plafrom/x86, including > > thinkkpad_acpi.c and I wonder if it would be suitable for this vendor > > specific debug info collection application (which I honestly doubt will > > materialize). > > fwiw, I suggested KEY_DOUBLECLICK because that is the action the user > takes. Whether that starts a particular application is mostly a > question of configuration, defaulting to something that emulates a > double-click seems prudent though. And if someone wants to remap that > to the compose key that'd be trivial too then. I think whether to create and use KEY_DOUBLECLICK is very much depends if we associate this with the pointer somehow, or if we keep it as a completely separate action. If we continue with KEY_DOUBLECLICK then we need to try and define what exactly it means to the applications. Actually same goes if we want another new keycode. As far as easy remapping, I think one can map this to KEY_RESERVED and then remap to whatever they want, you do not need to have a new keycode for that. Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
On Tue, Apr 09, 2024 at 10:17:05PM -0400, Mark Pearson wrote: > Hi Dmitry > > On Tue, Apr 9, 2024, at 9:20 PM, Dmitry Torokhov wrote: > > On Tue, Apr 09, 2024 at 02:47:05PM -0700, Dmitry Torokhov wrote: > >> On Tue, Apr 09, 2024 at 03:23:52PM +1000, Peter Hutterer wrote: > >> > On 09/04/2024 09:31, Dmitry Torokhov wrote: > >> > > Hi Mark, > >> > > > >> > > On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote: > >> > > > Add support for new input events on Lenovo laptops that need > >> > > > exporting to > >> > > > user space. > >> > > > > >> > > > Lenovo trackpoints are adding the ability to generate a doubletap > >> > > > event. > >> > > > Add a new keycode to allow this to be used by userspace. > >> > > > >> > > What is the intended meaning of this keycode? How does it differ from > >> > > the driver sending BTN_LEFT press/release twice? > >> > > > > >> > > > Lenovo support is using FN+N with Windows to collect needed details > >> > > > for > >> > > > support cases. Add a keycode so that we'll be able to provide similar > >> > > > support on Linux. > >> > > > >> > > Is there a userspace consumer for this? > >> > > >> > Funnily enough XKB has had a keysym for this for decades but it's not > >> > hooked up anywhere due to the way it's pointer keys accessibility > >> > feature was implemented. Theory is that most of userspace just needs > >> > to patch the various pieces together for the new evdev code + keysym, > >> > it's not really any different to handling a volume key (except this > >> > one needs to be assignable). > >> > >> What is the keysym? If we can make them relatable to each other that > >> would be good. Or maybe we could find a matching usage from HID usage > >> tables... > > > > I was looking through the existing codes and I see: > > > > #define KEY_INFO0x166 /* AL OEM Features/Tips/Tutorial */ > > > > We also have KEY_VENDOR used in a few drivers/plafrom/x86, including > > thinkkpad_acpi.c and I wonder if it would be suitable for this vendor > > specific debug info collection application (which I honestly doubt will > > materialize). > > > > That's a somewhat disappointing note on your doubts, is that based on > anything? Just wondering what we've done to deserve that criticism. Sorry, this was not meant as a criticism really, but you mentioned yourself that there isn't anything in the works yet, you just have some plans. For such a project to succeed Lenovo needs to invest into selling devices with Linux as a primary operating system, and it has to be consumer segment (or small business, because for corporate they typically roll their own support channels). The case of retrofitting Linux onto a that device originally came with Windows OS rarely gets much if any response from the normal support channels. Is this something that is actually happening? > > That aside, I guess KEY_INFO or KEY_VENDOR could be a good fit (I > personally don't think KEY_CONFIG matches well), but I would be > worried about clashing with existing functionality. > > Peter - do you have any opinion from the user space side of things, or > are these likely unused? KEY_VENDOR seems the safer bet to me (but I > don't love it). > > Dmitry - What are the downsides or concerns of introducing a new code? > I'd like to evaluate that against the potential to cause conflicts by > re-using existing codes. If you feel strongly about it then I'll defer > to your judgement, but I'd like to understand better the context. The keycode space is finite and extending bitmaps leads to more memory consumption and weird breakages (like uevent generation exceeding 4K memory page resulting in failures). I am trying to balance need for new keycodes with how likely they are to be used. Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
On Tue, Apr 09, 2024 at 02:47:05PM -0700, Dmitry Torokhov wrote: > On Tue, Apr 09, 2024 at 03:23:52PM +1000, Peter Hutterer wrote: > > On 09/04/2024 09:31, Dmitry Torokhov wrote: > > > Hi Mark, > > > > > > On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote: > > > > Add support for new input events on Lenovo laptops that need exporting > > > > to > > > > user space. > > > > > > > > Lenovo trackpoints are adding the ability to generate a doubletap event. > > > > Add a new keycode to allow this to be used by userspace. > > > > > > What is the intended meaning of this keycode? How does it differ from > > > the driver sending BTN_LEFT press/release twice? > > > > > > > > Lenovo support is using FN+N with Windows to collect needed details for > > > > support cases. Add a keycode so that we'll be able to provide similar > > > > support on Linux. > > > > > > Is there a userspace consumer for this? > > > > Funnily enough XKB has had a keysym for this for decades but it's not > > hooked up anywhere due to the way it's pointer keys accessibility > > feature was implemented. Theory is that most of userspace just needs > > to patch the various pieces together for the new evdev code + keysym, > > it's not really any different to handling a volume key (except this > > one needs to be assignable). > > What is the keysym? If we can make them relatable to each other that > would be good. Or maybe we could find a matching usage from HID usage > tables... I was looking through the existing codes and I see: #define KEY_INFO0x166 /* AL OEM Features/Tips/Tutorial */ We also have KEY_VENDOR used in a few drivers/plafrom/x86, including thinkkpad_acpi.c and I wonder if it would be suitable for this vendor specific debug info collection application (which I honestly doubt will materialize). Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
On Tue, Apr 09, 2024 at 12:16:04PM +0200, Hans de Goede wrote: > Hi Dmitry, > > On 4/9/24 2:00 AM, Mark Pearson wrote: > > Hi Dmitry > > > > On Mon, Apr 8, 2024, at 7:31 PM, Dmitry Torokhov wrote: > >> Hi Mark, > >> > >> On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote: > >>> Add support for new input events on Lenovo laptops that need exporting to > >>> user space. > >>> > >>> Lenovo trackpoints are adding the ability to generate a doubletap event. > >>> Add a new keycode to allow this to be used by userspace. > >> > >> What is the intended meaning of this keycode? How does it differ from > >> the driver sending BTN_LEFT press/release twice? > > > > Double tapping on the trackpoint is a unique event - it's not the same as > > BTN_LEFT twice. The BIOS will send a new ACPI event for it and it's not > > meant to be the same as mouse button clicks. > > To extend a bit on this, this double-tap event is not reported through > the PS/2 trackpoint interface at all. Instead it is reported to > the OS by the ACPI hotkey notifier, which is used to report various > multi-media hotkeys and things like that, this is handled by > the thinkpad_apci driver which sofar only reports key-presses. Ah, I see, so this is just an arbitrary action not connected with the pointer handling in any way. For such actions we typically assign keycodes based on their intended behavior, so instead of KEY_DOUBLECLICK which conveys user gesture but not the intent you should consider using KEY_CONFIG (with is typically mapped to Application Launcher - Consumer Control Configuration in HID spec) or KEY_CONTROLPANEL (Application Launcher - Control Panel). Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
On Tue, Apr 09, 2024 at 03:23:52PM +1000, Peter Hutterer wrote: > On 09/04/2024 09:31, Dmitry Torokhov wrote: > > Hi Mark, > > > > On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote: > > > Add support for new input events on Lenovo laptops that need exporting to > > > user space. > > > > > > Lenovo trackpoints are adding the ability to generate a doubletap event. > > > Add a new keycode to allow this to be used by userspace. > > > > What is the intended meaning of this keycode? How does it differ from > > the driver sending BTN_LEFT press/release twice? > > > > > > Lenovo support is using FN+N with Windows to collect needed details for > > > support cases. Add a keycode so that we'll be able to provide similar > > > support on Linux. > > > > Is there a userspace consumer for this? > > Funnily enough XKB has had a keysym for this for decades but it's not > hooked up anywhere due to the way it's pointer keys accessibility > feature was implemented. Theory is that most of userspace just needs > to patch the various pieces together for the new evdev code + keysym, > it's not really any different to handling a volume key (except this > one needs to be assignable). What is the keysym? If we can make them relatable to each other that would be good. Or maybe we could find a matching usage from HID usage tables... Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
Hi Mark, On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote: > Add support for new input events on Lenovo laptops that need exporting to > user space. > > Lenovo trackpoints are adding the ability to generate a doubletap event. > Add a new keycode to allow this to be used by userspace. What is the intended meaning of this keycode? How does it differ from the driver sending BTN_LEFT press/release twice? > > Lenovo support is using FN+N with Windows to collect needed details for > support cases. Add a keycode so that we'll be able to provide similar > support on Linux. Is there a userspace consumer for this? Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 2/3] platform/chrome: Add driver for ChromeOS privacy-screen
Hi Rajat,
On Tue, Dec 21, 2021 at 04:11:26PM -0800, Rajat Jain wrote:
> +static int chromeos_privacy_screen_remove(struct acpi_device *adev)
> +{
> + struct drm_privacy_screen *drm_privacy_screen = acpi_driver_data(adev);
Please add an empty line here:
WARNING: Missing a blank line after declarations
#292: FILE: drivers/platform/chrome/chromeos_privacy_screen.c:129:
+ struct drm_privacy_screen *drm_privacy_screen = acpi_driver_data(adev);
+ drm_privacy_screen_unregister(drm_privacy_screen);
> + drm_privacy_screen_unregister(drm_privacy_screen);
> + return 0;
> +}
> +
> +static const struct acpi_device_id chromeos_privacy_screen_device_ids[] = {
> + {"GOOG0010", 0}, /* Google's electronic privacy screen for eDP-1 */
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, chromeos_privacy_screen_device_ids);
> +
> +static struct acpi_driver chromeos_privacy_screen_driver = {
> + .name = "chromeos_privacy_screen_drvr",
Could I buy 2 move vowels? ;)
> + .class = "ChromeOS",
> + .ids = chromeos_privacy_screen_device_ids,
> + .ops = {
> + .add = chromeos_privacy_screen_add,
> + .remove = chromeos_privacy_screen_remove,
> + },
> +};
> +
> +module_acpi_driver(chromeos_privacy_screen_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("ChromeOS ACPI Privacy Screen driver");
> +MODULE_AUTHOR("Rajat Jain ");
Otherwise
Reviewed-by: Dmitry Torokhov
Thanks.
--
Dmitry
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
[ibm-acpi-devel] [PATCH] Input: cyapa - do not call input_device_enabled from power mode handler
Input device's user counter is supposed to be accessed only while holding
input->mutex. Commit d69f0a43c677 ("Input: use input_device_enabled()")
recently switched cyapa to using the dedicated API and it uncovered the
fact that cyapa driver violated this constraint.
This patch removes checks whether the input device is open when clearing
device queues when changing device's power mode as there is no harm in
sending input events through closed input device - the events will simply
be dropped by the input core.
Note that there are more places in cyapa driver that call
input_device_enabled() without holding input->mutex, those are left
unfixed for now.
Reported-by: Marek Szyprowski
Signed-off-by: Dmitry Torokhov
---
Marek, could you please try this one?
drivers/input/mouse/cyapa_gen3.c |5 +
drivers/input/mouse/cyapa_gen5.c |3 +--
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/mouse/cyapa_gen3.c b/drivers/input/mouse/cyapa_gen3.c
index a97f4acb6452..4a9022faf945 100644
--- a/drivers/input/mouse/cyapa_gen3.c
+++ b/drivers/input/mouse/cyapa_gen3.c
@@ -907,7 +907,6 @@ static u16 cyapa_get_wait_time_for_pwr_cmd(u8 pwr_mode)
static int cyapa_gen3_set_power_mode(struct cyapa *cyapa, u8 power_mode,
u16 always_unused, enum cyapa_pm_stage pm_stage)
{
- struct input_dev *input = cyapa->input;
u8 power;
int tries;
int sleep_time;
@@ -953,7 +952,6 @@ static int cyapa_gen3_set_power_mode(struct cyapa *cyapa,
u8 power_mode,
* depending on the command's content.
*/
if (cyapa->operational &&
- input && input_device_enabled(input) &&
(pm_stage == CYAPA_PM_RUNTIME_SUSPEND ||
pm_stage == CYAPA_PM_RUNTIME_RESUME)) {
/* Try to polling in 120Hz, read may fail, just ignore it. */
@@ -1223,8 +1221,7 @@ static int cyapa_gen3_try_poll_handler(struct cyapa
*cyapa)
(data.finger_btn & OP_DATA_VALID) != OP_DATA_VALID)
return -EINVAL;
- return cyapa_gen3_event_process(cyapa, );
-
+ return cyapa->input ? cyapa_gen3_event_process(cyapa, ) : 0;
}
static int cyapa_gen3_initialize(struct cyapa *cyapa) { return 0; }
diff --git a/drivers/input/mouse/cyapa_gen5.c b/drivers/input/mouse/cyapa_gen5.c
index abf42f77b4c5..afc5aa4dcf47 100644
--- a/drivers/input/mouse/cyapa_gen5.c
+++ b/drivers/input/mouse/cyapa_gen5.c
@@ -518,8 +518,7 @@ int cyapa_empty_pip_output_data(struct cyapa *cyapa,
*len = length;
/* Response found, success. */
return 0;
- } else if (cyapa->operational &&
- input && input_device_enabled(input) &&
+ } else if (cyapa->operational && input &&
(pm_stage == CYAPA_PM_RUNTIME_RESUME ||
pm_stage == CYAPA_PM_RUNTIME_SUSPEND)) {
/* Parse the data and report it if it's valid. */
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 2/7] Input: use input_device_enabled()
On Tue, Dec 08, 2020 at 11:05:42AM +0100, Marek Szyprowski wrote:
> Hi Andrzej,
>
> On 07.12.2020 16:50, Andrzej Pietrasiewicz wrote:
> > Hi Marek,
> >
> > W dniu 07.12.2020 o 14:32, Marek Szyprowski pisze:
> >> Hi Andrzej,
> >>
> >> On 08.06.2020 13:22, Andrzej Pietrasiewicz wrote:
> >>> Use the newly added helper in relevant input drivers.
> >>>
> >>> Signed-off-by: Andrzej Pietrasiewicz
> >>
> >> This patch landed recently in linux-next as commit d69f0a43c677 ("Input:
> >> use input_device_enabled()"). Sadly it causes following warning during
> >> system suspend/resume cycle on ARM 32bit Samsung Exynos5250-based Snow
> >> Chromebook with kernel compiled from exynos_defconfig:
> >>
> >> [ cut here ]
> >> WARNING: CPU: 0 PID: 1777 at drivers/input/input.c:2230
> >> input_device_enabled+0x68/0x6c
> >> Modules linked in: cmac bnep mwifiex_sdio mwifiex sha256_generic
> >> libsha256 sha256_arm cfg80211 btmrvl_sdio btmrvl bluetooth s5p_mfc
> >> exynos_gsc v4l2_mem2mem videob
> >> CPU: 0 PID: 1777 Comm: rtcwake Not tainted
> >> 5.10.0-rc6-next-20201207-1-g49a0dc04c46d-dirty #9902
> >> Hardware name: Samsung Exynos (Flattened Device Tree)
> >> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> >> [] (show_stack) from [] (dump_stack+0xb4/0xd4)
> >> [] (dump_stack) from [] (__warn+0xd8/0x11c)
> >> [] (__warn) from [] (warn_slowpath_fmt+0xb0/0xb8)
> >> [] (warn_slowpath_fmt) from []
> >> (input_device_enabled+0x68/0x6c)
> >> [] (input_device_enabled) from []
> >
> > Apparently you are hitting this line of code in drivers/input/input.c:
> >
> > lockdep_assert_held(>mutex);
> >
> > Inspecting input device's "users" member should happen under dev's lock.
> >
> This check and warning has been introduced by this patch. I assume that
> the suspend/resume paths are correct, but it looks that they were not
> tested with this patch thus it has not been noticed that they are not
> called under the input's lock. This needs a fix. Dmitry: how would you
> like to handle this issue?
The check is proper and the warning is legit, cyapa should not be
checking this field without holding the lock. I think we can simply
remove this check from the power ops for gen3 and gen5, and this should
shut up the warning on suspend, but there other places in cyapa that do
check 'users', and they also need to be fixed.
Thanks.
--
Dmitry
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 7/7] Input: Add "inhibited" property
On Tue, Oct 06, 2020 at 06:12:49PM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 06, 2020 at 06:11:02PM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 06, 2020 at 03:04:28PM +0200, Andrzej Pietrasiewicz wrote:
> > > Hi Dmitry,
> > >
> > > W dniu 05.10.2020 o 20:10, Dmitry Torokhov pisze:
> > > > Hi Andrzej,
> > > >
> > > > On Mon, Jun 08, 2020 at 01:22:11PM +0200, Andrzej Pietrasiewicz wrote:
> > > > > @@ -284,8 +284,11 @@ static int input_get_disposition(struct
> > > > > input_dev *dev,
> > > > > case EV_KEY:
> > > > > if (is_event_supported(code, dev->keybit, KEY_MAX)) {
> > > > > - /* auto-repeat bypasses state updates */
> > > > > - if (value == 2) {
> > > > > + /*
> > > > > + * auto-repeat bypasses state updates but repeat
> > > > > + * events are ignored if the key is not pressed
> > > > > + */
> > > > > + if (value == 2 && test_bit(code, dev->key)) {
> > > > > disposition = INPUT_PASS_TO_HANDLERS;
> > > > > break;
> > > > > }
> > > >
> > > > Is this chunk really part of inhibit support? I'd think we cancel
> > > > autorepeat timer when we are releasing a key, no?
> > > >
> > >
> > > When I look at it now it seems to me the chunk might be redundant.
> > > But let me explain what I had in mind when adding it.
> > >
> > > It is a matter of what we do with input events generated while a
> > > device is inhibited. If ->open()/->close() are not provided by the
> > > driver then inhibiting amounts to merely ignoring input events from
> > > a device while it remains active. What else can you do if the driver
> > > does not provide a method to prepare the device for generating events/
> > > to stop generating events?
> > >
> > > In this special case a user might trigger a repeated event while the
> > > device is inhibited, then the user keeps holding the key down and the
> > > device is uninhibited. Do we pass anything to handlers then?
> > >
> > > In my opinion we should not. Such an event is "illegal" in a sense that it
> > > was generated at a time when nobody wanted any events from the device.
> > > Hence the test to let only those auto-repeat events through for which
> > > a key is actually pressed.
> > >
> > > However, what I see now is that if a device is inhibited, no key
> > > will ever reach neither the "1" nor "2" state because of the "if"
> > > in the very beginning of input_handle_event().
> >
> > OK, then let's drop it for now. We can revisit if we see that a problem.
>
> And by that I mean that I will drop it myself, no need to resend. I will
> be applying this shortly.
Well, "shortly" was just a tad optimistic, but I did apply it ;)
Thanks.
--
Dmitry
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v2] Input: document inhibiting
On Wed, Jun 17, 2020 at 12:18:22PM +0200, Andrzej Pietrasiewicz wrote: > Document inhibiting input devices and its relation to being > a wakeup source. > > Signed-off-by: Andrzej Pietrasiewicz Applied, thank you. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 1/7] Input: add input_device_enabled()
On Mon, Jun 08, 2020 at 01:22:05PM +0200, Andrzej Pietrasiewicz wrote: > A helper function for drivers to decide if the device is used or not. > A lockdep check is introduced as inspecting ->users should be done under > input device's mutex. > > Signed-off-by: Andrzej Pietrasiewicz Applied, thank you. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 2/7] Input: use input_device_enabled()
On Mon, Jun 08, 2020 at 01:22:06PM +0200, Andrzej Pietrasiewicz wrote: > Use the newly added helper in relevant input drivers. > > Signed-off-by: Andrzej Pietrasiewicz Applied, thank you. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 7/7] Input: Add "inhibited" property
On Tue, Oct 06, 2020 at 06:11:02PM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 06, 2020 at 03:04:28PM +0200, Andrzej Pietrasiewicz wrote:
> > Hi Dmitry,
> >
> > W dniu 05.10.2020 o 20:10, Dmitry Torokhov pisze:
> > > Hi Andrzej,
> > >
> > > On Mon, Jun 08, 2020 at 01:22:11PM +0200, Andrzej Pietrasiewicz wrote:
> > > > @@ -284,8 +284,11 @@ static int input_get_disposition(struct input_dev
> > > > *dev,
> > > > case EV_KEY:
> > > > if (is_event_supported(code, dev->keybit, KEY_MAX)) {
> > > > - /* auto-repeat bypasses state updates */
> > > > - if (value == 2) {
> > > > + /*
> > > > +* auto-repeat bypasses state updates but repeat
> > > > +* events are ignored if the key is not pressed
> > > > +*/
> > > > + if (value == 2 && test_bit(code, dev->key)) {
> > > > disposition = INPUT_PASS_TO_HANDLERS;
> > > > break;
> > > > }
> > >
> > > Is this chunk really part of inhibit support? I'd think we cancel
> > > autorepeat timer when we are releasing a key, no?
> > >
> >
> > When I look at it now it seems to me the chunk might be redundant.
> > But let me explain what I had in mind when adding it.
> >
> > It is a matter of what we do with input events generated while a
> > device is inhibited. If ->open()/->close() are not provided by the
> > driver then inhibiting amounts to merely ignoring input events from
> > a device while it remains active. What else can you do if the driver
> > does not provide a method to prepare the device for generating events/
> > to stop generating events?
> >
> > In this special case a user might trigger a repeated event while the
> > device is inhibited, then the user keeps holding the key down and the
> > device is uninhibited. Do we pass anything to handlers then?
> >
> > In my opinion we should not. Such an event is "illegal" in a sense that it
> > was generated at a time when nobody wanted any events from the device.
> > Hence the test to let only those auto-repeat events through for which
> > a key is actually pressed.
> >
> > However, what I see now is that if a device is inhibited, no key
> > will ever reach neither the "1" nor "2" state because of the "if"
> > in the very beginning of input_handle_event().
>
> OK, then let's drop it for now. We can revisit if we see that a problem.
And by that I mean that I will drop it myself, no need to resend. I will
be applying this shortly.
Thanks.
--
Dmitry
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 7/7] Input: Add "inhibited" property
On Tue, Oct 06, 2020 at 03:04:28PM +0200, Andrzej Pietrasiewicz wrote:
> Hi Dmitry,
>
> W dniu 05.10.2020 o 20:10, Dmitry Torokhov pisze:
> > Hi Andrzej,
> >
> > On Mon, Jun 08, 2020 at 01:22:11PM +0200, Andrzej Pietrasiewicz wrote:
> > > @@ -284,8 +284,11 @@ static int input_get_disposition(struct input_dev
> > > *dev,
> > > case EV_KEY:
> > > if (is_event_supported(code, dev->keybit, KEY_MAX)) {
> > > - /* auto-repeat bypasses state updates */
> > > - if (value == 2) {
> > > + /*
> > > + * auto-repeat bypasses state updates but repeat
> > > + * events are ignored if the key is not pressed
> > > + */
> > > + if (value == 2 && test_bit(code, dev->key)) {
> > > disposition = INPUT_PASS_TO_HANDLERS;
> > > break;
> > > }
> >
> > Is this chunk really part of inhibit support? I'd think we cancel
> > autorepeat timer when we are releasing a key, no?
> >
>
> When I look at it now it seems to me the chunk might be redundant.
> But let me explain what I had in mind when adding it.
>
> It is a matter of what we do with input events generated while a
> device is inhibited. If ->open()/->close() are not provided by the
> driver then inhibiting amounts to merely ignoring input events from
> a device while it remains active. What else can you do if the driver
> does not provide a method to prepare the device for generating events/
> to stop generating events?
>
> In this special case a user might trigger a repeated event while the
> device is inhibited, then the user keeps holding the key down and the
> device is uninhibited. Do we pass anything to handlers then?
>
> In my opinion we should not. Such an event is "illegal" in a sense that it
> was generated at a time when nobody wanted any events from the device.
> Hence the test to let only those auto-repeat events through for which
> a key is actually pressed.
>
> However, what I see now is that if a device is inhibited, no key
> will ever reach neither the "1" nor "2" state because of the "if"
> in the very beginning of input_handle_event().
OK, then let's drop it for now. We can revisit if we see that a problem.
Thanks.
--
Dmitry
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 7/7] Input: Add "inhibited" property
Hi Andrzej,
On Mon, Jun 08, 2020 at 01:22:11PM +0200, Andrzej Pietrasiewicz wrote:
> @@ -284,8 +284,11 @@ static int input_get_disposition(struct input_dev *dev,
> case EV_KEY:
> if (is_event_supported(code, dev->keybit, KEY_MAX)) {
>
> - /* auto-repeat bypasses state updates */
> - if (value == 2) {
> + /*
> + * auto-repeat bypasses state updates but repeat
> + * events are ignored if the key is not pressed
> + */
> + if (value == 2 && test_bit(code, dev->key)) {
> disposition = INPUT_PASS_TO_HANDLERS;
> break;
> }
Is this chunk really part of inhibit support? I'd think we cancel
autorepeat timer when we are releasing a key, no?
Thanks.
--
Dmitry
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 3/7] ACPI: button: Access input device's users under appropriate mutex
On Thu, Jun 25, 2020 at 12:55:29PM +0200, Rafael J. Wysocki wrote: > On Thu, Jun 25, 2020 at 7:23 AM Dmitry Torokhov > wrote: > > > > On Wed, Jun 24, 2020 at 05:00:09PM +0200, Rafael J. Wysocki wrote: > > > On Mon, Jun 8, 2020 at 1:22 PM Andrzej Pietrasiewicz > > > wrote: > > > > > > > > Inspecting input device's 'users' member should be done under device's > > > > mutex, so add appropriate invocations. > > > > > > > > Signed-off-by: Andrzej Pietrasiewicz > > > > > > This looks like a fix that might be applied independently of the other > > > patches in the series. > > > > > > Do you want me to pick it up? > > > > If you pick it we'll have to have a dance with this series. Can I apply > > instead? > > Yes, please. > > Also feel free to add > > Acked-by: Rafael J. Wysocki > > to it. Looking at the driver I think the patch and the original use of input->users is not proper. I'll post another patch addressing this shortly. Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 4/7] ACPI: button: Use input_device_enabled() helper
On Wed, Jun 24, 2020 at 10:24:46PM -0700, Dmitry Torokhov wrote:
> On Mon, Jun 08, 2020 at 01:22:08PM +0200, Andrzej Pietrasiewicz wrote:
> > A new helper is available, so use it.
> >
> > Signed-off-by: Andrzej Pietrasiewicz
> > ---
> > drivers/acpi/button.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index ff7ab291f678..4deb2b48d03c 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -411,7 +411,7 @@ static void acpi_button_notify(struct acpi_device
> > *device, u32 event)
> > input = button->input;
> > if (button->type == ACPI_BUTTON_TYPE_LID) {
> > mutex_lock(>input->mutex);
> > - users = button->input->users;
> > + users = input_device_enabled(button->input);
> > mutex_unlock(>input->mutex);
> > if (users)
>
> This chunk (pre-patch) is really wrong. 'users' value is obsolete and
> can not be trusted the moment we unlocked the mutex. "if" needs to be
> inside critical section.
So looking at this patch and the previous one again, I believe this
driver is wrong to key the behavior off input->users and we should not
apply either of the patches. I'll post another patch fixing this.
Thanks.
--
Dmitry
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v2 0/4] Add 4 new keycodes and use them for 4 new hotkeys on new Lenovo Thinkpads
Hi Hans, On Tue, Sep 08, 2020 at 03:51:43PM +0200, Hans de Goede wrote: > Hi All, > > Here is a v2 of my series to get a couple of new hotkeys, and matching > thinkpad_acpi hkey event codes seen on the last 2 generations of > Thinkpads supported. > > New in this version that Lenovo got back to me and explained that the 0x1316 > hkey event belongs to Fn + right Shift, so that is mapped now too. > > Since these make both include/uapi/linux/input-event-codes.h and > drivers/platform/x86 changes one open question with this series is how to > merge it. > > Dmitry, for v1 of this series you suggested you could merge them all > through the input tree ? Andy, one of the drivers/platform/x86 maintainers > gave his Acked-by for this: > > Acked-by: Andy Shevchenko I queued up the series, thank you. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 0/3] Add 3 new keycodes and use them for 3 new hotkeys on new Lenovo Thinkpads
On Sun, Jul 19, 2020 at 07:56:49PM -0300, Henrique de Moraes Holschuh wrote: > On Fri, 17 Jul 2020, Hans de Goede wrote: > > This is a simple patch-series adding support for 3 new hotkeys found > > on various new Lenovo Thinkpad models. > > For all three patches, pending an ack for the new keycodes by the input > maintainers: > > Acked-by: Henrique de Moraes Holschuh Do you want me to merge all 3 through input tree? Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 4/7] ACPI: button: Use input_device_enabled() helper
On Mon, Jun 08, 2020 at 01:22:08PM +0200, Andrzej Pietrasiewicz wrote:
> A new helper is available, so use it.
>
> Signed-off-by: Andrzej Pietrasiewicz
> ---
> drivers/acpi/button.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index ff7ab291f678..4deb2b48d03c 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -411,7 +411,7 @@ static void acpi_button_notify(struct acpi_device
> *device, u32 event)
> input = button->input;
> if (button->type == ACPI_BUTTON_TYPE_LID) {
> mutex_lock(>input->mutex);
> - users = button->input->users;
> + users = input_device_enabled(button->input);
> mutex_unlock(>input->mutex);
> if (users)
This chunk (pre-patch) is really wrong. 'users' value is obsolete and
can not be trusted the moment we unlocked the mutex. "if" needs to be
inside critical section.
> acpi_lid_update_state(device, true);
> @@ -460,7 +460,7 @@ static int acpi_button_resume(struct device *dev)
>
> button->suspended = false;
> mutex_lock(>mutex);
> - if (button->type == ACPI_BUTTON_TYPE_LID && input->users) {
> + if (button->type == ACPI_BUTTON_TYPE_LID &&
> input_device_enabled(input)) {
> button->last_state = !!acpi_lid_evaluate_state(device);
> button->last_time = ktime_get();
> acpi_lid_initialize_state(device);
> --
> 2.17.1
>
Thanks.
--
Dmitry
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 3/7] ACPI: button: Access input device's users under appropriate mutex
On Wed, Jun 24, 2020 at 05:00:09PM +0200, Rafael J. Wysocki wrote:
> On Mon, Jun 8, 2020 at 1:22 PM Andrzej Pietrasiewicz
> wrote:
> >
> > Inspecting input device's 'users' member should be done under device's
> > mutex, so add appropriate invocations.
> >
> > Signed-off-by: Andrzej Pietrasiewicz
>
> This looks like a fix that might be applied independently of the other
> patches in the series.
>
> Do you want me to pick it up?
If you pick it we'll have to have a dance with this series. Can I apply
instead?
I do not think this change has any practical effect as nobody
attaches/detached input handlers or opening/closing input devices when
system goes through device resume phase.
>
> > ---
> > drivers/acpi/button.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 78cfc70cb320..ff7ab291f678 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -456,13 +456,16 @@ static int acpi_button_resume(struct device *dev)
> > {
> > struct acpi_device *device = to_acpi_device(dev);
> > struct acpi_button *button = acpi_driver_data(device);
> > + struct input_dev *input = button->input;
> >
> > button->suspended = false;
> > - if (button->type == ACPI_BUTTON_TYPE_LID && button->input->users) {
> > + mutex_lock(>mutex);
> > + if (button->type == ACPI_BUTTON_TYPE_LID && input->users) {
> > button->last_state = !!acpi_lid_evaluate_state(device);
> > button->last_time = ktime_get();
> > acpi_lid_initialize_state(device);
> > }
> > + mutex_unlock(>mutex);
> > return 0;
> > }
> > #endif
> > --
> > 2.17.1
> >
Thanks.
--
Dmitry
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4 0/7] Support inhibiting input devices
On Wed, Jun 10, 2020 at 12:38:30PM +0200, Rafael J. Wysocki wrote: > On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede wrote: > > > > Hi All, > > > > On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: > > > This is a quick respin of v3, with just two small changes, please see > > > the changelog below. > > > > > > Userspace might want to implement a policy to temporarily disregard input > > > from certain devices. > > > > > > An example use case is a convertible laptop, whose keyboard can be folded > > > under the screen to create tablet-like experience. The user then must hold > > > the laptop in such a way that it is difficult to avoid pressing the > > > keyboard > > > keys. It is therefore desirable to temporarily disregard input from the > > > keyboard, until it is folded back. This obviously is a policy which should > > > be kept out of the kernel, but the kernel must provide suitable means to > > > implement such a policy. > > > > First of all sorry to start a somewhat new discussion about this > > while this patch set is also somewhat far along in the review process, > > but I believe what I discuss below needs to be taken into account. > > > > Yesterday I have been looking into why an Asus T101HA would not stay > > suspended when the LID is closed. The cause is that the USB HID multi-touch > > touchpad in the base of the device starts sending events when the screen > > gets close to the touchpad (so when the LID is fully closed) and these > > events are causing a wakeup from suspend. HID multi-touch devices > > do have a way to tell them to fully stop sending events, also disabling > > the USB remote wakeup the device is doing. The question is when to tell > > it to not send events though ... > > > > So now I've been thinking about how to fix this and I believe that there > > is some interaction between this problem and this patch-set. > > > > The problem I'm seeing on the T101HA is about wakeups, so the question > > which I want to discuss is: > > > > 1. How does inhibiting interact with enabling / > > disabling the device as a wakeup source ? One should not affect the other. > > > > 2. Since we have now made inhibiting equal open/close how does open/close > > interact with a device being a wakeup source ? One did not affect another, and it should not. > > > > And my own initial (to be discussed) answers to these questions: > > > > 1. It seems to me that when a device is inhibited it should not be a > > wakeup source, so where possible a input-device-driver should disable > > a device's wakeup capabilities on suspend if inhibited > > If "inhibit" means "do not generate any events going forward", then > this must also cover wakeup events, so I agree. Why? These are separate concepts. Do we disable wake on lan when bringing network interface down? Do we update power/wakeup when device is inhibited? Do we restore it afterwards? Do we un-inhibit if we reenable wakeup after device is inhibited? Do we return error? How? Inhibit works on logical level, i.e. if I have several input interfaces on the same hardware device, I cam inhibit one leaving others intact. This does not mean that the device should stop generating wakeup events. We can't even guarantee this for composite devices. > > > 2. This one is trickier I don't think we have really clearly specified > > any behavior here. The default behavior of most drivers seems to be > > using something like this in their suspend callback: > > > > if (device_may_wakeup(dev)) > > enable_irq_wake(data->irq); > > else if (input->users) > > foo_stop_receiving_events(data); > > > > Since this is what most drivers seem to do I believe we should keep > > this as is and that we should just clearly document that if the > > input_device has users (has been opened) or not does not matter > > for its wakeup behavior. > > > > Combining these 2 answers leads to this new pseudo code template > > for an input-device's suspend method: > > > > /* > > * If inhibited we have already disabled events and > > * we do NOT want to setup the device as wake source. > > */ > > if (input->inhibited) > > return 0; > > > > if (device_may_wakeup(dev)) > > enable_irq_wake(data->irq); > > else if (input->users) > > foo_stop_receiving_events(data); > > > > ### > > Sounds reasonable to me. However it will not work. For many input devices connected to i2c we declare interrupt as wakeup interrupt, and the driver does not need to issue enable_irq_wake() and disable_irq_wake(). The wakeup handling is happening in driver core, which is not aware of input-specific inhibit (nor should it be). I need to ping Mark about the patch adding the similar handling to SPI. > > > A different, but related issue is how to make devices actually use the > > new inhibit support on the builtin keyboard + touchpad when say the lid > > is closed.
Re: [ibm-acpi-devel] [PATCH v3 0/7] Support inhibiting input devices
On Sun, Jun 07, 2020 at 10:24:14PM +0200, Pavel Machek wrote: > On Fri 2020-06-05 19:33:28, Andrzej Pietrasiewicz wrote: > > Userspace might want to implement a policy to temporarily disregard input > > from certain devices. > > Wow, you certainly cc a lot of lists. > > > An example use case is a convertible laptop, whose keyboard can be folded > > under the screen to create tablet-like experience. The user then must hold > > the laptop in such a way that it is difficult to avoid pressing the keyboard > > keys. It is therefore desirable to temporarily disregard input from the > > keyboard, until it is folded back. This obviously is a policy which should > > be kept out of the kernel, but the kernel must provide suitable means to > > implement such a policy. > > > > Due to interactions with suspend/resume, a helper has been added for drivers > > to decide if the device is being used or not (PATCH 1/7) and it has been > > applied to relevant drivers (PATCH 2,4,5,6/7). > > But is that a right way to implement it? > > We want this for cellphones, too -- touchscreen should be disabled > while the device is locked in the pocket -- but we really want the > touchscreen hardware to be powered down in that case (because it keeps > SoC busy and eats a _lot_ of electricity). > > But simplistic "receive an event and then drop it if device is > inhibited" does not allow that... I do not think you read the entirety of this patch series... Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCHv2 0/7] Support inhibiting input devices
Hi Hans, Andrzej, On Wed, Jun 03, 2020 at 09:37:10PM +0200, Hans de Goede wrote: > Hi, > > On 6/3/20 7:54 PM, Andrzej Pietrasiewicz wrote: > > W dniu 03.06.2020 o 19:38, Hans de Goede pisze: > > > Hi, > > > > > > On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote: > > > > Hi Hans, hi Dmitry, > > > > > > > > > > > > > I'm taking one step back and looking at the ->open() and ->close() > > > > driver callbacks. They are called from input_open_device() and > > > > input_close_device(), respectively: > > > > > > > > input_open_device(): > > > > "This function should be called by input handlers when they > > > > want to start receive events from given input device." > > > > > > > > ->open() callback: > > > > "this method is called when the very first user calls > > > > input_open_device(). The driver must prepare the device to start > > > > generating events (start polling thread, request an IRQ, submit > > > > URB, etc.)" > > > > > > > > input_close_device(): > > > > "This function should be called by input handlers when they > > > > want to stop receive events from given input device." > > > > > > > > ->close() callback: > > > > "this method is called when the very last user calls > > > > input_close_device()" > > > > > > > > It seems to me that the callback names do not reflect their > > > > purpose: their meaning is not to "open" or to "close" but to > > > > give drivers a chance to control when they start or stop > > > > providing events to the input core. > > > > > > > > What would you say about changing the callbacks' names? > > > > I'd envsion: ->provide_events() instead of ->open() and > > > > ->stop_events() instead of ->close(). Of course drivers can > > > > exploit the fact of knowing that nobody wants any events > > > > from them and do whatever they consider appropriate, for > > > > example go into a low power mode - but the latter is beyond > > > > the scope of the input subsystem and is driver-specific. > > > > > > I don't have much of an opinion on changing the names, > > > to me open/close have always means start/stop receiving > > > events. This follows the everything is a file philosophy, > > > e.g. you can also not really "open" a serial port, > > > yet opening /dev/ttyS0 will activate the receive IRQ > > > of the UART, etc. So maybe we just need to make the > > > docs clearer rather then do the rename? Doing the > > > rename is certainly going to cause a lot of churn. > > > > Right, I can see now that the suggestion to change names is > > too far fetched. (I feel that release() would be better > > than close(), though). But it exposes the message I wanted to > > pass. release() usually means that the object is destroyedm, i.e this action, unlike close() is irrevocable. Let's leave the names as is, and adjust kerneldoc comments as needed. > > > > > > > > Anyways as said, I don't have much of an opinion, > > > so I'll leave commenting (more) on this to Dmitry. > > > > > > > With such a naming change in mind let's consider inhibiting. > > > > We want to be able to control when to disregard events from > > > > a given device. It makes sense to do it at device level, otherwise > > > > such an operation would have to be invoked in all associated > > > > handlers (those that have an open handle associating them with > > > > the device in question). But of course we can do better than > > > > merely ignoring the events received: we can tell the drivers > > > > that we don't want any events from them, and later, at uninhibit > > > > time, tell them to start providing the events again. Conceptually, > > > > the two operations (provide or don't provide envents) are exactly > > > > the same thing we want to be happening at input_open_device() and > > > > input_close_device() time. To me, changing the names of > > > > ->open() and ->close() exposes this fact very well. > > > > > > > > Consequently, ->inhibit() and ->uninhibit() won't be needed, > > > > and drivers which already implement ->provide_events() (formerly > > > > ->open()) and ->stop_events() (formerly ->close()) will receive > > > > full inhibit/uninhibit support for free (subject to how well they > > > > implement ->provide_events()/->stop_events()). Unless we can come > > > > up with what the drivers might be doing on top of ->stop_events() > > > > and ->provide_events() when inhibiting/uninhibiting, but it seems > > > > to me we can't. Can we? > > > > > > Right. I'm happy that you've come to see that both on open/close > > > and on inhibit/uninhibit we want to "start receiving events" and > > > "stop receiving events", so that we only need one set of callbacks. > > > > > > > Yeah, that's my conclusion - at least on a conceptual level. > > > > That said, what I can imagine is an existing driver (e.g. elan_i2c) > > which does not implement neither open() nor close(), but does have > > suspend() and resume(). Then it is maybe a bit easier to add inhibit() > > and uninhibit() /they would be similar to suspend and
Re: [ibm-acpi-devel] [PATCHv2 0/7] Support inhibiting input devices
Hi Andrzej, On Tue, Jun 02, 2020 at 06:56:40PM +0200, Andrzej Pietrasiewicz wrote: > Hi Dmitry, > > W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze: > > That said, I think the way we should handle inhibit/uninhibit, is that > > if we have the callback defined, then we call it, and only call open and > > close if uninhibit or inhibit are _not_ defined. > > > > If I understand you correctly you suggest to call either inhibit, > if provided or close, if inhibit is not provided, but not both, > that is, if both are provided then on the inhibit path only > inhibit is called. And, consequently, you suggest to call either > uninhibit or open, but not both. The rest of my mail makes this > assumption, so kindly confirm if I understand you correctly. Yes, that is correct. If a driver wants really fine-grained control, it will provide inhibit (or both inhibit and close), otherwise it will rely on close in place of inhibit. > > In my opinion this idea will not work. > > The first question is should we be able to inhibit a device > which is not opened? In my opinion we should, in order to be > able to inhibit a device in anticipation without needing to > open it first. I agree. > > Then what does opening (with input_open_device()) an inhibited > device mean? Should it succeed or should it fail? It should succeed. > If it is not > the first opening then effectively it boils down to increasing > device's and handle's counters, so we can allow it to succeed. > If, however, the device is being opened for the first time, > the ->open() method wants to be called, but that somehow > contradicts the device's inhibited state. So a logical thing > to do is to either fail input_open_device() or postpone ->open() > invocation to the moment of uninhibiting - and the latter is > what the patches in this series currently do. > > Failing input_open_device() because of the inhibited state is > not the right thing to do. Let me explain. Suppose that a device > is already inhibited and then a new matching handler appears > in the system. Most handlers (apm-power.c, evbug.c, input-leds.c, > mac_hid.c, sysrq.c, vt/keyboard.c and rfkill/input.c) don't create > any character devices (only evdev.c, joydev.c and mousedev.c do), > so for them it makes no sense to delay calling input_open_device() > and it is called in handler's ->connect(). If input_open_device() > now fails, we have lost the only chance for this ->connect() to > succeed. > > Summarizing, IMO the uninhibit path should be calling both > ->open() and ->uninhibit() (if provided), and conversely, the inhibit > path should be calling both ->inhibit() and ->close() (if provided). So what you are trying to say is that you see inhibit as something that is done in addition to what happens in close. But what exactly do you want to do in inhibit, in addition to what close is doing? In my view, if we want to have a dedicated inhibit callback, then it will do everything that close does, they both are aware of each other and can sort out the state transitions between them. For drivers that do not have dedicated inhibit/uninhibit, we can use open and close handlers, and have input core sort out when each should be called. That means that we should not call dev->open() in input_open_device() when device is inhibited (and same for dev->close() in input_close_device). And when uninhibiting, we should not call dev->open() when there are no users for the device, and no dev->close() when inhibiting with no users. Do you see any problems with this approach? Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCHv2 0/7] Support inhibiting input devices
On Tue, May 19, 2020 at 11:36:34AM +0200, Hans de Goede wrote:
> Hi,
>
> On 5/19/20 11:02 AM, Andrzej Pietrasiewicz wrote:
> > Hi Hans, Hi Dmitry,
> >
> > W dniu 18.05.2020 o 16:23, Hans de Goede pisze:
> > > Hi,
> >
> >
> >
> > > > > > >
> > > > > > > So I wonder what this series actually adds for functionality for
> > > > > > > userspace which can not already be achieved this way?
> > > > > > >
> > > > > > > I also noticed that you keep the device open (do not call the
> > > > > > > input_device's close callback) when inhibited and just throw away
> > > > > >
> > > > > > I'm not sure if I understand you correctly, it is called:
> > > > > >
> > > > > > +static inline void input_stop(struct input_dev *dev)
> > > > > > +{
> > > > > > + if (dev->poller)
> > > > > > + input_dev_poller_stop(dev->poller);
> > > > > > + if (dev->close)
> > > > > > + dev->close(dev);
> > > > > >
> > > > > > +static int input_inhibit(struct input_dev *dev)
> > > > > > +{
> > > > > > + int ret = 0;
> > > > > > +
> > > > > > + mutex_lock(>mutex);
> > > > > > +
> > > > > > + if (dev->inhibited)
> > > > > > + goto out;
> > > > > > +
> > > > > > + if (dev->users) {
> > > > > > + if (dev->inhibit) {
> > > > > > + ret = dev->inhibit(dev);
> > > > > > + if (ret)
> > > > > > + goto out;
> > > > > > + }
> > > > > > + input_stop(dev);
> > > > > >
> > > > > >
> > > > > > It will not be called when dev->users is zero, but if it is zero,
> > > > > > then nobody has opened the device yet so there is nothing to close.
> > > > >
> > > > > Ah, I missed that.
> > > > >
> > > > > So if the device implements the inhibit call back then on
> > > > > inhibit it will get both the inhibit and close callback called?
> > > > >
> > > >
> > > > That's right. And conversely, upon uninhibit open() and uninhibit()
> > > > callbacks will be invoked. Please note that just as with open()/close(),
> > > > providing inhibit()/uninhibit() is optional.
> > >
> > > Ack.
> > >
> > > > > And what happens if the last user goes away and the device
> > > > > is not inhibited?
> > > >
> > > > close() is called as usually.
> > >
> > > But not inhibit, hmm, see below.
> > >
> > > > > I'm trying to understand here what the difference between the 2
> > > > > is / what the goal of having a separate inhibit callback ?
> > > > >
> > > >
> > > > Drivers have very different ideas about what it means to suspend/resume
> > > > and open/close. The optional inhibit/uninhibit callbacks are meant for
> > > > the drivers to know that it is this particular action going on.
> > >
> > > So the inhibit() callback triggers the "suspend" behavior ?
> > > But shouldn't drivers which are capable of suspending the device
> > > always do so on close() ?
> > >
> > > Since your current proposal also calls close() on inhibit() I
> > > really see little difference between an inhibit() and the last
> > > user of the device closing it and IMHO unless there is a good
> > > reason to actually differentiate the 2 it would be better
> > > to only stick with the existing close() and in cases where
> > > that does not put the device in a low-power mode yet, fix
> > > the existing close() callback to do the low-power mode
> > > setting instead of adding a new callback.
> > >
> > > > For inhibit() there's one more argument: close() does not return a
> > > > value,
> > > > so its meaning is "do some last cleanup" and as such it is not allowed
> > > > to fail - whatever its effect is, we must deem it successful. inhibit()
> > > > does return a value and so it is allowed to fail.
> > >
> > > Well, we could make close() return an error and at least in the inhibit()
> > > case propagate that to userspace. I wonder if userspace is going to
> > > do anything useful with that error though...
It really can't do anything. Have you ever seen userspace handling
errors from close()? And what can be done? A program is terminating, but
the kernel says "no, you closing input device failed, you have to
continue running indefinitely..."
> > >
> > > In my experience errors during cleanup/shutdown are best logged
> > > (using dev_err) and otherwise ignored, so that we try to clean up
> > > as much possible. Unless the very first step of the shutdown process
> > > fails the device is going to be in some twilight zone state anyways
> > > at this point we might as well try to cleanup as much as possible.
> >
> > What you say makes sense to me.
> > @Dmitry?
I will note here, that inhibit is closer to suspend() than to close(),
and we do report errors for suspend(). Therefore we could conceivably
try to handle errors if driver really wants to be fancy. But I think
majority of cases will be quite happy with using close() and simply
logging errors, as Hans said.
That said, I think the way we should handle inhibit/uninhibit, is that
if we have the
Re: [ibm-acpi-devel] [PATCHv2 0/7] Support inhibiting input devices
Hi Hans, Peter, On Mon, May 18, 2020 at 08:55:10AM +1000, Peter Hutterer wrote: > On Fri, May 15, 2020 at 08:19:10PM +0200, Hans de Goede wrote: > > Hi Andrezj, > > > > On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote: > > > Userspace might want to implement a policy to temporarily disregard input > > > from certain devices, including not treating them as wakeup sources. > > > > > > An example use case is a laptop, whose keyboard can be folded under the > > > screen to create tablet-like experience. The user then must hold the > > > laptop > > > in such a way that it is difficult to avoid pressing the keyboard keys. It > > > is therefore desirable to temporarily disregard input from the keyboard, > > > until it is folded back. This obviously is a policy which should be kept > > > out of the kernel, but the kernel must provide suitable means to implement > > > such a policy. > > > > Actually libinput already binds together (inside libinput) SW_TABLET_MODE > > generating evdev nodes and e.g. internal keyboards on devices with 360° > > hinges for this reason. libinput simply closes the /dev/input/event# > > node when folded and re-opens it when the keyboard should become active > > again. Thus not only suppresses events but allows e.g. touchpads to > > enter runtime suspend mode which saves power. Typically closing the > > /dev/input/event# node will also disable the device as wakeup source. > > > > So I wonder what this series actually adds for functionality for > > userspace which can not already be achieved this way? > > Thanks Hans. To expand on this: > libinput has heuristics to guess which input devices (keyboards, touchpads) > are built-in ones. When the tablet mode switch is on, we disable these > devices internally (this is not visible to callers), and re-enable it again > later when the tablet mode switch is off again. I think that is great that libinput has tried solving this for the tablet mode, but unfortunately libinput only works for users of libinput, leaving cases such as: 1. In-kernel input handlers, such as SysRq, VT and others 2. Systems that do not rely on libinput for userspace handing (Android, Chrome OS) 3. Systems with policies that are more complex than tablet mode only. Because of libinput's inability to affect the kernel, and the presence of "always on" input handlers (sysrq, VT keyboard, potentially others), while libinput may control whether consumers receive events from certain input devices, it will not allow power savings that an explicit "inhibit" allows when coming from dedicated power policy manager. I think pushing policy decisions into a library, and trying to have all clients agree with it, is much harder and leaks unnecessary knowledge into quite a few layers. A dedicated power policy manager, that is not only responsible for input device, but power state of the system as a whole, is a very viable architecture. > > This is done for keyboards and touchpads atm (and I think pointing sticks) > and where the heuristics fail we have extra quirks in place. For example > the Lenovo Yogas tend to disable the keyboard mechanically in tablet mode > but buttons (e.g. volume keys) around the screen send events through the > same event node. So on those devices we don't disable the keyboard. > > We've had this code for a few years now and the only changes to it have been > the various device quirks for devices that must not suspend the keyboard, > it's otherwise working as expected. > > If we ever have a device where we need to disable parts of the keyboard > only, we could address this with EVIOCSMASK but so far that hasn't been > necessary. > > I agree with Hans, right now I don't see the usefulness of this new sysfs > toggle. For it to be really useful you'd have to guarantee that it's > available for 100% of the devices and that's IMO unlikely to happen. The inhibiting of the events works for 100% of input devices, the power savings work for the ones that implement it. It is responsibility of folds shipping the systems to make sure drivers they use support inhibit if they believe it will help their battery life. > > Cheers, >Peter > > > I also noticed that you keep the device open (do not call the > > input_device's close callback) when inhibited and just throw away > > any events generated. This seems inefficient and may lead to > > the internal state getting out of sync. What if a key is pressed > > while inhibited and then the device is uninhibited while the key > > is still pressed? Now the press event is lost and userspace > > querying the current state will see the pressed key as being > > released. This is a good point. We should look into signalling that some events have been dropped (via EV_SYN/SYN_DROPPED) so that clients are aware of it. > > > > On top of this you add special inhibit and uninhibit callbacks > > and implement those for just a few devices. How do these differ > > from just closing the device and later opening it again ? I
Re: [ibm-acpi-devel] 2nd Fan quirk for Thinkpad P50 causes spurios touchpad/trackpoint events on ThinkPad L570
On Sat, Oct 13, 2018 at 6:32 AM Jaak Ristioja wrote: > > On 27.08.2018 19:22, Jaak Ristioja wrote: > > Upgrading Linux from 4.16 to 4.17, a ThinkPad L570 started receiving > > spurious input events, mostly right mouse button click events, but also > > cursor jumps. > > > > I have not attempted to understand whether these events come from the > > trackpoint or touchpad or some other driver, but I managed to bisect > > this issue to commit a986c75a7df0 titled "platform/x86: thinkpad_acpi: > > Add 2nd Fan Support for Thinkpad P50" by Alexander Kappner. > > > > Apparently the quirk mitigation is applied when the BIOS version begins > > with N1. The BIOS version on the L570 in question is N1XET57W (1.35 ) > > which is probably why this commit causes the described problems on the > > ThinkPad L570. How exactly? - I don't know. > > > > The issue did not reproduce when running some stable 4.17 and 4.18 > > kernels with commit a986c75a7df0 reverted. > > > > Please fix this for future kernels. Thanks! :) > > Ping. Do you need any additional information? Sounds like we need tighter check for the quirk, maybe based on DMI/Board name? Can we revert the offending commit for now? Thanks. -- Dmitry ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 5/7] input: Add keycodes used by Lenovo Carbon X1 2014
On Fri, Feb 20, 2015 at 10:04:04AM -0500, Benjamin Tissoires wrote:
On Fri, Feb 20, 2015 at 9:44 AM, Bastien Nocera had...@hadess.net wrote:
Signed-off-by: Bastien Nocera had...@hadess.net
---
include/uapi/linux/input.h | 10 ++
1 file changed, 10 insertions(+)
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index b0a8130..affe40e 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -740,6 +740,16 @@ struct input_keymap_entry {
#define KEY_BRIGHTNESS_MIN 0x250 /* Set Brightness to
Minimum */
#define KEY_BRIGHTNESS_MAX 0x251 /* Set Brightness to
Maximum */
+#define KEY_CLIPPING_TOOL 0x252 /* Screenshot clipping tool
*/
Do we have users for this?
+#define KEY_CLOUD 0x253 /* Cloud Settings */
Just no. What is cloud settings? Use KEY_VENDOR if you must for this vendor
added value junk they come up on keyboards year after year. It used to be
WWW, not cloud, I bet in 2016 someone will come up with IoT-dedicated key.
+#define KEY_CAMERA_GESTURES0x254 /* Toggle Camera gestures */
+#define KEY_NEW_TAB0x255 /* Open a new tab */
+#define KEY_MICUP 0x256 /* Microphone Up */
+#define KEY_MICDOWN0x257 /* Microphone Down */
+#define KEY_MICCANCEL_MODE 0x258 /* Toggle Microphone
cancellation mode */
+#define KEY_CAMERA_ZOOM_MODE 0x259 /* Toggle camera zoom mode
*/
+#define KEY_ROTATE_DISPLAY 0x25a /* Rotate screen by 90
degrees */
+
This one should be acked by Dmitry (in CC)
Some of these (like microphone volume up/down) are generic and reasonable, but
I need to know if there will be any users for these newly defined keys.
Thanks.
--
Dmitry
--
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=190641631iu=/4140/ostg.clktrk
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [RFC PATCH] thinkpad-acpi: Improve hardware volume controls
Hi Andrew, On Thu, May 12, 2011 at 10:50:54AM -0400, Andrew Lutomirski wrote: I do, however, have a question for the input people. Dmitry: Lenovo makes laptops which are kind enough to tell us that the volume changed by sending a keystroke over the atkbd-based keyboard. (wtf!) I've modified the thinkpad-acpi driver to register an input handler to catch those events coming from the keyboard and send them to ALSA where they belong. But if there's a keyboard grab, it won't work. Would you accept a patch to the input layer to allow filters (or maybe just filters that specifically request it) to run even if there's a grab? There is a filter on i8042 level that was introduced specifically for cases when events not having any relation to the input are routed via KBC interface. It looks like this is the one you want to use. See include/linux/i8042.h::i8042_install_filter(). It allows for such events to completely bypass input layer. Hope this helps. -- Dmitry -- Achieve unprecedented app performance and reliability What every C/C++ and Fortran developer should know. Learn how Intel has extended the reach of its next-generation tools to help boost performance applications - inlcuding clusters. http://p.sf.net/sfu/intel-dev2devmay ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [RFC PATCH] thinkpad-acpi: Improve hardware volume controls
On Thu, May 12, 2011 at 03:33:54PM -0400, Andrew Lutomirski wrote: On Thu, May 12, 2011 at 11:39 AM, Dmitry Torokhov dmitry.torok...@gmail.com wrote: Hi Andrew, On Thu, May 12, 2011 at 10:50:54AM -0400, Andrew Lutomirski wrote: I do, however, have a question for the input people. Dmitry: Lenovo makes laptops which are kind enough to tell us that the volume changed by sending a keystroke over the atkbd-based keyboard. (wtf!) I've modified the thinkpad-acpi driver to register an input handler to catch those events coming from the keyboard and send them to ALSA where they belong. But if there's a keyboard grab, it won't work. Would you accept a patch to the input layer to allow filters (or maybe just filters that specifically request it) to run even if there's a grab? There is a filter on i8042 level that was introduced specifically for cases when events not having any relation to the input are routed via KBC interface. It looks like this is the one you want to use. See include/linux/i8042.h::i8042_install_filter(). It allows for such events to completely bypass input layer. Hope this helps. Sort of. dell-laptop and msi-laptop are content to take some action on the keys they see but still leave the keys in the input stream, so their job is a bit easier. I need to swallow one kind of extended key, detect and not swallow two others, and ignore all the ones that are normal keys. But that means that I don't know whether I should filter out 0xe0 until it's too late. So either I'd need a function to feed an event back into i8042 or I need to filter a little farther downstream when the keys are resolved into keycodes (or scancodes -- I'm not really up on the terminology). You can use serio_interrupt() to inject additional bytes into serio data stream. Thanks. -- Dmitry -- Achieve unprecedented app performance and reliability What every C/C++ and Fortran developer should know. Learn how Intel has extended the reach of its next-generation tools to help boost performance applications - inlcuding clusters. http://p.sf.net/sfu/intel-dev2devmay ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly
On Thu, Dec 10, 2009 at 11:19:18AM -0200, Henrique de Moraes Holschuh wrote:
On Wed, 09 Dec 2009, Dmitry Torokhov wrote:
Still, please look at the patch below... Would something like this be a
cleaner API? It is certainly more obvious, and it is cleaner on the
driver
side (one function call does everything, instead of a call to
input_set_capability plus a call to whatever the driver needs to issue the
initial EV_SW event)...
Yes, I think it is a good idea. However why don't we change it to:
input_setup_event(dev, type, code, value)
{
input_set_capability(...);
input_event(...);
}
So it would work for everything (who knows, maybe down the road some
driver wants to init its ABS axes properly and so on)?
Well, we already have input_set_abs_params, I'd say that's the correct place
to init the axes... and you'd compile-break any drivers that need love to
properly init the axes, so it is a win-win situation as it would make sure
the patch is feature-complete (i.e. converts all drivers to the new call and
makes sure they all init their abs params properly).
Right, input_set_abs_params()... No, I don't really want to go through
all touchpad, touchscreen and joystick drivers at once, thank you vey
much ;) Besides, for the vast majority of uses 0 is the proper initial
value for absolute axis.
The only other event that needs initialization are the switches, so I'd
argue that we might as well use my proposed patch, which is specific and
more lightweight, and convert the in-tree drivers to it. A few might be
missing the before-registering input_event...
As I think about it even more I think we should leave it asd is. You
should just split setting up the device's capabilities and seeding of
the initial values. Given that you need to re-seed the values upon
resume - and there you do need to use input_event() - just create a
function that queries the hardwware and sends the events and invoke it
once upon registration and also resume hardler.
BTW, looking at input.h, wouldn't it be better to force the init functions
to be run before registering the input device, doing a BUG_ON() if they're
misused?
What function are you referring to? Input devices are in useable state
as returned by input_allocate_device().
I'd also suggest a BUG_ON(!dev), or at least an if (!dev) return
-EINVAL; to the top of input_register_device(...)...
Nah, just let it crash (the same effect as BUG_ON really).
--
Dmitry
--
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly
On Thu, Dec 10, 2009 at 10:46:02PM -0200, Henrique de Moraes Holschuh wrote:
On Thu, 10 Dec 2009, Dmitry Torokhov wrote:
Right, input_set_abs_params()... No, I don't really want to go through
all touchpad, touchscreen and joystick drivers at once, thank you vey
much ;) Besides, for the vast majority of uses 0 is the proper initial
value for absolute axis.
Heh.
The only other event that needs initialization are the switches, so I'd
argue that we might as well use my proposed patch, which is specific and
more lightweight, and convert the in-tree drivers to it. A few might be
missing the before-registering input_event...
As I think about it even more I think we should leave it asd is. You
It needs to be documented somewhere, at least.
Something like the patch below?
BTW, looking at input.h, wouldn't it be better to force the init functions
to be run before registering the input device, doing a BUG_ON() if they're
misused?
What function are you referring to? Input devices are in useable state
The ones proposed in this subthread, which shouldn't be used after
registering.
as returned by input_allocate_device().
Unless, of course, someone screws up verifying if input_alocate_device
returned NULL...
Then it will crash way before registering, as soon as they try to set
name, phys and so on.. Just not worh it.
--
Dmitry
Input: document use of input_event() function
From: Dmitry Torokhov dmitry.torok...@gmail.com
Signed-off-by: Dmitry Torokhov d...@mail.ru
---
drivers/input/input.c | 10 --
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index e097176..6240c7a 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -302,9 +302,15 @@ static void input_handle_event(struct input_dev *dev,
* @value: value of the event
*
* This function should be used by drivers implementing various input
- * devices. See also input_inject_event().
+ * devices to report input events. See also input_inject_event().
+ *
+ * NOTE: input_event() may be safely used right after input device was
+ * allocated with input_allocate_device(), even before it is registered
+ * with input_register_device(), but the event will not reach any of the
+ * input handlers. Such early invocation of input_event() may be used
+ * to 'seed' initial state of a switch or initial position of absolute
+ * axis, etc.
*/
-
void input_event(struct input_dev *dev,
unsigned int type, unsigned int code, int value)
{
--
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly
On Wed, Dec 09, 2009 at 02:30:41PM -0200, Henrique de Moraes Holschuh wrote: On Tue, 08 Dec 2009, Dmitry Torokhov wrote: On Tuesday 08 December 2009 05:36:30 pm Henrique de Moraes Holschuh wrote: Before we register the input device, sync the input layer EV_SW state directly by setting the bitmaps, to avoid issuing a gratuitous event for the initial state of these switches. I will propose a clean input layer API for this and change the driver to use it later, but I'd rather get the driver fix in mainline ASAP. Just do input_report_switch() before registering the device, it will do the right thing. At device startup, the input core doesn't know the initial state of the switch. It assumes the switch is off (all bits clear in the sw bitmap). input_report_switch() will call input_event(), which will have a 50% chance of doing the wrong thing at startup (i.e. issue an event) since it will look at the state of the sw bitmap to decide whether to issue an event or not. It will not propagate events because until you register the device there won't be any consumers attached to it. So the only thing that will happen is that it will sync internal state of the device from input core point of view with the true state of the hardware. Input core guarantees (and will continue doing so) that it is safe to pass events to the device as soon as it was allocated with input_allocate_device(). Well, there really isn't a transparent way to do this... Either you change the core to use the first event reported to init the switch and never issue that first event outside the core (we will also have to make sure all drivers know they have to issue this initial event), or you add an API for the drivers to directly inform the core of the initial state of the switches (I have this already written) and check all drivers that export EV_SW events to make sure they use the API (I haven't done this yet). There is already such API, input_event(). See above. -- Dmitry -- Return on Information: Google Enterprise Search pays you back Get the facts. http://p.sf.net/sfu/google-dev2dev ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly
On Wed, Dec 09, 2009 at 06:32:01PM -0200, Henrique de Moraes Holschuh wrote:
On Wed, 09 Dec 2009, Dmitry Torokhov wrote:
input_report_switch() will call input_event(), which will have a 50%
chance
of doing the wrong thing at startup (i.e. issue an event) since it will
look
at the state of the sw bitmap to decide whether to issue an event or not.
It will not propagate events because until you register the device there
won't be any consumers attached to it. So the only thing that will
happen is that it will sync internal state of the device from input core
point of view with the true state of the hardware.
Ah, I see. Cute trick, and yes, that would work just fine. I will do that,
it certainly beats accessing the sw bitmap directly.
Is it documented anywhere?
Not explicetiley, no.
Still, please look at the patch below... Would something like this be a
cleaner API? It is certainly more obvious, and it is cleaner on the driver
side (one function call does everything, instead of a call to
input_set_capability plus a call to whatever the driver needs to issue the
initial EV_SW event)...
Yes, I think it is a good idea. However why don't we change it to:
input_setup_event(dev, type, code, value)
{
input_set_capability(...);
input_event(...);
}
So it would work for everything (who knows, maybe down the road some
driver wants to init its ABS axes properly and so on)?
--
Dmitry
--
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly
On Tuesday 08 December 2009 05:36:30 pm Henrique de Moraes Holschuh wrote: Before we register the input device, sync the input layer EV_SW state directly by setting the bitmaps, to avoid issuing a gratuitous event for the initial state of these switches. I will propose a clean input layer API for this and change the driver to use it later, but I'd rather get the driver fix in mainline ASAP. Just do input_report_switch() before registering the device, it will do the right thing. Input core guarantees (and will continue doing so) that it is safe to pass events to the device as soon as it was allocated with input_allocate_device(). -- Dmitry -- Return on Information: Google Enterprise Search pays you back Get the facts. http://p.sf.net/sfu/google-dev2dev ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] ACPI: thinkpad-acpi: make the input event mode the default
On 7/15/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: On Mon, 16 Jul 2007, Matthew Garrett wrote: Having checked other drivers, it looks like KEY_SWITCHVIDEOMODE is the one used for video toggling. If that's not actually how people are using it, we need to fix that - if it is, I'd just go for it. This looks like a breakage warning to me. Better decide once and for all what this keycode is supposed to do, and document it. Because switching video mode is not switch video output mode, and can easily be confused with switch screen resolution, switch video mode between expanded and normal or whatever. I will add a comment to input.h to the effect that KEY_SWITCHVIDEOMODE is to switch between available outputs (LCD/Monitor/TV-Out/Whatnot). Right now couple of RCs are using it for different purposes (cycling through inputs I think) so we should do somethig about these. -- Dmitry - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] Making KEY_UNKNOWN really useful to userland
On 5/31/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: On Thu, 31 May 2007, Richard Hughes wrote: I am on it on the thinkpad-acpi side, so at least for that, you don't have to worry. I am still waiting an answer about which event is the correct one to output scancodes, but the thinkpad-acpi GIT tree is already updated with the above, tentively using MSC_SCAN to report scan codes. I thought I sent out email saying MSC_SCAN is what you want to use, MSC_RAW is raw data from device, potentially having make/break codes included/encoded. Apparently my mail broke even more than I thought. Highjacking the thread somewhat - Richard, I saw your patch for toshiba_acpi. Please use input-polldev to set up polled devices. It will create work queue for you and will make sure that polling is stopped when device is closed. Also I don't think you want to use KEY_BREAK. What is the expected function of that key? Please copy me on input-related changes in the tree. Thank you. -- Dmitry - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] Making KEY_UNKNOWN really useful to userland
On Wednesday 30 May 2007 20:53, Henrique de Moraes Holschuh wrote: On Wed, 30 May 2007, Dmitry Torokhov wrote: On 5/30/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: It is trivial to guarantee that KEY_PROG is unique for a single input device (keyboard), but it certainly won't work across multiple devices. Userspace has to know what kind of input device it is talking to to map them to something, but since the input layer already provides this information, I was not going to raise a fuss about it. I figure it is the price of not increasing even more the keycode table. Actually I try to discourage people from using input_id during device discovery but rather tell them to analyze capability bits and then decide if their application is interested in that particular input device. I think input_id should pretty much only used by udev co. to adjust default kernel setup for the needs of local installation (fix keymap, adjust absmax, etc). It is likely going to be used by something like udev, or an init script, or a thinkpad-configurator helper, or whatever. Sounds like the sort of stuff that should be paying attention to input_id anyway. Right. We don't appear to have deployed a good kernel-userland interface that allows us to have generic press an unknown key and tell me what you want it to do application in userland, but if we do please correct me. If KEY_UNKNOWN was always required to send a EV_MSC MSC_RAW/MSC_SCAN, then we would have one. Yes, I think this is a resonable requrement. Maybe we can add that requirement and driver changes (if any, for all I know most drivers might already be generating such events) for 2.6.23? I think that we should start with drivers that allow adjusting keymaps via EVIOCGKEYCODE/EVIOCSKEYCODE. If driver does not allow changing keymap then it does not really matter (although I will try to convert drivers now that we have getkeycode/setkeycode per-device methods). I bet Richard would like that one a lot. Richard? Such functionality makes most of my requests for new keycodes irrelevant, so I will just trim that down. That said, how is one to know which hardware key was translated to KEY_UNKNOWN, so that he can inform the user to remap that key? Should I send another event together with KEY_UNKNOWN that has the raw keycode? Which one? You may try using EV_MSC/MSC_SCAN. You can even send it all the time, not only for KEY_UNKNOWN. Will do, thanks. And I will send it all the time, that will force people to properly implement code that can deal with them in userland. What is the exact difference between MSC_RAW and MSC_SCAN? Which one can be used as an index to reprogram the keymap using the IOCTLs? MSC_RAW is just raw data from device. It may carry make/break data encoded in it. MSC_SCAN is what one need. -- Dmitry - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH] ACPI: thinkpad-acpi: add thinkpad keys to input.h
On 5/29/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: On Mon, 28 May 2007, Dmitry Torokhov wrote: On Sunday 27 May 2007 08:15, Henrique de Moraes Holschuh wrote: On Sat, 26 May 2007, Dmitry Torokhov wrote: I am unconvinced that we need new keycodes. Isn't there a better default keycodes for these keys? You mentioned that fn+f5 controls radio on many thinkpads, why don't you use KEY_WLAN in your keymap? No can do the KEY-WLAN thing, sorry. I *don't* have a way to know, unless I add a model-specific map table to the kernel, and I have been told by numerous people to don't even try, unless it is for quirks, etc. Why not? It thinkpad-acpi is a box-specific driver and you could try to setup proper keymap depending on models. We do that in wistron_btns and it doers not even need alot of memory (keymaps and dmi data is marked __initdata and is discarded). Well, I will try, let's see if ACPI upstream will take it after I tell them it was decided to be the best approach by the input layer people. Yes, it can be __initdata, so it should not cause any drawbacks. But I will still need to add keys, and I still think that a bunch of 32 or so HOSTSPECIFIC keys is a very very good idea to have, *even* if I add some model specific knowledge and already remap a few of the hot key keyboard to less generic events where possible. I think that adding anything like HOSTSPECIFIC is a failure on our part. That means that you need to make programs out there aware of multiple hosts and their usage. You can't just say that you going to teach X and the rest will use X facilities because there is world outside of X. Programs like HAL, network management (rfkill) other daemons getting input directly from /dev/input/eventX will have to be made aware. This is not good. I really don't like KEY_FN_F1..KEY_FN_BACKSPACE either. What are they supposed to do? Just being an unique value to be mapped onto something useful? But why not use that useful keycode to begin with? I'd rather leave the keys unmapped and rely on initsripts (possibly with help from distributions vendors) to load proper keymap then add something that must be retranslated over and over again. Really, what are we to do with that input.h scancode map? It *IS* supposed to be absolute, i.e. one is not supposed to reuse keys in there if the functionality *or* the generic description is not an exact match. Are there any markings on those keys? Only a few of them. And the ones I wanted to add are *not* marked in most models :-) The markings change a bit from model to model, and we have a *very* incomplete list of those... Well, what kind of functions you would like them to have? You, as a maintainer, can chose defaults. Since you (well, not you, the driver) provide a way for a user to adjust keymap there should be no problem even if someone does not like the values you chose. Having sensible defaults is a good thing, otherwise many people will not even know that they have these separate keys. Alternatively, if you let me add keys, I will need a few of them, and they are also generic (not necessarily thinkpad-specific). Stuff like: KEY_FN_BACKSPACE, KEY_VENDORHOMEPAGE, And what are their intended functions would be? How KEY_VENDORHOMEPAGE is different from KEY_HOMEPAGE? -- Dmitry - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH] ACPI: thinkpad-acpi: add thinkpad keys to input.h
On 5/30/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: On Wed, 30 May 2007, Dmitry Torokhov wrote: On 5/29/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: But I will still need to add keys, and I still think that a bunch of 32 or so HOSTSPECIFIC keys is a very very good idea to have, *even* if I add some model specific knowledge and already remap a few of the hot key keyboard to less generic events where possible. I think that adding anything like HOSTSPECIFIC is a failure on our part. That means that you need to make programs out there aware of Well, you have to choose one of three possibilities for an unlabelled key: 1. Generate SOMETHING that has an undefined meaning or function, but which is unique for that keyboard (KEY_PROG/KEY_HOSTSPECIFIC) How do you guarantee that KEY_PROG* is unique for the keyboard? What do you do if you have 2 devices generating KEY_PROG1? 2. Generate SOMETHING that has a non-specific function, but a well defined meaning (KEY_FN_F1) And we have plenty of those. 3. Do nothing. Well, probably not nothing. Map it to KEY_UNKNOWN and have userspace listen to such events and inform user when it presses such a key that such and such happened and tell him how to map it to something useful. I *REALLY* do not like (3), and so far I have not seen a single good technical reason to go with it. Reasons: do not require expaning current keymap preserving space for more useful keycodes. From the technically sound ones, you need to either have the keycodes you need for (2) (i.e. KEY_FN_BACKSPACE), or a big enough set of keycodes to use (1) (i.e. KEY_PROG5..KEY_PROGn, where n should probably be at least 8). Why 8? Why not 16? Or 32 just to make sure? I really don't like KEY_FN_F1..KEY_FN_BACKSPACE either. What are they supposed to do? Just being an unique value to be mapped onto something useful? But why not use that useful keycode to begin with? Yes, just an unique value to be mapped onto something useful, by something in userspace. Just like KEY_PROG, actually. In _every_ program that gets events directly? One can't use a useful keycode for two reasons: 1. Because the key has no set function (it is unmarked) 2. Because it has, or could have, a set function, but we have no clue which is it. KEY_UNKNOWN then. I'd rather leave the keys unmapped and rely on initsripts (possibly with help from distributions vendors) to load proper keymap then add something that must be retranslated over and over again. I don't. I can live with it, of course, but if we are going to go that way, what IS the excuse for a big lot of the keys already in input.h? We have been adding the unique keycodes and functional keycodes because it is *useful*. Because they most of them describe an expected _action_ that would happend after keypress. [...skipped...] And what are their intended functions would be? How KEY_VENDORHOMEPAGE is different from KEY_HOMEPAGE? KEY_VENDORHOMEPAGE is exactly that. It is marked with the vendor's name. KEY_HOMEPAGE has a defined function inherited from that other O.S. which is to open up the default browser on the default homepage. I can easily see both keys existing on a system. OK, right now we have: KEY_WWW KEY_VENDOR KEY_HOMEPAGE defines in input.h. Are you sayign that none of these would suit? As for stuff like KEY_FN_BACKSPACE, well, I don't really care, as long as I have *something* unique and not incorrect to use. But if we are not going to add extra KEY_FN_ keycodes, why don't we just convert them all to KEY_PROG, so that they can be useful to all and not just to people who have FN_whatever keys? We could add aliases if you really want... Can you tell me on _your_ thinkpad what markings you have? I mean there should be a common pattern on thinkpads and the rest may be guessed (you mentioned that FN-F5 is used to turn off radio quite often so if thinkpad driver detects radio switch it makes sence to just go ahead and mark FN-F5 as KEY_WLAN, doesn't it?) -- Dmitry - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH] ACPI: thinkpad-acpi: allow hotkey to input event map to be modified
Hi, On Sunday 27 May 2007 08:03, Henrique de Moraes Holschuh wrote: On Sat, 26 May 2007, Dmitry Torokhov wrote: On Saturday 26 May 2007 13:31, Henrique de Moraes Holschuh wrote: Add a sysfs interface to allow userspace to modify the mapping between ThinkPad hotkeys and the keycode input events they generate. No, please do not do that. We have a standard way to adjust keymap for an input device via EVIOCGKEYCODE/EVIOCSKEYCODE ioctls on corresponding event device; there is no need to invent another interface. Just define getkeycodes() and setkeycode() methods for your input device and be done with it. Sure, I will change to the IOCTLs. Such stuff is exactly why I sent out a partway-done don't merge it yet patch set: I had a hunch that the code would need some changes as the documentation on how to use the input device in-kernel API is worth very little. IMO if there is an API that is dedicated to drivers (and not, say, kernel core), full documentation and keeping it up-to-date should be non-negotiable requirements for the initial merge in mainline, and any subsequent patches that touch it. Oh well. I am reading the corgikbd driver now, it looks sane enough to use as documentation. Documentation/input/input-programming.txt gives some pointers. On that topic, am I to send SYNC events between key-press and key-release events? Yes. The application is allowed to accumulate input events until it gets EV_SYN/SYN_REPORT so if you don't send sync in-between some applications may not see a button press. Just after a key-press+key release event? Or not at all for a EV_KEY ? And exactly what should go in the hardware port descriptor? I used module name/input device number relative to the module. Are you talking about phys? Then it is supposed to have an unique path to the input device. For devices on BUS_HOST module name + relative input device numbers seems to be the best choice. -- Dmitry - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH] ACPI: thinkpad-acpi: add thinkpad keys to input.h
On Sunday 27 May 2007 08:15, Henrique de Moraes Holschuh wrote: On Sat, 26 May 2007, Dmitry Torokhov wrote: On Saturday 26 May 2007 13:31, Henrique de Moraes Holschuh wrote: Add hotkeys available in almost all ThinkPads manufactured in the last five years (more than one million machines given the ammount of batteries recalled) to input.h, and make thinkpad-acpi use those instead of issuing KEY_UNKNOWN. KEY_FN_PAGEDOWN is not ever reported by the ThinkPad firmware due to random bogon-induced stupidity at IBM some years ago, but it is provided because it doesn't make sense to define KEY_FN_PAGEUP and not define KEY_FN_PAGEDOWN at the same time. I am unconvinced that we need new keycodes. Isn't there a better default keycodes for these keys? You mentioned that fn+f5 controls radio on many thinkpads, why don't you use KEY_WLAN in your keymap? No can do the KEY-WLAN thing, sorry. I *don't* have a way to know, unless I add a model-specific map table to the kernel, and I have been told by numerous people to don't even try, unless it is for quirks, etc. Why not? It thinkpad-acpi is a box-specific driver and you could try to setup proper keymap depending on models. We do that in wistron_btns and it doers not even need alot of memory (keymaps and dmi data is marked __initdata and is discarded). Really, what are we to do with that input.h scancode map? It *IS* supposed to be absolute, i.e. one is not supposed to reuse keys in there if the functionality *or* the generic description is not an exact match. Are there any markings on those keys? This is extremely clear, and it makes complete sense from the point of view of userland: HAL and others can properly assign functions to all scan codes and it will be always correct. Are you arguing for KEY_THINKPAD_FN_F1, etc? And it being different from KEY_ACER_FN_F1? I don't think it is a good idea... But then, it is expensive memory-wise, so it is near the current KEY_MAX, and people are very, very relutant to add another bit to it. Not really expensive. Right now keys cost 128 bytes per input device, absolute axis data cost much more. If we really need it we could increase KEY_MAX. This is definately a ridiculous and aggravating situation, that deserves a proper fix. If increasing the scancode table cannot be done (why?), it is time to stop denying reality, and remove everything after KEY_FN (0x1d0), and instead give us a block of KEY_HOSTSPECIFIC_* scancodes, from 0x1d0 to at least 0x1ef. Given the way that scancode table is being used, it is one way or the other. Either increase it to KEY_MAX=0x3ff, or do exactly what UNICODE did, give us a decently sized block of host-specific scancodes, and shunt the problem to userspace to clean up after. In this case I better do nothing and leave everything as KEY_RESERVED and let userspace load proper keymap for the device. -- Dmitry - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] Adding rfkill support to thinkpad_acpi
On Monday 21 May 2007 13:22, Henrique de Moraes Holschuh wrote: On Mon, 21 May 2007, Dmitry Torokhov wrote: RFkill is not only about please disable radios, it _does_ provide the standard sysfs interface to control the radios as well. To control? Fine, if it was really controlling them. The problem is that thinkpad-acpi needs it to *interface* with the firmware that really controls the radios: it is not like thinkpad-acpi or the kernel is in *control* of the radios. Ok, it interfaces the firmware that controls the radio in case of thinkpad_acpi. Is that precise enough? My issue with rfkill is that if the user queries the status of the rfkill switch (for what thinkpad-acpi would need it for), he should get the status of the rfkill switch in hardware, and not of whatever the kernel thinks it is. You know I got curious and looked at thinkpad_acpi driver and adding rfkill support for bluetooth to it turned out to be pretty easy. What It is easy, indeed... until the user slides the hardware rfkill switch to kill it. Now the kernel thinks the thing is on, and it has died. Etc. And *nothing* you do can get the radio back on, unless the user slides the hardware switch back to the on position, at which point it is anyone's guess if the firmware in a given thinkpad will turn it ON, or leave it off if it was off in software... And once you implement a polling input device for your slider that should work too. I know you don't like polling. But what you seem to be missing is that you still need to notify userspace that your radio state is changed so you need to poll anyway. -- Dmitry - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] Adding rfkill support to thinkpad_acpi
On 5/22/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: On Tue, 22 May 2007, Richard Hughes wrote: Yes, if there is no event on kill switch, I agree. We need a way so AFAIK one can poll it, but since it is in the EC space, it is a we don't have events thing. If someone finds a QXX DSDT method or somesuch we can trap, then we will have events for it. userspace knows about the change, and pops up a NetworkManager-applet libnotify You've switched off the wireless rather than just ramping the power to 100% and then failing to connect to networks. I can work with that, yes. In fact I have nothing against it, but current rfkill does not allow for the above. RFKill sure does not. Rfkill does not support buttons, sliders, knobs or anything else. That is what input layer for. Userspace is supposed to listen to kEY_WLAN/KEY_BLUETOOTH events and do its things. Alernatively rfkill-input will do smple on/off toggling on switches that userspace did not claim. How about polling twice a second? That should cause no appreciable load increase and give userspace a chance to do something clever. How about polling twice a second, PLUS a notification from rfkill every time something wants to read an attribute, so that we nearly close the window of opportunity where the status is incorrect? That's what I am asking of rfkill, but they are very reluctant to implement. Notification to where? The driver? Userspace? Does delay in status reading of ~0.5 sec matter? You are concerned with the load on the EC caused by constant polling but are you OK with apllications readig sysfs attributes constantly (rfkill attributs are exported 0644 or 0444 so anyone could read them)? Do you want the driver to do throttling? Do you want rfkill to implement throttling? Driver specific? All in all it is alot of pain for no reeal gain. -- Dmitry - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] How to handle WWAN card, together with WLAN or separate?
On Tuesday 08 May 2007 22:08, Henrique de Moraes Holschuh wrote: The rfkill class has been merged in Linux mainline, and if at all possible, thinkpad-acpi will use it to connect or disconnect the internal bluetooth and WWAN devices. The rfkill class has a concept of type of the radio, which is used to, e.g., turn on or off all radios of the same type. This is coupled to the input subsystem, so that the kernel can switch on/off all radios of a given type if KEY_WLAN or KEY_BLUETOOTH is pressed... (yes, I will add that to thinkpad-acpi shortly). This behaviour can be overriden on a card-by-card basis. Now, I can add a new WWAN type for the WWAN card, or I can group it with WLAN. What I am *not* going to do is to request that KEY_WWAN be added to the input subsystem, so if I add a new type, this means there will be no way to bind (in kernel) a key to a WWAN radio on/off software switch, and the usual scripts in userspace will be needed. It makes sense to group it with other WLAN rfkill switches, and thinkpads only have a single key (typically fn+f5) which is supposed to control all radios anyway, so I am inclined to do just that. Comments? I would add a new switch type and adjust rfkill-input to toggle both WLAN and WWAN switches when user presses KEY_WLAN. More fine-grained control can be done in userspace. Does this make sense? -- Dmitry - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
