diff mbox

mt9v022: support required register settings in snapshot mode

Message ID 1348786362-28586-1-git-send-email-agust@denx.de (mailing list archive)
State New, archived
Headers show

Commit Message

Anatolij Gustschin Sept. 27, 2012, 10:52 p.m. UTC
Some camera systems cannot operate mt9v022 in normal mode and use
only the snapshot mode. The TechNote for mt9v022 (TN0960) and mt9v024
(TN-09-225) describes required register settings when configuring the
snapshot operation. The snapshot mode requires that certain automatic
functions of the image sensor should be disabled or set to fixed values.

According to the TechNote bit 2 and bit 9 in the register 0x20 must be
set in snapshot mode and unset for normal operation. This applies for
mt9v022 Rev.3 and mt9v024. Add required reg. 0x20 settings dependent on
sensor chip version.

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

Comments

Guennadi Liakhovetski Sept. 28, 2012, 12:33 p.m. UTC | #1
Hi Anatolij

On Fri, 28 Sep 2012, Anatolij Gustschin wrote:

> Some camera systems cannot operate mt9v022 in normal mode and use
> only the snapshot mode. The TechNote for mt9v022 (TN0960) and mt9v024
> (TN-09-225) describes required register settings when configuring the
> snapshot operation. The snapshot mode requires that certain automatic
> functions of the image sensor should be disabled or set to fixed values.
> 
> According to the TechNote bit 2 and bit 9 in the register 0x20 must be
> set in snapshot mode and unset for normal operation. This applies for
> mt9v022 Rev.3 and mt9v024. Add required reg. 0x20 settings dependent on
> sensor chip version.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>

I'm afraid, I don't understand again

> ---
>  drivers/media/i2c/soc_camera/mt9v022.c |   31 ++++++++++++++++++++++++++++---
>  1 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
> index 8feaddc..2abe999 100644
> --- a/drivers/media/i2c/soc_camera/mt9v022.c
> +++ b/drivers/media/i2c/soc_camera/mt9v022.c
> @@ -51,6 +51,7 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"");
>  #define MT9V022_PIXEL_OPERATION_MODE	0x0f
>  #define MT9V022_LED_OUT_CONTROL		0x1b
>  #define MT9V022_ADC_MODE_CONTROL	0x1c
> +#define MT9V022_REG32			0x20
>  #define MT9V022_ANALOG_GAIN		0x35
>  #define MT9V022_BLACK_LEVEL_CALIB_CTRL	0x47
>  #define MT9V022_PIXCLK_FV_LV		0x74
> @@ -79,7 +80,8 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"");
>  #define MT9V022_VERTICAL_BLANKING_MAX	3000
>  #define MT9V022_VERTICAL_BLANKING_DEF	45
>  
> -#define is_mt9v024(id) (id == 0x1324)
> +#define is_mt9v022_rev3(id)	(id == 0x1313)
> +#define is_mt9v024(id)		(id == 0x1324)
>  
>  /* MT9V022 has only one fixed colorspace per pixelcode */
>  struct mt9v022_datafmt {
> @@ -153,6 +155,7 @@ struct mt9v022 {
>  	int num_fmts;
>  	int model;	/* V4L2_IDENT_MT9V022* codes from v4l2-chip-ident.h */
>  	u16 chip_control;
> +	u16 chip_version;
>  	unsigned short y_skip_top;	/* Lines to skip at the top */
>  };
>  
> @@ -235,12 +238,32 @@ static int mt9v022_s_stream(struct v4l2_subdev *sd, int enable)
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct mt9v022 *mt9v022 = to_mt9v022(client);
>  
> -	if (enable)
> +	if (enable) {
>  		/* Switch to master "normal" mode */
>  		mt9v022->chip_control &= ~0x10;
> -	else
> +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> +		    is_mt9v024(mt9v022->chip_version)) {
> +			/*
> +			 * Unset snapshot mode specific settings: clear bit 9
> +			 * and bit 2 in reg. 0x20 when in normal mode.
> +			 */
> +			if (reg_clear(client, MT9V022_REG32, 0x204))
> +				return -EIO;
> +		}
> +	} else {
>  		/* Switch to snapshot mode */
>  		mt9v022->chip_control |= 0x10;
> +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> +		    is_mt9v024(mt9v022->chip_version)) {
> +			/*
> +			 * Required settings for snapshot mode: set bit 9
> +			 * (RST enable) and bit 2 (CR enable) in reg. 0x20
> +			 * See TechNote TN0960 or TN-09-225.
> +			 */
> +			if (reg_set(client, MT9V022_REG32, 0x204))
> +				return -EIO;
> +		}
> +	}

