On Thu, 8 Feb 2024 21:03:26 +0000
Sean Rhodes <sean@starlabs.systems> wrote:
> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@xxxxxxxxxxxxxx>
> Signed-off-by: Sean Rhodes <sean@starlabs.systems>
Hi Sean.
Please make sure to match local style for patch titles.
Something along the lines of:
iio: accel: kxcjk-1013: Implement ACPI method ROTM to retrieve mount matrix.
A few minor things inline. Otherwise this looks fine.
At somepoint it might be nice to factor out a library for this call
as it seems to be reasonably common (I think it's a windows thing as it's not
standard ACPI spec stuff). *mutters darkly that they should at least have
reserved the ID rather than making it up*
Jonathan
> ---
> drivers/iio/accel/kxcjk-1013.c | 86 ++++++++++++++++++++++++++++++++--
> 1 file changed, 83 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 894709286b0c..f5ac06d5566c 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -619,6 +619,83 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
> return 0;
> }
>
> +#ifdef CONFIG_ACPI
> +static bool kxj_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 (!acpi_has_method(adev->handle, "ROTM")) {
> + return false;
> + }
Kernel style is only use brackets for multiple line statements.
if (!..)
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);
> + goto out;
It would be unusual if this has as side effect on error of leaving the buffer
allocated. Looking at the code there might be a path, if the copy fails,
acpi_ut_copy_iobject_to_eobject() but I think that can't actually fail because
we've already checked the source object is valid when querying it's size etc.
Other cases (I checked a couple only ) we have in tree assume that we don't need to free the buffer
if we fail this call.
> + }
> +
> + 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;
> + }
> +
> + 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;
> + }
> +
> + 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;
> + }
> +
> + 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;
? Where does that go?
> + }
> + orientation->rotation[i * 3 + j] = str;
> + }
> + }
> +
> + ret = true;
> +
> +out:
> + 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)
> +{
> + return false;
> +}
> +#endif
> +
> static int kxcjk1013_chip_update_thresholds(struct kxcjk1013_data *data)
> {
> int ret;
> @@ -1449,9 +1526,12 @@ static int kxcjk1013_probe(struct i2c_client *client)
> } else {
> data->active_high_intr = true; /* default polarity */
>
> - ret = iio_read_mount_matrix(&client->dev, &data->orientation);
> - if (ret)
> - return ret;
> + if (!kxj1009_apply_acpi_orientation(&client->dev, &data->orientation)) {
> + ret = iio_read_mount_matrix(&client->dev, &data->orientation);
> + if (ret)
> + return ret;
> + }
> +
> }
>
> ret = devm_regulator_bulk_get_enable(&client->dev,
