Skip to contentSkip to conversation

Add x_skip_left to soc_camera_device

PATCH In review [email protected] 2 participants 6 messages May 6, 2008 – May 19, 2008 Media (V4L/DVB)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Add x_skip_left to soc_camera_device

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

---

linux/drivers/media/video/mt9m001.c +13 -12
diff -r ead7cbcb4e49 linux/drivers/media/video/mt9m001.c
@@ -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. */
linux/drivers/media/video/mt9v022.c +13 -12
diff -r ead7cbcb4e49 linux/drivers/media/video/mt9v022.c
@@ -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. */
linux/drivers/media/video/pxa_camera.c +1 -1
diff -r ead7cbcb4e49 linux/drivers/media/video/pxa_camera.c
@@ -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;
linux/include/media/soc_camera.h +1 -0
diff -r ead7cbcb4e49 linux/include/media/soc_camera.h
@@ -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;

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#1 lore raw

Hi Stefan,

The patch looks good in general, thanks, but

Quoted 27 lines · On Tue, 6 May 2008, [email protected] wrote:

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

---
diff -r ead7cbcb4e49 linux/drivers/media/video/mt9m001.c

linux/drivers/media/video/mt9m001.c +3 -12
@@ -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;

linux/drivers/media/video/mt9m001.c static int mt9m001_probe(struct i2c_clie · line 664

Quoted 10 lines
linux/drivers/media/video/mt9m001.c +10 -0
+	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

Quoted 2 lines
Quoted 2 lines

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.

Quoted 16 lines

diff -r ead7cbcb4e49 linux/drivers/media/video/pxa_camera.c

linux/drivers/media/video/pxa_camera.c +1 -1
@@ -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

linux/include/media/soc_camera.h struct soc_camera_device { · line 35

Quoted 7 lines
linux/include/media/soc_camera.h +1 -0
@@ -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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#2 lore raw

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

Quoted 2 lines · Guennadi Liakhovetski

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

Signature · 19 lines
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 : [email protected]

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


--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
#3 lore raw

Hi Stefan,

Quoted 9 lines · Stefan Herbrechtsmeier

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

Quoted 6 lines · Guennadi Liakhovetski

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."

Quoted 6 lines

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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#4 lore raw
Quoted 2 lines · Guennadi Liakhovetski

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

Signature · 19 lines
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 : [email protected]

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


--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list