Received: by 2002:a05:7412:8d10:b0:f3:1519:9f41 with SMTP id bj16csp4154242rdb; Mon, 11 Dec 2023 10:16:03 -0800 (PST) X-Google-Smtp-Source: AGHT+IHXp3fVCDYSJuBmok1fyO2rljODmXwtnpqDX5IKVfHt5YQPwWu6A9F3NH8ps5yKtvPvFOmF X-Received: by 2002:a05:6a20:8b13:b0:18f:cc29:3d99 with SMTP id l19-20020a056a208b1300b0018fcc293d99mr5385521pzh.57.1702318562854; Mon, 11 Dec 2023 10:16:02 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1702318562; cv=none; d=google.com; s=arc-20160816; b=fNBAHA2m/CraG6L3pPLex2OME6EiUM4U58U7LiVbkad1JG9r+drZc/duiw43vXhffm B3xIxTJh8PuNvss1MLEckKLOnvC/yCWEKkwcps+1H1z4i5w5S92ZzVbmTm/noPsPYeCR 9Mt5TFdDFUoy8ctAdqz+ujMrIcFA3X0cZKzq5MQCTmZFm410FwDU3lsPzywoa75Ggrfi 3K/wfTaQ7uRnUKrhIFnfgkBYUl4X+EP9B5YhkCHg0XsArgTOemiRtRmWnALFd3c3Cw3L gurhn6x1xGy8Q0HiWALjrhO8iFIgFI7kNrqfzytzy8xVTZL4OvnXrACchQpP6o0zzO9f OGAw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=b/7kuAmOFZQRziRw6AW5IcuyxdP8Q7kWrcbda7drSB8=; fh=jqTWrQ5bCH5SYcBidMUA87AoUf4V55L1bAZhntpQF1g=; b=PfD0h8JR9RzYUUGhdOmpeXoXU/5X0MSaeTt6i1lNk0cUafTDeL0VBPtSg3JeeTpeP0 Ojz3PWGXHRsFv5GFUL1sHCwLLaNC36Qix+QB2FcA3fJEKQunlCLNUkOdOG9ReQmoTC3D KLzw2xZSS2iI2BlnYAtQDMY8Sypy+F/Ox/wT5w6DXKQgV6wRPA97Z3KVu8Ad0DuLPlfw ywMMK9Ai3XmPfBE/rcS3KDrK20agFlvrLXi/41MFcjYGzoy8O5KqwNFhSTZyKwcmU3da fSHgMI9HJ0OH12yz1Uqel1EUI92X46nnCm6Su+Aw3sbBJqKaEPDer38yTBtL+24A/Vtt +ZPw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=GoRnpURY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from howler.vger.email (howler.vger.email. [23.128.96.34]) by mx.google.com with ESMTPS id b1-20020a6567c1000000b005c6ac5b5fd1si6283359pgs.676.2023.12.11.10.16.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 Dec 2023 10:16:02 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34; Authentication-Results: mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=GoRnpURY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id 3E889807C5EA; Mon, 11 Dec 2023 10:15:35 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343918AbjLKSPR (ORCPT + 99 others); Mon, 11 Dec 2023 13:15:17 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52212 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231745AbjLKSPP (ORCPT ); Mon, 11 Dec 2023 13:15:15 -0500 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9B556C7; Mon, 11 Dec 2023 10:15:20 -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 DC060922; Mon, 11 Dec 2023 19:14:34 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1702318475; bh=8eSow/3dFTG1DAJOMkS/gxZZoikGXWE51x4cXT/zPqM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=GoRnpURY/6iclbnSYFnh+4jlqpz91ialh+KlheDfqIjgavbbwopABrkd9RpdD3cIE Owl7/f8IETX30TzkFtqrc+h+7LmnKHeaoqzqfU2eyCNGhGfgpGlR42IC41WTUxf7e1 FfP28n4m2pFYTCC/kv+i+zuPsZ54nOA7pMfhveL0= Date: Mon, 11 Dec 2023 20:15:26 +0200 From: Laurent Pinchart To: Mikhail Rudenko Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Sakari Ailus , Jacopo Mondi , Tommaso Merciai , Christophe JAILLET , Dave Stevenson , Mauro Carvalho Chehab Subject: Re: [PATCH 07/19] media: i2c: ov4689: Use sub-device active state Message-ID: <20231211181526.GF27535@pendragon.ideasonboard.com> References: <20231211175023.1680247-1-mike.rudenko@gmail.com> <20231211175023.1680247-8-mike.rudenko@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20231211175023.1680247-8-mike.rudenko@gmail.com> X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Mon, 11 Dec 2023 10:15:35 -0800 (PST) Hi Mikhail, Thank you for the patch. On Mon, Dec 11, 2023 at 08:50:10PM +0300, Mikhail Rudenko wrote: > Use sub-device active state. Employ control handler lock to > synchronize access to the active state and s_stream. > > Signed-off-by: Mikhail Rudenko > --- > drivers/media/i2c/ov4689.c | 68 +++++++++++++++----------------------- > 1 file changed, 27 insertions(+), 41 deletions(-) > > diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c > index 5392f650960c..2eef64cd0070 100644 > --- a/drivers/media/i2c/ov4689.c > +++ b/drivers/media/i2c/ov4689.c > @@ -86,7 +86,6 @@ struct ov4689 { > > u32 clock_rate; > > - struct mutex mutex; /* lock to protect ctrls and cur_mode */ > struct v4l2_ctrl_handler ctrl_handler; > struct v4l2_ctrl *exposure; > > @@ -319,19 +318,6 @@ static int ov4689_set_fmt(struct v4l2_subdev *sd, > return 0; > } > > -static int ov4689_get_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_format *fmt) > -{ > - struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > - struct ov4689 *ov4689 = to_ov4689(sd); > - > - /* only one mode supported for now */ > - ov4689_fill_fmt(ov4689->cur_mode, mbus_fmt); > - > - return 0; > -} > - > static int ov4689_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_mbus_code_enum *code) > @@ -405,10 +391,11 @@ static int ov4689_get_selection(struct v4l2_subdev *sd, > static int ov4689_s_stream(struct v4l2_subdev *sd, int on) > { > struct ov4689 *ov4689 = to_ov4689(sd); > + struct v4l2_subdev_state *sd_state; > struct device *dev = ov4689->dev; > int ret = 0; > > - mutex_lock(&ov4689->mutex); > + sd_state = v4l2_subdev_lock_and_get_active_state(&ov4689->subdev); > > if (on) { > ret = pm_runtime_resume_and_get(dev); > @@ -443,7 +430,7 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on) > } > > unlock_and_return: > - mutex_unlock(&ov4689->mutex); > + v4l2_subdev_unlock_state(sd_state); > > return ret; > } > @@ -506,19 +493,15 @@ static int __maybe_unused ov4689_power_off(struct device *dev) > return 0; > } > > -static int ov4689_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +static int ov4689_init_cfg(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state) > { > - struct ov4689 *ov4689 = to_ov4689(sd); > - struct v4l2_mbus_framefmt *try_fmt; > - > - mutex_lock(&ov4689->mutex); > + struct v4l2_mbus_framefmt *try_fmt = > + v4l2_subdev_state_get_format(sd_state, 0); > > - try_fmt = v4l2_subdev_state_get_format(fh->state, 0); > /* Initialize try_fmt */ It's not necessarily a "try format" anymore, so you cand rop this comment, and rename the variable from try_fmt to fmt. > ov4689_fill_fmt(&supported_modes[OV4689_MODE_2688_1520], try_fmt); > > - mutex_unlock(&ov4689->mutex); > - > return 0; > } > > @@ -526,18 +509,15 @@ static const struct dev_pm_ops ov4689_pm_ops = { > SET_RUNTIME_PM_OPS(ov4689_power_off, ov4689_power_on, NULL) > }; > > -static const struct v4l2_subdev_internal_ops ov4689_internal_ops = { > - .open = ov4689_open, > -}; > - > static const struct v4l2_subdev_video_ops ov4689_video_ops = { > .s_stream = ov4689_s_stream, > }; > > static const struct v4l2_subdev_pad_ops ov4689_pad_ops = { > + .init_cfg = ov4689_init_cfg, Please rebase on top of the latest media tree for the next version, the .init_cfg() operation has been renamed to .init_state() and moved to v4l2_subdev_internal_ops. Other than that, this patch looks good. Reviewed-by: Laurent Pinchart > .enum_mbus_code = ov4689_enum_mbus_code, > .enum_frame_size = ov4689_enum_frame_sizes, > - .get_fmt = ov4689_get_fmt, > + .get_fmt = v4l2_subdev_get_fmt, > .set_fmt = ov4689_set_fmt, > .get_selection = ov4689_get_selection, > }; > @@ -649,7 +629,6 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) > ret = v4l2_ctrl_handler_init(handler, 10); > if (ret) > return ret; > - handler->lock = &ov4689->mutex; > > ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0, > link_freq_menu_items); > @@ -862,13 +841,14 @@ static int ov4689_probe(struct i2c_client *client) > return dev_err_probe(dev, ret, > "Failed to get power regulators\n"); > > - mutex_init(&ov4689->mutex); > - > sd = &ov4689->subdev; > v4l2_i2c_subdev_init(sd, client, &ov4689_subdev_ops); > + > ret = ov4689_initialize_controls(ov4689); > - if (ret) > - goto err_destroy_mutex; > + if (ret) { > + dev_err(dev, "Failed to initialize controls\n"); > + return ret; > + } > > ret = ov4689_power_on(dev); > if (ret) > @@ -878,19 +858,26 @@ static int ov4689_probe(struct i2c_client *client) > if (ret) > goto err_power_off; > > - sd->internal_ops = &ov4689_internal_ops; > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR; > > ov4689->pad.flags = MEDIA_PAD_FL_SOURCE; > - sd->entity.function = MEDIA_ENT_F_CAM_SENSOR; > ret = media_entity_pads_init(&sd->entity, 1, &ov4689->pad); > if (ret < 0) > goto err_power_off; > > + sd->state_lock = ov4689->ctrl_handler.lock; > + ret = v4l2_subdev_init_finalize(sd); > + > + if (ret) { > + dev_err(dev, "Could not register v4l2 device\n"); > + goto err_clean_entity; > + } > + > ret = v4l2_async_register_subdev_sensor(sd); > if (ret) { > dev_err(dev, "v4l2 async register subdev failed\n"); > - goto err_clean_entity; > + goto err_clean_subdev; > } > > pm_runtime_set_active(dev); > @@ -899,14 +886,14 @@ static int ov4689_probe(struct i2c_client *client) > > return 0; > > +err_clean_subdev: > + v4l2_subdev_cleanup(sd); > err_clean_entity: > media_entity_cleanup(&sd->entity); > err_power_off: > ov4689_power_off(dev); > err_free_handler: > v4l2_ctrl_handler_free(&ov4689->ctrl_handler); > -err_destroy_mutex: > - mutex_destroy(&ov4689->mutex); > > return ret; > } > @@ -918,9 +905,8 @@ static void ov4689_remove(struct i2c_client *client) > > v4l2_async_unregister_subdev(sd); > media_entity_cleanup(&sd->entity); > - > + v4l2_subdev_cleanup(sd); > v4l2_ctrl_handler_free(&ov4689->ctrl_handler); > - mutex_destroy(&ov4689->mutex); > > pm_runtime_disable(&client->dev); > if (!pm_runtime_status_suspended(&client->dev)) -- Regards, Laurent Pinchart