Hi Linus,
On Sun, May 16, 2021 at 10:48:08AM +0100, Jonathan Cameron wrote:
> On Sat, 15 May 2021 02:00:58 +0200
> Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> > The ST accelerators support a special type of quirky
> > mounting matrix found in ACPI systems, but not a generic
> > mounting matrix such as from the device tree.
> >
> > Augment the ACPI hack to be a bit more generic and
> > accept a mounting matrix from device properties.
> >
> > This makes it possible to fix orientation on the Ux500
> > HREF device.
> >
> > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Cc: Denis Ciocca <denis.ciocca@xxxxxx>
> > Cc: Daniel Drake <drake@xxxxxxxxxxxx>
> > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > ---
> > drivers/iio/accel/st_accel_core.c | 51 ++++++++++++++++++++-----------
> > 1 file changed, 34 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> > index 43c50167d220..cfbcf740e3cb 100644
> > --- a/drivers/iio/accel/st_accel_core.c
> > +++ b/drivers/iio/accel/st_accel_core.c
> > @@ -1069,26 +1069,25 @@ static const struct iio_trigger_ops st_accel_trigger_ops = {
> > #define ST_ACCEL_TRIGGER_OPS NULL
> > #endif
> >
> > -#ifdef CONFIG_ACPI
> > static const struct iio_mount_matrix *
> > -get_mount_matrix(const struct iio_dev *indio_dev,
> > - const struct iio_chan_spec *chan)
> > +st_accel_get_mount_matrix(const struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan)
> > {
> > struct st_sensor_data *adata = iio_priv(indio_dev);
> >
> > return adata->mount_matrix;
> > }
> >
> > -static const struct iio_chan_spec_ext_info mount_matrix_ext_info[] = {
> > - IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, get_mount_matrix),
> > +static const struct iio_chan_spec_ext_info st_accel_mount_matrix_ext_info[] = {
> > + IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, st_accel_get_mount_matrix),
> > { },
> > };
> >
> > +#ifdef CONFIG_ACPI
> > /* Read ST-specific _ONT orientation data from ACPI and generate an
> > * appropriate mount matrix.
> > */
> > -static int apply_acpi_orientation(struct iio_dev *indio_dev,
> > - struct iio_chan_spec *channels)
> > +static int apply_acpi_orientation(struct iio_dev *indio_dev)
> > {
> > struct st_sensor_data *adata = iio_priv(indio_dev);
> > struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > @@ -1207,22 +1206,20 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev,
> > }
> > }
> >
> > - /* Expose the mount matrix via ext_info */
> > - for (i = 0; i < indio_dev->num_channels; i++)
> > - channels[i].ext_info = mount_matrix_ext_info;
> > -
> > ret = 0;
> > dev_info(&indio_dev->dev, "computed mount matrix from ACPI\n");
> >
> > out:
> > kfree(buffer.pointer);
> > + dev_warn(&indio_dev->dev,
> > + "failed to apply ACPI orientation data: %d\n", ret);
> > +
>
> As it would be valid to have a new ACPI table that uses a mount_matrix property
> rather than the ONT mess, perhaps we should demote the warnings to debug?
>
> > return ret;
> > }
> > #else /* !CONFIG_ACPI */
> > -static int apply_acpi_orientation(struct iio_dev *indio_dev,
> > - struct iio_chan_spec *channels)
> > +static int apply_acpi_orientation(struct iio_dev *indio_dev)
> > {
> > - return 0;
> > + return -EINVAL;
> > }
> > #endif
> >
> > @@ -1251,6 +1248,7 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
> > struct iio_chan_spec *channels;
> > size_t channels_size;
> > int err;
> > + int i;
> >
> > indio_dev->modes = INDIO_DIRECT_MODE;
> > indio_dev->info = &accel_info;
> > @@ -1275,9 +1273,28 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
> > goto st_accel_power_off;
> > }
> >
> > - if (apply_acpi_orientation(indio_dev, channels))
> > - dev_warn(&indio_dev->dev,
> > - "failed to apply ACPI orientation data: %d\n", err);
> > + /* First try ACPI orientation then try the generic function */
> > + err = apply_acpi_orientation(indio_dev);
> > + if (err) {
> > + adata->mount_matrix = devm_kmalloc(&indio_dev->dev,
> > + sizeof(*adata->mount_matrix),
> > + GFP_KERNEL);
> > + if (!adata->mount_matrix) {
> > + err = -ENOMEM;
> > + goto st_accel_power_off;
> > + }
> > +
> > + err = iio_read_mount_matrix(adata->dev, "mount-matrix",
> > + adata->mount_matrix);
> > + if (err)
> > + goto st_accel_power_off;
> > + }
> > + /*
> > + * We have at least an identity matrix, so expose the mount
> > + * matrix via ext_info
> > + */
> > + for (i = 0; i < indio_dev->num_channels; i++)
> > + channels[i].ext_info = st_accel_mount_matrix_ext_info;
>
Fun, I made pretty much the same patch at some point. Mine was really
bad though, since I additionally tried to detect if a mount-matrix is
defined in the device tree. (To keep the existing behavior that the
mount matrix is only exported if != identity. This behavior is
different from all other IIO drivers though so it doesn't make sense...)
I almost sent it a couple of days ago but then I realized that it's bad,
plus what Jonathan just mentioned:
> The current handling of this is odd. As the comment points out we are providing
> an orientation matrix whatever happens.
>
> As such I'm fairly sure we can rip out the duplication of the channels and just
> use the static ones, but with ext_info always set.
>
> Having done that you could also embed the mount_matrix in the private data and
> avoid the need to for a separate allocation. I guess that might add some small
> overhead to those sensors where orientation doesn't make sense, but I think the
> simplification is probably worth it.
>
I started implementing this last Tuesday but cannot test it myself
(I don't seem to have any device with a ST accelerometer so I'm still
waiting for some other people to test it). In case you didn't already
implement Jonathan's suggestion, feel free to take the diff below,
modify it further or throw it away if it's bad. :)
If I remember correctly, basically I tried to make the st_accel driver
implement the mount-matrix stuff similar to bmc150-accel-core, which has
similar ACPI code.
Stephan
---
>From aa4ea36374c6521674b3e1b6e3e5c4dc5a041081 Mon Sep 17 00:00:00 2001
From: Stephan Gerhold <stephan@xxxxxxxxxxx>
Date: Tue, 11 May 2021 14:45:09 +0200
Subject: [PATCH] iio: accel: st_accel_core: Read mount-matrix from device tree
Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
---
drivers/iio/accel/st_accel_core.c | 129 ++++++++++++--------------
include/linux/iio/common/st_sensors.h | 14 ++-
2 files changed, 71 insertions(+), 72 deletions(-)
diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index dc32ebefe3fc..3607745939e6 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -41,51 +41,74 @@
#define ST_ACCEL_FS_AVL_200G 200
#define ST_ACCEL_FS_AVL_400G 400
+static const struct iio_mount_matrix *
+get_mount_matrix(const struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct st_sensor_data *adata = iio_priv(indio_dev);
+
+ return &adata->mount_matrix;
+}
+
+static const struct iio_chan_spec_ext_info mount_matrix_ext_info[] = {
+ IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, get_mount_matrix),
+ { },
+};
+
static const struct iio_chan_spec st_accel_8bit_channels[] = {
- ST_SENSORS_LSM_CHANNELS(IIO_ACCEL,
+ ST_SENSORS_LSM_CHANNELS_EXT(IIO_ACCEL,
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
ST_SENSORS_SCAN_X, 1, IIO_MOD_X, 's', IIO_LE, 8, 8,
- ST_ACCEL_DEFAULT_OUT_X_L_ADDR+1),
- ST_SENSORS_LSM_CHANNELS(IIO_ACCEL,
+ ST_ACCEL_DEFAULT_OUT_X_L_ADDR+1,
+ mount_matrix_ext_info),
+ ST_SENSORS_LSM_CHANNELS_EXT(IIO_ACCEL,
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
ST_SENSORS_SCAN_Y, 1, IIO_MOD_Y, 's', IIO_LE, 8, 8,
- ST_ACCEL_DEFAULT_OUT_Y_L_ADDR+1),
- ST_SENSORS_LSM_CHANNELS(IIO_ACCEL,
+ ST_ACCEL_DEFAULT_OUT_Y_L_ADDR+1,
+ mount_matrix_ext_info),
+ ST_SENSORS_LSM_CHANNELS_EXT(IIO_ACCEL,
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
ST_SENSORS_SCAN_Z, 1, IIO_MOD_Z, 's', IIO_LE, 8, 8,
- ST_ACCEL_DEFAULT_OUT_Z_L_ADDR+1),
+ ST_ACCEL_DEFAULT_OUT_Z_L_ADDR+1,
+ mount_matrix_ext_info),
IIO_CHAN_SOFT_TIMESTAMP(3)
};
static const struct iio_chan_spec st_accel_12bit_channels[] = {
- ST_SENSORS_LSM_CHANNELS(IIO_ACCEL,
+ ST_SENSORS_LSM_CHANNELS_EXT(IIO_ACCEL,
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
ST_SENSORS_SCAN_X, 1, IIO_MOD_X, 's', IIO_LE, 12, 16,
- ST_ACCEL_DEFAULT_OUT_X_L_ADDR),
- ST_SENSORS_LSM_CHANNELS(IIO_ACCEL,
+ ST_ACCEL_DEFAULT_OUT_X_L_ADDR,
+ mount_matrix_ext_info),
+ ST_SENSORS_LSM_CHANNELS_EXT(IIO_ACCEL,
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
ST_SENSORS_SCAN_Y, 1, IIO_MOD_Y, 's', IIO_LE, 12, 16,
- ST_ACCEL_DEFAULT_OUT_Y_L_ADDR),
- ST_SENSORS_LSM_CHANNELS(IIO_ACCEL,
+ ST_ACCEL_DEFAULT_OUT_Y_L_ADDR,
+ mount_matrix_ext_info),
+ ST_SENSORS_LSM_CHANNELS_EXT(IIO_ACCEL,
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
ST_SENSORS_SCAN_Z, 1, IIO_MOD_Z, 's', IIO_LE, 12, 16,
- ST_ACCEL_DEFAULT_OUT_Z_L_ADDR),
+ ST_ACCEL_DEFAULT_OUT_Z_L_ADDR,
+ mount_matrix_ext_info),
IIO_CHAN_SOFT_TIMESTAMP(3)
};
static const struct iio_chan_spec st_accel_16bit_channels[] = {
- ST_SENSORS_LSM_CHANNELS(IIO_ACCEL,
+ ST_SENSORS_LSM_CHANNELS_EXT(IIO_ACCEL,
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
ST_SENSORS_SCAN_X, 1, IIO_MOD_X, 's', IIO_LE, 16, 16,
- ST_ACCEL_DEFAULT_OUT_X_L_ADDR),
- ST_SENSORS_LSM_CHANNELS(IIO_ACCEL,
+ ST_ACCEL_DEFAULT_OUT_X_L_ADDR,
+ mount_matrix_ext_info),
+ ST_SENSORS_LSM_CHANNELS_EXT(IIO_ACCEL,
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
ST_SENSORS_SCAN_Y, 1, IIO_MOD_Y, 's', IIO_LE, 16, 16,
- ST_ACCEL_DEFAULT_OUT_Y_L_ADDR),
- ST_SENSORS_LSM_CHANNELS(IIO_ACCEL,
+ ST_ACCEL_DEFAULT_OUT_Y_L_ADDR,
+ mount_matrix_ext_info),
+ ST_SENSORS_LSM_CHANNELS_EXT(IIO_ACCEL,
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
ST_SENSORS_SCAN_Z, 1, IIO_MOD_Z, 's', IIO_LE, 16, 16,
- ST_ACCEL_DEFAULT_OUT_Z_L_ADDR),
+ ST_ACCEL_DEFAULT_OUT_Z_L_ADDR,
+ mount_matrix_ext_info),
IIO_CHAN_SOFT_TIMESTAMP(3)
};
@@ -1162,25 +1185,10 @@ static const struct iio_trigger_ops st_accel_trigger_ops = {
#endif
#ifdef CONFIG_ACPI
-static const struct iio_mount_matrix *
-get_mount_matrix(const struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan)
-{
- struct st_sensor_data *adata = iio_priv(indio_dev);
-
- return adata->mount_matrix;
-}
-
-static const struct iio_chan_spec_ext_info mount_matrix_ext_info[] = {
- IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, get_mount_matrix),
- { },
-};
-
/* Read ST-specific _ONT orientation data from ACPI and generate an
* appropriate mount matrix.
*/
-static int apply_acpi_orientation(struct iio_dev *indio_dev,
- struct iio_chan_spec *channels)
+static bool apply_acpi_orientation(struct iio_dev *indio_dev)
{
struct st_sensor_data *adata = iio_priv(indio_dev);
struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
@@ -1188,7 +1196,7 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev,
union acpi_object *ont;
union acpi_object *elements;
acpi_status status;
- int ret = -EINVAL;
+ bool result = false;
unsigned int val;
int i, j;
int final_ont[3][3] = { { 0 }, };
@@ -1208,16 +1216,16 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev,
adev = ACPI_COMPANION(adata->dev);
if (!adev)
- return 0;
+ return false;
/* Read _ONT data, which should be a package of 6 integers. */
status = acpi_evaluate_object(adev->handle, "_ONT", NULL, &buffer);
if (status == AE_NOT_FOUND) {
- return 0;
+ return false;
} else if (ACPI_FAILURE(status)) {
dev_warn(&indio_dev->dev, "failed to execute _ONT: %d\n",
status);
- return status;
+ return false;
}
ont = buffer.pointer;
@@ -1269,14 +1277,6 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev,
}
/* Convert our integer matrix to a string-based iio_mount_matrix */
- adata->mount_matrix = devm_kmalloc(&indio_dev->dev,
- sizeof(*adata->mount_matrix),
- GFP_KERNEL);
- if (!adata->mount_matrix) {
- ret = -ENOMEM;
- goto out;
- }
-
for (i = 0; i < 3; i++) {
for (j = 0; j < 3; j++) {
int matrix_val = final_ont[i][j];
@@ -1295,26 +1295,23 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev,
default:
goto out;
}
- adata->mount_matrix->rotation[i * 3 + j] = str_value;
+ adata->mount_matrix.rotation[i * 3 + j] = str_value;
}
}
- /* Expose the mount matrix via ext_info */
- for (i = 0; i < indio_dev->num_channels; i++)
- channels[i].ext_info = mount_matrix_ext_info;
-
- ret = 0;
+ result = true;
dev_info(&indio_dev->dev, "computed mount matrix from ACPI\n");
out:
+ if (!result)
+ dev_warn(&indio_dev->dev, "failed to apply ACPI orientation data\n");
kfree(buffer.pointer);
- return ret;
+ return result;
}
#else /* !CONFIG_ACPI */
-static int apply_acpi_orientation(struct iio_dev *indio_dev,
- struct iio_chan_spec *channels)
+static bool apply_acpi_orientation(struct iio_dev *indio_dev)
{
- return 0;
+ return false;
}
#endif
@@ -1340,8 +1337,6 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
{
struct st_sensor_data *adata = iio_priv(indio_dev);
struct st_sensors_platform_data *pdata = dev_get_platdata(adata->dev);
- struct iio_chan_spec *channels;
- size_t channels_size;
int err;
indio_dev->modes = INDIO_DIRECT_MODE;
@@ -1352,20 +1347,16 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
return err;
adata->num_data_channels = ST_ACCEL_NUMBER_DATA_CHANNELS;
+ indio_dev->channels = adata->sensor_settings->ch;
indio_dev->num_channels = ST_SENSORS_NUMBER_ALL_CHANNELS;
- channels_size = indio_dev->num_channels * sizeof(struct iio_chan_spec);
- channels = devm_kmemdup(&indio_dev->dev,
- adata->sensor_settings->ch,
- channels_size, GFP_KERNEL);
- if (!channels)
- return -ENOMEM;
-
- if (apply_acpi_orientation(indio_dev, channels))
- dev_warn(&indio_dev->dev,
- "failed to apply ACPI orientation data: %d\n", err);
+ if (!apply_acpi_orientation(indio_dev)) {
+ err = iio_read_mount_matrix(&indio_dev->dev, "mount-matrix",
+ &adata->mount_matrix);
+ if (err)
+ return err;
+ }
- indio_dev->channels = channels;
adata->current_fullscale = &adata->sensor_settings->fs.fs_avl[0];
adata->odr = adata->sensor_settings->odr.odr_avl[0].hz;
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 0b9aeb479f48..6a4395ab6636 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -13,6 +13,7 @@
#include <linux/i2c.h>
#include <linux/spi/spi.h>
#include <linux/irqreturn.h>
+#include <linux/iio/iio.h>
#include <linux/iio/trigger.h>
#include <linux/bitops.h>
#include <linux/regulator/consumer.h>
@@ -48,8 +49,8 @@
#define ST_SENSORS_MAX_NAME 17
#define ST_SENSORS_MAX_4WAI 8
-#define ST_SENSORS_LSM_CHANNELS(device_type, mask, index, mod, \
- ch2, s, endian, rbits, sbits, addr) \
+#define ST_SENSORS_LSM_CHANNELS_EXT(device_type, mask, index, mod, \
+ ch2, s, endian, rbits, sbits, addr, ext) \
{ \
.type = device_type, \
.modified = mod, \
@@ -65,8 +66,14 @@
.storagebits = sbits, \
.endianness = endian, \
}, \
+ .ext_info = ext, \
}
+#define ST_SENSORS_LSM_CHANNELS(device_type, mask, index, mod, \
+ ch2, s, endian, rbits, sbits, addr) \
+ ST_SENSORS_LSM_CHANNELS_EXT(device_type, mask, index, mod, \
+ ch2, s, endian, rbits, sbits, addr, NULL)
+
#define ST_SENSORS_DEV_ATTR_SAMP_FREQ_AVAIL() \
IIO_DEV_ATTR_SAMP_FREQ_AVAIL( \
st_sensors_sysfs_sampling_frequency_avail)
@@ -215,6 +222,7 @@ struct st_sensor_settings {
* struct st_sensor_data - ST sensor device status
* @dev: Pointer to instance of struct device (I2C or SPI).
* @trig: The trigger in use by the core driver.
+ * @mount_matrix: The mounting matrix of the sensor.
* @sensor_settings: Pointer to the specific sensor settings in use.
* @current_fullscale: Maximum range of measure by the sensor.
* @vdd: Pointer to sensor's Vdd power supply
@@ -234,7 +242,7 @@ struct st_sensor_settings {
struct st_sensor_data {
struct device *dev;
struct iio_trigger *trig;
- struct iio_mount_matrix *mount_matrix;
+ struct iio_mount_matrix mount_matrix;
struct st_sensor_settings *sensor_settings;
struct st_sensor_fullscale_avl *current_fullscale;
struct regulator *vdd;
--
2.31.1
