Re: sysfs mount_matrix for st_lsm6dsx gyro

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6075 bytes --]

> On Wed, 11 Jan 2023 13:09:40 +0100
> Philippe De Muyter <phdm@macq.eu> wrote:
> 
> > Hello Lorenzo and list,
> > 
> > I do not find a "*mount_matrix" entry in sysfs for a 'ism330dlc_gyro'
> > iio device.
> > Is that normal ?
> > Is a fix available ?
> 
> Looks like the channel definition for the gyro does not include an
> appropriate ext_info entry unlike the accelerometer channels which
> have one with mount_matrix support.
> 
> From a quick glance looks like a simple fix. Add that entry.

something like (per-sensor mount_matrix):

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 5b6f195748fc..5e0c184e1055 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -113,6 +113,8 @@ enum st_lsm6dsx_hw_id {
 		.storagebits = 16,					\
 		.endianness = IIO_LE,					\
 	},								\
+	.ext_info = st_lsm6dsx_gyro_ext_info,				\
+	.num_event_specs = 1,						\
 }
 
 struct st_lsm6dsx_reg {
@@ -356,6 +358,7 @@ enum st_lsm6dsx_fifo_mode {
  * @decimator: Sensor decimation factor.
  * @sip: Number of samples in a given pattern.
  * @ts_ref: Sensor timestamp reference for hw one.
+ * @orientation: sensor chip orientation relative to main hardware.
  * @ext_info: Sensor settings if it is connected to i2c controller
  */
 struct st_lsm6dsx_sensor {
@@ -371,6 +374,8 @@ struct st_lsm6dsx_sensor {
 	u8 sip;
 	s64 ts_ref;
 
+	struct iio_mount_matrix orientation;
+
 	struct {
 		const struct st_lsm6dsx_ext_dev_settings *settings;
 		u32 slv_odr;
@@ -398,7 +403,6 @@ struct st_lsm6dsx_sensor {
  * @enable_event: enabled event bitmask.
  * @iio_devs: Pointers to acc/gyro iio_dev instances.
  * @settings: Pointer to the specific sensor settings in use.
- * @orientation: sensor chip orientation relative to main hardware.
  * @scan: Temporary buffers used to align data before iio_push_to_buffers()
  */
 struct st_lsm6dsx_hw {
@@ -427,7 +431,6 @@ struct st_lsm6dsx_hw {
 
 	const struct st_lsm6dsx_settings *settings;
 
-	struct iio_mount_matrix orientation;
 	/* Ensure natural alignment of buffer elements */
 	struct {
 		__le16 channels[3];
@@ -511,9 +514,8 @@ st_lsm6dsx_get_mount_matrix(const struct iio_dev *iio_dev,
 			    const struct iio_chan_spec *chan)
 {
 	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
-	struct st_lsm6dsx_hw *hw = sensor->hw;
 
-	return &hw->orientation;
+	return &sensor->orientation;
 }
 
 static inline int
@@ -533,4 +535,10 @@ struct iio_chan_spec_ext_info __maybe_unused st_lsm6dsx_accel_ext_info[] = {
 	{ }
 };
 
+static const
+struct iio_chan_spec_ext_info __maybe_unused st_lsm6dsx_gyro_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, st_lsm6dsx_get_mount_matrix),
+	{ }
+};
+
 #endif /* ST_LSM6DSX_H */
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 3f6060c64f32..15c7184a1895 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -2359,6 +2359,9 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 	sensor->gain = hw->settings->fs_table[id].fs_avl[0].gain;
 	sensor->watermark = 1;
 
+	if (iio_read_mount_matrix(hw->dev, &sensor->orientation))
+		return NULL;
+
 	switch (id) {
 	case ST_LSM6DSX_ID_ACC:
 		iio_dev->info = &st_lsm6dsx_acc_info;
@@ -2676,10 +2679,6 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 			return err;
 	}
 
-	err = iio_read_mount_matrix(hw->dev, &hw->orientation);
-	if (err)
-		return err;
-
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
 		if (!hw->iio_devs[i])
 			continue;

> > 
> > Some more info :
> > 
> > I have backported  drivers/iio/imu/st_lsm6dsx to linux-4.9 in order
> > to drive a ism330dlc imu on a custom board.  The chip is correctly
> > detected and two devices are created in /sys/bus/iio/devices/
> > 
> > the first one (where name is 'ism330dlc_gyro') has the following entries :
> > 
> >  me@proto4:~$ ls /sys/bus/iio/devices/iio\:device1/
> >  buffer                      in_anglvel_x_raw  sampling_frequency
> >  current_timestamp_clock     in_anglvel_y_raw  sampling_frequency_available
> >  dev                         in_anglvel_z_raw  scan_elements
> >  in_anglvel_scale            name              subsystem
> >  in_anglvel_scale_available  power             uevent
> >  me@proto4:~$
> > 
> > the second one (where name is 'ism330dlc_accel') has those entries :
> > 
> >  me@proto4:~$ ls /sys/bus/iio/devices/iio\:device2
> >  buffer                    in_accel_x_raw  sampling_frequency
> >  current_timestamp_clock   in_accel_y_raw  sampling_frequency_available
> >  dev                       in_accel_z_raw  scan_elements
> >  events                    mount_matrix    subsystem
> >  in_accel_scale            name            uevent
> >  in_accel_scale_available  power
> >  me@proto4:~$
> > 
> > The 'mount_matrix' entry is only present in the 'ism330dlc_accel' device
> > but not in the 'ism330dlc_gyro' device.
> > 
> > On a similar board, but with mpu9250 imu, I get only one iio:deviceX
> > entry but with two *mount_matrix entries : 
> > 
> > 	in_accel_mount_matrix
> > 	in_anglvel_mount_matrix
> > 
> > In both cases, I would have expected only one 'iio:deviceX' entry with
> > only one 'mount_matrix' entry.
> 
> There are multiple devices because the driver predates the addition
> of multiple buffer support to IIO and IIRC is capable of producing data
> at different sampling rates for the accelerometer and the gyros.
> Hence when it was implemented the only choice was to register multiple
> devices in order to get the multiple buffers. It's ABI now so we can't
> fix it in an old driver unfortunately.  We'd do this differently today..
> 
> The double mount_matrix for the mpu9250 is a little odd and I can't
> immediately spot why that one is happening.
> 
> 
> > 
> > Best regards
> > 
> > Philippe
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]