Skip to contentSkip to conversation

iio: gyro: mpu3050: read gyro samples via FIFO in IIO_CHAN_INFO_RAW

PATCH In review Herman van Hazendonk 4 participants 4 messages Jun 5 – Jun 7 1 message not in the archive

Subsystems (2)
IIO Invensense mpu-3050 gyroscope driver
lore
Share & tools
lore

Herman van Hazendonk

author
#1 lore raw

PATCH 1/1 iio: gyro: mpu3050: read gyro samples via FIFO in IIO_CHAN_INFO_RAW

In reply to a message not in the archive

The direct gyro register file (XOUT_H..ZOUT_L) returns stale data on at least the HP TouchPad (apq8060): X high byte is stuck at 0xFF and Z reads as 0x0000 even when the chip is otherwise sampling and the userspace i2c-dev path returns sensible values for the same chip from the same registers. Y reads correctly, suggesting the issue is in how the on-die register file responds to back-to-back two-byte reads at specific offsets after the reset / PLL settle that mpu3050_set_8khz_samplerate() performs immediately before the read.

The chip's on-chip FIFO does not have this hazard because the sample assembly is performed inside the chip and the host only reads a fully formed frame from FIFO_R.

Add a small mpu3050_fifo_read_one() helper that:

- lowers DLPF/SMPLRT to ~250 Hz so exactly one fresh sample lands in the FIFO during the wait loop (at the previously-set 8 kHz the FIFO accumulates a sample every 125 us and races the FIFO_COUNT_H / FIFO_R bulk reads, producing per-axis byte hazards even though the oldest 6 bytes are nominally a clean frame);
- configures FIFO_EN for gyro X / Y / Z only (the FIFO-captured TEMP slot occasionally returns stale data while gyro words are still consistent; mpu3050_read_raw() reads TEMP_H/L directly instead);
- resets the FIFO and enables FIFO read-out in a single USR_CTRL write so the chip atomically resets the write pointer with capture already on (two separate writes leave the FIFO in a brief transient state where the first sample after re-enable can land at a non-zero offset, producing per-byte-shifted frames in the readback);
- waits for one 6-byte sample to land with a generous retry loop;
- reads one 6-byte X / Y / Z frame from FIFO_R;
- tears the FIFO capture down so subsequent buffered/triggered reads aren't confused by stale FIFO_EN bits.

Route IIO_CHAN_INFO_RAW through this helper for IIO_ANGL_VEL. IIO_TEMP keeps its existing direct TEMP_H/L read because that register is not affected by the gyro register-file hazard, and the FIFO-captured TEMP slot occasionally diverges from the live register by several degrees. The triggered-buffer path (mpu3050_trigger_handler()) is unchanged and continues to manage the FIFO itself when the hardware interrupt trigger is active.

The chip is left at ~250 Hz when the helper returns; mpu3050->lpf and mpu3050->divisor are unchanged so the sysfs in_anglvel_sampling_- frequency value resolved via mpu3050_get_freq() keeps reporting the user-configured rate. The next caller of mpu3050_set_8khz_samplerate() (any subsequent IIO_CHAN_INFO_RAW read, or the buffered-mode enable path, or a companion runtime_resume fix sent as a separate patch) issues a full chip reset and reprograms the hardware from cached state.

Test results on the HP TouchPad with the chip lying flat, 15 IIO_CHAN_INFO_RAW reads at 200 ms intervals:

Before this patch (direct XOUT_H/YOUT_H/ZOUT_H reads):

    X = 255             (high byte stuck at 0xFF, permanently)
    Y = -99             (works)
    Z = 0               (low/high bytes both 0x00, permanently)

After this patch (FIFO read path):

    X = -95..-156       (span 61, mean -127)
    Y =  46..80         (span 34, mean  61)
    Z = -15..23         (span 38, mean   3)
    TEMP = -13440..-13552  (span 112, stable)

Steady-state per-read latency is ~89 ms, dominated by the existing mpu3050_set_8khz_samplerate() reset + msleep(50) before the FIFO helper.

Signed-off-by Herman van Hazendonk <[email protected]>
---
 drivers/iio/gyro/mpu3050-core.c | 162 ++++++++++++++++++++++++++++++--
 1 file changed, 152 insertions(+), 10 deletions(-)
drivers/iio/gyro/mpu3050-core.c +152 -10
index d84e04e4b431..be87053dba54 100644
@@ -260,6 +260,141 @@ static int mpu3050_set_8khz_samplerate(struct mpu3050 *mpu3050)
 	return ret;
 }
 