Do I understand it right, that now on mt9v022 rev.3 and mt9v024 you 
unconditionally added using REG32 for leaving the snapshot mode on 
streamon and entering it on streamoff. This should be ok in principle, 
since that's also what we're trying to do, using the CHIP_CONTROL 
register. But in your comment you say, that on some _systems_ you can only 
_operate_ in snapshot mode. I.e. the snapshot mode enabled during running 
streaming, right? Then how does this patch help you with that?

Thanks
Guennadi

>  
>  	if (reg_write(client, MT9V022_CHIP_CONTROL, mt9v022->chip_control) < 0)
>  		return -EIO;
> @@ -652,6 +675,8 @@ static int mt9v022_video_probe(struct i2c_client *client)
>  		goto ei2c;
>  	}
>  
> +	mt9v022->chip_version = data;
> +
>  	mt9v022->reg = is_mt9v024(data) ? &mt9v024_register :
>  			&mt9v022_register;
>  
> -- 
> 1.7.1
> 

---
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. 28, 2012, 1:10 p.m. UTC | #2
Hi Guennadi,

On Fri, 28 Sep 2012 14:33:34 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > @@ -235,12 +238,32 @@ static int mt9v022_s_stream(struct v4l2_subdev *sd, int enable)
> >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >  	struct mt9v022 *mt9v022 = to_mt9v022(client);
> >  
> > -	if (enable)
> > +	if (enable) {
> >  		/* Switch to master "normal" mode */
> >  		mt9v022->chip_control &= ~0x10;
> > -	else
> > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > +		    is_mt9v024(mt9v022->chip_version)) {
> > +			/*
> > +			 * Unset snapshot mode specific settings: clear bit 9
> > +			 * and bit 2 in reg. 0x20 when in normal mode.
> > +			 */
> > +			if (reg_clear(client, MT9V022_REG32, 0x204))
> > +				return -EIO;
> > +		}
> > +	} else {
> >  		/* Switch to snapshot mode */
> >  		mt9v022->chip_control |= 0x10;
> > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > +		    is_mt9v024(mt9v022->chip_version)) {
> > +			/*
> > +			 * Required settings for snapshot mode: set bit 9
> > +			 * (RST enable) and bit 2 (CR enable) in reg. 0x20
> > +			 * See TechNote TN0960 or TN-09-225.
> > +			 */
> > +			if (reg_set(client, MT9V022_REG32, 0x204))
> > +				return -EIO;
> > +		}
> > +	}
> 
> Do I understand it right, that now on mt9v022 rev.3 and mt9v024 you 
> unconditionally added using REG32 for leaving the snapshot mode on 
> streamon and entering it on streamoff. This should be ok in principle, 
> since that's also what we're trying to do, using the CHIP_CONTROL 
> register. But in your comment you say, that on some _systems_ you can only 
> _operate_ in snapshot mode. I.e. the snapshot mode enabled during running 
> streaming, right? Then how does this patch help you with that?

Yes. But i.e. the driver calling the sub-device stream control function
on streamon knows that the normal mode is not supported and therefore it
calls this function with argument enable == 0, effectively setting the
snapshot mode.

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 Sept. 28, 2012, 1:30 p.m. UTC | #3
On Fri, 28 Sep 2012, Anatolij Gustschin wrote:

