From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 32A6BC07E9C for ; Tue, 6 Jul 2021 16:03:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 12CD2613D1 for ; Tue, 6 Jul 2021 16:03:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229812AbhGFQFx (ORCPT ); Tue, 6 Jul 2021 12:05:53 -0400 Received: from relay8-d.mail.gandi.net ([217.70.183.201]:41323 "EHLO relay8-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229770AbhGFQFw (ORCPT ); Tue, 6 Jul 2021 12:05:52 -0400 Received: (Authenticated sender: jacopo@jmondi.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id E7B5F1BF208; Tue, 6 Jul 2021 16:03:11 +0000 (UTC) Date: Tue, 6 Jul 2021 18:04:01 +0200 From: Jacopo Mondi To: Niklas =?utf-8?Q?S=C3=B6derlund?= Cc: Hans Verkuil , linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org Subject: Re: [PATCH 01/11] rcar-vin: Refactor controls creation for video device Message-ID: <20210706160401.xssshab7nkxroxnp@uno.localdomain> References: <20210413180253.2575451-1-niklas.soderlund+renesas@ragnatech.se> <20210413180253.2575451-2-niklas.soderlund+renesas@ragnatech.se> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20210413180253.2575451-2-niklas.soderlund+renesas@ragnatech.se> Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Hi Niklas, On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote: > The controls for the video device are created in different code paths > depending on if the driver is using the media graph centric model (Gen3) > or the device centric model (Gen2 and earlier). This have lead to code > duplication that can be consolidated. > > Signed-off-by: Niklas Söderlund > --- > drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++---------- > 1 file changed, 45 insertions(+), 37 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index cb3025992817d625..c798dc9409e4cdcd 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = { > .s_ctrl = rvin_s_ctrl, > }; > > +static void rvin_free_controls(struct rvin_dev *vin) > +{ > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > + vin->vdev.ctrl_handler = NULL; > +} > + > +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev) > +{ > + int ret; > + > + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > + if (ret < 0) > + return ret; > + > + /* The VIN directly deals with alpha component. */ > + v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > + V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > + > + if (vin->ctrl_handler.error) { > + ret = vin->ctrl_handler.error; > + rvin_free_controls(vin); > + return ret; > + } > + > + /* For the non-MC mode add controls from the subdevice. */ > + if (subdev) { > + ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, > + subdev->ctrl_handler, NULL, true); > + if (ret < 0) { > + rvin_free_controls(vin); > + return ret; > + } > + } > + > + vin->vdev.ctrl_handler = &vin->ctrl_handler; > + > + return 0; > +} > + > /* ----------------------------------------------------------------------------- > * Async notifier > */ > @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > return ret; > > /* Add the controls */ > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > + ret = rvin_create_controls(vin, subdev); > if (ret < 0) > return ret; > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > - > - if (vin->ctrl_handler.error) { > - ret = vin->ctrl_handler.error; > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - return ret; > - } > - > - ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler, > - NULL, true); > - if (ret < 0) { > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - return ret; > - } > - > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > - > vin->parallel.subdev = subdev; > > return 0; > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > rvin_v4l2_unregister(vin); > vin->parallel.subdev = NULL; > > - if (!vin->info->use_mc) { > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - vin->vdev.ctrl_handler = NULL; > - } > + if (!vin->info->use_mc) I know it was there already, but give that rvin_parallel_notify_unbind() is only registered for parallel, can this happen ? Apart this small nit: Reviewed-by: Jacopo Mondi Thanks j > + rvin_free_controls(vin); > } > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin) > if (ret) > rvin_group_put(vin); > > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1); > + ret = rvin_create_controls(vin, NULL); > if (ret < 0) > return ret; > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > - > - if (vin->ctrl_handler.error) { > - ret = vin->ctrl_handler.error; > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - return ret; > - } > - > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > - > return ret; > } > > @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev) > return 0; > > error_group_unregister: > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > + rvin_free_controls(vin); > > if (vin->info->use_mc) { > mutex_lock(&vin->group->lock); > @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev) > rvin_group_put(vin); > } > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > + rvin_free_controls(vin); > > rvin_dma_unregister(vin); > > -- > 2.31.1 >