+/*
+ * mpu3050_fifo_read_one() - capture and return one combined sample frame
+ *
+ * The MPU-3050's direct gyro register file (XOUT_H..ZOUT_L) exhibits a
+ * stale-byte hazard on at least the apq8060 i2c controller path: on
+ * many devices the first byte of the X high register and the entire Z
+ * pair return constant 0xFF / 0x0000 even when the chip is otherwise
+ * sampling and the userspace i2c-dev path returns sensible values.
+ *
+ * The chip's on-chip FIFO does not have this problem because the
+ * sample assembly happens inside the chip and the host only reads a
+ * fully-formed frame from FIFO_R. Configure the FIFO to capture TEMP
+ * plus all three gyro axes (8 bytes per sample, no footer), drain any
+ * stale data with a FIFO reset, wait one sample period, then read a
+ * single complete frame.
+ */
+/*
+ * Size of one FIFO sample frame captured by mpu3050_fifo_read_one():
+ * three big-endian s16 gyro words (X, Y, Z). Temperature is read
+ * separately from its register file because the FIFO-captured TEMP
+ * occasionally returns a stale/partial value while gyro words remain
+ * consistent.
+ */
+#define MPU3050_FIFO_FRAME_BYTES	6
+
+static int mpu3050_fifo_read_one(struct mpu3050 *mpu3050, __be16 frame[3])
+{
+	__be16 raw_count;
+	u16 fifocnt;
+	int retries;
+	int cleanup_err;
+	int ret;
+
+	/*
+	 * Slow the chip down to 250 Hz for the duration of this read so
+	 * exactly one fresh sample lands in the FIFO during the wait below.
+	 * At the previously-set 8 kHz the FIFO accumulates a sample every
+	 * 125 us and can race the bulk_read of FIFO_COUNT_H / FIFO_R,
+	 * producing per-axis byte hazards in the readback even though the
+	 * oldest 8 bytes are nominally a clean frame.
+	 *
+	 * The chip is left at 250 Hz when this function returns; the next
+	 * caller will run mpu3050_set_8khz_samplerate() again (which does
+	 * a full chip reset) so there is nothing to restore here.
+	 * mpu3050->lpf and mpu3050->divisor remain the user-configured
+	 * values across this function, so sysfs
+	 * in_anglvel_sampling_frequency continues to report the right
+	 * thing via mpu3050_get_freq().
+	 */
+	ret = regmap_write(mpu3050->map, MPU3050_DLPF_FS_SYNC,
+			   mpu3050->fullscale << MPU3050_FS_SHIFT |
+			   MPU3050_DLPF_CFG_20HZ);
+	if (ret)
+		goto disable;
+	ret = regmap_write(mpu3050->map, MPU3050_SMPLRT_DIV, 3);
+	if (ret)
+		goto disable;
+
+	/*
+	 * Capture only gyro X, Y, Z into the FIFO. The temperature slot
+	 * is deliberately omitted: the chip occasionally writes a stale
+	 * TEMP_H/L pair into the FIFO that differs from the live register
+	 * by ~7C, and there's no obvious sync the host can use to detect
+	 * which samples are affected. Reading TEMP straight from the
+	 * register file (which mpu3050_read_raw() does for IIO_TEMP) does
+	 * not have this hazard.
+	 */
+	ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN,
+			   MPU3050_FIFO_EN_GYRO_XOUT |
+			   MPU3050_FIFO_EN_GYRO_YOUT |
+			   MPU3050_FIFO_EN_GYRO_ZOUT);
+	if (ret)
+		goto disable;
+
+	/*
+	 * Enable FIFO read-out and assert FIFO_RST in a single write so the
+	 * chip atomically resets the write pointer with capture already on.
+	 * Two separate writes (FIFO_RST then FIFO_EN) leave the FIFO in a
+	 * brief transient state where the first sample after re-enable can
+	 * land at a non-zero offset, producing per-byte-shifted frames in
+	 * the readback below. This matches the mpu3050_trigger_handler()
+	 * pattern.
+	 */
+	ret = regmap_write(mpu3050->map, MPU3050_USR_CTRL,
+			   MPU3050_USR_CTRL_FIFO_EN |
+			   MPU3050_USR_CTRL_FIFO_RST);
+	if (ret)
+		goto disable;
+
+	/*
+	 * Wait for at least one sample to land. At 250 Hz that's nominally
+	 * 4 ms; the retry loop gives a generous margin so a stretched i2c
+	 * clock or NEW sample-rate-change settle time doesn't trip the
+	 * timeout.
+	 */
+	for (retries = 0; retries < 10; retries++) {
+		usleep_range(2000, 4000);
+		ret = regmap_bulk_read(mpu3050->map, MPU3050_FIFO_COUNT_H,
+				       &raw_count, sizeof(raw_count));
+		if (ret)
+			goto disable;
+		fifocnt = be16_to_cpu(raw_count);
+		if (fifocnt >= MPU3050_FIFO_FRAME_BYTES)
+			break;
+	}
+
+	if (fifocnt < MPU3050_FIFO_FRAME_BYTES) {
+		dev_dbg(mpu3050->dev, "FIFO timeout (%u bytes)\n", fifocnt);
+		ret = -ETIMEDOUT;
+		goto disable;
+	}
+
+	ret = regmap_bulk_read(mpu3050->map, MPU3050_FIFO_R, frame,
+			       MPU3050_FIFO_FRAME_BYTES);
+
+disable:
+	/*
+	 * Tear FIFO capture down so the next _raw read starts clean and a
+	 * subsequent buffered/triggered mode setup isn't confused by stale
+	 * FIFO_EN bits. Cleanup write failures are logged but not
+	 * propagated: we prefer to return the original read error (if
+	 * any) rather than mask it with a teardown error, and there is no
+	 * useful recovery path from a transport failure here anyway.
+	 */
+	cleanup_err = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
+	if (cleanup_err)
+		dev_dbg(mpu3050->dev,
+			"FIFO_EN teardown write failed: %d\n", cleanup_err);
+	cleanup_err = regmap_write(mpu3050->map, MPU3050_USR_CTRL, 0);
+	if (cleanup_err)
+		dev_dbg(mpu3050->dev,
+			"USR_CTRL teardown write failed: %d\n", cleanup_err);
+	return ret;
+}
+
 static int mpu3050_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2,