> Hi Guennadi,
> 
> On Fri, 28 Sep 2012 14:33:34 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> ...
> > > @@ -235,12 +238,32 @@ static int mt9v022_s_stream(struct v4l2_subdev *sd, int enable)
> > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > >  	struct mt9v022 *mt9v022 = to_mt9v022(client);
> > >  
> > > -	if (enable)
> > > +	if (enable) {
> > >  		/* Switch to master "normal" mode */
> > >  		mt9v022->chip_control &= ~0x10;
> > > -	else
> > > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > > +		    is_mt9v024(mt9v022->chip_version)) {
> > > +			/*
> > > +			 * Unset snapshot mode specific settings: clear bit 9
> > > +			 * and bit 2 in reg. 0x20 when in normal mode.
> > > +			 */
> > > +			if (reg_clear(client, MT9V022_REG32, 0x204))
> > > +				return -EIO;
> > > +		}
> > > +	} else {
> > >  		/* Switch to snapshot mode */
> > >  		mt9v022->chip_control |= 0x10;
> > > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > > +		    is_mt9v024(mt9v022->chip_version)) {
> > > +			/*
> > > +			 * Required settings for snapshot mode: set bit 9
> > > +			 * (RST enable) and bit 2 (CR enable) in reg. 0x20
> > > +			 * See TechNote TN0960 or TN-09-225.
> > > +			 */
> > > +			if (reg_set(client, MT9V022_REG32, 0x204))
> > > +				return -EIO;
> > > +		}
> > > +	}
> > 
> > Do I understand it right, that now on mt9v022 rev.3 and mt9v024 you 
> > unconditionally added using REG32 for leaving the snapshot mode on 
> > streamon and entering it on streamoff. This should be ok in principle, 
> > since that's also what we're trying to do, using the CHIP_CONTROL 
> > register. But in your comment you say, that on some _systems_ you can only 
> > _operate_ in snapshot mode. I.e. the snapshot mode enabled during running 
> > streaming, right? Then how does this patch help you with that?
> 
> Yes. But i.e. the driver calling the sub-device stream control function
> on streamon knows that the normal mode is not supported and therefore it
> calls this function with argument enable == 0, effectively setting the
> snapshot mode.

Right, I thought you could be doing that... Well, on the one hand I should 
be happy, that the problem is solved without driver modifications, OTOH 
this isn't pretty... In fact this shouldn't work at all. After a 
stream-off the buffer queue should be stopped too. However, to properly 
implement this you'd have to add a new V4L2 control to switch into 
snapshot mode.

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. 28, 2012, 1:42 p.m. UTC | #4
On Fri, 28 Sep 2012 15:30:33 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > Yes. But i.e. the driver calling the sub-device stream control function
> > on streamon knows that the normal mode is not supported and therefore it
> > calls this function with argument enable == 0, effectively setting the
> > snapshot mode.
> 
> Right, I thought you could be doing that... Well, on the one hand I should 
> be happy, that the problem is solved without driver modifications, OTOH 
> this isn't pretty... In fact this shouldn't work at all. After a 
> stream-off the buffer queue should be stopped too.

Yes, the capture driver stops its buffer queue on stream-off.

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
Anatolij Gustschin Oct. 6, 2012, 10:57 a.m. UTC | #5
Hi Guennadi,

