diff mbox series

[2/2] Input: soc_button_array - Fix mapping of the 5th GPIO in a PNP0C40 device

Message ID 20190103232556.17965-2-hdegoede@redhat.com (mailing list archive)
State Mainlined
Commit e9eb788f9442d1b5d93efdb30c3be071ce8a22b1
Headers show
Series [1/2] Input: soc_button_array - Add usage-page 0x01 usage-id 0xca mapping | expand

Commit Message

Hans de Goede Jan. 3, 2019, 11:25 p.m. UTC
The Microsoft documenation for the PNP0C40 device aka the
"Windows-compatible button array" describes the 5th GpioInt listed in
the resources as: '5. Interrupt corresponding to the "Rotation Lock"
button, if supported'.

Notice this describes the 5th entry as a button while we sofar have been
mapping it to EV_SW, SW_ROTATE_LOCK. On my Point of View TAB P1006W-232
which actually comes with a rotation-lock button, the button indeed is a
button and not a slider/switch. An image search for other Windows tablets
has found 2 more models with a rotation-lock button and on both of those
it too is a push-button and not a slider/switch.

Further evidence can be found in the HUT extension HUTRR52 from Microsoft
which adds rotation lock support to the HUT, which describes 2 different
usages: "0xC9 System Display Rotation Lock Button" and
"0xCA System Display Rotation Lock Slider Switch" note that switch is seen
as a separate thing here and the non switch wording is an exact match for
the "Windows-compatible button array" spec wording.

TL;DR: our current mapping of the 5th GPIO to SW_ROTATE_LOCK is wrong
because the 5th GPIO is for a push-button not a switch.

This commit fixes this by maping the 5th GPIO to KEY_ROTATE_LOCK_TOGGLE.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
The referenced documents can currently be found here:
https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects
https://www.usb.org/sites/default/files/hutrr52_system_display_rotation_lock_controls_0.pdf
---
 drivers/input/misc/soc_button_array.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Benjamin Tissoires Jan. 7, 2019, 9:58 a.m. UTC | #1
On Fri, Jan 4, 2019 at 12:26 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The Microsoft documenation for the PNP0C40 device aka the
> "Windows-compatible button array" describes the 5th GpioInt listed in
> the resources as: '5. Interrupt corresponding to the "Rotation Lock"
> button, if supported'.
>
> Notice this describes the 5th entry as a button while we sofar have been
> mapping it to EV_SW, SW_ROTATE_LOCK. On my Point of View TAB P1006W-232
> which actually comes with a rotation-lock button, the button indeed is a
> button and not a slider/switch. An image search for other Windows tablets
> has found 2 more models with a rotation-lock button and on both of those
> it too is a push-button and not a slider/switch.
>
> Further evidence can be found in the HUT extension HUTRR52 from Microsoft
> which adds rotation lock support to the HUT, which describes 2 different
> usages: "0xC9 System Display Rotation Lock Button" and
> "0xCA System Display Rotation Lock Slider Switch" note that switch is seen
> as a separate thing here and the non switch wording is an exact match for
> the "Windows-compatible button array" spec wording.
>
> TL;DR: our current mapping of the 5th GPIO to SW_ROTATE_LOCK is wrong
> because the 5th GPIO is for a push-button not a switch.
>
> This commit fixes this by maping the 5th GPIO to KEY_ROTATE_LOCK_TOGGLE.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> The referenced documents can currently be found here:
> https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects
> https://www.usb.org/sites/default/files/hutrr52_system_display_rotation_lock_controls_0.pdf

We should probably add those links to the commit description.
They sometimes disappear, but that will help for regressions in the
short term if there are any.

Both patches are:
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> ---
>  drivers/input/misc/soc_button_array.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
> index f53923b1593b..bb458beecb43 100644
> --- a/drivers/input/misc/soc_button_array.c
> +++ b/drivers/input/misc/soc_button_array.c
> @@ -377,7 +377,7 @@ static struct soc_button_info soc_button_PNP0C40[] = {
>         { "home", 1, EV_KEY, KEY_LEFTMETA, false, true },
>         { "volume_up", 2, EV_KEY, KEY_VOLUMEUP, true, false },
>         { "volume_down", 3, EV_KEY, KEY_VOLUMEDOWN, true, false },
> -       { "rotation_lock", 4, EV_SW, SW_ROTATE_LOCK, false, false },
> +       { "rotation_lock", 4, EV_KEY, KEY_ROTATE_LOCK_TOGGLE, false, false },
>         { }
>  };
>
> --
> 2.20.1
>
Hans de Goede Jan. 7, 2019, 10:03 a.m. UTC | #2
Hi,

