* [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005
@ 2024-04-17 16:46 Hans de Goede
2024-04-17 16:46 ` [PATCH 1/4] iio: accel: kxcjk-1013: Simplify ACPI ROTM mount matrix retreival Hans de Goede
` (6 more replies)
0 siblings, 7 replies; 17+ messages in thread
From: Hans de Goede @ 2024-04-17 16:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hans de Goede, Lars-Peter Clausen, Sean Rhodes, linux-iio
Hi All,
Here is a patch series to refactor the ACPI ROTM orientation matrix
handling in kxcjk-1013 + bmc150-accel to share the code instead of
having 2 copies and then also use the shared implementation in
the mxc4005 driver since some MXC6655 ACPI firmware nodes also
include this.
Note the mxc4005 support is untested, I will ask the report of:
https://bugzilla.kernel.org/show_bug.cgi?id=218578 to test.
Regards,
Hans
Hans de Goede (4):
iio: accel: kxcjk-1013: Simplify ACPI ROTM mount matrix retreival
iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h
iio: bmc150-accel-core: Use acpi_read_mount_matrix() helper
iio: accel: mxc4005: Read orientation matrix from ACPI ROTM method
drivers/iio/accel/acpi-helpers.h | 76 +++++++++++++++++++++++++
drivers/iio/accel/bmc150-accel-core.c | 45 +--------------
drivers/iio/accel/kxcjk-1013.c | 82 +--------------------------
drivers/iio/accel/mxc4005.c | 24 ++++++++
4 files changed, 106 insertions(+), 121 deletions(-)
create mode 100644 drivers/iio/accel/acpi-helpers.h
--
2.44.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/4] iio: accel: kxcjk-1013: Simplify ACPI ROTM mount matrix retreival
2024-04-17 16:46 [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
@ 2024-04-17 16:46 ` Hans de Goede
2024-04-17 16:46 ` [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h Hans de Goede
` (5 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2024-04-17 16:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hans de Goede, Lars-Peter Clausen, Sean Rhodes, linux-iio
There is no reason to limit the parsing of the ROTM mount matrix to only
ACPI devices with an ACPI HID (Hardware-ID) of "KIOX000A". If kxcjk-1013
ACPI devices with another HID have a ROTM method that should still be
parsed and if the method is not there then the:
if (!adev || !acpi_has_method(adev->handle, "ROTM"))
return false;
check will skip checking the ROTM method silently.
Move the check to see if there is an ACPI companion at all into
kxj_acpi_orientation() and drop the wrapper checking the HID.
Cc: Sean Rhodes <sean@starlabs.systems>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/iio/accel/kxcjk-1013.c | 17 +++--------------
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 126e8bdd6d0e..bb1660667bb0 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -637,8 +637,8 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
}
#ifdef CONFIG_ACPI
-static bool kxj_acpi_orientation(struct device *dev,
- struct iio_mount_matrix *orientation)
+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);
@@ -648,7 +648,7 @@ static bool kxj_acpi_orientation(struct device *dev,
int i, j, val[3];
bool ret = false;
- if (!acpi_has_method(adev->handle, "ROTM"))
+ if (!adev || !acpi_has_method(adev->handle, "ROTM"))
return false;
status = acpi_evaluate_object(adev->handle, "ROTM", NULL, &buffer);
@@ -695,17 +695,6 @@ static bool kxj_acpi_orientation(struct device *dev,
kfree(buffer.pointer);
return ret;
}
-
-static bool kxj1009_apply_acpi_orientation(struct device *dev,
- struct iio_mount_matrix *orientation)
-{
- struct acpi_device *adev = ACPI_COMPANION(dev);
-
- if (adev && acpi_dev_hid_uid_match(adev, "KIOX000A", NULL))
- return kxj_acpi_orientation(dev, orientation);
-
- return false;
-}
#else
static bool kxj1009_apply_acpi_orientation(struct device *dev,
struct iio_mount_matrix *orientation)
--
2.44.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h
2024-04-17 16:46 [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
2024-04-17 16:46 ` [PATCH 1/4] iio: accel: kxcjk-1013: Simplify ACPI ROTM mount matrix retreival Hans de Goede
@ 2024-04-17 16:46 ` Hans de Goede
2024-04-20 11:13 ` Jonathan Cameron
2024-04-17 16:46 ` [PATCH 3/4] iio: bmc150-accel-core: Use acpi_read_mount_matrix() helper Hans de Goede
` (4 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2024-04-17 16:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hans de Goede, Lars-Peter Clausen, Sean Rhodes, linux-iio
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>
---
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;
--
2.44.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/4] iio: bmc150-accel-core: Use acpi_read_mount_matrix() helper
2024-04-17 16:46 [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
2024-04-17 16:46 ` [PATCH 1/4] iio: accel: kxcjk-1013: Simplify ACPI ROTM mount matrix retreival Hans de Goede
2024-04-17 16:46 ` [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h Hans de Goede
@ 2024-04-17 16:46 ` Hans de Goede
2024-04-17 16:46 ` [PATCH 4/4] iio: accel: mxc4005: Read orientation matrix from ACPI ROTM method Hans de Goede
` (3 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2024-04-17 16:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hans de Goede, Lars-Peter Clausen, Sean Rhodes, linux-iio
Replace the duplicate ACPI "ROTM" data parsing code with the new
shared acpi_read_mount_matrix() helper.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/iio/accel/bmc150-accel-core.c | 45 ++-------------------------
1 file changed, 3 insertions(+), 42 deletions(-)
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 110591804b4c..ace550b0ba40 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -23,6 +23,7 @@
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
+#include "acpi-helpers.h"
#include "bmc150-accel.h"
#define BMC150_ACCEL_DRV_NAME "bmc150_accel"
@@ -386,13 +387,9 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
static bool bmc150_apply_bosc0200_acpi_orientation(struct device *dev,
struct iio_mount_matrix *orientation)
{
- struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
struct iio_dev *indio_dev = dev_get_drvdata(dev);
struct acpi_device *adev = ACPI_COMPANION(dev);
- char *name, *alt_name, *label, *str;
- union acpi_object *obj, *elements;
- acpi_status status;
- int i, j, val[3];
+ char *name, *alt_name, *label;
if (strcmp(dev_name(dev), "i2c-BOSC0200:base") == 0) {
alt_name = "ROMK";
@@ -411,43 +408,7 @@ static bool bmc150_apply_bosc0200_acpi_orientation(struct device *dev,
return false;
}
- status = acpi_evaluate_object(adev->handle, name, NULL, &buffer);
- if (ACPI_FAILURE(status)) {
- dev_warn(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)
- goto unknown_format;
-
- elements = obj->package.elements;
- for (i = 0; i < 3; i++) {
- if (elements[i].type != ACPI_TYPE_STRING)
- goto unknown_format;
-
- str = elements[i].string.pointer;
- if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3)
- goto unknown_format;
-
- 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: goto unknown_format;
- }
- orientation->rotation[i * 3 + j] = str;
- }
- }
-
- kfree(buffer.pointer);
- return true;
-
-unknown_format:
- dev_warn(dev, "Unknown ACPI mount matrix format, ignoring\n");
- kfree(buffer.pointer);
- return false;
+ return acpi_read_mount_matrix(dev, orientation, name);
}
static bool bmc150_apply_dual250e_acpi_orientation(struct device *dev,
--
2.44.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/4] iio: accel: mxc4005: Read orientation matrix from ACPI ROTM method
2024-04-17 16:46 [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
` (2 preceding siblings ...)
2024-04-17 16:46 ` [PATCH 3/4] iio: bmc150-accel-core: Use acpi_read_mount_matrix() helper Hans de Goede
@ 2024-04-17 16:46 ` Hans de Goede
2024-04-19 7:36 ` [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
` (2 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2024-04-17 16:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hans de Goede, Lars-Peter Clausen, Sean Rhodes, linux-iio
Some devices use the semi-standard ACPI "ROTM" method to store
the accelerometers orientation matrix.
Add support for this using the new acpi_read_mount_matrix() helper, if
the helper fails to read the matrix fall back to iio_read_mount_matrix()
which will try to get it from device-properties (devicetree) and if
that fails it will fill the matrix with the identity matrix.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218578
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/iio/accel/mxc4005.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
index 9f38d3a08299..c54c98a4d902 100644
--- a/drivers/iio/accel/mxc4005.c
+++ b/drivers/iio/accel/mxc4005.c
@@ -17,6 +17,8 @@
#include <linux/iio/triggered_buffer.h>
#include <linux/iio/trigger_consumer.h>
+#include "acpi-helpers.h"
+
#define MXC4005_DRV_NAME "mxc4005"
#define MXC4005_IRQ_NAME "mxc4005_event"
#define MXC4005_REGMAP_NAME "mxc4005_regmap"
@@ -65,6 +67,7 @@ struct mxc4005_data {
struct mutex mutex;
struct regmap *regmap;
struct iio_trigger *dready_trig;
+ struct iio_mount_matrix orientation;
/* Ensure timestamp is naturally aligned */
struct {
__be16 chans[3];
@@ -272,6 +275,20 @@ static int mxc4005_write_raw(struct iio_dev *indio_dev,
}
}
+static const struct iio_mount_matrix *
+mxc4005_get_mount_matrix(const struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct mxc4005_data *data = iio_priv(indio_dev);
+
+ return &data->orientation;
+}
+
+static const struct iio_chan_spec_ext_info mxc4005_ext_info[] = {
+ IIO_MOUNT_MATRIX(IIO_SHARED_BY_TYPE, mxc4005_get_mount_matrix),
+ { }
+};
+
static const struct iio_info mxc4005_info = {
.read_raw = mxc4005_read_raw,
.write_raw = mxc4005_write_raw,
@@ -298,6 +315,7 @@ static const unsigned long mxc4005_scan_masks[] = {
.shift = 4, \
.endianness = IIO_BE, \
}, \
+ .ext_info = mxc4005_ext_info, \
}
static const struct iio_chan_spec mxc4005_channels[] = {
@@ -440,6 +458,12 @@ static int mxc4005_probe(struct i2c_client *client)
mutex_init(&data->mutex);
+ if (!acpi_read_mount_matrix(&client->dev, &data->orientation, "ROTM")) {
+ ret = iio_read_mount_matrix(&client->dev, &data->orientation);
+ if (ret)
+ return ret;
+ }
+
indio_dev->channels = mxc4005_channels;
indio_dev->num_channels = ARRAY_SIZE(mxc4005_channels);
indio_dev->available_scan_masks = mxc4005_scan_masks;
--
2.44.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005
2024-04-17 16:46 [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
` (3 preceding siblings ...)
2024-04-17 16:46 ` [PATCH 4/4] iio: accel: mxc4005: Read orientation matrix from ACPI ROTM method Hans de Goede
@ 2024-04-19 7:36 ` Hans de Goede
2024-04-22 7:51 ` Andy Shevchenko
2024-04-22 7:55 ` Andy Shevchenko
6 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2024-04-19 7:36 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Lars-Peter Clausen, Sean Rhodes, linux-iio
On 4/17/24 6:46 PM, Hans de Goede wrote:
> Hi All,
>
> Here is a patch series to refactor the ACPI ROTM orientation matrix
> handling in kxcjk-1013 + bmc150-accel to share the code instead of
> having 2 copies and then also use the shared implementation in
> the mxc4005 driver since some MXC6655 ACPI firmware nodes also
> include this.
>
> Note the mxc4005 support is untested, I will ask the report of:
> https://bugzilla.kernel.org/show_bug.cgi?id=218578 to test.
The ROTM support has been tested on a tablet with a MXC6655 now
and has been confirmed to work.
Regards,
Hans
> Hans de Goede (4):
> iio: accel: kxcjk-1013: Simplify ACPI ROTM mount matrix retreival
> iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h
> iio: bmc150-accel-core: Use acpi_read_mount_matrix() helper
> iio: accel: mxc4005: Read orientation matrix from ACPI ROTM method
>
> drivers/iio/accel/acpi-helpers.h | 76 +++++++++++++++++++++++++
> drivers/iio/accel/bmc150-accel-core.c | 45 +--------------
> drivers/iio/accel/kxcjk-1013.c | 82 +--------------------------
> drivers/iio/accel/mxc4005.c | 24 ++++++++
> 4 files changed, 106 insertions(+), 121 deletions(-)
> create mode 100644 drivers/iio/accel/acpi-helpers.h
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h
2024-04-17 16:46 ` [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h Hans de Goede
@ 2024-04-20 11:13 ` Jonathan Cameron
2024-04-22 9:17 ` Hans de Goede
2024-04-22 9:18 ` Hans de Goede
0 siblings, 2 replies; 17+ messages in thread
From: Jonathan Cameron @ 2024-04-20 11:13 UTC (permalink / raw)
To: Hans de Goede
Cc: Lars-Peter Clausen, Sean Rhodes, linux-iio, linux-acpi,
Rafael J. Wysocki
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;
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005
2024-04-17 16:46 [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
` (4 preceding siblings ...)
2024-04-19 7:36 ` [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
@ 2024-04-22 7:51 ` Andy Shevchenko
2024-04-22 7:55 ` Andy Shevchenko
6 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2024-04-22 7:51 UTC (permalink / raw)
To: Hans de Goede
Cc: Jonathan Cameron, Lars-Peter Clausen, Sean Rhodes, linux-iio
Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti:
> Hi All,
>
> Here is a patch series to refactor the ACPI ROTM orientation matrix
> handling in kxcjk-1013 + bmc150-accel to share the code instead of
> having 2 copies and then also use the shared implementation in
> the mxc4005 driver since some MXC6655 ACPI firmware nodes also
> include this.
You beat me up to it, as I (used to?) have it in my TODO list for a couple of
years or so. Thanks for doing this!
> Note the mxc4005 support is untested, I will ask the report of:
> https://bugzilla.kernel.org/show_bug.cgi?id=218578 to test.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005
2024-04-17 16:46 [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
` (5 preceding siblings ...)
2024-04-22 7:51 ` Andy Shevchenko
@ 2024-04-22 7:55 ` Andy Shevchenko
2024-04-22 8:24 ` Hans de Goede
6 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-04-22 7:55 UTC (permalink / raw)
To: Hans de Goede
Cc: Jonathan Cameron, Lars-Peter Clausen, Sean Rhodes, linux-iio
Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti:
> Hi All,
>
> Here is a patch series to refactor the ACPI ROTM orientation matrix
> handling in kxcjk-1013 + bmc150-accel to share the code instead of
> having 2 copies and then also use the shared implementation in
> the mxc4005 driver since some MXC6655 ACPI firmware nodes also
> include this.
>
> Note the mxc4005 support is untested, I will ask the report of:
I have briefly looked into this and I like this, except the part of the big
function being in the header. Why? Why can't it be in a C-file?
Note, 3 users justify very well to me that shared code, should be shared in
binary as well. (I.o.w. you may argue that IRL there will be no more than
one of such device connected, but in case of DIY and prototyping it might
still be the use case.)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005
2024-04-22 7:55 ` Andy Shevchenko
@ 2024-04-22 8:24 ` Hans de Goede
2024-04-22 11:33 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2024-04-22 8:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Sean Rhodes, linux-iio
Hi,
On 4/22/24 9:55 AM, Andy Shevchenko wrote:
> Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti:
>> Hi All,
>>
>> Here is a patch series to refactor the ACPI ROTM orientation matrix
>> handling in kxcjk-1013 + bmc150-accel to share the code instead of
>> having 2 copies and then also use the shared implementation in
>> the mxc4005 driver since some MXC6655 ACPI firmware nodes also
>> include this.
>>
>> Note the mxc4005 support is untested, I will ask the report of:
>
> I have briefly looked into this and I like this, except the part of the big
> function being in the header. Why? Why can't it be in a C-file?
>
> Note, 3 users justify very well to me that shared code, should be shared in
> binary as well. (I.o.w. you may argue that IRL there will be no more than
> one of such device connected, but in case of DIY and prototyping it might
> still be the use case.)
It is only 1 function and it is not that big. IMHO the static inline
in a header solution here is much better then making this a separate .ko
file with all the associated overhead.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h
2024-04-20 11:13 ` Jonathan Cameron
@ 2024-04-22 9:17 ` Hans de Goede
2024-04-22 9:18 ` Hans de Goede
1 sibling, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2024-04-22 9:17 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Sean Rhodes, linux-iio, linux-acpi,
Rafael J. Wysocki
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;
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h
2024-04-20 11:13 ` Jonathan Cameron
2024-04-22 9:17 ` Hans de Goede
@ 2024-04-22 9:18 ` Hans de Goede
2024-04-22 17:06 ` Jonathan Cameron
1 sibling, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2024-04-22 9:18 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Sean Rhodes, linux-iio, linux-acpi,
Rafael J. Wysocki
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;
>
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005
2024-04-22 8:24 ` Hans de Goede
@ 2024-04-22 11:33 ` Andy Shevchenko
2024-04-22 11:45 ` Hans de Goede
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-04-22 11:33 UTC (permalink / raw)
To: Hans de Goede
Cc: Jonathan Cameron, Lars-Peter Clausen, Sean Rhodes, linux-iio
On Mon, Apr 22, 2024 at 11:24 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 4/22/24 9:55 AM, Andy Shevchenko wrote:
> > Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti:
...
> > I have briefly looked into this and I like this, except the part of the big
> > function being in the header. Why? Why can't it be in a C-file?
> >
> > Note, 3 users justify very well to me that shared code, should be shared in
> > binary as well. (I.o.w. you may argue that IRL there will be no more than
> > one of such device connected, but in case of DIY and prototyping it might
> > still be the use case.)
>
> It is only 1 function and it is not that big. IMHO the static inline
> in a header solution here is much better then making this a separate .ko
> file with all the associated overhead.
Look how the i8042 RTC header became a disaster. :-)
Nevertheless, this can be part of the IIO core for the ACPI enabled
kernels. Which eliminates a need for a separate module.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005
2024-04-22 11:33 ` Andy Shevchenko
@ 2024-04-22 11:45 ` Hans de Goede
2024-04-22 12:28 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2024-04-22 11:45 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Sean Rhodes, linux-iio
Hi,
On 4/22/24 1:33 PM, Andy Shevchenko wrote:
> On Mon, Apr 22, 2024 at 11:24 AM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 4/22/24 9:55 AM, Andy Shevchenko wrote:
>>> Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti:
>
> ...
>
>>> I have briefly looked into this and I like this, except the part of the big
>>> function being in the header. Why? Why can't it be in a C-file?
>>>
>>> Note, 3 users justify very well to me that shared code, should be shared in
>>> binary as well. (I.o.w. you may argue that IRL there will be no more than
>>> one of such device connected, but in case of DIY and prototyping it might
>>> still be the use case.)
>>
>> It is only 1 function and it is not that big. IMHO the static inline
>> in a header solution here is much better then making this a separate .ko
>> file with all the associated overhead.
>
> Look how the i8042 RTC header became a disaster. :-)
> Nevertheless, this can be part of the IIO core for the ACPI enabled
> kernels. Which eliminates a need for a separate module.
Putting this in the IIO core, with an iio-prefix, so say something like:
#ifdef CONFIG_ACPI
bool iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name);
#else
static inline bool
iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name)
{
return false;
}
in include/linux/iio/iio.h ?
works for me and that also avoids Jonathan's worry about using an acpi_
prefix in iio code.
Jonathan how does that sound to you ?
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005
2024-04-22 11:45 ` Hans de Goede
@ 2024-04-22 12:28 ` Andy Shevchenko
2024-04-22 17:05 ` Jonathan Cameron
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2024-04-22 12:28 UTC (permalink / raw)
To: Hans de Goede
Cc: Jonathan Cameron, Lars-Peter Clausen, Sean Rhodes, linux-iio
On Mon, Apr 22, 2024 at 2:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 4/22/24 1:33 PM, Andy Shevchenko wrote:
> > On Mon, Apr 22, 2024 at 11:24 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 4/22/24 9:55 AM, Andy Shevchenko wrote:
> >>> Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti:
...
> >>> I have briefly looked into this and I like this, except the part of the big
> >>> function being in the header. Why? Why can't it be in a C-file?
> >>>
> >>> Note, 3 users justify very well to me that shared code, should be shared in
> >>> binary as well. (I.o.w. you may argue that IRL there will be no more than
> >>> one of such device connected, but in case of DIY and prototyping it might
> >>> still be the use case.)
> >>
> >> It is only 1 function and it is not that big. IMHO the static inline
> >> in a header solution here is much better then making this a separate .ko
> >> file with all the associated overhead.
> >
> > Look how the i8042 RTC header became a disaster. :-)
> > Nevertheless, this can be part of the IIO core for the ACPI enabled
> > kernels. Which eliminates a need for a separate module.
>
> Putting this in the IIO core, with an iio-prefix, so say something like:
>
> #ifdef CONFIG_ACPI
> bool iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name);
> #else
> static inline bool
> iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name)
> {
> return false;
> }
>
> in include/linux/iio/iio.h ?
Yes, like SPI, I²C, etc. do in similar cases.
> works for me and that also avoids Jonathan's worry about using an acpi_
> prefix in iio code.
>
> Jonathan how does that sound to you ?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005
2024-04-22 12:28 ` Andy Shevchenko
@ 2024-04-22 17:05 ` Jonathan Cameron
0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2024-04-22 17:05 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, Jonathan Cameron, Lars-Peter Clausen, Sean Rhodes,
linux-iio
On Mon, 22 Apr 2024 15:28:48 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Apr 22, 2024 at 2:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 4/22/24 1:33 PM, Andy Shevchenko wrote:
> > > On Mon, Apr 22, 2024 at 11:24 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > >> On 4/22/24 9:55 AM, Andy Shevchenko wrote:
> > >>> Wed, Apr 17, 2024 at 06:46:12PM +0200, Hans de Goede kirjoitti:
>
> ...
>
> > >>> I have briefly looked into this and I like this, except the part of the big
> > >>> function being in the header. Why? Why can't it be in a C-file?
> > >>>
> > >>> Note, 3 users justify very well to me that shared code, should be shared in
> > >>> binary as well. (I.o.w. you may argue that IRL there will be no more than
> > >>> one of such device connected, but in case of DIY and prototyping it might
> > >>> still be the use case.)
> > >>
> > >> It is only 1 function and it is not that big. IMHO the static inline
> > >> in a header solution here is much better then making this a separate .ko
> > >> file with all the associated overhead.
> > >
> > > Look how the i8042 RTC header became a disaster. :-)
> > > Nevertheless, this can be part of the IIO core for the ACPI enabled
> > > kernels. Which eliminates a need for a separate module.
> >
> > Putting this in the IIO core, with an iio-prefix, so say something like:
> >
> > #ifdef CONFIG_ACPI
> > bool iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name);
> > #else
> > static inline bool
> > iio_read_acpi_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix, const char *method_name)
> > {
> > return false;
> > }
> >
> > in include/linux/iio/iio.h ?
>
> Yes, like SPI, I²C, etc. do in similar cases.
>
> > works for me and that also avoids Jonathan's worry about using an acpi_
> > prefix in iio code.
> >
> > Jonathan how does that sound to you ?
>
Good
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h
2024-04-22 9:18 ` Hans de Goede
@ 2024-04-22 17:06 ` Jonathan Cameron
0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2024-04-22 17:06 UTC (permalink / raw)
To: Hans de Goede
Cc: Jonathan Cameron, Lars-Peter Clausen, Sean Rhodes, linux-iio,
linux-acpi, Rafael J. Wysocki
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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-04-22 17:06 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-17 16:46 [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
2024-04-17 16:46 ` [PATCH 1/4] iio: accel: kxcjk-1013: Simplify ACPI ROTM mount matrix retreival Hans de Goede
2024-04-17 16:46 ` [PATCH 2/4] iio: accel: kxcjk-1013: Move ACPI ROTM parsing to new acpi-helpers.h Hans de Goede
2024-04-20 11:13 ` Jonathan Cameron
2024-04-22 9:17 ` Hans de Goede
2024-04-22 9:18 ` Hans de Goede
2024-04-22 17:06 ` Jonathan Cameron
2024-04-17 16:46 ` [PATCH 3/4] iio: bmc150-accel-core: Use acpi_read_mount_matrix() helper Hans de Goede
2024-04-17 16:46 ` [PATCH 4/4] iio: accel: mxc4005: Read orientation matrix from ACPI ROTM method Hans de Goede
2024-04-19 7:36 ` [PATCH 0/4] iio: accel: Share ACPI ROTM parsing between drivers and add it to mxc4005 Hans de Goede
2024-04-22 7:51 ` Andy Shevchenko
2024-04-22 7:55 ` Andy Shevchenko
2024-04-22 8:24 ` Hans de Goede
2024-04-22 11:33 ` Andy Shevchenko
2024-04-22 11:45 ` Hans de Goede
2024-04-22 12:28 ` Andy Shevchenko
2024-04-22 17:05 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.