@@ -267,6 +402,7 @@ static int mpu3050_read_raw(struct iio_dev *indio_dev,
 {
 	struct mpu3050 *mpu3050 = iio_priv(indio_dev);
 	int ret;
+	__be16 frame[3];
 	__be16 raw_val;
 
 	switch (mask) {

@@ -333,6 +469,13 @@ static int mpu3050_read_raw(struct iio_dev *indio_dev,
 
 		switch (chan->type) {
 		case IIO_TEMP:
+			/*
+			 * TEMP_H/L is read directly from the register file.
+			 * The FIFO-captured TEMP slot occasionally returns
+			 * stale data while gyro words are still consistent,
+			 * so we deliberately do not pipe TEMP through the
+			 * FIFO path used by IIO_ANGL_VEL below.
+			 */
 			ret = regmap_bulk_read(mpu3050->map, MPU3050_TEMP_H,
 					       &raw_val, sizeof(raw_val));
 			if (ret) {

@@ -340,25 +483,24 @@ static int mpu3050_read_raw(struct iio_dev *indio_dev,
 					"error reading temperature\n");
 				goto out_read_raw_unlock;
 			}
-
 			*val = (s16)be16_to_cpu(raw_val);
 			ret = IIO_VAL_INT;
-
 			goto out_read_raw_unlock;
 		case IIO_ANGL_VEL:
-			ret = regmap_bulk_read(mpu3050->map,
-				       MPU3050_AXIS_REGS(chan->scan_index-1),
-				       &raw_val,
-				       sizeof(raw_val));
+			ret = mpu3050_fifo_read_one(mpu3050, frame);
 			if (ret) {
 				dev_err(mpu3050->dev,
-					"error reading axis data\n");
+					"error reading sample frame from FIFO: %d\n",
+					ret);
 				goto out_read_raw_unlock;
 			}
-
-			*val = be16_to_cpu(raw_val);
+			/*
+			 * frame[0..2] = XOUT/YOUT/ZOUT in the order the chip
+			 * packs gyro words into the FIFO. scan_index is 1..3
+			 * for X/Y/Z so subtract one to land on the frame.
+			 */
+			*val = (s16)be16_to_cpu(frame[chan->scan_index - 1]);
 			ret = IIO_VAL_INT;
-
 			goto out_read_raw_unlock;
 		default:
 			ret = -EINVAL;
Signature · 1 line
2.43.0
maintainer
#2 lore raw
Quoted 2 lines · On Fri, 5 Jun 2026 10:46:21 +0200 Herman van Hazendonk…

The direct gyro register file (XOUT_H..ZOUT_L) returns stale data on at least the HP TouchPad (apq8060): X high byte is stuck at 0xFF and

Hi Herman,

That doesn't sound stale, rather just corrupted.

Quoted 10 lines

Z reads as 0x0000 even when the chip is otherwise sampling and the userspace i2c-dev path returns sensible values for the same chip from the same registers. Y reads correctly, suggesting the issue is in how the on-die register file responds to back-to-back two-byte reads at specific offsets after the reset / PLL settle that mpu3050_set_8khz_samplerate() performs immediately before the read.

The chip's on-chip FIFO does not have this hazard because the sample assembly is performed inside the chip and the host only reads a fully formed frame from FIFO_R.

This feels like a heavy weight solution to what I think you are suggesting is possibly just an instability after the samplerate set? The sysfs read path is slow anyway, so have you tried a short sleep to see if things
stabilise? There is already a sleep in the call to start sampling,
perhaps that is too short, or perhaps we should wait for the 2nd sample?

Quoted 8 lines

Add a small mpu3050_fifo_read_one() helper that:

- lowers DLPF/SMPLRT to ~250 Hz so exactly one fresh sample lands in the FIFO during the wait loop (at the previously-set 8 kHz the FIFO accumulates a sample every 125 us and races the FIFO_COUNT_H / FIFO_R bulk reads, producing per-axis byte hazards even though the oldest 6 bytes are nominally a clean frame);

That's a pretty nasty bug if there is a race there :(

Quoted 15 lines

- configures FIFO_EN for gyro X / Y / Z only (the FIFO-captured TEMP slot occasionally returns stale data while gyro words are still consistent; mpu3050_read_raw() reads TEMP_H/L directly instead);
- resets the FIFO and enables FIFO read-out in a single USR_CTRL write so the chip atomically resets the write pointer with capture already on (two separate writes leave the FIFO in a brief transient state where the first sample after re-enable can land at a non-zero offset, producing per-byte-shifted frames in the readback);
- waits for one 6-byte sample to land with a generous retry loop;
- reads one 6-byte X / Y / Z frame from FIFO_R;
- tears the FIFO capture down so subsequent buffered/triggered reads aren't confused by stale FIFO_EN bits.

Quoted 4 lines

Route IIO_CHAN_INFO_RAW through this helper for IIO_ANGL_VEL. IIO_TEMP keeps its existing direct TEMP_H/L read because that register is not affected by the gyro register-file hazard, and the FIFO-captured TEMP slot occasionally diverges from the live register by several degrees.

Hmm. This device is full of nasty surprises!

Quoted 22 lines

The triggered-buffer path (mpu3050_trigger_handler()) is unchanged and continues to manage the FIFO itself when the hardware interrupt trigger is active.

The chip is left at ~250 Hz when the helper returns; mpu3050->lpf and mpu3050->divisor are unchanged so the sysfs in_anglvel_sampling_- frequency value resolved via mpu3050_get_freq() keeps reporting the user-configured rate. The next caller of mpu3050_set_8khz_samplerate() (any subsequent IIO_CHAN_INFO_RAW read, or the buffered-mode enable path, or a companion runtime_resume fix sent as a separate patch) issues a full chip reset and reprograms the hardware from cached state.

Test results on the HP TouchPad with the chip lying flat, 15 IIO_CHAN_INFO_RAW reads at 200 ms intervals:

Before this patch (direct XOUT_H/YOUT_H/ZOUT_H reads):

    X = 255             (high byte stuck at 0xFF, permanently)
    Y = -99             (works)
    Z = 0               (low/high bytes both 0x00, permanently)

After this patch (FIFO read path):

Quoted 8 lines
    X = -95..-156       (span 61, mean -127)
    Y =  46..80         (span 34, mean  61)
    Z = -15..23         (span 38, mean   3)
    TEMP = -13440..-13552  (span 112, stable)

Steady-state per-read latency is ~89 ms, dominated by the existing mpu3050_set_8khz_samplerate() reset + msleep(50) before the FIFO helper.

Do you have a usecase that cares that much about the latency of such a read? As you say it's dominated by the reset anyway - so I'm just curious rather than this being important.

Quoted 16 lines
Signed-off-by Herman van Hazendonk <[email protected]>
---
 drivers/iio/gyro/mpu3050-core.c | 162 ++++++++++++++++++++++++++++++--
 1 file changed, 152 insertions(+), 10 deletions(-)
drivers/iio/gyro/mpu3050-core.c +2 -0
index d84e04e4b431..be87053dba54 100644
@@ -260,6 +260,141 @@ static int mpu3050_set_8khz_samplerate(struct mpu3050 *mpu3050)
 	return ret;
 }
 
+/*
+ * mpu3050_fifo_read_one() - capture and return one combined sample frame

drivers/iio/gyro/mpu3050-core.c static int mpu3050_set_8khz_samplerate(struct mpu3050 *mpu3050) · line 274

Quoted 10 lines
drivers/iio/gyro/mpu3050-core.c +10 -0
+ *
+ * The MPU-3050's direct gyro register file (XOUT_H..ZOUT_L) exhibits a
+ * stale-byte hazard on at least the apq8060 i2c controller path: on
+ * many devices the first byte of the X high register and the entire Z
+ * pair return constant 0xFF / 0x0000 even when the chip is otherwise
+ * sampling and the userspace i2c-dev path returns sensible values.
+ *
+ * The chip's on-chip FIFO does not have this problem because the
+ * sample assembly happens inside the chip and the host only reads a
+ * fully-formed frame from FIFO_R. Configure the FIFO to capture TEMP

Looks like you don't capture temp.

Quoted 14 lines
drivers/iio/gyro/mpu3050-core.c +14 -0
+ * plus all three gyro axes (8 bytes per sample, no footer), drain any
+ * stale data with a FIFO reset, wait one sample period, then read a
+ * single complete frame.
+ */
+/*
+ * Size of one FIFO sample frame captured by mpu3050_fifo_read_one():
+ * three big-endian s16 gyro words (X, Y, Z). Temperature is read
+ * separately from its register file because the FIFO-captured TEMP
+ * occasionally returns a stale/partial value while gyro words remain
+ * consistent.
+ */
+#define MPU3050_FIFO_FRAME_BYTES	6
+
+static int mpu3050_fifo_read_one(struct mpu3050 *mpu3050, __be16 frame[3])

drivers/iio/gyro/mpu3050-core.c static int mpu3050_set_8khz_samplerate(struct mpu3050 *mpu3050) · line 298

Quoted 10 lines
drivers/iio/gyro/mpu3050-core.c +10 -0
+{
+	__be16 raw_count;
+	u16 fifocnt;
+	int retries;
+	int cleanup_err;
+	int ret;
+
+	/*
+	 * Slow the chip down to 250 Hz for the duration of this read so
+	 * exactly one fresh sample lands in the FIFO during the wait below.

You can't guaranteed that as sleeps can always be longer than requested.

Quoted 5 lines
drivers/iio/gyro/mpu3050-core.c +5 -0
+	 * At the previously-set 8 kHz the FIFO accumulates a sample every
+	 * 125 us and can race the bulk_read of FIFO_COUNT_H / FIFO_R,
+	 * producing per-axis byte hazards in the readback even though the
+	 * oldest 8 bytes are nominally a clean frame.
+	 *

drivers/iio/gyro/mpu3050-core.c static int mpu3050_set_8khz_samplerate(struct mpu3050 *mpu3050) · line 313

Quoted 10 lines
drivers/iio/gyro/mpu3050-core.c +10 -0
+	 * The chip is left at 250 Hz when this function returns; the next
+	 * caller will run mpu3050_set_8khz_samplerate() again (which does
+	 * a full chip reset) so there is nothing to restore here.
+	 * mpu3050->lpf and mpu3050->divisor remain the user-configured
+	 * values across this function, so sysfs
+	 * in_anglvel_sampling_frequency continues to report the right
+	 * thing via mpu3050_get_freq().
+	 */
+	ret = regmap_write(mpu3050->map, MPU3050_DLPF_FS_SYNC,
+			   mpu3050->fullscale << MPU3050_FS_SHIFT |

Even though rest of driver isn't using it, for new code FIELD_PREP() and use the mask rather than the sift macro. Ultimately ripping out the SHIFT macros from here is something we should do.

Quoted 36 lines
drivers/iio/gyro/mpu3050-core.c +36 -0
+			   MPU3050_DLPF_CFG_20HZ);
+	if (ret)
+		goto disable;
+	ret = regmap_write(mpu3050->map, MPU3050_SMPLRT_DIV, 3);
+	if (ret)
+		goto disable;
+
+	/*
+	 * Capture only gyro X, Y, Z into the FIFO. The temperature slot
+	 * is deliberately omitted: the chip occasionally writes a stale
+	 * TEMP_H/L pair into the FIFO that differs from the live register
+	 * by ~7C, and there's no obvious sync the host can use to detect
+	 * which samples are affected. Reading TEMP straight from the
+	 * register file (which mpu3050_read_raw() does for IIO_TEMP) does
+	 * not have this hazard.
+	 */
+	ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN,
+			   MPU3050_FIFO_EN_GYRO_XOUT |
+			   MPU3050_FIFO_EN_GYRO_YOUT |
+			   MPU3050_FIFO_EN_GYRO_ZOUT);
+	if (ret)
+		goto disable;
+
+	/*
+	 * Enable FIFO read-out and assert FIFO_RST in a single write so the
+	 * chip atomically resets the write pointer with capture already on.
+	 * Two separate writes (FIFO_RST then FIFO_EN) leave the FIFO in a
+	 * brief transient state where the first sample after re-enable can
+	 * land at a non-zero offset, producing per-byte-shifted frames in
+	 * the readback below. This matches the mpu3050_trigger_handler()
+	 * pattern.
+	 */
+	ret = regmap_write(mpu3050->map, MPU3050_USR_CTRL,
+			   MPU3050_USR_CTRL_FIFO_EN |
+			   MPU3050_USR_CTRL_FIFO_RST);
+	if (ret)

drivers/iio/gyro/mpu3050-core.c static int mpu3050_set_8khz_samplerate(struct mpu3050 *mpu3050) · line 359

Quoted 10 lines
drivers/iio/gyro/mpu3050-core.c +10 -0
+		goto disable;
+
+	/*
+	 * Wait for at least one sample to land. At 250 Hz that's nominally
+	 * 4 ms; the retry loop gives a generous margin so a stretched i2c
+	 * clock or NEW sample-rate-change settle time doesn't trip the
+	 * timeout.
+	 */
+	for (retries = 0; retries < 10; retries++) {
+		usleep_range(2000, 4000);

fsleep()

drivers/iio/gyro/mpu3050-core.c static int mpu3050_set_8khz_samplerate(struct mpu3050 *mpu3050) · line 369

Quoted 10 lines
drivers/iio/gyro/mpu3050-core.c +10 -0
+		ret = regmap_bulk_read(mpu3050->map, MPU3050_FIFO_COUNT_H,
+				       &raw_count, sizeof(raw_count));
+		if (ret)
+			goto disable;
+		fifocnt = be16_to_cpu(raw_count);
+		if (fifocnt >= MPU3050_FIFO_FRAME_BYTES)
+			break;
+	}
+
+	if (fifocnt < MPU3050_FIFO_FRAME_BYTES) {

This is the timeout path, so clearer as

	if (retries == 10) //define for 10 probably makes sense

rather than checking the early exit condition wasn't true.

Quoted 6 lines
drivers/iio/gyro/mpu3050-core.c +6 -0
+		dev_dbg(mpu3050->dev, "FIFO timeout (%u bytes)\n", fifocnt);
+		ret = -ETIMEDOUT;
+		goto disable;
+	}
+
+	ret = regmap_bulk_read(mpu3050->map, MPU3050_FIFO_R, frame,

drivers/iio/gyro/mpu3050-core.c static int mpu3050_set_8khz_samplerate(struct mpu3050 *mpu3050) · line 385

Quoted 10 lines
drivers/iio/gyro/mpu3050-core.c +10 -0
+			       MPU3050_FIFO_FRAME_BYTES);
+
+disable:
+	/*
+	 * Tear FIFO capture down so the next _raw read starts clean and a
+	 * subsequent buffered/triggered mode setup isn't confused by stale
+	 * FIFO_EN bits. Cleanup write failures are logged but not
+	 * propagated: we prefer to return the original read error (if
+	 * any) rather than mask it with a teardown error, and there is no
+	 * useful recovery path from a transport failure here anyway.

I'd rather we returned an error anyway. Recovery path would likely be driver rebind.

Quoted 72 lines
drivers/iio/gyro/mpu3050-core.c +29 -10
+	 */
+	cleanup_err = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0);
+	if (cleanup_err)
+		dev_dbg(mpu3050->dev,
+			"FIFO_EN teardown write failed: %d\n", cleanup_err);
+	cleanup_err = regmap_write(mpu3050->map, MPU3050_USR_CTRL, 0);
+	if (cleanup_err)
+		dev_dbg(mpu3050->dev,
+			"USR_CTRL teardown write failed: %d\n", cleanup_err);
+	return ret;
+}
+
 static int mpu3050_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2,

@@ -267,6 +402,7 @@ static int mpu3050_read_raw(struct iio_dev *indio_dev,
 {
 	struct mpu3050 *mpu3050 = iio_priv(indio_dev);
 	int ret;
+	__be16 frame[3];
 	__be16 raw_val;
 
 	switch (mask) {

@@ -333,6 +469,13 @@ static int mpu3050_read_raw(struct iio_dev *indio_dev,
 
 		switch (chan->type) {
 		case IIO_TEMP:
+			/*
+			 * TEMP_H/L is read directly from the register file.
+			 * The FIFO-captured TEMP slot occasionally returns
+			 * stale data while gyro words are still consistent,
+			 * so we deliberately do not pipe TEMP through the
+			 * FIFO path used by IIO_ANGL_VEL below.
+			 */
 			ret = regmap_bulk_read(mpu3050->map, MPU3050_TEMP_H,
 					       &raw_val, sizeof(raw_val));
 			if (ret) {

@@ -340,25 +483,24 @@ static int mpu3050_read_raw(struct iio_dev *indio_dev,
 					"error reading temperature\n");
 				goto out_read_raw_unlock;
 			}
-
 			*val = (s16)be16_to_cpu(raw_val);
 			ret = IIO_VAL_INT;
-
 			goto out_read_raw_unlock;
 		case IIO_ANGL_VEL:
-			ret = regmap_bulk_read(mpu3050->map,
-				       MPU3050_AXIS_REGS(chan->scan_index-1),
-				       &raw_val,
-				       sizeof(raw_val));
+			ret = mpu3050_fifo_read_one(mpu3050, frame);
 			if (ret) {
 				dev_err(mpu3050->dev,
-					"error reading axis data\n");
+					"error reading sample frame from FIFO: %d\n",
+					ret);
 				goto out_read_raw_unlock;
 			}
-
-			*val = be16_to_cpu(raw_val);
+			/*
+			 * frame[0..2] = XOUT/YOUT/ZOUT in the order the chip
+			 * packs gyro words into the FIFO. scan_index is 1..3
+			 * for X/Y/Z so subtract one to land on the frame.
+			 */
+			*val = (s16)be16_to_cpu(frame[chan->scan_index - 1]);
 			ret = IIO_VAL_INT;
-
 			goto out_read_raw_unlock;
 		default:
 			ret = -EINVAL;
#3 lore raw
In reply to Herman van Hazendonk
Quoted 61 lines · Herman van Hazendonk

The direct gyro register file (XOUT_H..ZOUT_L) returns stale data on at least the HP TouchPad (apq8060): X high byte is stuck at 0xFF and Z reads as 0x0000 even when the chip is otherwise sampling and the userspace i2c-dev path returns sensible values for the same chip from the same registers. Y reads correctly, suggesting the issue is in how the on-die register file responds to back-to-back two-byte reads at specific offsets after the reset / PLL settle that mpu3050_set_8khz_samplerate() performs immediately before the read.

The chip's on-chip FIFO does not have this hazard because the sample assembly is performed inside the chip and the host only reads a fully formed frame from FIFO_R.

Add a small mpu3050_fifo_read_one() helper that:

- lowers DLPF/SMPLRT to ~250 Hz so exactly one fresh sample lands in the FIFO during the wait loop (at the previously-set 8 kHz the FIFO accumulates a sample every 125 us and races the FIFO_COUNT_H / FIFO_R bulk reads, producing per-axis byte hazards even though the oldest 6 bytes are nominally a clean frame);
- configures FIFO_EN for gyro X / Y / Z only (the FIFO-captured TEMP slot occasionally returns stale data while gyro words are still consistent; mpu3050_read_raw() reads TEMP_H/L directly instead);
- resets the FIFO and enables FIFO read-out in a single USR_CTRL write so the chip atomically resets the write pointer with capture already on (two separate writes leave the FIFO in a brief transient state where the first sample after re-enable can land at a non-zero offset, producing per-byte-shifted frames in the readback);
- waits for one 6-byte sample to land with a generous retry loop;
- reads one 6-byte X / Y / Z frame from FIFO_R;
- tears the FIFO capture down so subsequent buffered/triggered reads aren't confused by stale FIFO_EN bits.

Route IIO_CHAN_INFO_RAW through this helper for IIO_ANGL_VEL. IIO_TEMP keeps its existing direct TEMP_H/L read because that register is not affected by the gyro register-file hazard, and the FIFO-captured TEMP slot occasionally diverges from the live register by several degrees. The triggered-buffer path (mpu3050_trigger_handler()) is unchanged and continues to manage the FIFO itself when the hardware interrupt trigger is active.

The chip is left at ~250 Hz when the helper returns; mpu3050->lpf and mpu3050->divisor are unchanged so the sysfs in_anglvel_sampling_- frequency value resolved via mpu3050_get_freq() keeps reporting the user-configured rate. The next caller of mpu3050_set_8khz_samplerate() (any subsequent IIO_CHAN_INFO_RAW read, or the buffered-mode enable path, or a companion runtime_resume fix sent as a separate patch) issues a full chip reset and reprograms the hardware from cached state.

Test results on the HP TouchPad with the chip lying flat, 15 IIO_CHAN_INFO_RAW reads at 200 ms intervals:

Before this patch (direct XOUT_H/YOUT_H/ZOUT_H reads):

    X = 255             (high byte stuck at 0xFF, permanently)
    Y = -99             (works)
    Z = 0               (low/high bytes both 0x00, permanently)

After this patch (FIFO read path):

Quoted 8 lines
    X = -95..-156       (span 61, mean -127)
    Y =  46..80         (span 34, mean  61)
    Z = -15..23         (span 38, mean   3)
    TEMP = -13440..-13552  (span 112, stable)

Steady-state per-read latency is ~89 ms, dominated by the existing mpu3050_set_8khz_samplerate() reset + msleep(50) before the FIFO helper.

AI assisted?

Quoted 1 line
Signed-off-by Herman van Hazendonk <[email protected]>

...

drivers/iio/gyro/mpu3050-core.c static int mpu3050_set_8khz_samplerate(struct mpu3050 *mpu3050) · line 292

Quoted 1 line
drivers/iio/gyro/mpu3050-core.c +1 -0
+	int retries;

Why signed?

...

Quoted 6 lines
drivers/iio/gyro/mpu3050-core.c +6 -0
+	/*
+	 * Wait for at least one sample to land. At 250 Hz that's nominally
+	 * 4 ms; the retry loop gives a generous margin so a stretched i2c
+	 * clock or NEW sample-rate-change settle time doesn't trip the
+	 * timeout.
+	 */

drivers/iio/gyro/mpu3050-core.c static int mpu3050_set_8khz_samplerate(struct mpu3050 *mpu3050) · line 367

Quoted 10 lines
drivers/iio/gyro/mpu3050-core.c +10 -0
+	for (retries = 0; retries < 10; retries++) {
+		usleep_range(2000, 4000);
+		ret = regmap_bulk_read(mpu3050->map, MPU3050_FIFO_COUNT_H,
+				       &raw_count, sizeof(raw_count));
+		if (ret)
+			goto disable;
+		fifocnt = be16_to_cpu(raw_count);
+		if (fifocnt >= MPU3050_FIFO_FRAME_BYTES)
+			break;
+	}

Can't you use a macro from iopoll.h?

Quoted 5 lines
drivers/iio/gyro/mpu3050-core.c +5 -0
+	if (fifocnt < MPU3050_FIFO_FRAME_BYTES) {
+		dev_dbg(mpu3050->dev, "FIFO timeout (%u bytes)\n", fifocnt);
+		ret = -ETIMEDOUT;
+		goto disable;
+	}

...

drivers/iio/gyro/mpu3050-core.c static int mpu3050_read_raw(struct iio_dev *indio_dev, · line 488

Quoted 10 lines

static int mpu3050_read_raw(struct iio_dev *indio_dev,

drivers/iio/gyro/mpu3050-core.c +0 -2
 					"error reading temperature\n");
 				goto out_read_raw_unlock;
 			}
-
 			*val = (s16)be16_to_cpu(raw_val);
 			ret = IIO_VAL_INT;
-
 			goto out_read_raw_unlock;

Stray changes.

...

drivers/iio/gyro/mpu3050-core.c static int mpu3050_read_raw(struct iio_dev *indio_dev, · line 343

Quoted 4 lines
drivers/iio/gyro/mpu3050-core.c +0 -1
 				goto out_read_raw_unlock;
 			}
 
-

Stray blank removal again.

drivers/iio/gyro/mpu3050-core.c static int mpu3050_read_raw(struct iio_dev *indio_dev, · line 361

Quoted 10 lines
drivers/iio/gyro/mpu3050-core.c +6 -2
-			*val = be16_to_cpu(raw_val);
+			/*
+			 * frame[0..2] = XOUT/YOUT/ZOUT in the order the chip
+			 * packs gyro words into the FIFO. scan_index is 1..3
+			 * for X/Y/Z so subtract one to land on the frame.
+			 */
+			*val = (s16)be16_to_cpu(frame[chan->scan_index - 1]);
 			ret = IIO_VAL_INT;
 
-

...and again.

Quoted 1 line

goto out_read_raw_unlock;

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#4 lore raw
In reply to Herman van Hazendonk

Hi Herman,

I think the others are onto the review, but noticed this:

Quoted 2 lines · Herman van Hazendonk

The direct gyro register file (XOUT_H..ZOUT_L) returns stale data on at least the HP TouchPad (apq8060):

Is that the Qualcomm APQ8060. Do you have graphics working on it?

I have a stale branch for enabling graphics on the APQ8060 but I never managed to finish it...
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git/log/?h=apq8060-dragonboard-graphics-v6.2-rc1

You might want to look into it so just pointing it out.

Yours,
Linus Walleij