On Fri, 28 Sep 2012 15:30:33 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> On Fri, 28 Sep 2012, Anatolij Gustschin wrote:
> 
> > Hi Guennadi,
> > 
> > On Fri, 28 Sep 2012 14:33:34 +0200 (CEST)
> > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > ...
> > > > @@ -235,12 +238,32 @@ static int mt9v022_s_stream(struct v4l2_subdev *sd, int enable)
> > > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > >  	struct mt9v022 *mt9v022 = to_mt9v022(client);
> > > >  
> > > > -	if (enable)
> > > > +	if (enable) {
> > > >  		/* Switch to master "normal" mode */
> > > >  		mt9v022->chip_control &= ~0x10;
> > > > -	else
> > > > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > > > +		    is_mt9v024(mt9v022->chip_version)) {
> > > > +			/*
> > > > +			 * Unset snapshot mode specific settings: clear bit 9
> > > > +			 * and bit 2 in reg. 0x20 when in normal mode.
> > > > +			 */
> > > > +			if (reg_clear(client, MT9V022_REG32, 0x204))
> > > > +				return -EIO;
> > > > +		}
> > > > +	} else {
> > > >  		/* Switch to snapshot mode */
> > > >  		mt9v022->chip_control |= 0x10;
> > > > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > > > +		    is_mt9v024(mt9v022->chip_version)) {
> > > > +			/*
> > > > +			 * Required settings for snapshot mode: set bit 9
> > > > +			 * (RST enable) and bit 2 (CR enable) in reg. 0x20
> > > > +			 * See TechNote TN0960 or TN-09-225.
> > > > +			 */
> > > > +			if (reg_set(client, MT9V022_REG32, 0x204))
> > > > +				return -EIO;
> > > > +		}
> > > > +	}
> > > 
> > > Do I understand it right, that now on mt9v022 rev.3 and mt9v024 you 
> > > unconditionally added using REG32 for leaving the snapshot mode on 
> > > streamon and entering it on streamoff. This should be ok in principle, 
> > > since that's also what we're trying to do, using the CHIP_CONTROL 
> > > register. But in your comment you say, that on some _systems_ you can only 
> > > _operate_ in snapshot mode. I.e. the snapshot mode enabled during running 
> > > streaming, right? Then how does this patch help you with that?
> > 
> > Yes. But i.e. the driver calling the sub-device stream control function
> > on streamon knows that the normal mode is not supported and therefore it
> > calls this function with argument enable == 0, effectively setting the
> > snapshot mode.
> 
> Right, I thought you could be doing that... Well, on the one hand I should 
> be happy, that the problem is solved without driver modifications, OTOH 
> this isn't pretty... In fact this shouldn't work at all. After a 
> stream-off the buffer queue should be stopped too.

Why shouldn't it work? The buffer queue is handled by the host driver,
not by the sensor driver. And in my case the host driver stops the buffer
queue in its streamoff, as it should. It works without issues and doesn't
cause any problems for other mt9v022 users.

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 Oct. 8, 2012, 9:22 a.m. UTC | #6
On Sat, 6 Oct 2012, Anatolij Gustschin wrote:

> Hi Guennadi,
> 
> On Fri, 28 Sep 2012 15:30:33 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > On Fri, 28 Sep 2012, Anatolij Gustschin wrote:
> > 
> > > Hi Guennadi,
> > > 
> > > On Fri, 28 Sep 2012 14:33:34 +0200 (CEST)
> > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > ...
> > > > > @@ -235,12 +238,32 @@ static int mt9v022_s_stream(struct v4l2_subdev *sd, int enable)
> > > > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > > >  	struct mt9v022 *mt9v022 = to_mt9v022(client);
> > > > >  
> > > > > -	if (enable)
> > > > > +	if (enable) {
> > > > >  		/* Switch to master "normal" mode */
> > > > >  		mt9v022->chip_control &= ~0x10;
> > > > > -	else
> > > > > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > > > > +		    is_mt9v024(mt9v022->chip_version)) {
> > > > > +			/*
> > > > > +			 * Unset snapshot mode specific settings: clear bit 9
> > > > > +			 * and bit 2 in reg. 0x20 when in normal mode.
> > > > > +			 */
> > > > > +			if (reg_clear(client, MT9V022_REG32, 0x204))
> > > > > +				return -EIO;
> > > > > +		}
> > > > > +	} else {
> > > > >  		/* Switch to snapshot mode */
> > > > >  		mt9v022->chip_control |= 0x10;
> > > > > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > > > > +		    is_mt9v024(mt9v022->chip_version)) {
> > > > > +			/*
> > > > > +			 * Required settings for snapshot mode: set bit 9
> > > > > +			 * (RST enable) and bit 2 (CR enable) in reg. 0x20
> > > > > +			 * See TechNote TN0960 or TN-09-225.
> > > > > +			 */
> > > > > +			if (reg_set(client, MT9V022_REG32, 0x204))
> > > > > +				return -EIO;
> > > > > +		}
> > > > > +	}
> > > > 
> > > > Do I understand it right, that now on mt9v022 rev.3 and mt9v024 you 
> > > > unconditionally added using REG32 for leaving the snapshot mode on 
> > > > streamon and entering it on streamoff. This should be ok in principle, 
> > > > since that's also what we're trying to do, using the CHIP_CONTROL 
> > > > register. But in your comment you say, that on some _systems_ you can only 
> > > > _operate_ in snapshot mode. I.e. the snapshot mode enabled during running 
> > > > streaming, right? Then how does this patch help you with that?
> > > 
> > > Yes. But i.e. the driver calling the sub-device stream control function
> > > on streamon knows that the normal mode is not supported and therefore it
> > > calls this function with argument enable == 0, effectively setting the
> > > snapshot mode.
> > 
> > Right, I thought you could be doing that... Well, on the one hand I should 
> > be happy, that the problem is solved without driver modifications, OTOH 
> > this isn't pretty... In fact this shouldn't work at all. After a 
> > stream-off the buffer queue should be stopped too.
> 
> Why shouldn't it work? The buffer queue is handled by the host driver,
> not by the sensor driver. And in my case the host driver stops the buffer
> queue in its streamoff, as it should. It works without issues and doesn't
> cause any problems for other mt9v022 users.

