Re: [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h

Re: [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h

[Jonathan Cameron]

On Wed, 17 Apr 2024 18:46:14 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> The ACPI "ROTM" rotation matrix parsing code atm is already duplicated
> between bmc150-accel-core.c and kxcjk-1013.c and a third user of this is
> coming.
> 
> Move the ROTM parsing from kxcjk-1013.c, which has slightly better error
> logging (and otherwise is 100% identical), into a new acpi-helpers.h file
> so that it can be shared.
> 
> Other then moving the code the only 2 other changes are:
> 
> 1. Rename the function to acpi_read_mount_matrix() to make clear this
>    is a generic ACPI mount matrix read function.
> 2. Add a "char *acpi_method" parameter since some bmc150 dual-accel setups
>    (360° hinges with 1 accel in kbd/base + 1 in display half) declare both
>    accels in a single ACPI device with 2 different method names for
>    the 2 matrices.
> 
> Cc: Sean Rhodes <sean@starlabs.systems>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Tempted to ask you to rename this to
acpi_non_standard_microsoft_extension_that_they_never_agreed_with_aswg_read_mount_matrix()
but meh, I'll cope with a reference to:
https://learn.microsoft.com/en-us/windows-hardware/drivers/sensors/sensors-acpi-entries
and a comment saying this is not part of the ACPI standard.

I'm not grumpy at all about companies who add non standard stuff without
at least reserving the ID space.

Why isn't it a _DSM (Device Specific Method) with a microsoft defined UUID? Harrumph.

+CC Rafael and linux-acpi as whilst this remains in IIO, it is named such that it
ends up in the acpi_* namespace.  They may not care, but best to check!

Jonathan


> ---
>  drivers/iio/accel/acpi-helpers.h | 76 ++++++++++++++++++++++++++++++++
>  drivers/iio/accel/kxcjk-1013.c   | 71 ++---------------------------
>  2 files changed, 79 insertions(+), 68 deletions(-)
>  create mode 100644 drivers/iio/accel/acpi-helpers.h
> 
> diff --git a/drivers/iio/accel/acpi-helpers.h b/drivers/iio/accel/acpi-helpers.h
> new file mode 100644
> index 000000000000..a4357925bf07
> --- /dev/null
> +++ b/drivers/iio/accel/acpi-helpers.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* ACPI helper functions for parsing ACPI rotation matrices */
> +
> +#include <linux/acpi.h>
> +#include <linux/dev_printk.h>
> +#include <linux/iio/iio.h>
> +#include <linux/sprintf.h>
> +
> +#ifdef CONFIG_ACPI
> +static inline bool acpi_read_mount_matrix(struct device *dev,
> +					  struct iio_mount_matrix *orientation,
> +					  char *acpi_method)
> +{
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	char *str;
> +	union acpi_object *obj, *elements;
> +	acpi_status status;
> +	int i, j, val[3];
> +	bool ret = false;
> +
> +	if (!adev || !acpi_has_method(adev->handle, acpi_method))
> +		return false;
> +
> +	status = acpi_evaluate_object(adev->handle, acpi_method, NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(dev, "Failed to get ACPI mount matrix: %d\n", status);
> +		return false;
> +	}
> +
> +	obj = buffer.pointer;
> +	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 3) {
> +		dev_err(dev, "Unknown ACPI mount matrix package format\n");
> +		goto out_free_buffer;
> +	}
> +
> +	elements = obj->package.elements;
> +	for (i = 0; i < 3; i++) {
> +		if (elements[i].type != ACPI_TYPE_STRING) {
> +			dev_err(dev, "Unknown ACPI mount matrix element format\n");
> +			goto out_free_buffer;
> +		}
> +
> +		str = elements[i].string.pointer;
> +		if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3) {
> +			dev_err(dev, "Incorrect ACPI mount matrix string format\n");
> +			goto out_free_buffer;
> +		}
> +
> +		for (j = 0; j < 3; j++) {
> +			switch (val[j]) {
> +			case -1: str = "-1"; break;
> +			case 0:  str = "0";  break;
> +			case 1:  str = "1";  break;
> +			default:
> +				dev_err(dev, "Invalid value in ACPI mount matrix: %d\n", val[j]);
> +				goto out_free_buffer;
> +			}
> +			orientation->rotation[i * 3 + j] = str;
> +		}
> +	}
> +
> +	ret = true;
> +
> +out_free_buffer:
> +	kfree(buffer.pointer);
> +	return ret;
> +}
> +#else
> +static inline bool acpi_read_mount_matrix(struct device *dev,
> +					  struct iio_mount_matrix *orientation,
> +					  char *acpi_method)
> +{
> +	return false;
> +}
> +#endif
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index bb1660667bb0..7e19278491dc 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -24,6 +24,8 @@
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/accel/kxcjk_1013.h>
>  
> +#include "acpi-helpers.h"
> +
>  #define KXCJK1013_DRV_NAME "kxcjk1013"
>  #define KXCJK1013_IRQ_NAME "kxcjk1013_event"
>  
> @@ -636,73 +638,6 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_ACPI
> -static bool kxj1009_apply_acpi_orientation(struct device *dev,
> -					   struct iio_mount_matrix *orientation)
> -{
> -	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> -	struct acpi_device *adev = ACPI_COMPANION(dev);
> -	char *str;
> -	union acpi_object *obj, *elements;
> -	acpi_status status;
> -	int i, j, val[3];
> -	bool ret = false;
> -
> -	if (!adev || !acpi_has_method(adev->handle, "ROTM"))
> -		return false;
> -
> -	status = acpi_evaluate_object(adev->handle, "ROTM", NULL, &buffer);
> -	if (ACPI_FAILURE(status)) {
> -		dev_err(dev, "Failed to get ACPI mount matrix: %d\n", status);
> -		return false;
> -	}
> -
> -	obj = buffer.pointer;
> -	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 3) {
> -		dev_err(dev, "Unknown ACPI mount matrix package format\n");
> -		goto out_free_buffer;
> -	}
> -
> -	elements = obj->package.elements;
> -	for (i = 0; i < 3; i++) {
> -		if (elements[i].type != ACPI_TYPE_STRING) {
> -			dev_err(dev, "Unknown ACPI mount matrix element format\n");
> -			goto out_free_buffer;
> -		}
> -
> -		str = elements[i].string.pointer;
> -		if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3) {
> -			dev_err(dev, "Incorrect ACPI mount matrix string format\n");
> -			goto out_free_buffer;
> -		}
> -
> -		for (j = 0; j < 3; j++) {
> -			switch (val[j]) {
> -			case -1: str = "-1"; break;
> -			case 0:  str = "0";  break;
> -			case 1:  str = "1";  break;
> -			default:
> -				dev_err(dev, "Invalid value in ACPI mount matrix: %d\n", val[j]);
> -				goto out_free_buffer;
> -			}
> -			orientation->rotation[i * 3 + j] = str;
> -		}
> -	}
> -
> -	ret = true;
> -
> -out_free_buffer:
> -	kfree(buffer.pointer);
> -	return ret;
> -}
> -#else
> -static bool kxj1009_apply_acpi_orientation(struct device *dev,
> -					  struct iio_mount_matrix *orientation)
> -{
> -	return false;
> -}
> -#endif
> -
>  static int kxcjk1013_chip_update_thresholds(struct kxcjk1013_data *data)
>  {
>  	int ret;
> @@ -1533,7 +1468,7 @@ static int kxcjk1013_probe(struct i2c_client *client)
>  	} else {
>  		data->active_high_intr = true; /* default polarity */
>  
> -		if (!kxj1009_apply_acpi_orientation(&client->dev, &data->orientation)) {
> +		if (!acpi_read_mount_matrix(&client->dev, &data->orientation, "ROTM")) {
>  			ret = iio_read_mount_matrix(&client->dev, &data->orientation);
>  			if (ret)
>  				return ret;

Re: [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h

[Hans de Goede]

Hi,

On 4/20/24 1:13 PM, Jonathan Cameron wrote:
> On Wed, 17 Apr 2024 18:46:14 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> The ACPI "ROTM" rotation matrix parsing code atm is already duplicated
>> between bmc150-accel-core.c and kxcjk-1013.c and a third user of this is
>> coming.
>>
>> Move the ROTM parsing from kxcjk-1013.c, which has slightly better error
>> logging (and otherwise is 100% identical), into a new acpi-helpers.h file
>> so that it can be shared.
>>
>> Other then moving the code the only 2 other changes are:
>>
>> 1. Rename the function to acpi_read_mount_matrix() to make clear this
>>    is a generic ACPI mount matrix read function.
>> 2. Add a "char *acpi_method" parameter since some bmc150 dual-accel setups
>>    (360° hinges with 1 accel in kbd/base + 1 in display half) declare both
>>    accels in a single ACPI device with 2 different method names for
>>    the 2 matrices.
>>
>> Cc: Sean Rhodes <sean@starlabs.systems>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Tempted to ask you to rename this to
> acpi_non_standard_microsoft_extension_that_they_never_agreed_with_aswg_read_mount_matrix()
> but meh, I'll cope with a reference to:
> https://learn.microsoft.com/en-us/windows-hardware/drivers/sensors/sensors-acpi-entries
> and a comment saying this is not part of the ACPI standard.

Ok, so I'll add a comment like this for v2:

> 
> I'm not grumpy at all about companies who add non standard stuff without
> at least reserving the ID space.
> 
> Why isn't it a _DSM (Device Specific Method) with a microsoft defined UUID? Harrumph.
> 
> +CC Rafael and linux-acpi as whilst this remains in IIO, it is named such that it
> ends up in the acpi_* namespace.  They may not care, but best to check!
> 
> Jonathan
> 
> 
>> ---
>>  drivers/iio/accel/acpi-helpers.h | 76 ++++++++++++++++++++++++++++++++
>>  drivers/iio/accel/kxcjk-1013.c   | 71 ++---------------------------
>>  2 files changed, 79 insertions(+), 68 deletions(-)
>>  create mode 100644 drivers/iio/accel/acpi-helpers.h
>>
>> diff --git a/drivers/iio/accel/acpi-helpers.h b/drivers/iio/accel/acpi-helpers.h
>> new file mode 100644
>> index 000000000000..a4357925bf07
>> --- /dev/null
>> +++ b/drivers/iio/accel/acpi-helpers.h
>> @@ -0,0 +1,76 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* ACPI helper functions for parsing ACPI rotation matrices */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/sprintf.h>
>> +
>> +#ifdef CONFIG_ACPI
>> +static inline bool acpi_read_mount_matrix(struct device *dev,
>> +					  struct iio_mount_matrix *orientation,
>> +					  char *acpi_method)
>> +{
>> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +	struct acpi_device *adev = ACPI_COMPANION(dev);
>> +	char *str;
>> +	union acpi_object *obj, *elements;
>> +	acpi_status status;
>> +	int i, j, val[3];
>> +	bool ret = false;
>> +
>> +	if (!adev || !acpi_has_method(adev->handle, acpi_method))
>> +		return false;
>> +
>> +	status = acpi_evaluate_object(adev->handle, acpi_method, NULL, &buffer);
>> +	if (ACPI_FAILURE(status)) {
>> +		dev_err(dev, "Failed to get ACPI mount matrix: %d\n", status);
>> +		return false;
>> +	}
>> +
>> +	obj = buffer.pointer;
>> +	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 3) {
>> +		dev_err(dev, "Unknown ACPI mount matrix package format\n");
>> +		goto out_free_buffer;
>> +	}
>> +
>> +	elements = obj->package.elements;
>> +	for (i = 0; i < 3; i++) {
>> +		if (elements[i].type != ACPI_TYPE_STRING) {
>> +			dev_err(dev, "Unknown ACPI mount matrix element format\n");
>> +			goto out_free_buffer;
>> +		}
>> +
>> +		str = elements[i].string.pointer;
>> +		if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3) {
>> +			dev_err(dev, "Incorrect ACPI mount matrix string format\n");
>> +			goto out_free_buffer;
>> +		}
>> +
>> +		for (j = 0; j < 3; j++) {
>> +			switch (val[j]) {
>> +			case -1: str = "-1"; break;
>> +			case 0:  str = "0";  break;
>> +			case 1:  str = "1";  break;
>> +			default:
>> +				dev_err(dev, "Invalid value in ACPI mount matrix: %d\n", val[j]);
>> +				goto out_free_buffer;
>> +			}
>> +			orientation->rotation[i * 3 + j] = str;
>> +		}
>> +	}
>> +
>> +	ret = true;
>> +
>> +out_free_buffer:
>> +	kfree(buffer.pointer);
>> +	return ret;
>> +}
>> +#else
>> +static inline bool acpi_read_mount_matrix(struct device *dev,
>> +					  struct iio_mount_matrix *orientation,
>> +					  char *acpi_method)
>> +{
>> +	return false;
>> +}
>> +#endif
>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>> index bb1660667bb0..7e19278491dc 100644
>> --- a/drivers/iio/accel/kxcjk-1013.c
>> +++ b/drivers/iio/accel/kxcjk-1013.c
>> @@ -24,6 +24,8 @@
>>  #include <linux/iio/triggered_buffer.h>
>>  #include <linux/iio/accel/kxcjk_1013.h>
>>  
>> +#include "acpi-helpers.h"
>> +
>>  #define KXCJK1013_DRV_NAME "kxcjk1013"
>>  #define KXCJK1013_IRQ_NAME "kxcjk1013_event"
>>  
>> @@ -636,73 +638,6 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
>>  	return 0;
>>  }
>>  
>> -#ifdef CONFIG_ACPI
>> -static bool kxj1009_apply_acpi_orientation(struct device *dev,
>> -					   struct iio_mount_matrix *orientation)
>> -{
>> -	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> -	struct acpi_device *adev = ACPI_COMPANION(dev);
>> -	char *str;
>> -	union acpi_object *obj, *elements;
>> -	acpi_status status;
>> -	int i, j, val[3];
>> -	bool ret = false;
>> -
>> -	if (!adev || !acpi_has_method(adev->handle, "ROTM"))
>> -		return false;
>> -
>> -	status = acpi_evaluate_object(adev->handle, "ROTM", NULL, &buffer);
>> -	if (ACPI_FAILURE(status)) {
>> -		dev_err(dev, "Failed to get ACPI mount matrix: %d\n", status);
>> -		return false;
>> -	}
>> -
>> -	obj = buffer.pointer;
>> -	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 3) {
>> -		dev_err(dev, "Unknown ACPI mount matrix package format\n");
>> -		goto out_free_buffer;
>> -	}
>> -
>> -	elements = obj->package.elements;
>> -	for (i = 0; i < 3; i++) {
>> -		if (elements[i].type != ACPI_TYPE_STRING) {
>> -			dev_err(dev, "Unknown ACPI mount matrix element format\n");
>> -			goto out_free_buffer;
>> -		}
>> -
>> -		str = elements[i].string.pointer;
>> -		if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3) {
>> -			dev_err(dev, "Incorrect ACPI mount matrix string format\n");
>> -			goto out_free_buffer;
>> -		}
>> -
>> -		for (j = 0; j < 3; j++) {
>> -			switch (val[j]) {
>> -			case -1: str = "-1"; break;
>> -			case 0:  str = "0";  break;
>> -			case 1:  str = "1";  break;
>> -			default:
>> -				dev_err(dev, "Invalid value in ACPI mount matrix: %d\n", val[j]);
>> -				goto out_free_buffer;
>> -			}
>> -			orientation->rotation[i * 3 + j] = str;
>> -		}
>> -	}
>> -
>> -	ret = true;
>> -
>> -out_free_buffer:
>> -	kfree(buffer.pointer);
>> -	return ret;
>> -}
>> -#else
>> -static bool kxj1009_apply_acpi_orientation(struct device *dev,
>> -					  struct iio_mount_matrix *orientation)
>> -{
>> -	return false;
>> -}
>> -#endif
>> -
>>  static int kxcjk1013_chip_update_thresholds(struct kxcjk1013_data *data)
>>  {
>>  	int ret;
>> @@ -1533,7 +1468,7 @@ static int kxcjk1013_probe(struct i2c_client *client)
>>  	} else {
>>  		data->active_high_intr = true; /* default polarity */
>>  
>> -		if (!kxj1009_apply_acpi_orientation(&client->dev, &data->orientation)) {
>> +		if (!acpi_read_mount_matrix(&client->dev, &data->orientation, "ROTM")) {
>>  			ret = iio_read_mount_matrix(&client->dev, &data->orientation);
>>  			if (ret)
>>  				return ret;
> 

Re: [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h

[Hans de Goede]

Hi,

On 4/20/24 1:13 PM, Jonathan Cameron wrote:
> On Wed, 17 Apr 2024 18:46:14 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> The ACPI "ROTM" rotation matrix parsing code atm is already duplicated
>> between bmc150-accel-core.c and kxcjk-1013.c and a third user of this is
>> coming.
>>
>> Move the ROTM parsing from kxcjk-1013.c, which has slightly better error
>> logging (and otherwise is 100% identical), into a new acpi-helpers.h file
>> so that it can be shared.
>>
>> Other then moving the code the only 2 other changes are:
>>
>> 1. Rename the function to acpi_read_mount_matrix() to make clear this
>>    is a generic ACPI mount matrix read function.
>> 2. Add a "char *acpi_method" parameter since some bmc150 dual-accel setups
>>    (360° hinges with 1 accel in kbd/base + 1 in display half) declare both
>>    accels in a single ACPI device with 2 different method names for
>>    the 2 matrices.
>>
>> Cc: Sean Rhodes <sean@starlabs.systems>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Tempted to ask you to rename this to
> acpi_non_standard_microsoft_extension_that_they_never_agreed_with_aswg_read_mount_matrix()
> but meh, I'll cope with a reference to:
> https://learn.microsoft.com/en-us/windows-hardware/drivers/sensors/sensors-acpi-entries
> and a comment saying this is not part of the ACPI standard.

Ok, so I have added the following comment to the v2 which I will send out soon:

diff --git a/drivers/iio/accel/acpi-helpers.h b/drivers/iio/accel/acpi-helpers.h
index a4357925bf07..4f4140694b59 100644
--- a/drivers/iio/accel/acpi-helpers.h
+++ b/drivers/iio/accel/acpi-helpers.h
@@ -7,6 +7,13 @@
 #include <linux/sprintf.h>
 
 #ifdef CONFIG_ACPI
+/*
+ * Parse mount matrixes defined in the ACPI "ROTM" format from:
+ * https://learn.microsoft.com/en-us/windows-hardware/drivers/sensors/sensors-acpi-entries
+ * This is a Microsoft extension and not part of the official ACPI spec.
+ * The method name is configurable because some dual-accel setups define 2 mount
+ * matrices in a single ACPI device using separate "ROMK" and "ROMS" methods.
+ */
 static inline bool acpi_read_mount_matrix(struct device *dev,
 					  struct iio_mount_matrix *orientation,
 					  char *acpi_method)

Regards,

Hans



>> ---
>>  drivers/iio/accel/acpi-helpers.h | 76 ++++++++++++++++++++++++++++++++
>>  drivers/iio/accel/kxcjk-1013.c   | 71 ++---------------------------
>>  2 files changed, 79 insertions(+), 68 deletions(-)
>>  create mode 100644 drivers/iio/accel/acpi-helpers.h
>>
>> diff --git a/drivers/iio/accel/acpi-helpers.h b/drivers/iio/accel/acpi-helpers.h
>> new file mode 100644
>> index 000000000000..a4357925bf07
>> --- /dev/null
>> +++ b/drivers/iio/accel/acpi-helpers.h
>> @@ -0,0 +1,76 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* ACPI helper functions for parsing ACPI rotation matrices */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/sprintf.h>
>> +
>> +#ifdef CONFIG_ACPI
>> +static inline bool acpi_read_mount_matrix(struct device *dev,
>> +					  struct iio_mount_matrix *orientation,
>> +					  char *acpi_method)
>> +{
>> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +	struct acpi_device *adev = ACPI_COMPANION(dev);
>> +	char *str;
>> +	union acpi_object *obj, *elements;
>> +	acpi_status status;
>> +	int i, j, val[3];
>> +	bool ret = false;
>> +
>> +	if (!adev || !acpi_has_method(adev->handle, acpi_method))
>> +		return false;
>> +
>> +	status = acpi_evaluate_object(adev->handle, acpi_method, NULL, &buffer);
>> +	if (ACPI_FAILURE(status)) {
>> +		dev_err(dev, "Failed to get ACPI mount matrix: %d\n", status);
>> +		return false;
>> +	}
>> +
>> +	obj = buffer.pointer;
>> +	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 3) {
>> +		dev_err(dev, "Unknown ACPI mount matrix package format\n");
>> +		goto out_free_buffer;
>> +	}
>> +
>> +	elements = obj->package.elements;
>> +	for (i = 0; i < 3; i++) {
>> +		if (elements[i].type != ACPI_TYPE_STRING) {
>> +			dev_err(dev, "Unknown ACPI mount matrix element format\n");
>> +			goto out_free_buffer;
>> +		}
>> +
>> +		str = elements[i].string.pointer;
>> +		if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3) {
>> +			dev_err(dev, "Incorrect ACPI mount matrix string format\n");
>> +			goto out_free_buffer;
>> +		}
>> +
>> +		for (j = 0; j < 3; j++) {
>> +			switch (val[j]) {
>> +			case -1: str = "-1"; break;
>> +			case 0:  str = "0";  break;
>> +			case 1:  str = "1";  break;
>> +			default:
>> +				dev_err(dev, "Invalid value in ACPI mount matrix: %d\n", val[j]);
>> +				goto out_free_buffer;
>> +			}
>> +			orientation->rotation[i * 3 + j] = str;
>> +		}
>> +	}
>> +
>> +	ret = true;
>> +
>> +out_free_buffer:
>> +	kfree(buffer.pointer);
>> +	return ret;
>> +}
>> +#else
>> +static inline bool acpi_read_mount_matrix(struct device *dev,
>> +					  struct iio_mount_matrix *orientation,
>> +					  char *acpi_method)
>> +{
>> +	return false;
>> +}
>> +#endif
>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>> index bb1660667bb0..7e19278491dc 100644
>> --- a/drivers/iio/accel/kxcjk-1013.c
>> +++ b/drivers/iio/accel/kxcjk-1013.c
>> @@ -24,6 +24,8 @@
>>  #include <linux/iio/triggered_buffer.h>
>>  #include <linux/iio/accel/kxcjk_1013.h>
>>  
>> +#include "acpi-helpers.h"
>> +
>>  #define KXCJK1013_DRV_NAME "kxcjk1013"
>>  #define KXCJK1013_IRQ_NAME "kxcjk1013_event"
>>  
>> @@ -636,73 +638,6 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
>>  	return 0;
>>  }
>>  
>> -#ifdef CONFIG_ACPI
>> -static bool kxj1009_apply_acpi_orientation(struct device *dev,
>> -					   struct iio_mount_matrix *orientation)
>> -{
>> -	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> -	struct acpi_device *adev = ACPI_COMPANION(dev);
>> -	char *str;
>> -	union acpi_object *obj, *elements;
>> -	acpi_status status;
>> -	int i, j, val[3];
>> -	bool ret = false;
>> -
>> -	if (!adev || !acpi_has_method(adev->handle, "ROTM"))
>> -		return false;
>> -
>> -	status = acpi_evaluate_object(adev->handle, "ROTM", NULL, &buffer);
>> -	if (ACPI_FAILURE(status)) {
>> -		dev_err(dev, "Failed to get ACPI mount matrix: %d\n", status);
>> -		return false;
>> -	}
>> -
>> -	obj = buffer.pointer;
>> -	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 3) {
>> -		dev_err(dev, "Unknown ACPI mount matrix package format\n");
>> -		goto out_free_buffer;
>> -	}
>> -
>> -	elements = obj->package.elements;
>> -	for (i = 0; i < 3; i++) {
>> -		if (elements[i].type != ACPI_TYPE_STRING) {
>> -			dev_err(dev, "Unknown ACPI mount matrix element format\n");
>> -			goto out_free_buffer;
>> -		}
>> -
>> -		str = elements[i].string.pointer;
>> -		if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3) {
>> -			dev_err(dev, "Incorrect ACPI mount matrix string format\n");
>> -			goto out_free_buffer;
>> -		}
>> -
>> -		for (j = 0; j < 3; j++) {
>> -			switch (val[j]) {
>> -			case -1: str = "-1"; break;
>> -			case 0:  str = "0";  break;
>> -			case 1:  str = "1";  break;
>> -			default:
>> -				dev_err(dev, "Invalid value in ACPI mount matrix: %d\n", val[j]);
>> -				goto out_free_buffer;
>> -			}
>> -			orientation->rotation[i * 3 + j] = str;
>> -		}
>> -	}
>> -
>> -	ret = true;
>> -
>> -out_free_buffer:
>> -	kfree(buffer.pointer);
>> -	return ret;
>> -}
>> -#else
>> -static bool kxj1009_apply_acpi_orientation(struct device *dev,
>> -					  struct iio_mount_matrix *orientation)
>> -{
>> -	return false;
>> -}
>> -#endif
>> -
>>  static int kxcjk1013_chip_update_thresholds(struct kxcjk1013_data *data)
>>  {
>>  	int ret;
>> @@ -1533,7 +1468,7 @@ static int kxcjk1013_probe(struct i2c_client *client)
>>  	} else {
>>  		data->active_high_intr = true; /* default polarity */
>>  
>> -		if (!kxj1009_apply_acpi_orientation(&client->dev, &data->orientation)) {
>> +		if (!acpi_read_mount_matrix(&client->dev, &data->orientation, "ROTM")) {
>>  			ret = iio_read_mount_matrix(&client->dev, &data->orientation);
>>  			if (ret)
>>  				return ret;
> 

Re: [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h

[Jonathan Cameron]

On Mon, 22 Apr 2024 11:18:26 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 4/20/24 1:13 PM, Jonathan Cameron wrote:
> > On Wed, 17 Apr 2024 18:46:14 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >   
> >> The ACPI "ROTM" rotation matrix parsing code atm is already duplicated
> >> between bmc150-accel-core.c and kxcjk-1013.c and a third user of this is
> >> coming.
> >>
> >> Move the ROTM parsing from kxcjk-1013.c, which has slightly better error
> >> logging (and otherwise is 100% identical), into a new acpi-helpers.h file
> >> so that it can be shared.
> >>
> >> Other then moving the code the only 2 other changes are:
> >>
> >> 1. Rename the function to acpi_read_mount_matrix() to make clear this
> >>    is a generic ACPI mount matrix read function.
> >> 2. Add a "char *acpi_method" parameter since some bmc150 dual-accel setups
> >>    (360° hinges with 1 accel in kbd/base + 1 in display half) declare both
> >>    accels in a single ACPI device with 2 different method names for
> >>    the 2 matrices.
> >>
> >> Cc: Sean Rhodes <sean@starlabs.systems>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>  
> > 
> > Tempted to ask you to rename this to
> > acpi_non_standard_microsoft_extension_that_they_never_agreed_with_aswg_read_mount_matrix()
> > but meh, I'll cope with a reference to:
> > https://learn.microsoft.com/en-us/windows-hardware/drivers/sensors/sensors-acpi-entries
> > and a comment saying this is not part of the ACPI standard.  
> 
> Ok, so I have added the following comment to the v2 which I will send out soon:
> 
> diff --git a/drivers/iio/accel/acpi-helpers.h b/drivers/iio/accel/acpi-helpers.h
> index a4357925bf07..4f4140694b59 100644
> --- a/drivers/iio/accel/acpi-helpers.h
> +++ b/drivers/iio/accel/acpi-helpers.h
> @@ -7,6 +7,13 @@
>  #include <linux/sprintf.h>
>  
>  #ifdef CONFIG_ACPI
> +/*
> + * Parse mount matrixes defined in the ACPI "ROTM" format from:
> + * https://learn.microsoft.com/en-us/windows-hardware/drivers/sensors/sensors-acpi-entries
> + * This is a Microsoft extension and not part of the official ACPI spec.
> + * The method name is configurable because some dual-accel setups define 2 mount
> + * matrices in a single ACPI device using separate "ROMK" and "ROMS" methods.
LGTM

> + */
>  static inline bool acpi_read_mount_matrix(struct device *dev,
>  					  struct iio_mount_matrix *orientation,
>  					  char *acpi_method)
> 
> Regards,
> 
> Hans