2023-01-18 10:47:53

by Andy Shevchenko

Subject: [PATCH v1 1/1] platform/surface: Switch to use acpi_evaluate_dsm_typed()

The acpi_evaluate_dsm_typed() provides a way to check the type of the
object evaluated by _DSM call. Use it instead of open coded variant.

Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/platform/surface/surface_hotplug.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/surface/surface_hotplug.c b/drivers/platform/surface/surface_hotplug.c
index f004a2495201..7b6d887dccdb 100644
--- a/drivers/platform/surface/surface_hotplug.c
+++ b/drivers/platform/surface/surface_hotplug.c
@@ -101,18 +101,12 @@ static void shps_dsm_notify_irq(struct platform_device *pdev, enum shps_irq_type
param.type = ACPI_TYPE_INTEGER;
param.integer.value = value;

- result = acpi_evaluate_dsm(handle, &shps_dsm_guid, SHPS_DSM_REVISION,
- shps_dsm_fn_for_irq(type), &param);
-
+ result = acpi_evaluate_dsm_typed(handle, &shps_dsm_guid, SHPS_DSM_REVISION,
+ shps_dsm_fn_for_irq(type), &param, ACPI_TYPE_BUFFER);
if (!result) {
dev_err(&pdev->dev, "IRQ notification via DSM failed (irq=%d, gpio=%d)\n",
type, value);

- } else if (result->type != ACPI_TYPE_BUFFER) {
- dev_err(&pdev->dev,
- "IRQ notification via DSM failed: unexpected result type (irq=%d, gpio=%d)\n",
- type, value);
-
} else if (result->buffer.length != 1 || result->buffer.pointer[0] != 0) {
dev_err(&pdev->dev,
"IRQ notification via DSM failed: unexpected result value (irq=%d, gpio=%d)\n",
@@ -121,8 +115,7 @@ static void shps_dsm_notify_irq(struct platform_device *pdev, enum shps_irq_type

mutex_unlock(&sdev->lock[type]);

- if (result)
- ACPI_FREE(result);
+ ACPI_FREE(result);
}

static irqreturn_t shps_handle_irq(int irq, void *data)
--
2.39.0

2023-01-18 10:51:41

by Maximilian Luz

Subject: Re: [PATCH v1 1/1] platform/surface: Switch to use acpi_evaluate_dsm_typed()

On 1/18/23 10:38, Andy Shevchenko wrote:
> The acpi_evaluate_dsm_typed() provides a way to check the type of the
> object evaluated by _DSM call. Use it instead of open coded variant.
>
> Signed-off-by: Andy Shevchenko <[email protected]>

I think the reason I went with the non-typed version here was for the
explicit error message. Specifically, to simplify debugging in case we'd
get any new device that behaves differently. But since then I think MS
discontinued the use of this driver (or at least this part) in their
devices (no more detachable GPUs). Also, the likelihood of that
interface changing is pretty small regardless.

So I'm fine with changing that.

Reviewed-by: Maximilian Luz <[email protected]>

> ---
> drivers/platform/surface/surface_hotplug.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/surface/surface_hotplug.c b/drivers/platform/surface/surface_hotplug.c
> index f004a2495201..7b6d887dccdb 100644
> --- a/drivers/platform/surface/surface_hotplug.c
> +++ b/drivers/platform/surface/surface_hotplug.c
> @@ -101,18 +101,12 @@ static void shps_dsm_notify_irq(struct platform_device *pdev, enum shps_irq_type
> param.type = ACPI_TYPE_INTEGER;
> param.integer.value = value;
>
> - result = acpi_evaluate_dsm(handle, &shps_dsm_guid, SHPS_DSM_REVISION,
> - shps_dsm_fn_for_irq(type), &param);
> -
> + result = acpi_evaluate_dsm_typed(handle, &shps_dsm_guid, SHPS_DSM_REVISION,
> + shps_dsm_fn_for_irq(type), &param, ACPI_TYPE_BUFFER);
> if (!result) {
> dev_err(&pdev->dev, "IRQ notification via DSM failed (irq=%d, gpio=%d)\n",
> type, value);
>
> - } else if (result->type != ACPI_TYPE_BUFFER) {
> - dev_err(&pdev->dev,
> - "IRQ notification via DSM failed: unexpected result type (irq=%d, gpio=%d)\n",
> - type, value);
> -
> } else if (result->buffer.length != 1 || result->buffer.pointer[0] != 0) {
> dev_err(&pdev->dev,
> "IRQ notification via DSM failed: unexpected result value (irq=%d, gpio=%d)\n",
> @@ -121,8 +115,7 @@ static void shps_dsm_notify_irq(struct platform_device *pdev, enum shps_irq_type
>
> mutex_unlock(&sdev->lock[type]);
>
> - if (result)
> - ACPI_FREE(result);
> + ACPI_FREE(result);
> }
>
> static irqreturn_t shps_handle_irq(int irq, void *data)

2023-01-23 15:56:12

by Hans de Goede

Subject: Re: [PATCH v1 1/1] platform/surface: Switch to use acpi_evaluate_dsm_typed()

Hi Andy,

On 1/18/23 10:38, Andy Shevchenko wrote:
> The acpi_evaluate_dsm_typed() provides a way to check the type of the
> object evaluated by _DSM call. Use it instead of open coded variant.
>
> Signed-off-by: Andy Shevchenko <[email protected]>

Thank you for the 3 patches switching to acpi_evaluate_dsm_typed().
I have added all 3 to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Regards,

Hans


> ---
> drivers/platform/surface/surface_hotplug.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/surface/surface_hotplug.c b/drivers/platform/surface/surface_hotplug.c
> index f004a2495201..7b6d887dccdb 100644
> --- a/drivers/platform/surface/surface_hotplug.c
> +++ b/drivers/platform/surface/surface_hotplug.c
> @@ -101,18 +101,12 @@ static void shps_dsm_notify_irq(struct platform_device *pdev, enum shps_irq_type
> param.type = ACPI_TYPE_INTEGER;
> param.integer.value = value;
>
> - result = acpi_evaluate_dsm(handle, &shps_dsm_guid, SHPS_DSM_REVISION,
> - shps_dsm_fn_for_irq(type), &param);
> -
> + result = acpi_evaluate_dsm_typed(handle, &shps_dsm_guid, SHPS_DSM_REVISION,
> + shps_dsm_fn_for_irq(type), &param, ACPI_TYPE_BUFFER);
> if (!result) {
> dev_err(&pdev->dev, "IRQ notification via DSM failed (irq=%d, gpio=%d)\n",
> type, value);
>
> - } else if (result->type != ACPI_TYPE_BUFFER) {
> - dev_err(&pdev->dev,
> - "IRQ notification via DSM failed: unexpected result type (irq=%d, gpio=%d)\n",
> - type, value);
> -
> } else if (result->buffer.length != 1 || result->buffer.pointer[0] != 0) {
> dev_err(&pdev->dev,
> "IRQ notification via DSM failed: unexpected result value (irq=%d, gpio=%d)\n",
> @@ -121,8 +115,7 @@ static void shps_dsm_notify_irq(struct platform_device *pdev, enum shps_irq_type
>
> mutex_unlock(&sdev->lock[type]);
>
> - if (result)
> - ACPI_FREE(result);
> + ACPI_FREE(result);
> }
>
> static irqreturn_t shps_handle_irq(int irq, void *data)