From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B7921395DBC for ; Thu, 16 Apr 2026 16:24:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776356661; cv=none; b=cgbp20l1DUjlhglhpEby48b4jzqfNffATA9yL64r0KhAbusCllveRFKEUDWnggKNNVoxjEM3JyaWqg2zXPJT9UFKXsT1VgEZhn04acKmJT5yyBz65vX86WaZ9gbl8y3q/jVrN3QOOI4rVkYuKEnJYMzSsH27kqMj7c2UeUYdGBw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776356661; c=relaxed/simple; bh=8CH3r9aR9kdGPIRsKGpFV1YmuoWiK9twlPnH0Sjusmc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=EFdHiEZg2/85Qn5Wd0MNIsw/n/qYzT4txx3dBvlZ2IU8X37Vj7emhLn501HNFNYHGZEWOYndltj38Zm0Pu8I9hsKSlZvUUF0HUQ94kzoTZXipOQycj6vMvL0v24FJnVsGaTg/oqlVw1A2MaRoV1IqaPdGPX1F7aR5fFQCahNEuM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=ZeZs6ufl; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="ZeZs6ufl" Received: from killaraus.ideasonboard.com (2001-14ba-703d-e500--2a1.rev.dnainternet.fi [IPv6:2001:14ba:703d:e500::2a1]) by perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 8590E132; Thu, 16 Apr 2026 18:22:42 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1776356562; bh=8CH3r9aR9kdGPIRsKGpFV1YmuoWiK9twlPnH0Sjusmc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ZeZs6uflxGLm8TCKQv2I2hU+l7CCUBEgbFpRkQCVECstAGGaRJERhq0MhlHxGXl77 uOyZ1zz9MWsQd+aioawaAjbUKh+iIP25ZeLVeTIPaXhAcT5kEgisFRmqDs7hj0mgxg ncDKcq/B2z86tqLaWRfieCEuxqEEptPJ2G6dEoG0= Date: Thu, 16 Apr 2026 19:24:15 +0300 From: Laurent Pinchart To: Sakari Ailus Cc: linux-media@vger.kernel.org, hans@jjverkuil.nl, Prabhakar , Kate Hsuan , Dave Stevenson , Tommaso Merciai , Benjamin Mugnier , Sylvain Petinot , Christophe JAILLET , Julien Massot , Naushir Patuck , "Yan, Dongcheng" , "Cao, Bingbu" , "Qiu, Tian Shu" , Stefan Klug , Mirela Rabulea , =?utf-8?B?QW5kcsOp?= Apitzsch , Heimir Thor Sverrisson , Kieran Bingham , Mehdi Djait , Ricardo Ribalda Delgado , Hans de Goede , Jacopo Mondi , Tomi Valkeinen , David Plowman , "Yu, Ong Hock" , "Ng, Khai Wen" , Jai Luthra , Rishikesh Donadkar Subject: Re: [PATCH v4 16/29] media: v4l2-subdev: Refactor returning routes Message-ID: <20260416162415.GE1823068@killaraus.ideasonboard.com> References: <20260408153939.969381-1-sakari.ailus@linux.intel.com> <20260408153939.969381-17-sakari.ailus@linux.intel.com> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20260408153939.969381-17-sakari.ailus@linux.intel.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 > Reviewed-by: Michael Riesch > --- > 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 > + > + 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