[PATCH] Add x_skip_left to soc_camera_device

[PATCH] Add x_skip_left to soc_camera_device

[hbmeier]

[-- Attachment #1: Type: text/plain, Size: 3496 bytes --]

Add x_skip_left to soc_camera_device and use it as "Beginning-of-Line
Pixel Clock Wait Count" in pxa_camera driver

Signed-off-by: Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>
---
diff -r ead7cbcb4e49 linux/drivers/media/video/mt9m001.c
--- a/linux/drivers/media/video/mt9m001.c	Tue May 06 07:50:51 2008 -0300
+++ b/linux/drivers/media/video/mt9m001.c	Tue May 06 13:56:27 2008 +0200
@@ -649,18 +649,19 @@ static int mt9m001_probe(struct i2c_clie
 
 	/* Second stage probe - when a capture adapter is there */
 	icd = &mt9m001->icd;
-	icd->ops	= &mt9m001_ops;
-	icd->control	= &client->dev;
-	icd->x_min	= 20;
-	icd->y_min	= 12;
-	icd->x_current	= 20;
-	icd->y_current	= 12;
-	icd->width_min	= 48;
-	icd->width_max	= 1280;
-	icd->height_min	= 32;
-	icd->height_max	= 1024;
-	icd->y_skip_top	= 1;
-	icd->iface	= icl->bus_id;
+	icd->ops = &mt9m001_ops;
+	icd->control = &client->dev;
+	icd->x_min = 20;
+	icd->y_min = 12;
+	icd->x_current = 20;
+	icd->y_current = 12;
+	icd->width_min = 48;
+	icd->width_max = 1280;
+	icd->height_min = 32;
+	icd->height_max = 1024;
+	icd->x_skip_left = 0;
+	icd->y_skip_top = 1;
+	icd->iface = icl->bus_id;
 	/* Default datawidth - this is the only width this camera (normally)
 	 * supports. It is only with extra logic that it can support
 	 * other widths. Therefore it seems to be a sensible default. */
diff -r ead7cbcb4e49 linux/drivers/media/video/mt9v022.c
--- a/linux/drivers/media/video/mt9v022.c	Tue May 06 07:50:51 2008 -0300
+++ b/linux/drivers/media/video/mt9v022.c	Tue May 06 13:56:26 2008 +0200
@@ -774,18 +774,19 @@ static int mt9v022_probe(struct i2c_clie
 	i2c_set_clientdata(client, mt9v022);
 
 	icd = &mt9v022->icd;
-	icd->ops	= &mt9v022_ops;
-	icd->control	= &client->dev;
-	icd->x_min	= 1;
-	icd->y_min	= 4;
-	icd->x_current	= 1;
-	icd->y_current	= 4;
-	icd->width_min	= 48;
-	icd->width_max	= 752;
-	icd->height_min	= 32;
-	icd->height_max	= 480;
-	icd->y_skip_top	= 1;
-	icd->iface	= icl->bus_id;
+	icd->ops = &mt9v022_ops;
+	icd->control = &client->dev;
+	icd->x_min = 1;
+	icd->y_min = 4;
+	icd->x_current = 1;
+	icd->y_current = 4;
+	icd->width_min = 48;
+	icd->width_max = 752;
+	icd->height_min = 32;
+	icd->height_max = 480;
+	icd->x_skip_left = 0;
+	icd->y_skip_top = 1;
+	icd->iface = icl->bus_id;
 	/* Default datawidth - this is the only width this camera (normally)
 	 * supports. It is only with extra logic that it can support
 	 * other widths. Therefore it seems to be a sensible default. */
diff -r ead7cbcb4e49 linux/drivers/media/video/pxa_camera.c
--- a/linux/drivers/media/video/pxa_camera.c	Tue May 06 07:50:51 2008 -0300
+++ b/linux/drivers/media/video/pxa_camera.c	Tue May 06 13:51:28 2008 +0200
@@ -883,7 +883,7 @@ static int pxa_camera_set_bus_param(stru
 	}
 
 	CICR1 = cicr1;
-	CICR2 = 0;
+	CICR2 = CICR2_BLW_VAL(min((unsigned short)255, icd->x_skip_left));
 	CICR3 = CICR3_LPF_VAL(icd->height - 1) |
 		CICR3_BFW_VAL(min((unsigned short)255, icd->y_skip_top));
 	CICR4 = mclk_get_divisor(pcdev) | cicr4;
diff -r ead7cbcb4e49 linux/include/media/soc_camera.h
--- a/linux/include/media/soc_camera.h	Tue May 06 07:50:51 2008 -0300
+++ b/linux/include/media/soc_camera.h	Tue May 06 13:51:28 2008 +0200
@@ -29,6 +29,7 @@ struct soc_camera_device {
 	unsigned short width_max;
 	unsigned short height_min;
 	unsigned short height_max;
+	unsigned short x_skip_left;	/* Pixel to skip at the left */
 	unsigned short y_skip_top;	/* Lines to skip at the top */
 	unsigned short gain;
 	unsigned short exposure;


[-- Attachment #2: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Add x_skip_left to soc_camera_device

[Guennadi Liakhovetski]

Hi Stefan,

The patch looks good in general, thanks, but

On Tue, 6 May 2008, hbmeier@hni.uni-paderborn.de wrote:

> Add x_skip_left to soc_camera_device and use it as "Beginning-of-Line
> Pixel Clock Wait Count" in pxa_camera driver
> 
> Signed-off-by: Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>
> ---
> diff -r ead7cbcb4e49 linux/drivers/media/video/mt9m001.c
> --- a/linux/drivers/media/video/mt9m001.c	Tue May 06 07:50:51 2008 -0300
> +++ b/linux/drivers/media/video/mt9m001.c	Tue May 06 13:56:27 2008 +0200
> @@ -649,18 +649,19 @@ static int mt9m001_probe(struct i2c_clie
>  
>  	/* Second stage probe - when a capture adapter is there */
>  	icd = &mt9m001->icd;
> -	icd->ops	= &mt9m001_ops;
> -	icd->control	= &client->dev;
> -	icd->x_min	= 20;
> -	icd->y_min	= 12;
> -	icd->x_current	= 20;
> -	icd->y_current	= 12;
> -	icd->width_min	= 48;
> -	icd->width_max	= 1280;
> -	icd->height_min	= 32;
> -	icd->height_max	= 1024;
> -	icd->y_skip_top	= 1;
> -	icd->iface	= icl->bus_id;
> +	icd->ops = &mt9m001_ops;
> +	icd->control = &client->dev;
> +	icd->x_min = 20;
> +	icd->y_min = 12;
> +	icd->x_current = 20;
> +	icd->y_current = 12;
> +	icd->width_min = 48;
> +	icd->width_max = 1280;
> +	icd->height_min = 32;
> +	icd->height_max = 1024;
> +	icd->x_skip_left = 0;
> +	icd->y_skip_top = 1;
> +	icd->iface = icl->bus_id;

hmmm... I did tell you

> > Use your esthetic feeling:-) But if it doesn't coincide with mine, I'll 
> > reject the patch:-))

and, I am sorry, it is really pretty far from mine, so...:-) But the main 
reason why I don't quite like it is, that you _needlessly_ modify 12 
lines. It is good to keep patches as small as possible, to simplify review 
and to reduce the chances for a mistake. You see, I can think of several 
ways to add this line to these two files (mt9m001 and mt9v022), e.g., just 
add it with one space at the end,

	icd->width_max	= 1280;
	icd->height_min	= 32;
	icd->height_max	= 1024;
	icd->y_skip_top	= 1;
+	icd->x_skip_left = 0;

Yes, alignment will be broken, but only on one line. Or add one more TAB. 
But I don't think your solution is the best. Actually, I think, we 
shouldn't add this line to these two files at all. icd is embedded in 
mt9m001 / mt9v022 structs, and they are allocated per kzalloc, so, already 
nullified. I would just leave these two files as they are.

> diff -r ead7cbcb4e49 linux/drivers/media/video/pxa_camera.c
> --- a/linux/drivers/media/video/pxa_camera.c	Tue May 06 07:50:51 2008 -0300
> +++ b/linux/drivers/media/video/pxa_camera.c	Tue May 06 13:51:28 2008 +0200
> @@ -883,7 +883,7 @@ static int pxa_camera_set_bus_param(stru
>  	}
>  
>  	CICR1 = cicr1;
> -	CICR2 = 0;
> +	CICR2 = CICR2_BLW_VAL(min((unsigned short)255, icd->x_skip_left));
>  	CICR3 = CICR3_LPF_VAL(icd->height - 1) |
>  		CICR3_BFW_VAL(min((unsigned short)255, icd->y_skip_top));
>  	CICR4 = mclk_get_divisor(pcdev) | cicr4;
> diff -r ead7cbcb4e49 linux/include/media/soc_camera.h
> --- a/linux/include/media/soc_camera.h	Tue May 06 07:50:51 2008 -0300
> +++ b/linux/include/media/soc_camera.h	Tue May 06 13:51:28 2008 +0200
> @@ -29,6 +29,7 @@ struct soc_camera_device {
>  	unsigned short width_max;
>  	unsigned short height_min;
>  	unsigned short height_max;
> +	unsigned short x_skip_left;	/* Pixel to skip at the left */
>  	unsigned short y_skip_top;	/* Lines to skip at the top */
>  	unsigned short gain;
>  	unsigned short exposure;

I think, this is all we need for now - small and nice. Actually, it would 
make even more sense to submit this when your new camera driver is ready, 
but if you prefer, I'll accept it now. Just, please, resubmit it without 
the above two hunks, and, maybe, add a sentence to the patch comment, 
saying "will be used in xxx driver."

Sorry, and I promise, I'll accept the patch in proposed form, as long as 
it still applies and works at the time you submit it:-)

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Add x_skip_left to soc_camera_device

[Stefan Herbrechtsmeier]

Sorry for the late answer, but I have to rework my driver.

Guennadi Liakhovetski schrieb:
> I think, this is all we need for now - small and nice. Actually, it would 
> make even more sense to submit this when your new camera driver is ready, 
> but if you prefer, I'll accept it now. Just, please, resubmit it without 
> the above two hunks, and, maybe, add a sentence to the patch comment, 
> saying "will be used in xxx driver."
>   
Because of problems with the HSYNC support for different resolutions, I 
skipped it.
I remove the HSYNC specific code (configuration of HSYNC or HREF) and
only used HREF as signal. This makes this Patch obsolete for now.

Should I skip it or resubmit it for further use?

Thanks
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Add x_skip_left to soc_camera_device

[Guennadi Liakhovetski]

Hi Stefan,

On Fri, 16 May 2008, Stefan Herbrechtsmeier wrote:

> Sorry for the late answer, but I have to rework my driver.
> 
> Guennadi Liakhovetski schrieb:
> > I think, this is all we need for now - small and nice. Actually, it would
> > make even more sense to submit this when your new camera driver is ready,
> > but if you prefer, I'll accept it now. Just, please, resubmit it without the
> > above two hunks, and, maybe, add a sentence to the patch comment, saying
> > "will be used in xxx driver."
> >   
> Because of problems with the HSYNC support for different resolutions, I
> skipped it.
> I remove the HSYNC specific code (configuration of HSYNC or HREF) and
> only used HREF as signal. This makes this Patch obsolete for now.
> 
> Should I skip it or resubmit it for further use?

I think, reviewing and testing of patches is easier, if they either 1) fix 
bugs that can either be reproduced with currently supported 
configurations, or can be proven by studying the code; or 2) implement 
improvements, that can be verified with currently supported 
configurations; or 3) implement new features, that can be tested with 
currently or newly supported configurations. As you see, new features 
(x_skip_left support is a new feature), that cannot be tested are not in 
the above list:-)

So, I think, it would be easier for you and for reviewers, if you submit 
your new driver together with any necessary supporting modifications to 
the existing code, when you are reasonably happy with your results. 
Agree?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Add x_skip_left to soc_camera_device

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> So, I think, it would be easier for you and for reviewers, if you submit 
> your new driver together with any necessary supporting modifications to 
> the existing code, when you are reasonably happy with your results. 
> Agree?
>   
Thanks for your help and patience with me.

I submit the changes together with my driver.

Thanks
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Add x_skip_left to soc_camera_device

[Guennadi Liakhovetski]

On Fri, 16 May 2008, Stefan Herbrechtsmeier wrote:

> Thanks for your help and patience with me.

Thank you for your work.

> I submit the changes together with my driver.

Great! Looking forward to it.

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list