From: Rafael J. Wysocki <[email protected]>
The ACPI_HANDLE() macro is a wrapper arond the ACPI_COMPANION()
macro and the ACPI handle produced by the former comes from the
ACPI device object produced by the latter, so it is way more
straightforward to evaluate the latter directly instead of passing
the handle produced by the former to acpi_bus_get_device().
Modify mshw0011_notify() accordingly (no intentional functional
impact).
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/platform/surface/surface3_power.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/platform/surface/surface3_power.c
===================================================================
--- linux-pm.orig/drivers/platform/surface/surface3_power.c
+++ linux-pm/drivers/platform/surface/surface3_power.c
@@ -160,15 +160,14 @@ mshw0011_notify(struct mshw0011_data *cd
{
union acpi_object *obj;
struct acpi_device *adev;
- acpi_handle handle;
unsigned int i;
- handle = ACPI_HANDLE(&cdata->adp1->dev);
- if (!handle || acpi_bus_get_device(handle, &adev))
+ adev = ACPI_COMPANION(&cdata->adp1->dev);
+ if (!adev)
return -ENODEV;
- obj = acpi_evaluate_dsm_typed(handle, &mshw0011_guid, arg1, arg2, NULL,
- ACPI_TYPE_BUFFER);
+ obj = acpi_evaluate_dsm_typed(adev->handle, &mshw0011_guid, arg1, arg2,
+ NULL, ACPI_TYPE_BUFFER);
if (!obj) {
dev_err(&cdata->adp1->dev, "device _DSM execution failed\n");
return -ENODEV;
On 10/12/21 19:46, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> The ACPI_HANDLE() macro is a wrapper arond the ACPI_COMPANION()
> macro and the ACPI handle produced by the former comes from the
> ACPI device object produced by the latter, so it is way more
> straightforward to evaluate the latter directly instead of passing
> the handle produced by the former to acpi_bus_get_device().
>
> Modify mshw0011_notify() accordingly (no intentional functional
> impact).
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Looks mostly good to me, small comment/question inline.
> ---
> drivers/platform/surface/surface3_power.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/platform/surface/surface3_power.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/surface/surface3_power.c
> +++ linux-pm/drivers/platform/surface/surface3_power.c
> @@ -160,15 +160,14 @@ mshw0011_notify(struct mshw0011_data *cd
> {
> union acpi_object *obj;
> struct acpi_device *adev;
> - acpi_handle handle;
> unsigned int i;
>
> - handle = ACPI_HANDLE(&cdata->adp1->dev);
> - if (!handle || acpi_bus_get_device(handle, &adev))
> + adev = ACPI_COMPANION(&cdata->adp1->dev);
> + if (!adev)
> return -ENODEV;
Do we need to get the ACPI device (adev) here? To me it looks like only
its handle is actually used so why not keep ACPI_HANDLE() and remove the
acpi_bus_get_device() call instead?
>
> - obj = acpi_evaluate_dsm_typed(handle, &mshw0011_guid, arg1, arg2, NULL,
> - ACPI_TYPE_BUFFER);
> + obj = acpi_evaluate_dsm_typed(adev->handle, &mshw0011_guid, arg1, arg2,
> + NULL, ACPI_TYPE_BUFFER);
> if (!obj) {
> dev_err(&cdata->adp1->dev, "device _DSM execution failed\n");
> return -ENODEV;
>
>
>
Regards,
Max
On Tue, Oct 12, 2021 at 8:21 PM Maximilian Luz <[email protected]> wrote:
>
> On 10/12/21 19:46, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > The ACPI_HANDLE() macro is a wrapper arond the ACPI_COMPANION()
> > macro and the ACPI handle produced by the former comes from the
> > ACPI device object produced by the latter, so it is way more
> > straightforward to evaluate the latter directly instead of passing
> > the handle produced by the former to acpi_bus_get_device().
> >
> > Modify mshw0011_notify() accordingly (no intentional functional
> > impact).
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
>
> Looks mostly good to me, small comment/question inline.
>
> > ---
> > drivers/platform/surface/surface3_power.c | 9 ++++-----
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > Index: linux-pm/drivers/platform/surface/surface3_power.c
> > ===================================================================
> > --- linux-pm.orig/drivers/platform/surface/surface3_power.c
> > +++ linux-pm/drivers/platform/surface/surface3_power.c
> > @@ -160,15 +160,14 @@ mshw0011_notify(struct mshw0011_data *cd
> > {
> > union acpi_object *obj;
> > struct acpi_device *adev;
> > - acpi_handle handle;
> > unsigned int i;
> >
> > - handle = ACPI_HANDLE(&cdata->adp1->dev);
> > - if (!handle || acpi_bus_get_device(handle, &adev))
> > + adev = ACPI_COMPANION(&cdata->adp1->dev);
> > + if (!adev)
> > return -ENODEV;
>
> Do we need to get the ACPI device (adev) here? To me it looks like only
> its handle is actually used so why not keep ACPI_HANDLE() and remove the
> acpi_bus_get_device() call instead?
It actually doesn't really matter, but you're right,
acpi_bus_get_device() is simply redundant here, so ACPI_HANDLE() is
sufficient.
I'll send a v2 of this one.
> >
> > - obj = acpi_evaluate_dsm_typed(handle, &mshw0011_guid, arg1, arg2, NULL,
> > - ACPI_TYPE_BUFFER);
> > + obj = acpi_evaluate_dsm_typed(adev->handle, &mshw0011_guid, arg1, arg2,
> > + NULL, ACPI_TYPE_BUFFER);
> > if (!obj) {
> > dev_err(&cdata->adp1->dev, "device _DSM execution failed\n");
> > return -ENODEV;
> >
> >
> >
>
> Regards,
> Max
From: Rafael J. Wysocki <[email protected]>
If the ACPI companion of a given device is not present, the result
of the ACPI_HANDLE() evaluation for it will be NULL, so calling
acpi_bus_get_device() on ACPI_HANDLE() result in order to validate
it is redundant.
Drop the redundant acpi_bus_get_device() call from mshw0011_notify()
along with a local variable related to it.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v1 -> v2:
* Instead of switching over to using ACPI_COMPANION(), just drop the
redundant acpi_bus_get_device() call from mshw0011_notify() and
update the subject and changelog accordingly.
---
drivers/platform/surface/surface3_power.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Index: linux-pm/drivers/platform/surface/surface3_power.c
===================================================================
--- linux-pm.orig/drivers/platform/surface/surface3_power.c
+++ linux-pm/drivers/platform/surface/surface3_power.c
@@ -159,12 +159,11 @@ mshw0011_notify(struct mshw0011_data *cd
unsigned int *ret_value)
{
union acpi_object *obj;
- struct acpi_device *adev;
acpi_handle handle;
unsigned int i;
handle = ACPI_HANDLE(&cdata->adp1->dev);
- if (!handle || acpi_bus_get_device(handle, &adev))
+ if (!handle)
return -ENODEV;
obj = acpi_evaluate_dsm_typed(handle, &mshw0011_guid, arg1, arg2, NULL,
On 10/12/21 21:32, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> If the ACPI companion of a given device is not present, the result
> of the ACPI_HANDLE() evaluation for it will be NULL, so calling
> acpi_bus_get_device() on ACPI_HANDLE() result in order to validate
> it is redundant.
>
> Drop the redundant acpi_bus_get_device() call from mshw0011_notify()
> along with a local variable related to it.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> v1 -> v2:
> * Instead of switching over to using ACPI_COMPANION(), just drop the
> redundant acpi_bus_get_device() call from mshw0011_notify() and
> update the subject and changelog accordingly.
>
Thanks, looks good to me!
Reviewed-by: Maximilian Luz <[email protected]>
> ---
> drivers/platform/surface/surface3_power.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> Index: linux-pm/drivers/platform/surface/surface3_power.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/surface/surface3_power.c
> +++ linux-pm/drivers/platform/surface/surface3_power.c
> @@ -159,12 +159,11 @@ mshw0011_notify(struct mshw0011_data *cd
> unsigned int *ret_value)
> {
> union acpi_object *obj;
> - struct acpi_device *adev;
> acpi_handle handle;
> unsigned int i;
>
> handle = ACPI_HANDLE(&cdata->adp1->dev);
> - if (!handle || acpi_bus_get_device(handle, &adev))
> + if (!handle)
> return -ENODEV;
>
> obj = acpi_evaluate_dsm_typed(handle, &mshw0011_guid, arg1, arg2, NULL,
>
>
>
From: Rafael J. Wysocki <[email protected]>
If the ACPI companion of a given device is not present, the result
of the ACPI_HANDLE() evaluation for it will be NULL, so calling
acpi_bus_get_device() on ACPI_HANDLE() result in order to validate
it is redundant.
Drop the redundant acpi_bus_get_device() call from mshw0011_notify()
along with a local variable related to it.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Maximilian Luz <[email protected]>
---
v2 -> v3:
* Resend with a different From and S-o-b address and with R-by from
Maximilian. No other changes.
---
drivers/platform/surface/surface3_power.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Index: linux-pm/drivers/platform/surface/surface3_power.c
===================================================================
--- linux-pm.orig/drivers/platform/surface/surface3_power.c
+++ linux-pm/drivers/platform/surface/surface3_power.c
@@ -159,12 +159,11 @@ mshw0011_notify(struct mshw0011_data *cd
unsigned int *ret_value)
{
union acpi_object *obj;
- struct acpi_device *adev;
acpi_handle handle;
unsigned int i;
handle = ACPI_HANDLE(&cdata->adp1->dev);
- if (!handle || acpi_bus_get_device(handle, &adev))
+ if (!handle)
return -ENODEV;
obj = acpi_evaluate_dsm_typed(handle, &mshw0011_guid, arg1, arg2, NULL,
Hi,
On 10/13/21 18:12, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> If the ACPI companion of a given device is not present, the result
> of the ACPI_HANDLE() evaluation for it will be NULL, so calling
> acpi_bus_get_device() on ACPI_HANDLE() result in order to validate
> it is redundant.
>
> Drop the redundant acpi_bus_get_device() call from mshw0011_notify()
> along with a local variable related to it.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> Reviewed-by: Maximilian Luz <[email protected]>
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Regards,
Hans
> ---
>
> v2 -> v3:
> * Resend with a different From and S-o-b address and with R-by from
> Maximilian. No other changes.
>
> ---
> drivers/platform/surface/surface3_power.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> Index: linux-pm/drivers/platform/surface/surface3_power.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/surface/surface3_power.c
> +++ linux-pm/drivers/platform/surface/surface3_power.c
> @@ -159,12 +159,11 @@ mshw0011_notify(struct mshw0011_data *cd
> unsigned int *ret_value)
> {
> union acpi_object *obj;
> - struct acpi_device *adev;
> acpi_handle handle;
> unsigned int i;
>
> handle = ACPI_HANDLE(&cdata->adp1->dev);
> - if (!handle || acpi_bus_get_device(handle, &adev))
> + if (!handle)
> return -ENODEV;
>
> obj = acpi_evaluate_dsm_typed(handle, &mshw0011_guid, arg1, arg2, NULL,
>
>
>