Enhance DS5 sensor configuration with retry logic for I2C operations and caching of parameters - #348
Enhance DS5 sensor configuration with retry logic for I2C operations and caching of parameters#348Nikolai-L wants to merge 1 commit into
Conversation
…and caching of parameters
There was a problem hiding this comment.
Pull request overview
Adds robustness and reduces redundant I2C programming for DS5 sensors by introducing retry logic around regmap operations and caching previously programmed pipeline/stream parameters.
Changes:
- Add retry-with-backoff for I2C regmap read/write operations on transient errors.
- Cache pipeline configuration inputs (pipe id, data types, VC) to avoid re-setting the pipeline when unchanged.
- Cache frequently-written DS5 configuration register values to skip redundant writes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (sensor->cached_dt_value != dt_value) { | ||
| ret = ds5_write(state, dt_addr, dt_value); | ||
| if (ret < 0) | ||
| return ret; | ||
| sensor->cached_dt_value = dt_value; | ||
| } |
There was a problem hiding this comment.
The cache fields appear to rely on default initialization, which can cause the very first write to be skipped when the desired value is 0 (e.g., dt_value == 0 and cached_dt_value defaults to 0). That changes behavior from always writing to potentially never programming the register. Fix by initializing cached_* to an invalid sentinel (e.g., 0xFFFF for u16, 0xFFFFFFFF for u32) or by adding per-field 'valid' flags so the first configuration always writes.
| dev_warn(&state->client->dev, "release pipe failed\n"); | ||
| sensor->pipe_id = -1; | ||
| sensor->last_pipe_id = -1; | ||
| sensor->pipe_configured = false; |
There was a problem hiding this comment.
On stream stop / pipe release you invalidate pipe_configured/last_pipe_id, but the cached register values (cached_dt_value, cached_md_value, cached_override_value, cached_fps_value, cached_width_value, cached_height_value) are left intact. If the device/firmware loses state across stop/start (or if registers are reset elsewhere), ds5_configure() may skip required writes and leave hardware misconfigured. Consider explicitly invalidating all cached_* fields here (and in the corresponding stream-on reset path) by setting them back to the sentinel/invalid state (or clearing 'valid' flags).
| sensor->pipe_configured = false; | |
| sensor->pipe_configured = false; | |
| sensor->cached_dt_value = -1; | |
| sensor->cached_md_value = -1; | |
| sensor->cached_override_value = -1; | |
| sensor->cached_fps_value = -1; | |
| sensor->cached_width_value = -1; | |
| sensor->cached_height_value = -1; |
| dev_warn(&state->client->dev, | ||
| "pipe %d configured (dt1=0x%x dt2=0x%x vc=%u)\n", | ||
| sensor->pipe_id, data_type1, data_type2, vc_id); | ||
| } else { | ||
| dev_warn(&state->client->dev, |
There was a problem hiding this comment.
These dev_warn() logs will likely fire on every start/configure call and can spam dmesg in normal operation (especially the 'already configured' case). Consider switching these to dev_dbg(), or dev_warn_ratelimited() if you want visibility without flooding logs.
| dev_warn(&state->client->dev, | |
| "pipe %d configured (dt1=0x%x dt2=0x%x vc=%u)\n", | |
| sensor->pipe_id, data_type1, data_type2, vc_id); | |
| } else { | |
| dev_warn(&state->client->dev, | |
| dev_dbg(&state->client->dev, | |
| "pipe %d configured (dt1=0x%x dt2=0x%x vc=%u)\n", | |
| sensor->pipe_id, data_type1, data_type2, vc_id); | |
| } else { | |
| dev_dbg(&state->client->dev, |
| for (retry = 0; retry < 3; retry++) { | ||
| ret = regmap_raw_write(state->regmap, reg, value, sizeof(value)); | ||
| if (ret == 0) | ||
| break; | ||
|
|
||
| /* Retry only on timeout/remote IO errors */ | ||
| if (ret != -ETIMEDOUT && ret != -EREMOTEIO) { | ||
| dev_err(&state->client->dev, | ||
| "%s(): i2c write failed %d, 0x%04x = 0x%x\n", | ||
| __func__, ret, reg, val); | ||
| return ret; | ||
| } | ||
|
|
||
| if (retry < 2) { | ||
| dev_dbg(&state->client->dev, | ||
| "%s(): retry %d after %dms delay, 0x%04x\n", | ||
| __func__, retry + 1, delay_ms, reg); | ||
| msleep(delay_ms); | ||
| delay_ms *= 2; | ||
| } | ||
| } |
There was a problem hiding this comment.
The retry/backoff logic is duplicated across ds5_write(), ds5_raw_write(), and ds5_read(). To reduce the chance of these drifting (different retry counts, delays, error handling, log messages), consider factoring the common retry policy into a shared helper (e.g., a small internal function that accepts an op callback or separate helpers for read/write) and reuse it in all three call sites.
Caching doesn't work yet as expected. I suspect (sensor->pipe_id != sensor->last_pipe_id) condition is not the right one to check.