On 07-01-19 10:58, Benjamin Tissoires wrote:
> On Fri, Jan 4, 2019 at 12:26 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The Microsoft documenation for the PNP0C40 device aka the
>> "Windows-compatible button array" describes the 5th GpioInt listed in
>> the resources as: '5. Interrupt corresponding to the "Rotation Lock"
>> button, if supported'.
>>
>> Notice this describes the 5th entry as a button while we sofar have been
>> mapping it to EV_SW, SW_ROTATE_LOCK. On my Point of View TAB P1006W-232
>> which actually comes with a rotation-lock button, the button indeed is a
>> button and not a slider/switch. An image search for other Windows tablets
>> has found 2 more models with a rotation-lock button and on both of those
>> it too is a push-button and not a slider/switch.
>>
>> Further evidence can be found in the HUT extension HUTRR52 from Microsoft
>> which adds rotation lock support to the HUT, which describes 2 different
>> usages: "0xC9 System Display Rotation Lock Button" and
>> "0xCA System Display Rotation Lock Slider Switch" note that switch is seen
>> as a separate thing here and the non switch wording is an exact match for
>> the "Windows-compatible button array" spec wording.
>>
>> TL;DR: our current mapping of the 5th GPIO to SW_ROTATE_LOCK is wrong
>> because the 5th GPIO is for a push-button not a switch.
>>
>> This commit fixes this by maping the 5th GPIO to KEY_ROTATE_LOCK_TOGGLE.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> The referenced documents can currently be found here:
>> https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects
>> https://www.usb.org/sites/default/files/hutrr52_system_display_rotation_lock_controls_0.pdf
> 
> We should probably add those links to the commit description.
> They sometimes disappear, but that will help for regressions in the
> short term if there are any.

That is fine with me, I put them here because some subsystems don't
want URLs in the commit message since they go stale over time.

Do you want me to do a resend with these added to the commit message?

> Both patches are:
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks & Regards,

Hans


>> ---
>>   drivers/input/misc/soc_button_array.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
>> index f53923b1593b..bb458beecb43 100644
>> --- a/drivers/input/misc/soc_button_array.c
>> +++ b/drivers/input/misc/soc_button_array.c
>> @@ -377,7 +377,7 @@ static struct soc_button_info soc_button_PNP0C40[] = {
>>          { "home", 1, EV_KEY, KEY_LEFTMETA, false, true },
>>          { "volume_up", 2, EV_KEY, KEY_VOLUMEUP, true, false },
>>          { "volume_down", 3, EV_KEY, KEY_VOLUMEDOWN, true, false },
>> -       { "rotation_lock", 4, EV_SW, SW_ROTATE_LOCK, false, false },
>> +       { "rotation_lock", 4, EV_KEY, KEY_ROTATE_LOCK_TOGGLE, false, false },
>>          { }
>>   };
>>
>> --
>> 2.20.1
>>
Benjamin Tissoires Jan. 7, 2019, 10:05 a.m. UTC | #3
On Mon, Jan 7, 2019 at 11:03 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 07-01-19 10:58, Benjamin Tissoires wrote:
> > On Fri, Jan 4, 2019 at 12:26 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> The Microsoft documenation for the PNP0C40 device aka the
> >> "Windows-compatible button array" describes the 5th GpioInt listed in
> >> the resources as: '5. Interrupt corresponding to the "Rotation Lock"
> >> button, if supported'.
> >>
> >> Notice this describes the 5th entry as a button while we sofar have been
> >> mapping it to EV_SW, SW_ROTATE_LOCK. On my Point of View TAB P1006W-232
> >> which actually comes with a rotation-lock button, the button indeed is a
> >> button and not a slider/switch. An image search for other Windows tablets
> >> has found 2 more models with a rotation-lock button and on both of those
> >> it too is a push-button and not a slider/switch.
> >>
> >> Further evidence can be found in the HUT extension HUTRR52 from Microsoft
> >> which adds rotation lock support to the HUT, which describes 2 different
> >> usages: "0xC9 System Display Rotation Lock Button" and
> >> "0xCA System Display Rotation Lock Slider Switch" note that switch is seen
> >> as a separate thing here and the non switch wording is an exact match for
> >> the "Windows-compatible button array" spec wording.
> >>
> >> TL;DR: our current mapping of the 5th GPIO to SW_ROTATE_LOCK is wrong
> >> because the 5th GPIO is for a push-button not a switch.
> >>
> >> This commit fixes this by maping the 5th GPIO to KEY_ROTATE_LOCK_TOGGLE.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> The referenced documents can currently be found here:
> >> https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects
> >> https://www.usb.org/sites/default/files/hutrr52_system_display_rotation_lock_controls_0.pdf
> >
> > We should probably add those links to the commit description.
> > They sometimes disappear, but that will help for regressions in the
> > short term if there are any.
>
> That is fine with me, I put them here because some subsystems don't
> want URLs in the commit message since they go stale over time.

That is a policy Dmitry needs to make then :)
But I often prefer having those for the reasons above. At least we
know the docs were available even if the link is broken :)

