Re: [PATCH v4 16/29] media: v4l2-subdev: Refactor returning routes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

Thank you for the patch.

On Wed, Apr 08, 2026 at 06:39:25PM +0300, Sakari Ailus wrote:
> Refactor returning the routes by adding a new function that essentially
> does a memcopy and sets the number of the routes in the routing table.

I'd write "factor out" instead of "refactor", here and in the subject
line. Then you can add

This avoids code duplication.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Michael Riesch <michael.riesch@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 34 +++++++++++++--------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 831c69c958b8..f8fde395a53a 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -629,6 +629,19 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
>  			     v4l2_subdev_get_unlocked_active_state(sd);
>  }
>  
> +static void copy_routes_state_to_routing(struct v4l2_subdev_routing *routing,
> +					 const struct v4l2_subdev_state *state)

v4l2_subdev_ prefix.

> +{
> +	struct v4l2_subdev_route *routes =
> +		(struct v4l2_subdev_route *)(uintptr_t)routing->routes;
> +	u32 copy_routes = min(routing->len_routes, state->routing.num_routes);
> +
> +	for (u32 i = 0; i < copy_routes; i++)
> +		routes[i] = state->routing.routes[i];

Any reason you use a loop instead of memcpy() ? If so, please document
it in the commit message.

With all that addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> +
> +	routing->num_routes = state->routing.num_routes;
> +}
> +
>  static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>  			    struct v4l2_subdev_state *state)
>  {
> @@ -1000,7 +1013,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>  
>  	case VIDIOC_SUBDEV_G_ROUTING: {
>  		struct v4l2_subdev_routing *routing = arg;
> -		struct v4l2_subdev_krouting *krouting;
>  
>  		if (!v4l2_subdev_enable_streams_api)
>  			return -ENOIOCTLCMD;
> @@ -1010,13 +1022,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>  
>  		memset(routing->reserved, 0, sizeof(routing->reserved));
>  
> -		krouting = &state->routing;
> -
> -		memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
> -		       krouting->routes,
> -		       min(krouting->num_routes, routing->len_routes) *
> -		       sizeof(*krouting->routes));
> -		routing->num_routes = krouting->num_routes;
> +		copy_routes_state_to_routing(routing, state);
>  
>  		return 0;
>  	}
> @@ -1084,11 +1090,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>  		 * the routing table.
>  		 */
>  		if (!v4l2_subdev_has_op(sd, pad, set_routing)) {
> -			memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
> -			       state->routing.routes,
> -			       min(state->routing.num_routes, routing->len_routes) *
> -			       sizeof(*state->routing.routes));
> -			routing->num_routes = state->routing.num_routes;
> +			copy_routes_state_to_routing(routing, state);
>  
>  			return 0;
>  		}
> @@ -1102,11 +1104,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>  		if (rval < 0)
>  			return rval;
>  
> -		memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
> -		       state->routing.routes,
> -		       min(state->routing.num_routes, routing->len_routes) *
> -		       sizeof(*state->routing.routes));
> -		routing->num_routes = state->routing.num_routes;
> +		copy_routes_state_to_routing(routing, state);
>  
>  		return 0;
>  	}

-- 
Regards,

Laurent Pinchart