| Message ID | 1a034599-ff6c-4a42-8252-4f6007894f4a@rwthex-w2-a.rwth-ad.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote: > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but > not > on BIOS A2). > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > > Changes in v2: > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY > - Use separate up/down events > > drivers/platform/x86/intel-vbtn.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c > index e3f6375af85c..a484bcc6393b 100644 > --- a/drivers/platform/x86/intel-vbtn.c > +++ b/drivers/platform/x86/intel-vbtn.c > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = { > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key release */ > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key press */ > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } }, /* volume-down key release */ > + { KE_KEY, 0xC8, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key press */ > + { KE_KEY, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key release */ How are those events sent? When pressing and releasing the key, do you receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and releasing the first time, and 0xC9 when pressing and releasing a second time? If the former, then it's not going to work. The release is supposed to be ignored, as you send the event with sparse_keymap_report_event(). If the latter, and there's an actual state, does it disable a device on-board, or activate an LED? If so, then it would need to be a switch, not a key. > { KE_SW, 0xCC, { .sw = { SW_TABLET_MODE, 1 } } }, /* Tablet */ > { KE_SW, 0xCD, { .sw = { SW_TABLET_MODE, 0 } } }, /* Laptop */ > { KE_END }, -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 10, 2017 at 12:30:46AM +0100, Bastien Nocera wrote: > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote: > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but > > not > > on BIOS A2). > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > --- > > > > Changes in v2: > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY > > - Use separate up/down events > > > > drivers/platform/x86/intel-vbtn.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c > > index e3f6375af85c..a484bcc6393b 100644 > > --- a/drivers/platform/x86/intel-vbtn.c > > +++ b/drivers/platform/x86/intel-vbtn.c > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = { > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key release */ > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key press */ > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } }, /* volume-down key release */ > > + { KE_KEY, 0xC8, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key press */ > > + { KE_KEY, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key release */ > > How are those events sent? When pressing and releasing the key, do you > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and > releasing the first time, and 0xC9 when pressing and releasing a second > time? > > If the former, then it's not going to work. The release is supposed to > be ignored, as you send the event with sparse_keymap_report_event(). I expect the former, which is consistent with the volume keys preceding it (also ignoring the release).
On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote: > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote: > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but > > not > > on BIOS A2). > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > --- > > > > Changes in v2: > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY > > - Use separate up/down events > > > > drivers/platform/x86/intel-vbtn.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/platform/x86/intel-vbtn.c > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b > > 100644 > > --- a/drivers/platform/x86/intel-vbtn.c > > +++ b/drivers/platform/x86/intel-vbtn.c > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = { > > > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key release */ > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key press */ > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } }, /* volume-down key release */ > > > > + { KE_KEY, 0xC8, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key > > press */ + { KE_KEY, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } }, /* > > rotate-lock key release */ > How are those events sent? When pressing and releasing the key, do you > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and > releasing the first time, and 0xC9 when pressing and releasing a second > time? > > If the former, then it's not going to work. The release is supposed to > be ignored, as you send the event with sparse_keymap_report_event(). > > If the latter, and there's an actual state, does it disable a device > on-board, or activate an LED? If so, then it would need to be a switch, > not a key. Do you think I don't test the patches before sending? Let me tell you, it *does* work. You could also read the cover letter, where you find more details, putting the patches in relation to each other. Just in case its not yet clear: The codes are emitted when pressing a button. It is a button, not a switch. There is no state handled in hardware. On press (as noted by the code comment), event code 0xc8 is emitted. On release, event code 0xc9 is emitted. Regards, Stefan
On Friday, November 10, 2017 12:46:30 AM CET Darren Hart wrote: > On Fri, Nov 10, 2017 at 12:30:46AM +0100, Bastien Nocera wrote: > > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote: > > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but > > > not > > > on BIOS A2). > > > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > > --- > > > > > > Changes in v2: > > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY > > > - Use separate up/down events > > > > > > drivers/platform/x86/intel-vbtn.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/platform/x86/intel-vbtn.c > > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b > > > 100644 > > > --- a/drivers/platform/x86/intel-vbtn.c > > > +++ b/drivers/platform/x86/intel-vbtn.c > > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = { > > > > > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key release */ > > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key press */ > > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } }, /* volume-down key release */ > > > > > > + { KE_KEY, 0xC8, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key > > > press */ + { KE_KEY, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } }, /* > > > rotate-lock key release */> > > How are those events sent? When pressing and releasing the key, do you > > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and > > releasing the first time, and 0xC9 when pressing and releasing a second > > time? > > > > If the former, then it's not going to work. The release is supposed to > > be ignored, as you send the event with sparse_keymap_report_event(). > > I expect the former, which is consistent with the volume keys preceding it > (also ignoring the release). Read the whole series and the cover letter, and stop making assumptions. Regards, Stefan
On Fri, 2017-11-10 at 01:15 +0100, Stefan Brüns wrote: > On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote: > > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote: > > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, > > > but > > > not > > > on BIOS A2). > > > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > > --- > > > > > > Changes in v2: > > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY > > > - Use separate up/down events > > > > > > drivers/platform/x86/intel-vbtn.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/platform/x86/intel-vbtn.c > > > b/drivers/platform/x86/intel-vbtn.c index > > > e3f6375af85c..a484bcc6393b > > > 100644 > > > --- a/drivers/platform/x86/intel-vbtn.c > > > +++ b/drivers/platform/x86/intel-vbtn.c > > > @@ -42,6 +42,8 @@ static const struct key_entry > > > intel_vbtn_keymap[] = { > > > > > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* > > > volume-up key release */ > > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* > > > volume-down key press */ > > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } }, /* > > > volume-down key release > > */ > > > > > > + { KE_KEY, 0xC8, { KEY_ROTATE_LOCK_TOGGLE } }, > > > /* rotate-lock key > > > press */ + { KE_KEY, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } > > > }, /* > > > rotate-lock key release */ > > > > How are those events sent? When pressing and releasing the key, do > > you > > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing > > and > > releasing the first time, and 0xC9 when pressing and releasing a > > second > > time? > > > > If the former, then it's not going to work. The release is supposed > > to > > be ignored, as you send the event with > > sparse_keymap_report_event(). > > > > If the latter, and there's an actual state, does it disable a > > device > > on-board, or activate an LED? If so, then it would need to be a > > switch, > > not a key. > > Do you think I don't test the patches before sending? Let me tell > you, it > *does* work. > > You could also read the cover letter, where you find more details, > putting the > patches in relation to each other. I guess this was so clear that Darren made the same assumption as me. I also never said that it wouldn't work, which is why I used "pretty sure", and asked questions. > Just in case its not yet clear: > The codes are emitted when pressing a button. It is a button, not a > switch. > There is no state handled in hardware. On press (as noted by the > code > comment), event code 0xc8 is emitted. On release, event code 0xc9 is > emitted. From the looks of things, some devices only send the keypresses, and never the key release, otherwise what would be the point of ignoring the key release in that table, say for the volume buttons? It's clear as mud, and there's no comments in the driver to explain that. If you don't want to answer questions, or are going to be as pissy as your replies have been, I'm happy to not continue reviewing your patches. I find patch 2 unreadable, but it might be my tiny tiny little brain. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 10, 2017 at 01:15:09AM +0100, Stefan Brüns wrote: > On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote: > > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote: > > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but > > > not > > > on BIOS A2). > > > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > > --- > > > > > > Changes in v2: > > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY > > > - Use separate up/down events > > > > > > drivers/platform/x86/intel-vbtn.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/platform/x86/intel-vbtn.c > > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b > > > 100644 > > > --- a/drivers/platform/x86/intel-vbtn.c > > > +++ b/drivers/platform/x86/intel-vbtn.c > > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = { > > > > > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key release */ > > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key press */ > > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } }, /* volume-down key release > */ > > > > > > + { KE_KEY, 0xC8, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key > > > press */ + { KE_KEY, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } }, /* > > > rotate-lock key release */ > > How are those events sent? When pressing and releasing the key, do you > > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and > > releasing the first time, and 0xC9 when pressing and releasing a second > > time? > > > > If the former, then it's not going to work. The release is supposed to > > be ignored, as you send the event with sparse_keymap_report_event(). > > > > If the latter, and there's an actual state, does it disable a device > > on-board, or activate an LED? If so, then it would need to be a switch, > > not a key. > > Do you think I don't test the patches before sending? Let me tell you, it > *does* work. > > You could also read the cover letter, where you find more details, putting the > patches in relation to each other. Dude, Knock it off. Unacceptable. The number of patches we receive which have not seen any testing are higher than you might think. Bastien took the time to review and ask questions in the context of his experience with patches dealing with input. These are very reasonable and necessary questions given we cannot independently verify patches for most of these platforms.
On Fri, Nov 10, 2017 at 01:15:09AM +0100, Stefan Brüns wrote: > On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote: > > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote: > > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but > > > not > > > on BIOS A2). > > > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > > --- > > > > > > Changes in v2: > > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY > > > - Use separate up/down events > > > > > > drivers/platform/x86/intel-vbtn.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/platform/x86/intel-vbtn.c > > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b > > > 100644 > > > --- a/drivers/platform/x86/intel-vbtn.c > > > +++ b/drivers/platform/x86/intel-vbtn.c > > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = { > > > > > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key release */ > > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key press */ > > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } }, /* volume-down key release > */ > > > > > > + { KE_KEY, 0xC8, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key > > > press */ + { KE_KEY, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } }, /* > > > rotate-lock key release */ > > How are those events sent? When pressing and releasing the key, do you > > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and > > releasing the first time, and 0xC9 when pressing and releasing a second > > time? > > > > If the former, then it's not going to work. The release is supposed to > > be ignored, as you send the event with sparse_keymap_report_event(). > > > > If the latter, and there's an actual state, does it disable a device > > on-board, or activate an LED? If so, then it would need to be a switch, > > not a key. > > Do you think I don't test the patches before sending? Let me tell you, it > *does* work. > > You could also read the cover letter, where you find more details, putting the > patches in relation to each other. A point of process. If there is context that is needed to explain the patch, it belongs in the patch, not just in the cover letter. The cover letter is effectively lost once the patches are merged. > > Just in case its not yet clear: > The codes are emitted when pressing a button. It is a button, not a switch. > There is no state handled in hardware. On press (as noted by the code > comment), event code 0xc8 is emitted. On release, event code 0xc9 is emitted. This sounds like the "former" scenario Bastien described, for which I understand the use case to be: User presses and releases the rotate lock button to prevent the accelerometer for triggering screen rotation. User presses and releases the rotate lock button to allow the accelerometer to trigger screen rotation. Is that correct? If so, why do we need to emit two KEY_ROTATE_LOCK_TOGGLE events each time instead of just the press event like the volume buttons?
On Friday, November 10, 2017 2:54:22 AM CET Darren Hart wrote: > On Fri, Nov 10, 2017 at 01:15:09AM +0100, Stefan Brüns wrote: > > On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote: > > > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote: > > > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but > > > > not > > > > on BIOS A2). > > > > > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > > > --- > > > > > > > > Changes in v2: > > > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY > > > > - Use separate up/down events > > > > > > > > drivers/platform/x86/intel-vbtn.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/platform/x86/intel-vbtn.c > > > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b > > > > 100644 > > > > --- a/drivers/platform/x86/intel-vbtn.c > > > > +++ b/drivers/platform/x86/intel-vbtn.c > > > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = > > > > { > > > > > > > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key release */ > > > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key press */ > > > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } }, /* volume-down key release > > > > */ > > > > > > + { KE_KEY, 0xC8, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key > > > > press */ + { KE_KEY, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } }, /* > > > > rotate-lock key release */ > > > > > > How are those events sent? When pressing and releasing the key, do you > > > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and > > > releasing the first time, and 0xC9 when pressing and releasing a second > > > time? > > > > > > If the former, then it's not going to work. The release is supposed to > > > be ignored, as you send the event with sparse_keymap_report_event(). > > > > > > If the latter, and there's an actual state, does it disable a device > > > on-board, or activate an LED? If so, then it would need to be a switch, > > > not a key. > > > > Do you think I don't test the patches before sending? Let me tell you, it > > *does* work. > > > > You could also read the cover letter, where you find more details, putting > > the patches in relation to each other. > > A point of process. If there is context that is needed to explain the > patch, it belongs in the patch, not just in the cover letter. The cover > letter is effectively lost once the patches are merged. > > > Just in case its not yet clear: > > The codes are emitted when pressing a button. It is a button, not a > > switch. > > There is no state handled in hardware. On press (as noted by the code > > comment), event code 0xc8 is emitted. On release, event code 0xc9 is > > emitted. > This sounds like the "former" scenario Bastien described, for which I > understand the use case to be: > > User presses and releases the rotate lock button to prevent the > accelerometer for triggering screen rotation. > > User presses and releases the rotate lock button to allow the accelerometer > to trigger screen rotation. > > Is that correct? > > If so, why do we need to emit two KEY_ROTATE_LOCK_TOGGLE events each time > instead of just the press event like the volume buttons? Volume buttons *should* send separate press/release events, to allow software autorepeat. For the rotate lock button, I see no reason *not* to report the actual state instead of doing an autorelease. Kind regards, Stefan
diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b 100644 --- a/drivers/platform/x86/intel-vbtn.c +++ b/drivers/platform/x86/intel-vbtn.c @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = { { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key release */ { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key press */ { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } }, /* volume-down key release */ + { KE_KEY, 0xC8, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key press */ + { KE_KEY, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } }, /* rotate-lock key release */ { KE_SW, 0xCC, { .sw = { SW_TABLET_MODE, 1 } } }, /* Tablet */ { KE_SW, 0xCD, { .sw = { SW_TABLET_MODE, 0 } } }, /* Laptop */ { KE_END },
The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but not on BIOS A2). Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- Changes in v2: - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY - Use separate up/down events drivers/platform/x86/intel-vbtn.c | 2 ++ 1 file changed, 2 insertions(+)