diff mbox

[2/3] mt9v022: fix the V4L2_CID_EXPOSURE control

Message ID 1345799431-29426-3-git-send-email-agust@denx.de (mailing list archive)
State Superseded, archived
Headers

Commit Message

Anatolij Gustschin Aug. 24, 2012, 9:10 a.m. UTC 
  Since the MT9V022_TOTAL_SHUTTER_WIDTH register is controlled in manual
mode by V4L2_CID_EXPOSURE control, it shouldn't be written directly in
mt9v022_s_crop(). In manual mode this register should be set to the
V4L2_CID_EXPOSURE control value. Changing this register directly and
outside of the actual control function means that the register value
is not in sync with the corresponding control value. Thus, the following
problem is observed:

    - setting this control initially succeeds
    - VIDIOC_S_CROP ioctl() overwrites the MT9V022_TOTAL_SHUTTER_WIDTH
      register
    - setting this control to the same value again doesn't
      result in setting the register since the control value
      was previously cached and doesn't differ

Fix it by always setting the register to the controlled value, when
in manual mode.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
 drivers/media/i2c/soc_camera/mt9v022.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Comments

Guennadi Liakhovetski Aug. 24, 2012, 11:22 a.m. UTC | #1 
On Fri, 24 Aug 2012, Anatolij Gustschin wrote:

> Since the MT9V022_TOTAL_SHUTTER_WIDTH register is controlled in manual
> mode by V4L2_CID_EXPOSURE control, it shouldn't be written directly in
> mt9v022_s_crop(). In manual mode this register should be set to the
> V4L2_CID_EXPOSURE control value. Changing this register directly and
> outside of the actual control function means that the register value
> is not in sync with the corresponding control value. Thus, the following
> problem is observed:
> 
>     - setting this control initially succeeds
>     - VIDIOC_S_CROP ioctl() overwrites the MT9V022_TOTAL_SHUTTER_WIDTH
>       register
>     - setting this control to the same value again doesn't
>       result in setting the register since the control value
>       was previously cached and doesn't differ
> 
> Fix it by always setting the register to the controlled value, when
> in manual mode.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
>  drivers/media/i2c/soc_camera/mt9v022.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
> index d13c8c4..d26c071 100644
> --- a/drivers/media/i2c/soc_camera/mt9v022.c
> +++ b/drivers/media/i2c/soc_camera/mt9v022.c
> @@ -274,9 +274,9 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
>  		if (ret & 1) /* Autoexposure */
>  			ret = reg_write(client, mt9v022->reg->max_total_shutter_width,
>  					rect.height + mt9v022->y_skip_top + 43);
> -		else
> -			ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
> -					rect.height + mt9v022->y_skip_top + 43);
> +		else /* Set to the manually controlled value */
> +			ret = v4l2_ctrl_s_ctrl(mt9v022->exposure,
> +					       mt9v022->exposure->val);

But why do we have to write it here at all then? Autoexposure can be off 
only if the user has set exposure manually, using V4L2_CID_EXPOSURE_AUTO. 
In this case MT9V022_TOTAL_SHUTTER_WIDTH already contains the correct 
value. Why do we have to set it again? Maybe just adding a comment, 
explaining the above, would suffice?

>  	}
>  	/* Setup frame format: defaults apart from width and height */
>  	if (!ret)
> -- 
> 1.7.1

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
Anatolij Gustschin Aug. 24, 2012, 2:17 p.m. UTC | #2 
Hi Guennadi,

