From patchwork Tue Jul 17 01:24:44 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 1202721 Return-Path: X-Original-To: patchwork-linux-media@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 72E993FC33 for ; Tue, 17 Jul 2012 01:25:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753874Ab2GQBYm (ORCPT ); Mon, 16 Jul 2012 21:24:42 -0400 Received: from perceval.ideasonboard.com ([95.142.166.194]:51116 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753758Ab2GQBYl (ORCPT ); Mon, 16 Jul 2012 21:24:41 -0400 Received: from avalon.localnet (unknown [91.178.73.93]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 691597B0B; Tue, 17 Jul 2012 03:24:40 +0200 (CEST) From: Laurent Pinchart To: David Cohen Cc: Guennadi Liakhovetski , linux-media@vger.kernel.org Subject: Re: [PATCH v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions Date: Tue, 17 Jul 2012 03:24:44 +0200 Message-ID: <1785362.kzK4PIgmvB@avalon> User-Agent: KMail/4.8.3 (Linux/3.3.8-gentoo; KDE/4.8.3; x86_64; ; ) In-Reply-To: <50034F97.9060208@linux.intel.com> References: <1341520728-2707-1-git-send-email-laurent.pinchart@ideasonboard.com> <1341520728-2707-9-git-send-email-laurent.pinchart@ideasonboard.com> <50034F97.9060208@linux.intel.com> MIME-Version: 1.0 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Hi David, Thanks for the review. On Monday 16 July 2012 02:17:43 David Cohen wrote: > On 07/05/2012 11:38 PM, Laurent Pinchart wrote: > > Instead of forcing all soc-camera drivers to go through the mid-layer to > > handle power management, create soc_camera_power_[on|off]() functions > > that can be called from the subdev .s_power() operation to manage > > regulators and platform-specific power handling. This allows non > > soc-camera hosts to use soc-camera-aware clients. > > > > Signed-off-by: Laurent Pinchart > > --- > > > > drivers/media/video/imx074.c | 9 +++ > > drivers/media/video/mt9m001.c | 9 +++ > > drivers/media/video/mt9m111.c | 52 +++++++++++++----- > > drivers/media/video/mt9t031.c | 11 +++- > > drivers/media/video/mt9t112.c | 9 +++ > > drivers/media/video/mt9v022.c | 9 +++ > > drivers/media/video/ov2640.c | 9 +++ > > drivers/media/video/ov5642.c | 10 +++- > > drivers/media/video/ov6650.c | 9 +++ > > drivers/media/video/ov772x.c | 9 +++ > > drivers/media/video/ov9640.c | 10 +++- > > drivers/media/video/ov9740.c | 15 +++++- > > drivers/media/video/rj54n1cb0c.c | 9 +++ > > drivers/media/video/soc_camera.c | 83 ++++++++++++-------- > > drivers/media/video/soc_camera_platform.c | 11 ++++- > > drivers/media/video/tw9910.c | 9 +++ > > include/media/soc_camera.h | 10 ++++ > > 17 files changed, 225 insertions(+), 58 deletions(-) > > [snip] > > > diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c > > index 3eb07c2..effd0f1 100644 > > --- a/drivers/media/video/ov9740.c > > +++ b/drivers/media/video/ov9740.c > > @@ -786,16 +786,29 @@ static int ov9740_g_chip_ident(struct v4l2_subdev > > *sd,> > > static int ov9740_s_power(struct v4l2_subdev *sd, int on) > > { > > > > + struct i2c_client *client = v4l2_get_subdevdata(sd); > > + struct soc_camera_link *icl = soc_camera_i2c_to_link(client); > > > > struct ov9740_priv *priv = to_ov9740(sd); > > > > + int ret; > > > > - if (!priv->current_enable) > > + if (on) { > > + ret = soc_camera_power_on(&client->dev, icl); > > + if (ret < 0) > > + return ret; > > + } > > + > > + if (!priv->current_enable) { > > + if (!on) > > + soc_camera_power_off(&client->dev, icl); > > After your changes, this function has 3 if's (one nested) where all of > them checks "on" variable due to you need to mix "on" and > "priv->current_enable" checks. However, code's traceability is not so > trivial. > How about if you nest "priv->current_enable" into last "if" and keep > only that one? > > See an incomplete code below: > > return 0; > > > > + } > > > > if (on) { > > soc_camera_power_on(); > if (!priv->current_enable) > return; > > > ov9740_s_fmt(sd, &priv->current_mf); > > ov9740_s_stream(sd, priv->current_enable); > > > > } else { > > > > ov9740_s_stream(sd, 0); > > Execute ov9740_s_stream() conditionally: > if (priv->current_enable) { > ov9740_s_stream(); > priv->current_enable = true; > } > > > + soc_camera_power_off(&client->dev, icl); > > > > priv->current_enable = true; > > priv->current_enable is set to false when ov9740_s_stream(0) is called > then this function sets it back to true afterwards. So, in case you want > to have no functional change, it seems to me you should call > soc_camera_power_off() after that variable has its original value set > back. > In this case, even if you don't like my suggestion, you still need to > swap those 2 lines above. :) What do you think of diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c index 3eb07c2..10c0ba9 100644 --- a/drivers/media/video/ov9740.c +++ b/drivers/media/video/ov9740.c @@ -786,17 +786,27 @@ static int ov9740_g_chip_ident(struct v4l2_subdev *sd, static int ov9740_s_power(struct v4l2_subdev *sd, int on) { + struct i2c_client *client = v4l2_get_subdevdata(sd); + struct soc_camera_link *icl = soc_camera_i2c_to_link(client); struct ov9740_priv *priv = to_ov9740(sd); - - if (!priv->current_enable) - return 0; + int ret; if (on) { - ov9740_s_fmt(sd, &priv->current_mf); - ov9740_s_stream(sd, priv->current_enable); + ret = soc_camera_power_on(&client->dev, icl); + if (ret < 0) + return ret; + + if (priv->current_enable) { + ov9740_s_fmt(sd, &priv->current_mf); + ov9740_s_stream(sd, 1); + } } else { - ov9740_s_stream(sd, 0); - priv->current_enable = true; + if (priv->current_enable) { + ov9740_s_stream(sd, 0); + priv->current_enable = true; + } + + soc_camera_power_off(&client->dev, icl); } return 0;