Because one shouldn't abuse the API by activating streaming on the bridge 
driver and deactivating it on the sensor.

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 Oct. 8, 2012, 9:47 a.m. UTC | #7
On Mon, 8 Oct 2012 11:22:05 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> On Sat, 6 Oct 2012, Anatolij Gustschin wrote:
> 
> > Hi Guennadi,
> > 
> > On Fri, 28 Sep 2012 15:30:33 +0200 (CEST)
> > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > 
> > > On Fri, 28 Sep 2012, Anatolij Gustschin wrote:
> > > 
> > > > Hi Guennadi,
> > > > 
> > > > On Fri, 28 Sep 2012 14:33:34 +0200 (CEST)
> > > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > > ...
> > > > > > @@ -235,12 +238,32 @@ static int mt9v022_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > > > >  	struct mt9v022 *mt9v022 = to_mt9v022(client);
> > > > > >  
> > > > > > -	if (enable)
> > > > > > +	if (enable) {
> > > > > >  		/* Switch to master "normal" mode */
> > > > > >  		mt9v022->chip_control &= ~0x10;
> > > > > > -	else
> > > > > > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > > > > > +		    is_mt9v024(mt9v022->chip_version)) {
> > > > > > +			/*
> > > > > > +			 * Unset snapshot mode specific settings: clear bit 9
> > > > > > +			 * and bit 2 in reg. 0x20 when in normal mode.
> > > > > > +			 */
> > > > > > +			if (reg_clear(client, MT9V022_REG32, 0x204))
> > > > > > +				return -EIO;
> > > > > > +		}
> > > > > > +	} else {
> > > > > >  		/* Switch to snapshot mode */
> > > > > >  		mt9v022->chip_control |= 0x10;
> > > > > > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > > > > > +		    is_mt9v024(mt9v022->chip_version)) {
> > > > > > +			/*
> > > > > > +			 * Required settings for snapshot mode: set bit 9
> > > > > > +			 * (RST enable) and bit 2 (CR enable) in reg. 0x20
> > > > > > +			 * See TechNote TN0960 or TN-09-225.
> > > > > > +			 */
> > > > > > +			if (reg_set(client, MT9V022_REG32, 0x204))
> > > > > > +				return -EIO;
> > > > > > +		}
> > > > > > +	}
> > > > > 
> > > > > Do I understand it right, that now on mt9v022 rev.3 and mt9v024 you 
> > > > > unconditionally added using REG32 for leaving the snapshot mode on 
> > > > > streamon and entering it on streamoff. This should be ok in principle, 
> > > > > since that's also what we're trying to do, using the CHIP_CONTROL 
> > > > > register. But in your comment you say, that on some _systems_ you can only 
> > > > > _operate_ in snapshot mode. I.e. the snapshot mode enabled during running 
> > > > > streaming, right? Then how does this patch help you with that?
> > > > 
> > > > Yes. But i.e. the driver calling the sub-device stream control function
> > > > on streamon knows that the normal mode is not supported and therefore it
> > > > calls this function with argument enable == 0, effectively setting the
> > > > snapshot mode.
> > > 
> > > Right, I thought you could be doing that... Well, on the one hand I should 
> > > be happy, that the problem is solved without driver modifications, OTOH 
> > > this isn't pretty... In fact this shouldn't work at all. After a 
> > > stream-off the buffer queue should be stopped too.
> > 
> > Why shouldn't it work? The buffer queue is handled by the host driver,
> > not by the sensor driver. And in my case the host driver stops the buffer
> > queue in its streamoff, as it should. It works without issues and doesn't
> > cause any problems for other mt9v022 users.
> 
> Because one shouldn't abuse the API by activating streaming on the bridge 
> driver and deactivating it on the sensor.

