From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="bRsY/8PI" Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E7BE610C9 for ; Wed, 29 Nov 2023 06:01:19 -0800 (PST) Received: from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi [213.243.189.158]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id B1895276; Wed, 29 Nov 2023 15:00:39 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1701266439; bh=bsCUEPHkbprB2cJmvODkL1CKSTSpgzCDjFBzaqTwR14=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bRsY/8PI+I3BXcVDSj1253PkM11rFlJGLnaZNjiruXI1p0RWZD8xwq9U0Org+j+gt tuMCzi67VmC40UA2IgagKqMsbUCEBUte9An2hzrO+w31AJaIz7PpiL9LZHqpdjxVfe rKcmNDAZcusdMJFQ3nr63z9/FQYfpRxxrSVf2DE0= Date: Wed, 29 Nov 2023 16:01:22 +0200 From: Laurent Pinchart To: Hans Verkuil Cc: linux-media@vger.kernel.org, Sakari Ailus , Lars-Peter Clausen , Tomi Valkeinen , Martin Kepplinger , Ricardo Ribalda , Dave Stevenson , Manivannan Sadhasivam , Michael Riesch , Jacopo Mondi , Bingbu Cao , Rui Miguel Silva , Hans de Goede , Tianshu Qiu , Steve Longerbeam , Nicholas Roth , Benjamin Mugnier , Sylvain Petinot , Tim Harvey , Maxime Ripard , Eugen Hristev , Kieran Bingham , Dafna Hirschfeld , Yong Deng , Paul Kocialkowski , Benoit Parrot , Philipp Zabel , Shuah Khan , Mauro Carvalho Chehab , Hugues Fruchet , Alain Volmat Subject: Re: [PATCH v4 1/1] media: v4l2-subdev: Rename .init_cfg() operation to .init_state() Message-ID: <20231129140122.GE18109@pendragon.ideasonboard.com> References: <20231127090744.30012-1-laurent.pinchart@ideasonboard.com> <20231127090744.30012-2-laurent.pinchart@ideasonboard.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: Hi Hans, On Tue, Nov 28, 2023 at 09:55:06AM +0100, Hans Verkuil wrote: > On 27/11/2023 10:07, Laurent Pinchart wrote: > > The subdev .init_cfg() operation is affected by two issues: > > > > - It has long been extended to initialize a whole v4l2_subdev_state > > instead of just a v4l2_subdev_pad_config, but its name has stuck > > around. > > > > - Despite operating on a whole subdev state and not being directly > > exposed to the subdev users (either in-kernel or through the userspace > > API), .init_cfg() is categorized as a subdev pad operation. > > > > This participates in making the subdev API confusing for new developers. > > Fix it by renaming the operation to .init_state(), and make it a subdev > > internal operation. > > > > Signed-off-by: Laurent Pinchart > > Acked-by: Michael Riesch # for imx415 > > Acked-by: Shuah Khan # for vimc > > --- > > Changes since v3: > > > > - Rebase on top of the new stm32-dcmipp driver > > - Fix uninitialized variable in __v4l2_subdev_state_alloc() due to bad > > rebase > > > > Changes since v2: > > > > - Rebase on top of the latest media tree > > > > Changes since v1: > > > > - Fix compilation of the vsp1 driver > > --- > > > > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.c b/drivers/media/platform/renesas/vsp1/vsp1_entity.c > > index 0280b8ff7ba9..0a5a7f9cc870 100644 > > --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.c > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.c > > @@ -170,33 +170,6 @@ vsp1_entity_get_pad_selection(struct vsp1_entity *entity, > > } > > } > > > > -/* > > - * vsp1_entity_init_cfg - Initialize formats on all pads > > - * @subdev: V4L2 subdevice > > - * @sd_state: V4L2 subdev state > > - * > > - * Initialize all pad formats with default values in the given subdev state. > > - * This function can be used as a handler for the subdev pad::init_cfg > > - * operation. > > - */ > > -int vsp1_entity_init_cfg(struct v4l2_subdev *subdev, > > - struct v4l2_subdev_state *sd_state) > > -{ > > - unsigned int pad; > > - > > - for (pad = 0; pad < subdev->entity.num_pads - 1; ++pad) { > > - struct v4l2_subdev_format format = { > > - .pad = pad, > > - .which = sd_state ? V4L2_SUBDEV_FORMAT_TRY > > - : V4L2_SUBDEV_FORMAT_ACTIVE, > > - }; > > - > > - v4l2_subdev_call(subdev, pad, set_fmt, sd_state, &format); > > - } > > - > > - return 0; > > -} > > - > > /* > > * vsp1_subdev_get_pad_format - Subdev pad get_fmt handler > > * @subdev: V4L2 subdevice > > @@ -424,6 +397,29 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev, > > return ret; > > } > > > > +static int vsp1_entity_init_state(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_state *sd_state) > > +{ > > + unsigned int pad; > > + > > + /* Initialize all pad formats with default values. */ > > + for (pad = 0; pad < subdev->entity.num_pads - 1; ++pad) { > > + struct v4l2_subdev_format format = { > > + .pad = pad, > > + .which = sd_state ? V4L2_SUBDEV_FORMAT_TRY > > + : V4L2_SUBDEV_FORMAT_ACTIVE, > > + }; > > + > > + v4l2_subdev_call(subdev, pad, set_fmt, sd_state, &format); > > + } > > + > > + return 0; > > +} > > + > > +static const struct v4l2_subdev_internal_ops vsp1_entity_internal_ops = { > > + .init_state = vsp1_entity_init_state, > > +}; > > + > > /* ----------------------------------------------------------------------------- > > * Media Operations > > */ > > @@ -658,6 +654,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, > > /* Initialize the V4L2 subdev. */ > > subdev = &entity->subdev; > > v4l2_subdev_init(subdev, ops); > > + subdev->internal_ops = &vsp1_entity_internal_ops; > > > > subdev->entity.function = function; > > subdev->entity.ops = &vsp1->media_ops; > > @@ -666,7 +663,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, > > snprintf(subdev->name, sizeof(subdev->name), "%s %s", > > dev_name(vsp1->dev), name); > > > > - vsp1_entity_init_cfg(subdev, NULL); > > + vsp1_entity_init_state(subdev, NULL); > > This is the only media driver that calls init_cfg/state directly. > That's a bit unexpected, and perhaps this could use a comment. That > can be a follow-up patch as well. I have already posted a patch series to drop all this and use the V4L2 subdev active state API. That is hopefully better than a comment :-) There are actually quite a few sensor drivers that don't use the active state, and call the .init_cfg() handler at probe time to initialize their private copy of the active state. All this should eventually go away as drivers are converter. I try to keep an eye when reviewing new code and push all new drivers to use the subdev active state. > > > > /* > > * Allocate the subdev state to store formats and selection > > In any case, you can add my: > > Reviewed-by: Hans Verkuil > > to this patch. -- Regards, Laurent Pinchart