Re: [PATCH] iio: gyro: mpu3050: read gyro samples via FIFO in IIO_CHAN_INFO_RAW

[Andy Shevchenko]

On Fri, Jun 05, 2026 at 10:46:21AM +0200, Herman van Hazendonk wrote:
> 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.

AI assisted?

> Signed-off-by: Herman van Hazendonk <github.com@xxxxxxxxxx>

...

> +	int retries;

Why signed?

...

> +	/*
> +	 * 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;
> +	}

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

> +	if (fifocnt < MPU3050_FIFO_FRAME_BYTES) {
> +		dev_dbg(mpu3050->dev, "FIFO timeout (%u bytes)\n", fifocnt);
> +		ret = -ETIMEDOUT;
> +		goto disable;
> +	}

...

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

Stray changes.

...

>  				goto out_read_raw_unlock;
>  			}

> -

Stray blank removal again.

> -			*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.

>  			goto out_read_raw_unlock;

-- 
With Best Regards,
Andy Shevchenko