I don't see how is it an abuse of the API at all. The sensor is just
configured to snapshot mode but is still able to stream if it is
triggered continuously by external events. It is actually a streaming
case.

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 Oct. 8, 2012, 9:55 a.m. UTC | #8
On Mon, 8 Oct 2012, Anatolij Gustschin wrote:

> On Mon, 8 Oct 2012 11:22:05 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > On Sat, 6 Oct 2012, Anatolij Gustschin wrote:
> > 
> > > Hi Guennadi,
> > > 
> > > On Fri, 28 Sep 2012 15:30:33 +0200 (CEST)
> > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > 
> > > > On Fri, 28 Sep 2012, Anatolij Gustschin wrote:
> > > > 
> > > > > Hi Guennadi,
> > > > > 
> > > > > On Fri, 28 Sep 2012 14:33:34 +0200 (CEST)
> > > > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > > > ...
> > > > > > > @@ -235,12 +238,32 @@ static int mt9v022_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > > > > >  	struct mt9v022 *mt9v022 = to_mt9v022(client);
> > > > > > >  
> > > > > > > -	if (enable)
> > > > > > > +	if (enable) {
> > > > > > >  		/* Switch to master "normal" mode */
> > > > > > >  		mt9v022->chip_control &= ~0x10;
> > > > > > > -	else
> > > > > > > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > > > > > > +		    is_mt9v024(mt9v022->chip_version)) {
> > > > > > > +			/*
> > > > > > > +			 * Unset snapshot mode specific settings: clear bit 9
> > > > > > > +			 * and bit 2 in reg. 0x20 when in normal mode.
> > > > > > > +			 */
> > > > > > > +			if (reg_clear(client, MT9V022_REG32, 0x204))
> > > > > > > +				return -EIO;
> > > > > > > +		}
> > > > > > > +	} else {
> > > > > > >  		/* Switch to snapshot mode */
> > > > > > >  		mt9v022->chip_control |= 0x10;
> > > > > > > +		if (is_mt9v022_rev3(mt9v022->chip_version) ||
> > > > > > > +		    is_mt9v024(mt9v022->chip_version)) {
> > > > > > > +			/*
> > > > > > > +			 * Required settings for snapshot mode: set bit 9
> > > > > > > +			 * (RST enable) and bit 2 (CR enable) in reg. 0x20
> > > > > > > +			 * See TechNote TN0960 or TN-09-225.
> > > > > > > +			 */
> > > > > > > +			if (reg_set(client, MT9V022_REG32, 0x204))
> > > > > > > +				return -EIO;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > 
> > > > > > Do I understand it right, that now on mt9v022 rev.3 and mt9v024 you 
> > > > > > unconditionally added using REG32 for leaving the snapshot mode on 
> > > > > > streamon and entering it on streamoff. This should be ok in principle, 
> > > > > > since that's also what we're trying to do, using the CHIP_CONTROL 
> > > > > > register. But in your comment you say, that on some _systems_ you can only 
> > > > > > _operate_ in snapshot mode. I.e. the snapshot mode enabled during running 
> > > > > > streaming, right? Then how does this patch help you with that?
> > > > > 
> > > > > Yes. But i.e. the driver calling the sub-device stream control function
> > > > > on streamon knows that the normal mode is not supported and therefore it
> > > > > calls this function with argument enable == 0, effectively setting the
> > > > > snapshot mode.
> > > > 
> > > > Right, I thought you could be doing that... Well, on the one hand I should 
> > > > be happy, that the problem is solved without driver modifications, OTOH 
> > > > this isn't pretty... In fact this shouldn't work at all. After a 
> > > > stream-off the buffer queue should be stopped too.
> > > 
> > > Why shouldn't it work? The buffer queue is handled by the host driver,
> > > not by the sensor driver. And in my case the host driver stops the buffer
> > > queue in its streamoff, as it should. It works without issues and doesn't
> > > cause any problems for other mt9v022 users.
> > 
> > Because one shouldn't abuse the API by activating streaming on the bridge 
> > driver and deactivating it on the sensor.
> 
> I don't see how is it an abuse of the API at all. The sensor is just
> configured to snapshot mode but is still able to stream if it is
> triggered continuously by external events. It is actually a streaming
> case.

The STREAMOFF API is supposed to stop streaming completely. It is just on 
mt9v022 driver, that it happens, that no other way has been found to stop 
video operation but to switch to the snapshot mode. But the API itself 
prohibits any streaming in this mode. So, any other use of it is an abuse.

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
diff mbox

Patch

diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
index 8feaddc..2abe999 100644
--- a/drivers/media/i2c/soc_camera/mt9v022.c
+++ b/drivers/media/i2c/soc_camera/mt9v022.c
@@ -51,6 +51,7 @@  MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"");
 #define MT9V022_PIXEL_OPERATION_MODE	0x0f
 #define MT9V022_LED_OUT_CONTROL		0x1b
 #define MT9V022_ADC_MODE_CONTROL	0x1c
+#define MT9V022_REG32			0x20
 #define MT9V022_ANALOG_GAIN		0x35
 #define MT9V022_BLACK_LEVEL_CALIB_CTRL	0x47
 #define MT9V022_PIXCLK_FV_LV		0x74
@@ -79,7 +80,8 @@  MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"");
 #define MT9V022_VERTICAL_BLANKING_MAX	3000
 #define MT9V022_VERTICAL_BLANKING_DEF	45
 
