[PATCH] MT9T031: write xskip and yskip at each set_params call

[PATCH] MT9T031: write xskip and yskip at each set_params call

[Valentin Longchamp]

This prevents the registers to be different to the computed values
the second time you open the same camera with the sames parameters.

The images were different between the first device open and the
second one with the same parameters.

Signed-off-by: Valentin Longchamp <valentin.longchamp@epfl.ch>
---
 drivers/media/video/mt9t031.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
index a9061bf..e4a9095 100644
--- a/drivers/media/video/mt9t031.c
+++ b/drivers/media/video/mt9t031.c
@@ -17,6 +17,7 @@
 #include <media/v4l2-chip-ident.h>
 #include <media/soc_camera.h>
 
+
 /*
  * mt9t031 i2c address 0x5d
  * The platform has to define i2c_board_info and link to it from
@@ -337,15 +338,13 @@ static int mt9t031_set_params(struct i2c_client *client,
 	if (ret >= 0)
 		ret = reg_write(client, MT9T031_VERTICAL_BLANKING, vblank);
 
-	if (yskip != mt9t031->yskip || xskip != mt9t031->xskip) {
-		/* Binning, skipping */
-		if (ret >= 0)
-			ret = reg_write(client, MT9T031_COLUMN_ADDRESS_MODE,
-					((xbin - 1) << 4) | (xskip - 1));
-		if (ret >= 0)
-			ret = reg_write(client, MT9T031_ROW_ADDRESS_MODE,
-					((ybin - 1) << 4) | (yskip - 1));
-	}
+	/* Binning, skipping */
+	if (ret >= 0)
+		ret = reg_write(client, MT9T031_COLUMN_ADDRESS_MODE,
+				((xbin - 1) << 4) | (xskip - 1));
+	if (ret >= 0)
+		ret = reg_write(client, MT9T031_ROW_ADDRESS_MODE,
+				((ybin - 1) << 4) | (yskip - 1));
 	dev_dbg(&client->dev, "new physical left %u, top %u\n",
 		rect->left, rect->top);
 
-- 
1.6.3.3

Re: [PATCH] MT9T031: write xskip and yskip at each set_params call

[Guennadi Liakhovetski]

On Wed, 20 Jan 2010, Valentin Longchamp wrote:

> This prevents the registers to be different to the computed values
> the second time you open the same camera with the sames parameters.
> 
> The images were different between the first device open and the
> second one with the same parameters.

But why they were different? Weren't xskip and yskip preserved from the 
previous S_CROP / S_FMT configuration? If so, then, I am afraid, this is 
the behaviour, mandated by the API, and as such shall not be changed. Or 
have I misunderstood you?

Thanks
Guennadi

> 
> Signed-off-by: Valentin Longchamp <valentin.longchamp@epfl.ch>
> ---
>  drivers/media/video/mt9t031.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
> index a9061bf..e4a9095 100644
> --- a/drivers/media/video/mt9t031.c
> +++ b/drivers/media/video/mt9t031.c
> @@ -17,6 +17,7 @@
>  #include <media/v4l2-chip-ident.h>
>  #include <media/soc_camera.h>
>  
> +
>  /*
>   * mt9t031 i2c address 0x5d
>   * The platform has to define i2c_board_info and link to it from
> @@ -337,15 +338,13 @@ static int mt9t031_set_params(struct i2c_client *client,
>  	if (ret >= 0)
>  		ret = reg_write(client, MT9T031_VERTICAL_BLANKING, vblank);
>  
> -	if (yskip != mt9t031->yskip || xskip != mt9t031->xskip) {
> -		/* Binning, skipping */
> -		if (ret >= 0)
> -			ret = reg_write(client, MT9T031_COLUMN_ADDRESS_MODE,
> -					((xbin - 1) << 4) | (xskip - 1));
> -		if (ret >= 0)
> -			ret = reg_write(client, MT9T031_ROW_ADDRESS_MODE,
> -					((ybin - 1) << 4) | (yskip - 1));
> -	}
> +	/* Binning, skipping */
> +	if (ret >= 0)
> +		ret = reg_write(client, MT9T031_COLUMN_ADDRESS_MODE,
> +				((xbin - 1) << 4) | (xskip - 1));
> +	if (ret >= 0)
> +		ret = reg_write(client, MT9T031_ROW_ADDRESS_MODE,
> +				((ybin - 1) << 4) | (yskip - 1));
>  	dev_dbg(&client->dev, "new physical left %u, top %u\n",
>  		rect->left, rect->top);
>  
> -- 
> 1.6.3.3
> 

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

