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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8A8ACC433FE for ; Wed, 26 Oct 2022 17:35:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233951AbiJZRfx (ORCPT ); Wed, 26 Oct 2022 13:35:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56626 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233859AbiJZRfv (ORCPT ); Wed, 26 Oct 2022 13:35:51 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C0067A2ABC for ; Wed, 26 Oct 2022 10:35:49 -0700 (PDT) Received: from ptx.hi.pengutronix.de ([2001:67c:670:100:1d::c0]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1onkJG-0005YF-1O; Wed, 26 Oct 2022 19:35:22 +0200 Received: from mfe by ptx.hi.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1onkJD-00060f-7f; Wed, 26 Oct 2022 19:35:19 +0200 Date: Wed, 26 Oct 2022 19:35:19 +0200 From: Marco Felsch To: "Lad, Prabhakar" Cc: Sakari Ailus , Laurent Pinchart , Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Geert Uytterhoeven , Magnus Damm , Hans Verkuil , Shawn Tu , devicetree@vger.kernel.org, Jacopo Mondi , linux-kernel@vger.kernel.org, Lad Prabhakar , linux-renesas-soc@vger.kernel.org, Biju Das , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Subject: Re: [PATCH v3 8/9] media: i2c: ov5645: Don't return early on failures for s_stream(0) Message-ID: <20221026173519.bm22im7uov6b4nnp@pengutronix.de> References: <20221026130658.45601-1-prabhakar.mahadev-lad.rj@bp.renesas.com> <20221026130658.45601-9-prabhakar.mahadev-lad.rj@bp.renesas.com> <20221026171721.4nfvhamguwnrw6zf@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20180716 X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: mfe@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 22-10-26, Lad, Prabhakar wrote: > Hi Marco, > > Thank you for the review. > > On Wed, Oct 26, 2022 at 6:17 PM Marco Felsch wrote: > > > > Hi Prabhakar, > > > > thanks for the patch, please see below my comments. > > > > On 22-10-26, Prabhakar wrote: > > > From: Lad Prabhakar > > > > > > Make sure we dont stop the code flow in case of errors while stopping > > > the stream and return the error code of the first error case if any. > > > > > > v4l2-core takes care of warning the user so no need to add a warning > > > message in the driver. > > > > > > Suggested-by: Sakari Ailus > > > Signed-off-by: Lad Prabhakar > > > --- > > > v2->v3 > > > * Now propagating the first error code in case of failure. > > > > > > v1->v2 > > > * New patch > > > --- > > > drivers/media/i2c/ov5645.c | 11 ++++++++--- > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > > index eea3067ddc8b..5702a55607fc 100644 > > > --- a/drivers/media/i2c/ov5645.c > > > +++ b/drivers/media/i2c/ov5645.c > > > @@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) > > > if (ret < 0) > > > goto err_rpm_put; > > > } else { > > > + int stream_off_ret = 0; > > > + > > > ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40); > > > > If this write failed.. > > > > > if (ret < 0) > > > - return ret; > > > + stream_off_ret = ret; > > > > > > ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0, > > > OV5645_SYSTEM_CTRL0_STOP); > > > > why should this write be successful? > > > Indeed that will fail unless I have a lucky day ;-) > > But it seemed to be an overkill for adding an additional check to see > if the previous write succeeded. Do you think this will create an > issue? Why not just say? ret = ov5645_write_reg(); if (ret < 0) goto out; ... out: dev_pm_xxx(); return ret; Regards, Marco 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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E6323C433FE for ; Wed, 26 Oct 2022 17:36:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=C11ofGyTfEAjWy8FA5eRZiyX+1M0h5ryHb1ZgK7pH8U=; b=lX7x6krCoHx1jk QwcaZOgQJRRp5tf/TndibulQUztQpjLlkECNwlvF29RezUXE9FvxhdduBBicrHdtyPg6EviP/Mhlv 6ip5q02y8VskVI2KZ8HhqGXTLM+rht9KtCIa0Z3861CxL9vvJbyvoQQqv3P6RilRDqtky7JgvFWZv jkzHqeAoPrfXVXU518HInEudhl8pSbbgVxEysuOhhOjSGFNQ49NSVFUV7gGEwX3bRt0NPe95mRZT9 6qPqmRmVIqyKuEjZOwjjJ82mpzKkPAs7XP9wJYkcEapBvrY2EAr7FVmullAnPwDwdxchNfUh2vP+x sO0hO8Wz0GW47MqMR9jA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1onkJh-00AOhi-98; Wed, 26 Oct 2022 17:35:49 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1onkJe-00AOh0-Th for linux-arm-kernel@lists.infradead.org; Wed, 26 Oct 2022 17:35:48 +0000 Received: from ptx.hi.pengutronix.de ([2001:67c:670:100:1d::c0]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1onkJG-0005YF-1O; Wed, 26 Oct 2022 19:35:22 +0200 Received: from mfe by ptx.hi.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1onkJD-00060f-7f; Wed, 26 Oct 2022 19:35:19 +0200 Date: Wed, 26 Oct 2022 19:35:19 +0200 From: Marco Felsch To: "Lad, Prabhakar" Cc: Sakari Ailus , Laurent Pinchart , Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Geert Uytterhoeven , Magnus Damm , Hans Verkuil , Shawn Tu , devicetree@vger.kernel.org, Jacopo Mondi , linux-kernel@vger.kernel.org, Lad Prabhakar , linux-renesas-soc@vger.kernel.org, Biju Das , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Subject: Re: [PATCH v3 8/9] media: i2c: ov5645: Don't return early on failures for s_stream(0) Message-ID: <20221026173519.bm22im7uov6b4nnp@pengutronix.de> References: <20221026130658.45601-1-prabhakar.mahadev-lad.rj@bp.renesas.com> <20221026130658.45601-9-prabhakar.mahadev-lad.rj@bp.renesas.com> <20221026171721.4nfvhamguwnrw6zf@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20180716 X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: mfe@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221026_103546_966554_0996C9B3 X-CRM114-Status: GOOD ( 34.32 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 22-10-26, Lad, Prabhakar wrote: > Hi Marco, > > Thank you for the review. > > On Wed, Oct 26, 2022 at 6:17 PM Marco Felsch wrote: > > > > Hi Prabhakar, > > > > thanks for the patch, please see below my comments. > > > > On 22-10-26, Prabhakar wrote: > > > From: Lad Prabhakar > > > > > > Make sure we dont stop the code flow in case of errors while stopping > > > the stream and return the error code of the first error case if any. > > > > > > v4l2-core takes care of warning the user so no need to add a warning > > > message in the driver. > > > > > > Suggested-by: Sakari Ailus > > > Signed-off-by: Lad Prabhakar > > > --- > > > v2->v3 > > > * Now propagating the first error code in case of failure. > > > > > > v1->v2 > > > * New patch > > > --- > > > drivers/media/i2c/ov5645.c | 11 ++++++++--- > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > > index eea3067ddc8b..5702a55607fc 100644 > > > --- a/drivers/media/i2c/ov5645.c > > > +++ b/drivers/media/i2c/ov5645.c > > > @@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) > > > if (ret < 0) > > > goto err_rpm_put; > > > } else { > > > + int stream_off_ret = 0; > > > + > > > ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40); > > > > If this write failed.. > > > > > if (ret < 0) > > > - return ret; > > > + stream_off_ret = ret; > > > > > > ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0, > > > OV5645_SYSTEM_CTRL0_STOP); > > > > why should this write be successful? > > > Indeed that will fail unless I have a lucky day ;-) > > But it seemed to be an overkill for adding an additional check to see > if the previous write succeeded. Do you think this will create an > issue? Why not just say? ret = ov5645_write_reg(); if (ret < 0) goto out; ... out: dev_pm_xxx(); return ret; Regards, Marco _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel