From: Niklas Söderlund niklas.soderlund+renesas@ragnatech.se
[ Upstream commit 91c5d7c849273d14bc4bae1b92666bdb5409294a ]
The .querystd callback should not program the device with the detected standard, it should only report the standard to user-space. User-space may then use .s_std to set the standard, if it wants to use it.
All that is required of .querystd is to setup the auto detection of standards and report its findings.
While at it add some documentation on why this can't happen while streaming and improve the error handling using a scoped guard.
Signed-off-by: Niklas Söderlund niklas.soderlund+renesas@ragnatech.se Signed-off-by: Hans Verkuil hverkuil+cisco@kernel.org Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
YES
- What it fixes - Removes unintended device reprogramming in `.querystd`: Previously, after enabling autodetection and sampling, the code reprogrammed the decoder using the configured `curr_norm` instead of leaving hardware untouched. If userspace had never called `.s_std`, `curr_norm` defaults to NTSC (drivers/media/i2c/adv7180.c:1463), so a simple `.querystd` could force NTSC regardless of the detected standard. The new code stops doing that and only reports the detected standard. - Aligns with V4L2 semantics: `.querystd` should detect and report, not change the active standard. Userspace can call `.s_std` to set it.
- Key code changes - adv7180_querystd only sets autodetect, waits, and reports the detected standard: - Sets autodetect: drivers/media/i2c/adv7180.c:388 - Returns detection result directly: drivers/media/i2c/adv7180.c:388 - Adds clear rationale comment about not running during streaming since it touches VID_SEL: drivers/media/i2c/adv7180.c:388 - Removes the reprogramming step via `v4l2_std_to_adv7180(state->curr_norm)` and the second `adv7180_set_video_standard(...)` (these were in the old body and are now gone), eliminating side effects of `.querystd`. - Improves error handling and robustness by using a scoped guard for the mutex (auto-unlock on all paths) and by returning the result of `__adv7180_status()` instead of ignoring it (drivers/media/i2c/adv7180.c:388).
- Why it’s safe and minimal - Localized change: confined to `adv7180_querystd` only (drivers/media/i2c/adv7180.c:388). - No ABI or architectural changes; just corrects behavior to be read- only. - Streaming safety preserved: returns `-EBUSY` while streaming to avoid touching VID_SEL mid-capture (drivers/media/i2c/adv7180.c:388). - Consistent control flow: `.s_std` now only validates and stores the intended standard in `curr_norm` (drivers/media/i2c/adv7180.c:463), and actual programming is done at stream start via `adv7180_program_std()` (drivers/media/i2c/adv7180.c:449) called by initialization/streaming code paths. Leaving the device in autodetect after `.querystd` does not affect users because streaming is off (enforced by `-EBUSY`) and streaming will reprogram from `curr_norm` anyway.
- User-visible impact addressed - Prevents `.querystd` from changing hardware state (e.g., enforcing NTSC because `curr_norm` defaults to NTSC at probe: drivers/media/i2c/adv7180.c:1463), which could break subsequent expectations if userspace hasn’t explicitly called `.s_std`.
- Backport considerations - The `guard(mutex)` scoped guard may not exist in older stable series. That’s trivial to adapt to explicit `mutex_lock_interruptible()`/`mutex_unlock()` with identical behavior. No other dependencies or refactors are required.
- Subsystem and risk profile - Touches a single V4L2 i2c decoder driver; small, self-contained change with clear behavioral bugfix and minimal regression risk. - Follows stable rules: bugfix, no new features, no architectural churn, and limited scope.
Given these points, this is a good candidate for stable backporting.
drivers/media/i2c/adv7180.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c index 8100fe6b0f1d4..5accf3020e076 100644 --- a/drivers/media/i2c/adv7180.c +++ b/drivers/media/i2c/adv7180.c @@ -357,32 +357,27 @@ static inline struct adv7180_state *to_state(struct v4l2_subdev *sd) static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std) { struct adv7180_state *state = to_state(sd); - int err = mutex_lock_interruptible(&state->mutex); - if (err) - return err; - - if (state->streaming) { - err = -EBUSY; - goto unlock; - } + int ret;
- err = adv7180_set_video_standard(state, - ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM); - if (err) - goto unlock; + guard(mutex)(&state->mutex);
- msleep(100); - __adv7180_status(state, NULL, std); + /* + * We can't sample the standard if the device is streaming as that would + * interfere with the capture session as the VID_SEL reg is touched. + */ + if (state->streaming) + return -EBUSY;
- err = v4l2_std_to_adv7180(state->curr_norm); - if (err < 0) - goto unlock; + /* Set the standard to autodetect PAL B/G/H/I/D, NTSC J or SECAM */ + ret = adv7180_set_video_standard(state, + ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM); + if (ret) + return ret;
- err = adv7180_set_video_standard(state, err); + /* Allow some time for the autodetection to run. */ + msleep(100);
-unlock: - mutex_unlock(&state->mutex); - return err; + return __adv7180_status(state, NULL, std); }
static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,