Re: [PATCH] MT9T031: write xskip and yskip at each set_params call

[Valentin Longchamp]

Guennadi Liakhovetski wrote:
> On Wed, 20 Jan 2010, Valentin Longchamp wrote:
> 
>> This prevents the registers to be different to the computed values
>> the second time you open the same camera with the sames parameters.
>>
>> The images were different between the first device open and the
>> second one with the same parameters.
> 
> But why they were different? Weren't xskip and yskip preserved from the 
> previous S_CROP / S_FMT configuration? If so, then, I am afraid, this is 
> the behaviour, mandated by the API, and as such shall not be changed. Or 
> have I misunderstood you?

Here are more details about what I debugged:

First more details about what I do with the camera: I open the device, 
issue the S_CROP / S_FMT calls and read images, the behaviour is fine, 
then close the device.

Then if I reopen the device, reissue the S_CROP / S_FMT calls with the 
same params, but the images is not the sames because of different xskip 
and yskip. From what I have debugged in the driver at the second S_CROP 
/S_FMT, xskip and yskip are computed by mt9t031_skip (and have the same 
value that the one stored in the mt9t031 struct) and thus with the 
current code are not rewritten.

However, if I read the register values containing bin and skip values on 
the camera chip they have been reset (does a open/close do some reset to 
the cam ?) and thus different than the ones that should be written.

I hope this clarifies the problem that I am experiencing. I don't think 
that the API wants you to get two different images when you open the 
device and issue the same parameters twice.

Best Regards

Val

> 
> Thanks
> Guennadi
> 
>> Signed-off-by: Valentin Longchamp <valentin.longchamp@epfl.ch>
>> ---
>>  drivers/media/video/mt9t031.c |   17 ++++++++---------
>>  1 files changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
>> index a9061bf..e4a9095 100644
>> --- a/drivers/media/video/mt9t031.c
>> +++ b/drivers/media/video/mt9t031.c
>> @@ -17,6 +17,7 @@
>>  #include <media/v4l2-chip-ident.h>
>>  #include <media/soc_camera.h>
>>  
>> +
>>  /*
>>   * mt9t031 i2c address 0x5d
>>   * The platform has to define i2c_board_info and link to it from
>> @@ -337,15 +338,13 @@ static int mt9t031_set_params(struct i2c_client *client,
>>  	if (ret >= 0)
>>  		ret = reg_write(client, MT9T031_VERTICAL_BLANKING, vblank);
>>  
>> -	if (yskip != mt9t031->yskip || xskip != mt9t031->xskip) {
>> -		/* Binning, skipping */
>> -		if (ret >= 0)
>> -			ret = reg_write(client, MT9T031_COLUMN_ADDRESS_MODE,
>> -					((xbin - 1) << 4) | (xskip - 1));
>> -		if (ret >= 0)
>> -			ret = reg_write(client, MT9T031_ROW_ADDRESS_MODE,
>> -					((ybin - 1) << 4) | (yskip - 1));
>> -	}
>> +	/* Binning, skipping */
>> +	if (ret >= 0)
>> +		ret = reg_write(client, MT9T031_COLUMN_ADDRESS_MODE,
>> +				((xbin - 1) << 4) | (xskip - 1));
>> +	if (ret >= 0)
>> +		ret = reg_write(client, MT9T031_ROW_ADDRESS_MODE,
>> +				((ybin - 1) << 4) | (yskip - 1));
>>  	dev_dbg(&client->dev, "new physical left %u, top %u\n",
>>  		rect->left, rect->top);
>>  
>> -- 
>> 1.6.3.3
>>
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/


-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp@epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEB3494, Station 9, CH-1015 Lausanne

Re: [PATCH] MT9T031: write xskip and yskip at each set_params call

[Guennadi Liakhovetski]

(added two more persons to CC, because I think, this discussion can be 
interesting for them too)

On Thu, 21 Jan 2010, Valentin Longchamp wrote:

> Guennadi Liakhovetski wrote:
> > On Wed, 20 Jan 2010, Valentin Longchamp wrote:
> > 
> > > This prevents the registers to be different to the computed values
> > > the second time you open the same camera with the sames parameters.
> > > 
> > > The images were different between the first device open and the
> > > second one with the same parameters.
> > 
> > But why they were different? Weren't xskip and yskip preserved from the
> > previous S_CROP / S_FMT configuration? If so, then, I am afraid, this is the
> > behaviour, mandated by the API, and as such shall not be changed. Or have I
> > misunderstood you?
> 
> Here are more details about what I debugged:

Sorry for taking so long to reply.

> First more details about what I do with the camera: I open the device, issue
> the S_CROP / S_FMT calls and read images, the behaviour is fine, then close
> the device.
> 
> Then if I reopen the device, reissue the S_CROP / S_FMT calls with the same
> params, but the images is not the sames because of different xskip and yskip.
> From what I have debugged in the driver at the second S_CROP /S_FMT, xskip and
> yskip are computed by mt9t031_skip (and have the same value that the one
> stored in the mt9t031 struct) and thus with the current code are not
> rewritten.
> 
> However, if I read the register values containing bin and skip values on the
> camera chip they have been reset (does a open/close do some reset to the cam
> ?) and thus different than the ones that should be written.

Yes, if hooks are provided by the platform, on last close we power down 
cameras, on first open we power on and reset them.

> I hope this clarifies the problem that I am experiencing. I don't think that
> the API wants you to get two different images when you open the device and
> issue the same parameters twice.

Yes, sorry, this is a different issue. The issue is - too crude power 
management in soc-camera. What we should do is implement proper (runtime) 
power-management in soc-camera (ideally, in v4l2-subdev API) and call 
suspend() to save registers before powering down, and resume() after 
powering on and resetting.

I gave it a shot and just posted a patch

http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/15686

(hm, would have been smart to cc you, sorry), doing just that. Below is an 
example dummy implementation for mt9v022. Please, use it as example to 
implement suspend / resume for mt9t031, for now, I think, it would suffice 
if you just restore xskip and yskip in restore and skip suspend. If more 
is needed in the future, we can always extend it.

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

[PATCH] soc-camera: DUMMY runtime power-management for mt9v022 (not for mainline)

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index 1a34d29..08a3478 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -8,15 +8,17 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/videodev2.h>
-#include <linux/slab.h>
-#include <linux/i2c.h>
 #include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
 #include <linux/log2.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/videodev2.h>
 
-#include <media/v4l2-subdev.h>
-#include <media/v4l2-chip-ident.h>
 #include <media/soc_camera.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-subdev.h>
 
 /*
  * mt9v022 i2c address 0x48, 0x4c, 0x58, 0x5c
@@ -718,6 +720,28 @@ static int mt9v022_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
 	return 0;
 }
 
+static int mt9v022_runtime_suspend(struct device *dev)
+{
+	dev_info(dev, "%s\n", __func__);
+	return 0;
+}
+
+static int mt9v022_runtime_resume(struct device *dev)
+{
+	dev_info(dev, "%s\n", __func__);
+	return 0;
+}
+
+static struct dev_pm_ops mt9v022_dev_pm_ops = {
+	.runtime_suspend	= mt9v022_runtime_suspend,
+	.runtime_resume		= mt9v022_runtime_resume,
+};
+
+static struct device_type mt9v022_dev_type = {
+	.name		= "MT9V022",
+	.pm		= &mt9v022_dev_pm_ops,
+};
+
 /*
  * Interface active, can use i2c. If it fails, it can indeed mean, that
  * this wasn't our capture interface, so, we wait for the right one
@@ -727,6 +751,7 @@ static int mt9v022_video_probe(struct soc_camera_device *icd,
 {
 	struct mt9v022 *mt9v022 = to_mt9v022(client);
 	struct soc_camera_link *icl = to_soc_camera_link(icd);
+	struct video_device *vdev = soc_camera_i2c_to_vdev(client);
 	s32 data;
 	int ret;
 	unsigned long flags;
@@ -803,6 +828,8 @@ static int mt9v022_video_probe(struct soc_camera_device *icd,
 	ret = mt9v022_init(client);
 	if (ret < 0)
 		dev_err(&client->dev, "Failed to initialise the camera\n");
+	else
+		vdev->dev.type = &mt9v022_dev_type;
 
 ei2c:
 	return ret;

Re: [PATCH] MT9T031: write xskip and yskip at each set_params call

[Valentin Longchamp]

Hello Guennadi,

Guennadi Liakhovetski wrote:
>> First more details about what I do with the camera: I open the device, issue
>> the S_CROP / S_FMT calls and read images, the behaviour is fine, then close
>> the device.
>>
>> Then if I reopen the device, reissue the S_CROP / S_FMT calls with the same
>> params, but the images is not the sames because of different xskip and yskip.
>> From what I have debugged in the driver at the second S_CROP /S_FMT, xskip and
>> yskip are computed by mt9t031_skip (and have the same value that the one
>> stored in the mt9t031 struct) and thus with the current code are not
>> rewritten.
>>
>> However, if I read the register values containing bin and skip values on the
>> camera chip they have been reset (does a open/close do some reset to the cam
>> ?) and thus different than the ones that should be written.
> 
> Yes, if hooks are provided by the platform, on last close we power down 
> cameras, on first open we power on and reset them.
> 
>> I hope this clarifies the problem that I am experiencing. I don't think that
>> the API wants you to get two different images when you open the device and
>> issue the same parameters twice.
> 
> Yes, sorry, this is a different issue. The issue is - too crude power 
> management in soc-camera. What we should do is implement proper (runtime) 
> power-management in soc-camera (ideally, in v4l2-subdev API) and call 
> suspend() to save registers before powering down, and resume() after 
> powering on and resetting.
> 
> I gave it a shot and just posted a patch
> 
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/15686
> 
> (hm, would have been smart to cc you, sorry), doing just that. Below is an 
> example dummy implementation for mt9v022. Please, use it as example to 
> implement suspend / resume for mt9t031, for now, I think, it would suffice 
> if you just restore xskip and yskip in restore and skip suspend. If more 
> is needed in the future, we can always extend it.

OK, I agree with your analysis and I have tried to write the part for 
runtime suspend and resume to solve this issue. I however find myself in 
a deadend because of my limited knowledge of the kernel device model.

Here is my attempt with my questions in it (it compiles, but I have not 
tested it yet because I am pretty sure something is wrong):

> /*
>  * Power Management:
>  * This function does nothing for now but must be present for pm to work
>  */
> static int mt9t031_runtime_suspend(struct device *dev)
> {
> 	return 0;
> }

First, can you confirm me that this function is needed even if it does 
nothing ? I have read in the code that if the function is not present, 
__pm_runtime_suspend is going to return -ENOSYS and will set 
runtime_status to RPM_ACTIVE, which is not what we want. That's why I 
left it.

> 
> /*
>  * Power Management:
>  * COLUMN_ADDRESS_MODE and ROW_ADDRESS_MODE are not rewritten if unchanged
>  * they are however changed at reset if the platform hook is present
>  * thus we rewrite them with the values stored by the driver
>  */
> static int mt9t031_runtime_resume(struct device *dev)
> {
> 	struct video_device *vdev = to_video_device(dev);
> 	struct soc_camera_device *icd = container_of(vdev->parent,
> 		struct soc_camera_device, dev);
> 	struct device *i2c_dev = container_of(icd, struct device, 
> 		platform_data);
> 	struct i2c_client *client = to_i2c_client(i2c_dev);
> 	struct mt9t031 *mt9t031 = to_mt9t031(client);

Here I have a problem ... I am pretty sure that the third assignation 
has a problem, because platform_data is a pointer and not a normal 
member of struct device, and I thus cannot use container_of with it. 
What would then be the way to go from the soc_camera_device to the 
i2c_client (I'm a little bit confused with all the different structs and 
layers involved with i2c, soc-camera, v4l2_device and v4l2_subdevice) ? 
Just as a remark, this is the exact reverse path of this that is present 
in your patch to add runtime pm support to soc-camera:

/* This is only temporary here - until v4l2-subdev begins to link to 
video_device */
#include <linux/i2c.h>
static inline struct video_device *soc_camera_i2c_to_vdev(struct 
i2c_client *client)
{
	struct soc_camera_device *icd = client->dev.platform_data;
	return icd->vdev;
}

> 
> 	int ret;
> 	u16 xbin, ybin;
> 
> 	xbin = min(mt9t031->xskip, (u16)3);
> 	ybin = min(mt9t031->yskip, (u16)3);
> 
> 	ret = reg_write(client, MT9T031_COLUMN_ADDRESS_MODE,
> 		((xbin-1)<<4) | (mt9t031->xskip-1));
> 	if (ret < 0)
> 		return ret;
> 
> 	ret = reg_write(client, MT9T031_ROW_ADDRESS_MODE,
> 		((ybin-1)<<4) | (mt9t031->yskip-1));
> 	if (ret < 0)
> 		return ret;
> 
> 	return 0;
> }
> 
> static struct dev_pm_ops mt9t031_dev_pm_ops = {
> 	.runtime_suspend	= mt9t031_runtime_suspend,
> 	.runtime_resume		= mt9t031_runtime_resume,
> };
> 
> static struct device_type mt9t031_dev_type = {
> 	.name	= "MT9T031",
> 	.pm	= &mt9t031_dev_pm_ops,
> };

