| Message ID | 20210515000058.204601-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: accel: st_sensors: Support generic mounting matrix | expand |
On Sat, 15 May 2021 02:00:58 +0200 Linus Walleij <linus.walleij@linaro.org> 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. Hi Linus, Why wrap at 58ish columns? 75 please as it looks nicer :) What you have here is fine, but I think there is room to tidy up a little more given we now always have the mount matrix. Thanks, Jonathan > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Denis Ciocca <denis.ciocca@st.com> > Cc: Daniel Drake <drake@endlessm.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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; 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. > > indio_dev->channels = channels; > adata->current_fullscale = &adata->sensor_settings->fs.fs_avl[0];
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@linaro.org> 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@linux.intel.com> > > Cc: Denis Ciocca <denis.ciocca@st.com> > > Cc: Daniel Drake <drake@endlessm.com> > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > --- > > 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@gerhold.net> 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@gerhold.net> --- 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;
On Sat, May 15, 2021 at 6:17 AM Linus Walleij <linus.walleij@linaro.org> 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. I think this entire thread may be interesting to Hans, hence Cc him. > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Denis Ciocca <denis.ciocca@st.com> > Cc: Daniel Drake <drake@endlessm.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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); > + > 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; > > indio_dev->channels = channels; > adata->current_fullscale = &adata->sensor_settings->fs.fs_avl[0]; > -- > 2.31.1 >
Hi, On 5/17/21 9:46 AM, Andy Shevchenko wrote: > On Sat, May 15, 2021 at 6:17 AM Linus Walleij <linus.walleij@linaro.org> 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. > > I think this entire thread may be interesting to Hans, hence Cc him. Thanks I'm fine with the suggested change, but I've some review marks below (inline). > > >> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Cc: Denis Ciocca <denis.ciocca@st.com> >> Cc: Daniel Drake <drake@endlessm.com> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> --- >> 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); >> + This path is also taken on a successful return, so the dev_warn needs to be guarded by a "if (ret)" condition. >> 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; >> + } So now we to this devm_kmalloc both in apply_acpi_orientation() if it succeeds as well as here if apply_acpi_orientation() fails. It would be better to just stop kmallocing it all together in both places and just embed the struct in adata, iow make adata->mount_matrix be the actual iio_mount_matrix struct, rather then a pointer to it. This will allow removing the 2 devm_kmalloc calls + their error handling. Regards, Hans >> + >> + 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; >> >> indio_dev->channels = channels; >> adata->current_fullscale = &adata->sensor_settings->fs.fs_avl[0]; >> -- >> 2.31.1 >> > >
Hi Hans, thanks for looking into this. On Mon, May 17, 2021 at 11:23 AM Hans de Goede <hdegoede@redhat.com> wrote: > So now we to this devm_kmalloc both in apply_acpi_orientation() if it succeeds > as well as here if apply_acpi_orientation() fails. It would be better to just > stop kmallocing it all together in both places and just embed the struct in adata, > iow make adata->mount_matrix be the actual iio_mount_matrix struct, rather then > a pointer to it. The reason it is just a pointer (IIUC) is that adata is shared across all ST sensors, including accelerometers, gyro and magnetometer (which may indeed use it) but also including pressure sensors, so there it is surplus. That said, I don't think it's a big deal, a few bytes less or more, so let's take that approach. Yours, Linus Walleij
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); + 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; indio_dev->channels = channels; adata->current_fullscale = &adata->sensor_settings->fs.fs_avl[0];
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@linux.intel.com> Cc: Denis Ciocca <denis.ciocca@st.com> Cc: Daniel Drake <drake@endlessm.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/iio/accel/st_accel_core.c | 51 ++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 17 deletions(-)