Re: [PATCH 3/3] media: subdev: Split v4l2_subdev_get_frame_desc_passthrough() into locked and unlocked

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Moi,

On Thu, Mar 12, 2026 at 02:15:30PM +0200, Tomi Valkeinen wrote:
> The recently added v4l2_subdev_get_frame_desc_passthrough() can be used
> directly as an implementation for .get_frame_desc subdev op. However, in
> some cases the drivers may want to add some customizations, while the
> bulk of the work is still identical to what
> v4l2_subdev_get_frame_desc_passthrough() does. Current locking scheme
> makes this impossible to do properly.
> 
> Split v4l2_subdev_get_frame_desc_passthrough() into two functions:
> 
> v4l2_subdev_get_frame_desc_passthrough_locked(), which takes a locked
> subdev state as a parameter, instead of locking and getting the active
> state internally. Other than that, it does the same as
> v4l2_subdev_get_frame_desc_passthrough() used to do.
> 
> v4l2_subdev_get_frame_desc_passthrough(), which locks the active state
> and calls v4l2_subdev_get_frame_desc_passthrough_locked().
> 
> In other words, v4l2_subdev_get_frame_desc_passthrough() works as
> before, but drivers can now alternatively add custom .get_frame_desc
> code and call v4l2_subdev_get_frame_desc_passthrough().
> 
> An example use case is with DS90UB953 serializer: in normal use the
> serializer passes through everything, but when test-pattern-generator
> (TPG) is used, an internal TPG source is used. After this commit, the
> UB953 get_frame_desc() can lock the state, look at the routing table to
> see if we're in normal or TPG mode, then either call
> v4l2_subdev_get_frame_desc_passthrough_locked() if in normal mode, or
> construct a TPG frame desc if in TPG mode.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 46 ++++++++++++++++++++---------------
>  include/media/v4l2-subdev.h           | 38 +++++++++++++++++++++++++----
>  2 files changed, 59 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 2757378c628a..8b1a7f00c86b 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2545,21 +2545,19 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_s_stream_helper);
>  
> -int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
> -					   unsigned int pad,
> -					   struct v4l2_mbus_frame_desc *fd)
> +int v4l2_subdev_get_frame_desc_passthrough_locked(struct v4l2_subdev *sd,
> +						  struct v4l2_subdev_state *state,
> +						  unsigned int pad,
> +						  struct v4l2_mbus_frame_desc *fd)
>  {
>  	struct media_pad *local_sink_pad;
>  	struct v4l2_subdev_route *route;
> -	struct v4l2_subdev_state *state;
>  	struct device *dev = sd->dev;
>  	int ret = 0;
>  
>  	if (WARN_ON(!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)))
>  		return -EINVAL;
>  
> -	state = v4l2_subdev_lock_and_get_active_state(sd);

This variant appears to be unlocked rather than locked.

Could you instead use two underscores as a prefix to the same name? That's
an established practice.

-- 
Terveisin,

Sakari Ailus