-#define is_mt9v024(id) (id == 0x1324)
+#define is_mt9v022_rev3(id)	(id == 0x1313)
+#define is_mt9v024(id)		(id == 0x1324)
 
 /* MT9V022 has only one fixed colorspace per pixelcode */
 struct mt9v022_datafmt {
@@ -153,6 +155,7 @@  struct mt9v022 {
 	int num_fmts;
 	int model;	/* V4L2_IDENT_MT9V022* codes from v4l2-chip-ident.h */
 	u16 chip_control;
+	u16 chip_version;
 	unsigned short y_skip_top;	/* Lines to skip at the top */
 };
 
@@ -235,12 +238,32 @@  static int mt9v022_s_stream(struct v4l2_subdev *sd, int enable)
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct mt9v022 *mt9v022 = to_mt9v022(client);
 
-	if (enable)
+	if (enable) {
 		/* Switch to master "normal" mode */
 		mt9v022->chip_control &= ~0x10;
-	else
+		if (is_mt9v022_rev3(mt9v022->chip_version) ||
+		    is_mt9v024(mt9v022->chip_version)) {
+			/*
+			 * Unset snapshot mode specific settings: clear bit 9
+			 * and bit 2 in reg. 0x20 when in normal mode.
+			 */
+			if (reg_clear(client, MT9V022_REG32, 0x204))
+				return -EIO;
+		}
+	} else {
 		/* Switch to snapshot mode */
 		mt9v022->chip_control |= 0x10;
+		if (is_mt9v022_rev3(mt9v022->chip_version) ||
+		    is_mt9v024(mt9v022->chip_version)) {
+			/*
+			 * Required settings for snapshot mode: set bit 9
+			 * (RST enable) and bit 2 (CR enable) in reg. 0x20
+			 * See TechNote TN0960 or TN-09-225.
+			 */
+			if (reg_set(client, MT9V022_REG32, 0x204))
+				return -EIO;
+		}
+	}
 
 	if (reg_write(client, MT9V022_CHIP_CONTROL, mt9v022->chip_control) < 0)
 		return -EIO;
@@ -652,6 +675,8 @@  static int mt9v022_video_probe(struct i2c_client *client)
 		goto ei2c;
 	}
 
+	mt9v022->chip_version = data;
+
 	mt9v022->reg = is_mt9v024(data) ? &mt9v024_register :
 			&mt9v022_register;