On Fri, 24 Aug 2012 13:22:18 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > --- a/drivers/media/i2c/soc_camera/mt9v022.c
> > +++ b/drivers/media/i2c/soc_camera/mt9v022.c
> > @@ -274,9 +274,9 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> >  		if (ret & 1) /* Autoexposure */
> >  			ret = reg_write(client, mt9v022->reg->max_total_shutter_width,
> >  					rect.height + mt9v022->y_skip_top + 43);
> > -		else
> > -			ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
> > -					rect.height + mt9v022->y_skip_top + 43);
> > +		else /* Set to the manually controlled value */
> > +			ret = v4l2_ctrl_s_ctrl(mt9v022->exposure,
> > +					       mt9v022->exposure->val);
> 
> But why do we have to write it here at all then? Autoexposure can be off 
> only if the user has set exposure manually, using V4L2_CID_EXPOSURE_AUTO. 
> In this case MT9V022_TOTAL_SHUTTER_WIDTH already contains the correct 
> value. Why do we have to set it again? Maybe just adding a comment, 
> explaining the above, would suffice?

Actually we do not have to write it here, yes. Should I remove the shutter
register setting here entirely? And add a comment explaining, why?

Thanks,

Anatolij
--
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
Guennadi Liakhovetski Aug. 24, 2012, 2:32 p.m. UTC | #3 
On Fri, 24 Aug 2012, Anatolij Gustschin wrote:

> Hi Guennadi,
> 
> On Fri, 24 Aug 2012 13:22:18 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> ...
> > > --- a/drivers/media/i2c/soc_camera/mt9v022.c
> > > +++ b/drivers/media/i2c/soc_camera/mt9v022.c
> > > @@ -274,9 +274,9 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> > >  		if (ret & 1) /* Autoexposure */
> > >  			ret = reg_write(client, mt9v022->reg->max_total_shutter_width,
> > >  					rect.height + mt9v022->y_skip_top + 43);
> > > -		else
> > > -			ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
> > > -					rect.height + mt9v022->y_skip_top + 43);
> > > +		else /* Set to the manually controlled value */
> > > +			ret = v4l2_ctrl_s_ctrl(mt9v022->exposure,
> > > +					       mt9v022->exposure->val);
> > 
> > But why do we have to write it here at all then? Autoexposure can be off 
> > only if the user has set exposure manually, using V4L2_CID_EXPOSURE_AUTO. 
> > In this case MT9V022_TOTAL_SHUTTER_WIDTH already contains the correct 
> > value. Why do we have to set it again? Maybe just adding a comment, 
> > explaining the above, would suffice?
> 
> Actually we do not have to write it here, yes. Should I remove the shutter
> register setting here entirely? And add a comment explaining, why?

Remove it from the "else" clause, yes, please. And a comment would be 
good!

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
Anatolij Gustschin Sept. 21, 2012, 9:30 a.m. UTC | #4 
On Fri, 24 Aug 2012 16:32:57 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > > But why do we have to write it here at all then? Autoexposure can be off 
> > > only if the user has set exposure manually, using V4L2_CID_EXPOSURE_AUTO. 
> > > In this case MT9V022_TOTAL_SHUTTER_WIDTH already contains the correct 
> > > value. Why do we have to set it again? Maybe just adding a comment, 
> > > explaining the above, would suffice?
> > 
> > Actually we do not have to write it here, yes. Should I remove the shutter
> > register setting here entirely? And add a comment explaining, why?
> 
> Remove it from the "else" clause, yes, please. And a comment would be 
> good!

Ok, I'll resubmit a reworked patch.

Thanks,
Anatolij
--
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 mbox

Patch

diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
index d13c8c4..d26c071 100644
--- a/drivers/media/i2c/soc_camera/mt9v022.c
+++ b/drivers/media/i2c/soc_camera/mt9v022.c
@@ -274,9 +274,9 @@  static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 		if (ret & 1) /* Autoexposure */
 			ret = reg_write(client, mt9v022->reg->max_total_shutter_width,
 					rect.height + mt9v022->y_skip_top + 43);
-		else
-			ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
-					rect.height + mt9v022->y_skip_top + 43);
+		else /* Set to the manually controlled value */
+			ret = v4l2_ctrl_s_ctrl(mt9v022->exposure,
+					       mt9v022->exposure->val);
 	}
 	/* Setup frame format: defaults apart from width and height */
 	if (!ret)