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 <laurent.pinchart@xxxxxxxxxxxxxxxx> > > --- > > > > 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; -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html