On Tue, Nov 09, 2021 at 11:12:34AM +0200, Andy Shevchenko wrote: > On Tue, Nov 9, 2021 at 10:11 AM Sachi King <nakato@xxxxxxxxx> wrote: > > > > The AMD variant of the Surface Laptop report 0 for their OEM platform > > revision. The Surface devices that require the surfacepro3_button > > driver do not have the _DSM that gets the OEM platform revision. If the > > method does not exist, load surfacepro3_button. > > ... > > > * 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. > > + * device by checking for the _DSM method and OEM Platform Revision DSM > > + * function. > > Not sure what this change means (not a native speaker). > > ... > > > - dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev); > > I think this is useful to have. > > What about leaving it as is for debugging purposes and just replacing > the last test? I agree it is nice to be able to print it for debug purposes, but it has to be fetched separately, as with the proposed change we are not reading it. If I am understanding the change it wants to call acpi_dsm_check() to verify whether MSHW0040_DSM_GET_OMPR function exists at all (which is done by reading _DSM MSHW0040_DSM_UUID, revision MSHW0040_DSM_REVISION, function 0. Only if function 0 indicates that function MSHW0040_DSM_GET_OMPR is supported in this _DSM, we can read it to get the real version number, which can be 0. Treating 0 as an invalid version as it was done in original change is wrong. I propose we combine the old and new code, call acpi_dsm_check() and bail if it returns false, otherwise proceed to calling acpi_evaluate_dsm_typed() and dev_dbg() the version. Sachi, are you going to update the patch? You do not need to adjust the surface driver as Hans is getting rid of it. Thanks. -- Dmitry
