Re: [PATCH 5/5 v2] [media] ov9740: Add suspend/resume

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

On Wed, 25 May 2011, achew@nvidia.com wrote:

> From: Andrew Chew <achew@nvidia.com>
> 
> On suspend, remember whether we are streaming or not, and at what frame format,
> so that on resume, we can start streaming again.
> 
> Signed-off-by: Andrew Chew <achew@nvidia.com>
> ---
>  drivers/media/video/ov9740.c |   39 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
> index 6c28ae8..4abe943 100644
> --- a/drivers/media/video/ov9740.c
> +++ b/drivers/media/video/ov9740.c
> @@ -201,6 +201,10 @@ struct ov9740_priv {
>  
>  	bool				flag_vflip;
>  	bool				flag_hflip;
> +
> +	/* For suspend/resume. */
> +	struct v4l2_mbus_framefmt	current_mf;
> +	int				current_enable;

bool?

>  };
>  
>  static const struct ov9740_reg ov9740_defaults[] = {
> @@ -551,6 +555,8 @@ static int ov9740_s_stream(struct v4l2_subdev *sd, int enable)
>  					       0x00);
>  	}
>  
> +	priv->current_enable = enable;
> +
>  	return ret;
>  }
>  
> @@ -786,6 +792,7 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
>  			struct v4l2_mbus_framefmt *mf)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct ov9740_priv *priv = to_ov9740(sd);
>  	enum v4l2_colorspace cspace;
>  	enum v4l2_mbus_pixelcode code = mf->code;
>  	int ret;
> @@ -812,6 +819,8 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
>  	mf->code	= code;
>  	mf->colorspace	= cspace;
>  
> +	memcpy(&priv->current_mf, mf, sizeof(struct v4l2_mbus_framefmt));
> +
>  	return ret;
>  }
>  
> @@ -922,7 +931,37 @@ err:
>  	return ret;
>  }
>  
> +static int ov9740_suspend(struct soc_camera_device *icd, pm_message_t state)
> +{
> +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> +	struct ov9740_priv *priv = to_ov9740(sd);
> +
> +	if (priv->current_enable) {
> +		int current_enable = priv->current_enable;
> +
> +		ov9740_s_stream(sd, 0);
> +		priv->current_enable = current_enable;

You don't need the local variable, just set

	priv->current_enable = true;

> +	}
> +
> +	return 0;
> +}
> +
> +static int ov9740_resume(struct soc_camera_device *icd)
> +{
> +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> +	struct ov9740_priv *priv = to_ov9740(sd);
> +
> +	if (priv->current_enable) {
> +		ov9740_s_fmt(sd, &priv->current_mf);
> +		ov9740_s_stream(sd, priv->current_enable);
> +	}
> +
> +	return 0;
> +}
> +
>  static struct soc_camera_ops ov9740_ops = {
> +	.suspend		= ov9740_suspend,
> +	.resume			= ov9740_resume,

No, we don't want to use these, whey should disappear some time... Please, 
use .s_power() from struct v4l2_subdev_core_ops, you can check 
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/33105 
for an example. If your host is not using these ops, it has to be fixed. 
So far in the mainline only one soc-camera host driver is using these 
callbacks: pxa_camera.c, which, looking at your email address, I doubt is 
the driver, that you're using;)

>  	.set_bus_param		= ov9740_set_bus_param,
>  	.query_bus_param	= ov9740_query_bus_param,
>  	.controls		= ov9740_controls,
> -- 
> 1.7.5.2
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/