| Message ID | 1785362.kzK4PIgmvB@avalon (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Laurent, On 07/17/2012 04:24 AM, Laurent Pinchart wrote: > Hi David, > > Thanks for the review. You're welcome. > > 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@ideasonboard.com> >>> --- >>> >>> 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 Sounds good to me :) Br, David Cohen > > 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; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;