| Message ID | 20190702003740.75970-2-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support for buttons on newer MS Surface devices | expand |
Hi, On Tue, Jul 02, 2019 at 02:37:39AM +0200, Maximilian Luz wrote: > Do not use the surfacepro3_button driver on newer Microsoft Surface > models, only use it on the Surface Pro 3 and 4. Newer models (5th, 6th > and possibly future generations) use the same device as the Surface Pro > 4 to represent their volume and power buttons (MSHW0040), but their > acutal implementation is significantly different. This patch ensures > that the surfacepro3_button driver is only used on the Pro 3 and 4 > models, allowing a different driver to bind on other models. > This method overall looks ok to me. > Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> > --- > drivers/platform/x86/surfacepro3_button.c | 38 +++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/drivers/platform/x86/surfacepro3_button.c b/drivers/platform/x86/surfacepro3_button.c > index 47c6d000465a..0e2c7dfafd9f 100644 > --- a/drivers/platform/x86/surfacepro3_button.c > +++ b/drivers/platform/x86/surfacepro3_button.c > @@ -20,6 +20,12 @@ > #define SURFACE_BUTTON_OBJ_NAME "VGBI" > #define SURFACE_BUTTON_DEVICE_NAME "Surface Pro 3/4 Buttons" > > +#define MSHW0040_DSM_REVISION 0x01 > +#define MSHW0040_DSM_GET_OMPR 0x02 // get OEM Platform Revision > +static const guid_t MSHW0040_DSM_UUID = > + GUID_INIT(0x6fd05c69, 0xcde3, 0x49f4, 0x95, 0xed, 0xab, 0x16, 0x65, > + 0x49, 0x80, 0x35); > + > #define SURFACE_BUTTON_NOTIFY_TABLET_MODE 0xc8 > > #define SURFACE_BUTTON_NOTIFY_PRESS_POWER 0xc6 > @@ -142,6 +148,34 @@ static int surface_button_resume(struct device *dev) > } > #endif > > +/* > + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device > + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right > + * device by checking for the _DSM method and OEM Platform Revision. > + */ > +static int surface_button_check_MSHW0040(struct acpi_device *dev) > +{ > + acpi_handle handle = dev->handle; > + union acpi_object *result; > + u64 oem_platform_rev = 0; > + > + // get OEM platform revision > + result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID, > + MSHW0040_DSM_REVISION, > + MSHW0040_DSM_GET_OMPR, > + NULL, ACPI_TYPE_INTEGER); > + Does it mean, only 5th, 6th and newer platforms have OEM platform revision? 3rd/4th will get NULL result? Or the opposite? > + if (result) { > + oem_platform_rev = result->integer.value; > + ACPI_FREE(result); > + } > + > + dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev); > + > + return oem_platform_rev == 0 ? 0 : -ENODEV; if 3rd/4th do not have this oem rev information while 5th/newer have, why the latter returns NODEV(it actually has this info)? > +} > + > + > static int surface_button_add(struct acpi_device *device) > { > struct surface_button *button; > @@ -154,6 +188,10 @@ static int surface_button_add(struct acpi_device *device) > strlen(SURFACE_BUTTON_OBJ_NAME))) > return -ENODEV; > > + error = surface_button_check_MSHW0040(device); > + if (error) > + return error; > + ditto, 3rd/4th get error=0? > button = kzalloc(sizeof(struct surface_button), GFP_KERNEL); > if (!button) > return -ENOMEM; > -- > 2.22.0 > Best, Yu
On 7/2/19 3:14 AM, Yu Chen wrote: > On Tue, Jul 02, 2019 at 02:37:39AM +0200, Maximilian Luz wrote: >> +/* >> + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device >> + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right >> + * device by checking for the _DSM method and OEM Platform Revision. >> + */ >> +static int surface_button_check_MSHW0040(struct acpi_device *dev) >> +{ >> + acpi_handle handle = dev->handle; >> + union acpi_object *result; >> + u64 oem_platform_rev = 0; >> + >> + // get OEM platform revision >> + result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID, >> + MSHW0040_DSM_REVISION, >> + MSHW0040_DSM_GET_OMPR, >> + NULL, ACPI_TYPE_INTEGER); >> + > Does it mean, only 5th, 6th and newer platforms have OEM platform revision? > 3rd/4th will get NULL result? Or the opposite? Correct, from my testing (with limited sample size) and AML code: 5th and 6th generation devices have a non-zero OEM platform revision, whereas 3rd and 4th gen. devices do not have any (i.e. result will be NULL). >> + if (result) { >> + oem_platform_rev = result->integer.value; >> + ACPI_FREE(result); >> + } >> + >> + dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev); >> + >> + return oem_platform_rev == 0 ? 0 : -ENODEV; > if 3rd/4th do not have this oem rev information while 5th/newer have, > why the latter returns NODEV(it actually has this info)? Since we always expect a non-zero platform revision (for 5th/6th gen.), we can initialize it to zero and use that as "unknown"/"not available". So if it can not be determined, we return NODEV. >> +} Cheers, Maximilian
On 7/2/19 3:25 AM, Maximilian Luz wrote: > On 7/2/19 3:14 AM, Yu Chen wrote: >> On Tue, Jul 02, 2019 at 02:37:39AM +0200, Maximilian Luz wrote: >>> +/* >>> + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device >>> + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right >>> + * device by checking for the _DSM method and OEM Platform Revision. >>> + */ >>> +static int surface_button_check_MSHW0040(struct acpi_device *dev) >>> +{ >>> + acpi_handle handle = dev->handle; >>> + union acpi_object *result; >>> + u64 oem_platform_rev = 0; >>> + >>> + // get OEM platform revision >>> + result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID, >>> + MSHW0040_DSM_REVISION, >>> + MSHW0040_DSM_GET_OMPR, >>> + NULL, ACPI_TYPE_INTEGER); >>> + >> Does it mean, only 5th, 6th and newer platforms have OEM platform revision? >> 3rd/4th will get NULL result? Or the opposite? > > Correct, from my testing (with limited sample size) and AML code: 5th > and 6th generation devices have a non-zero OEM platform revision, > whereas 3rd and 4th gen. devices do not have any (i.e. result will be > NULL). > >>> + if (result) { >>> + oem_platform_rev = result->integer.value; >>> + ACPI_FREE(result); >>> + } >>> + >>> + dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev); >>> + >>> + return oem_platform_rev == 0 ? 0 : -ENODEV; >> if 3rd/4th do not have this oem rev information while 5th/newer have, >> why the latter returns NODEV(it actually has this info)? > > Since we always expect a non-zero platform revision (for 5th/6th gen.), > we can initialize it to zero and use that as "unknown"/"not available". > So if it can not be determined, we return NODEV. Sorry, small mistake here: If it can be determined (i.e. is 5th or 6th gen.) then we return NODEV. Not the other way around. Also to clarify on your last question: On 7/2/19 3:14 AM, Yu Chen wrote: >> static int surface_button_add(struct acpi_device *device) >> { >> struct surface_button *button; >> @@ -154,6 +188,10 @@ static int surface_button_add(struct acpi_device *device) >> strlen(SURFACE_BUTTON_OBJ_NAME))) >> return -ENODEV; >> >> + error = surface_button_check_MSHW0040(device); >> + if (error) >> + return error; >> + > ditto, 3rd/4th get error=0? You are correct. Maximilian
On Tue, Jul 02, 2019 at 03:33:20AM +0200, Maximilian Luz wrote: > On 7/2/19 3:25 AM, Maximilian Luz wrote: > > On 7/2/19 3:14 AM, Yu Chen wrote: > > > On Tue, Jul 02, 2019 at 02:37:39AM +0200, Maximilian Luz wrote: > > > > +/* > > > > + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device > > > > + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right > > > > + * device by checking for the _DSM method and OEM Platform Revision. > > > > + */ > > > > +static int surface_button_check_MSHW0040(struct acpi_device *dev) > > > > +{ > > > > + acpi_handle handle = dev->handle; > > > > + union acpi_object *result; > > > > + u64 oem_platform_rev = 0; > > > > + > > > > + // get OEM platform revision > > > > + result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID, > > > > + MSHW0040_DSM_REVISION, > > > > + MSHW0040_DSM_GET_OMPR, > > > > + NULL, ACPI_TYPE_INTEGER); > > > > + > > > Does it mean, only 5th, 6th and newer platforms have OEM platform revision? > > > 3rd/4th will get NULL result? Or the opposite? > > > > Correct, from my testing (with limited sample size) and AML code: 5th > > and 6th generation devices have a non-zero OEM platform revision, > > whereas 3rd and 4th gen. devices do not have any (i.e. result will be > > NULL). > > > > > > + if (result) { > > > > + oem_platform_rev = result->integer.value; > > > > + ACPI_FREE(result); > > > > + } > > > > + > > > > + dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev); > > > > + > > > > + return oem_platform_rev == 0 ? 0 : -ENODEV; > > > if 3rd/4th do not have this oem rev information while 5th/newer have, > > > why the latter returns NODEV(it actually has this info)? > > > > Since we always expect a non-zero platform revision (for 5th/6th gen.), > > we can initialize it to zero and use that as "unknown"/"not available". > > So if it can not be determined, we return NODEV. > > Sorry, small mistake here: If it can be determined (i.e. is 5th or 6th > gen.) then we return NODEV. Not the other way around. > How about using a boolean, according to the function name, if a mshw0040 revison is detected then returns true other wise false. Other than that, Acked-by: Chen Yu <yu.c.chen@intel.com> Best, Chenyu > Also to clarify on your last question: > > On 7/2/19 3:14 AM, Yu Chen wrote: > > > static int surface_button_add(struct acpi_device *device) > > > { > > > struct surface_button *button; > > > @@ -154,6 +188,10 @@ static int surface_button_add(struct acpi_device *device) > > > strlen(SURFACE_BUTTON_OBJ_NAME))) > > > return -ENODEV; > > > + error = surface_button_check_MSHW0040(device); > > > + if (error) > > > + return error; > > > + > > ditto, 3rd/4th get error=0? > > You are correct. > > Maximilian
On 7/2/19 3:57 AM, Yu Chen wrote: > How about using a boolean, according to the function name, if a mshw0040 revison > is detected then returns true other wise false. Other than that, > Acked-by: Chen Yu <yu.c.chen@intel.com> I can change that if you want me to. Just thought this might be a bit more flexible in case we ever have to adapt the check for future device generations. Thanks, Maximilian
On Tue, Jul 2, 2019 at 3:38 AM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > Do not use the surfacepro3_button driver on newer Microsoft Surface > models, only use it on the Surface Pro 3 and 4. Newer models (5th, 6th > and possibly future generations) use the same device as the Surface Pro > 4 to represent their volume and power buttons (MSHW0040), but their > acutal implementation is significantly different. This patch ensures > that the surfacepro3_button driver is only used on the Pro 3 and 4 > models, allowing a different driver to bind on other models. > Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> Assuming it will go thru Input subsystem. > Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> > --- > drivers/platform/x86/surfacepro3_button.c | 38 +++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/drivers/platform/x86/surfacepro3_button.c b/drivers/platform/x86/surfacepro3_button.c > index 47c6d000465a..0e2c7dfafd9f 100644 > --- a/drivers/platform/x86/surfacepro3_button.c > +++ b/drivers/platform/x86/surfacepro3_button.c > @@ -20,6 +20,12 @@ > #define SURFACE_BUTTON_OBJ_NAME "VGBI" > #define SURFACE_BUTTON_DEVICE_NAME "Surface Pro 3/4 Buttons" > > +#define MSHW0040_DSM_REVISION 0x01 > +#define MSHW0040_DSM_GET_OMPR 0x02 // get OEM Platform Revision > +static const guid_t MSHW0040_DSM_UUID = > + GUID_INIT(0x6fd05c69, 0xcde3, 0x49f4, 0x95, 0xed, 0xab, 0x16, 0x65, > + 0x49, 0x80, 0x35); > + > #define SURFACE_BUTTON_NOTIFY_TABLET_MODE 0xc8 > > #define SURFACE_BUTTON_NOTIFY_PRESS_POWER 0xc6 > @@ -142,6 +148,34 @@ static int surface_button_resume(struct device *dev) > } > #endif > > +/* > + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device > + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right > + * device by checking for the _DSM method and OEM Platform Revision. > + */ > +static int surface_button_check_MSHW0040(struct acpi_device *dev) > +{ > + acpi_handle handle = dev->handle; > + union acpi_object *result; > + u64 oem_platform_rev = 0; > + > + // get OEM platform revision > + result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID, > + MSHW0040_DSM_REVISION, > + MSHW0040_DSM_GET_OMPR, > + NULL, ACPI_TYPE_INTEGER); > + > + if (result) { > + oem_platform_rev = result->integer.value; > + ACPI_FREE(result); > + } > + > + dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev); > + > + return oem_platform_rev == 0 ? 0 : -ENODEV; > +} > + > + > static int surface_button_add(struct acpi_device *device) > { > struct surface_button *button; > @@ -154,6 +188,10 @@ static int surface_button_add(struct acpi_device *device) > strlen(SURFACE_BUTTON_OBJ_NAME))) > return -ENODEV; > > + error = surface_button_check_MSHW0040(device); > + if (error) > + return error; > + > button = kzalloc(sizeof(struct surface_button), GFP_KERNEL); > if (!button) > return -ENOMEM; > -- > 2.22.0 >
diff --git a/drivers/platform/x86/surfacepro3_button.c b/drivers/platform/x86/surfacepro3_button.c index 47c6d000465a..0e2c7dfafd9f 100644 --- a/drivers/platform/x86/surfacepro3_button.c +++ b/drivers/platform/x86/surfacepro3_button.c @@ -20,6 +20,12 @@ #define SURFACE_BUTTON_OBJ_NAME "VGBI" #define SURFACE_BUTTON_DEVICE_NAME "Surface Pro 3/4 Buttons" +#define MSHW0040_DSM_REVISION 0x01 +#define MSHW0040_DSM_GET_OMPR 0x02 // get OEM Platform Revision +static const guid_t MSHW0040_DSM_UUID = + GUID_INIT(0x6fd05c69, 0xcde3, 0x49f4, 0x95, 0xed, 0xab, 0x16, 0x65, + 0x49, 0x80, 0x35); + #define SURFACE_BUTTON_NOTIFY_TABLET_MODE 0xc8 #define SURFACE_BUTTON_NOTIFY_PRESS_POWER 0xc6 @@ -142,6 +148,34 @@ static int surface_button_resume(struct device *dev) } #endif +/* + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right + * device by checking for the _DSM method and OEM Platform Revision. + */ +static int surface_button_check_MSHW0040(struct acpi_device *dev) +{ + acpi_handle handle = dev->handle; + union acpi_object *result; + u64 oem_platform_rev = 0; + + // get OEM platform revision + result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID, + MSHW0040_DSM_REVISION, + MSHW0040_DSM_GET_OMPR, + NULL, ACPI_TYPE_INTEGER); + + if (result) { + oem_platform_rev = result->integer.value; + ACPI_FREE(result); + } + + dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev); + + return oem_platform_rev == 0 ? 0 : -ENODEV; +} + + static int surface_button_add(struct acpi_device *device) { struct surface_button *button; @@ -154,6 +188,10 @@ static int surface_button_add(struct acpi_device *device) strlen(SURFACE_BUTTON_OBJ_NAME))) return -ENODEV; + error = surface_button_check_MSHW0040(device); + if (error) + return error; + button = kzalloc(sizeof(struct surface_button), GFP_KERNEL); if (!button) return -ENOMEM;
Do not use the surfacepro3_button driver on newer Microsoft Surface models, only use it on the Surface Pro 3 and 4. Newer models (5th, 6th and possibly future generations) use the same device as the Surface Pro 4 to represent their volume and power buttons (MSHW0040), but their acutal implementation is significantly different. This patch ensures that the surfacepro3_button driver is only used on the Pro 3 and 4 models, allowing a different driver to bind on other models. Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> --- drivers/platform/x86/surfacepro3_button.c | 38 +++++++++++++++++++++++ 1 file changed, 38 insertions(+)