Thank you in advance for your help.

Val

-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp@epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEB3494, Station 9, CH-1015 Lausanne

Re: [PATCH] MT9T031: write xskip and yskip at each set_params call

[Guennadi Liakhovetski]

Hi Val

On Mon, 8 Feb 2010, Valentin Longchamp wrote:

> > /*
> >  * Power Management:
> >  * This function does nothing for now but must be present for pm to work
> >  */
> > static int mt9t031_runtime_suspend(struct device *dev)
> > {
> > 	return 0;
> > }
> 
> First, can you confirm me that this function is needed even if it does nothing
> ? I have read in the code that if the function is not present,
> __pm_runtime_suspend is going to return -ENOSYS and will set runtime_status to
> RPM_ACTIVE, which is not what we want. That's why I left it.

Yes, I think, you're right - if neither bus, nor type, nor class suspend 
is debined, -ENOSYS is returned (see __pm_runtime_suspend()).

> > /*
> >  * Power Management:
> >  * COLUMN_ADDRESS_MODE and ROW_ADDRESS_MODE are not rewritten if unchanged
> >  * they are however changed at reset if the platform hook is present
> >  * thus we rewrite them with the values stored by the driver
> >  */
> > static int mt9t031_runtime_resume(struct device *dev)
> > {
> > 	struct video_device *vdev = to_video_device(dev);
> > 	struct soc_camera_device *icd = container_of(vdev->parent,
> > 		struct soc_camera_device, dev);
> > 	struct device *i2c_dev = container_of(icd, struct device,
> > platform_data);
> > 	struct i2c_client *client = to_i2c_client(i2c_dev);
> > 	struct mt9t031 *mt9t031 = to_mt9t031(client);
> 
> Here I have a problem ... I am pretty sure that the third assignation has a
> problem, because platform_data is a pointer and not a normal member of struct
> device, and I thus cannot use container_of with it. What would then be the way
> to go from the soc_camera_device to the i2c_client (I'm a little bit confused
> with all the different structs and layers involved with i2c, soc-camera,
> v4l2_device and v4l2_subdevice) ?

	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
	struct i2c_client *client = sd->priv;

> Just as a remark, this is the exact reverse
> path of this that is present in your patch to add runtime pm support to
> soc-camera:
> 
> /* This is only temporary here - until v4l2-subdev begins to link to
> video_device */
> #include <linux/i2c.h>
> static inline struct video_device *soc_camera_i2c_to_vdev(struct i2c_client
> *client)
> {
> 	struct soc_camera_device *icd = client->dev.platform_data;
> 	return icd->vdev;
> }
> 
> > 
> > 	int ret;
> > 	u16 xbin, ybin;
> > 
> > 	xbin = min(mt9t031->xskip, (u16)3);
> > 	ybin = min(mt9t031->yskip, (u16)3);
> > 
> > 	ret = reg_write(client, MT9T031_COLUMN_ADDRESS_MODE,
> > 		((xbin-1)<<4) | (mt9t031->xskip-1));
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 	ret = reg_write(client, MT9T031_ROW_ADDRESS_MODE,
> > 		((ybin-1)<<4) | (mt9t031->yskip-1));
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 	return 0;
> > }
> > 
> > static struct dev_pm_ops mt9t031_dev_pm_ops = {
> > 	.runtime_suspend	= mt9t031_runtime_suspend,
> > 	.runtime_resume		= mt9t031_runtime_resume,
> > };
> > 
> > static struct device_type mt9t031_dev_type = {
> > 	.name	= "MT9T031",
> > 	.pm	= &mt9t031_dev_pm_ops,
> > };
> 
> Thank you in advance for your help.

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