>
> Do you want me to do a resend with these added to the commit message?

Up to Dmitry again. But if it doesn't bother you, maybe a quick v2
would be fine.

Cheers,
Benjamin

>
> > Both patches are:
> > Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Thanks & Regards,
>
> Hans
>
>
> >> ---
> >>   drivers/input/misc/soc_button_array.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
> >> index f53923b1593b..bb458beecb43 100644
> >> --- a/drivers/input/misc/soc_button_array.c
> >> +++ b/drivers/input/misc/soc_button_array.c
> >> @@ -377,7 +377,7 @@ static struct soc_button_info soc_button_PNP0C40[] = {
> >>          { "home", 1, EV_KEY, KEY_LEFTMETA, false, true },
> >>          { "volume_up", 2, EV_KEY, KEY_VOLUMEUP, true, false },
> >>          { "volume_down", 3, EV_KEY, KEY_VOLUMEDOWN, true, false },
> >> -       { "rotation_lock", 4, EV_SW, SW_ROTATE_LOCK, false, false },
> >> +       { "rotation_lock", 4, EV_KEY, KEY_ROTATE_LOCK_TOGGLE, false, false },
> >>          { }
> >>   };
> >>
> >> --
> >> 2.20.1
> >>
Dmitry Torokhov Jan. 7, 2019, 7 p.m. UTC | #4
On Mon, Jan 07, 2019 at 11:05:23AM +0100, Benjamin Tissoires wrote:
> On Mon, Jan 7, 2019 at 11:03 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > On 07-01-19 10:58, Benjamin Tissoires wrote:
> > > On Fri, Jan 4, 2019 at 12:26 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > >>
> > >> The Microsoft documenation for the PNP0C40 device aka the
> > >> "Windows-compatible button array" describes the 5th GpioInt listed in
> > >> the resources as: '5. Interrupt corresponding to the "Rotation Lock"
> > >> button, if supported'.
> > >>
> > >> Notice this describes the 5th entry as a button while we sofar have been
> > >> mapping it to EV_SW, SW_ROTATE_LOCK. On my Point of View TAB P1006W-232
> > >> which actually comes with a rotation-lock button, the button indeed is a
> > >> button and not a slider/switch. An image search for other Windows tablets
> > >> has found 2 more models with a rotation-lock button and on both of those
> > >> it too is a push-button and not a slider/switch.
> > >>
> > >> Further evidence can be found in the HUT extension HUTRR52 from Microsoft
> > >> which adds rotation lock support to the HUT, which describes 2 different
> > >> usages: "0xC9 System Display Rotation Lock Button" and
> > >> "0xCA System Display Rotation Lock Slider Switch" note that switch is seen
> > >> as a separate thing here and the non switch wording is an exact match for
> > >> the "Windows-compatible button array" spec wording.
> > >>
> > >> TL;DR: our current mapping of the 5th GPIO to SW_ROTATE_LOCK is wrong
> > >> because the 5th GPIO is for a push-button not a switch.
> > >>
> > >> This commit fixes this by maping the 5th GPIO to KEY_ROTATE_LOCK_TOGGLE.
> > >>
> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >> ---
> > >> The referenced documents can currently be found here:
> > >> https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects
> > >> https://www.usb.org/sites/default/files/hutrr52_system_display_rotation_lock_controls_0.pdf
> > >
> > > We should probably add those links to the commit description.
> > > They sometimes disappear, but that will help for regressions in the
> > > short term if there are any.
> >
> > That is fine with me, I put them here because some subsystems don't
> > want URLs in the commit message since they go stale over time.
> 
> That is a policy Dmitry needs to make then :)
> But I often prefer having those for the reasons above. At least we
> know the docs were available even if the link is broken :)

"'Tis better to have loved and lost, / Than never to have loved
at all". Even if links go stale there sometimes enough breadcrumbs to
locate the document somewhere on a mirror.

> 
> >
> > Do you want me to do a resend with these added to the commit message?
> 
> Up to Dmitry again. But if it doesn't bother you, maybe a quick v2
> would be fine.

Sorry, I think I already pushed the patches out to my next branch.

Thanks.
diff mbox series

Patch

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index f53923b1593b..bb458beecb43 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -377,7 +377,7 @@  static struct soc_button_info soc_button_PNP0C40[] = {
 	{ "home", 1, EV_KEY, KEY_LEFTMETA, false, true },
 	{ "volume_up", 2, EV_KEY, KEY_VOLUMEUP, true, false },
 	{ "volume_down", 3, EV_KEY, KEY_VOLUMEDOWN, true, false },
-	{ "rotation_lock", 4, EV_SW, SW_ROTATE_LOCK, false, false },
+	{ "rotation_lock", 4, EV_KEY, KEY_ROTATE_LOCK_TOGGLE, false, false },
 	{ }
 };