From patchwork Fri Jan 19 07:29:46 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Yeh, Andy" X-Patchwork-Id: 10174685 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 3E2FB60386 for ; Fri, 19 Jan 2018 07:29:53 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2D073285A2 for ; Fri, 19 Jan 2018 07:29:53 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 208FC285C4; Fri, 19 Jan 2018 07:29:53 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 69A5B285A2 for ; Fri, 19 Jan 2018 07:29:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750931AbeASH3v (ORCPT ); Fri, 19 Jan 2018 02:29:51 -0500 Received: from mga14.intel.com ([192.55.52.115]:1793 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750785AbeASH3u (ORCPT ); Fri, 19 Jan 2018 02:29:50 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jan 2018 23:29:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,380,1511856000"; d="scan'208";a="196848151" Received: from pgsmsx105.gar.corp.intel.com ([10.221.44.96]) by fmsmga005.fm.intel.com with ESMTP; 18 Jan 2018 23:29:49 -0800 Received: from pgsmsx111.gar.corp.intel.com ([169.254.2.98]) by PGSMSX105.gar.corp.intel.com ([169.254.4.167]) with mapi id 14.03.0319.002; Fri, 19 Jan 2018 15:29:48 +0800 From: "Yeh, Andy" To: Tomasz Figa CC: Linux Media Mailing List , Sakari Ailus Subject: RE: [PATCH v4] media: imx258: Add imx258 camera sensor driver Thread-Topic: [PATCH v4] media: imx258: Add imx258 camera sensor driver Thread-Index: AQHTkNaX00d7OCIRD0SFCbFa8ZIEuaN6ESmAgACipLA= Date: Fri, 19 Jan 2018 07:29:46 +0000 Deferred-Delivery: Fri, 19 Jan 2018 07:29:30 +0000 Message-ID: <8E0971CCB6EA9D41AF58191A2D3978B61D4E49E8@PGSMSX111.gar.corp.intel.com> References: <1516333071-9766-1-git-send-email-andy.yeh@intel.com> In-Reply-To: Accept-Language: zh-TW, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiY2JjNjUyNTEtMjc2Zi00ODE0LTkxZGItNWE2NDM5YWRkN2YzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJOb0xyVkttdWVWTng4bE5iTHRiY1ZWR0ljcDRIelJNTnhuMldXclMwcVNuOWo3UnZ1U21QcGg5ZmFlXC9TWjU2KyJ9 x-ctpclassification: CTP_NT x-originating-ip: [172.30.20.205] MIME-Version: 1.0 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Thanks Tomasz, Agree with your point, if so, we could just change as below with a simple check of streaming flag. And for Sakari, do you agree with Tomasz's comment? Kindly review and I would send v5 with the change. - pm_runtime_put(&client->dev); - return ret; } Regards, Andy -----Original Message----- From: Tomasz Figa [mailto:tfiga@chromium.org] Sent: Friday, January 19, 2018 12:18 PM To: Yeh, Andy Cc: Linux Media Mailing List ; Sakari Ailus Subject: Re: [PATCH v4] media: imx258: Add imx258 camera sensor driver Hi Andy, Thanks for the patch. Please see my comments inline. On Fri, Jan 19, 2018 at 12:37 PM, Andy Yeh wrote: > Add a V4L2 sub-device driver for the Sony IMX258 image sensor. > This is a camera sensor using the I2C bus for control and the > CSI-2 bus for data. > > Signed-off-by: Andy Yeh > --- > - v2->v3 > -- Update the streaming function to remove SW_STANDBY in the beginning. > -- Adjust the delay time from 1ms to 12ms before set stream-on register. > - v3->v4 > -- fix the sd.entity to make code be compiled on the mainline kernel. > -- " - imx258->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;" > -- " + imx258->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;" > > MAINTAINERS | 7 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/imx258.c | 1148 > ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1167 insertions(+) > create mode 100644 drivers/media/i2c/imx258.c [snip] > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) { > + struct imx258 *imx258 = > + container_of(ctrl->handler, struct imx258, ctrl_handler); > + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > + s64 max; > + int ret = 0; > + > + /* Propagate change of current control to all related controls */ > + if (ctrl->id == V4L2_CID_VBLANK) { > + /* Update max exposure while meeting expected vblanking */ > + max = imx258->cur_mode->height + ctrl->val - 8; > + __v4l2_ctrl_modify_range(imx258->exposure, > + imx258->exposure->minimum, > + max, imx258->exposure->step, max); > + } > + > + /* > + * Applying V4L2 control value only happens > + * when power is up for streaming > + */ > + if (pm_runtime_get_if_in_use(&client->dev) <= 0) > + return 0; This won't work if runtime PM is not compiled in or is disabled at runtime, i.e. pm_runtime_get_if_in_use() returns -EINVAL. But actually, do we need to care about runtime PM here? Could we just return early if we're not streaming? Then the controls would be handled when streaming starts, since we call __v4l2_ctrl_handler_setup() from _start_streaming(). Best regards, Tomasz diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c index a7e58bd2..cf1c5ee 100644 --- a/drivers/media/i2c/imx258.c +++ b/drivers/media/i2c/imx258.c @@ -561,10 +561,13 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) /* * Applying V4L2 control value only happens - * when power is up for qstreaming + * when streaming flag is on */ - if (pm_runtime_get_if_in_use(&client->dev) <= 0) + if (imx258->streaming == 0) return 0; switch (ctrl->id) { case V4L2_CID_ANALOGUE_GAIN: @@ -590,8 +593,6 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) break; }