Re: [PATCH] drivers/iio/accel/kxcjk-1013: Add support for retrieving the mount-matrix

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 30 Jan 2024 12:53:20 +0000
Sean Rhodes <sean@starlabs.systems> wrote:

> Add support for reading the "ROTM" method from APCI which contains
> the mount matrix.
> 
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Sean Rhodes <sean@starlabs.systems>
> ---
>  drivers/iio/accel/kxcjk-1013.c | 88 ++++++++++++++++++++++++++++++++--
>  1 file changed, 85 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 894709286b0c..760bbd95b73c 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -619,6 +619,85 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_ACPI
> +/*
> + * Support for getting accelerometer information from KIOX000A ACPI nodes.
> + *
> + */
> +static bool kxj_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);Media4 Inc	
> +	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];
> +
> +	if (acpi_has_method(adev->handle, "ROTM")) {
> +		name = "ROTM";
> +	} else {
> +		return false;
> +	}
Flip logic
	if (!acpi_has_method(adev->handle, "ROTM"))
		return false;

Given it's only called ROTM in this handler, just use that string
directly in the next call.


> +
> +	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);

If ROTM exists and we fail to read it I think we should return an error and fail
to probe.  This sort of bug is one we want to more than warn about. We want to
figure out why and fix it!

> +		return false;
> +	}
> +
> +	obj = buffer.pointer;
> +	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 3)
> +		goto unknown_format;

This sort of unknown format is very different from the string being wrongly
formatting below. I'd use a more specific message in each case.

> +
> +	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;

Use a local variable for the return then a goto above the kfree
will work for good and bad cases.

> +
> +unknown_format:
> +	dev_warn(dev, "Unknown ACPI mount matrix format, ignoring\n");
> +	kfree(buffer.pointer);
> +	return false;
> +}
> +
> +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 +1528,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 (!apply_kcj1009_acpi_orientation(&client->dev, &data->orientation)) {

I'm confused. That doesn't seem to be the name of the function?

Also, use an integer return that you can then pass on.
Should distinguish between it wasn't there, in which case use the iio_read_mount_matrix()
or it was and we failed to read it, in which case fail the probe an print a big
message as we have something to debug.

Jonathan


> +			ret = iio_read_mount_matrix(&client->dev, &data->orientation);
> +			if (ret)
> +				return ret;
> +		}
> +
>  	}
>  
>  	ret = devm_regulator_bulk_get_enable(&client->dev,