Re: [PATCH 2/2] platform/surface: surfacepro3_button: don't load on amd variant

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Sachi King <nakato@nakato.io>, Chen Yu <yu.c.chen@intel.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Mark Gross <markgross@kernel.org>,
	Maximilian Luz <luzmaximilian@gmail.com>,
	Platform Driver <platform-driver-x86@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] platform/surface: surfacepro3_button: don't load on amd variant
Date: Tue, 1 Mar 2022 13:01:23 -0800	[thread overview]
Message-ID: <Yh6Joy2HDnsuK6LN@google.com> (raw)
In-Reply-To: <CAHp75Vcw-ARNZCRRJGzbQ7xc3ZB=98eFCuEFc4cj5W3vAj5EZw@mail.gmail.com>

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@nakato.io> 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