* [PATCH] Add x_skip_left to soc_camera_device
@ 2008-05-06 13:00 hbmeier
2008-05-06 13:45 ` Guennadi Liakhovetski
0 siblings, 1 reply; 6+ messages in thread
From: hbmeier @ 2008-05-06 13:00 UTC (permalink / raw)
To: video4linux-list@redhat.com; +Cc: g.liakhovetski@gmx.de
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Add x_skip_left to soc_camera_device 2008-05-06 13:00 [PATCH] Add x_skip_left to soc_camera_device hbmeier @ 2008-05-06 13:45 ` Guennadi Liakhovetski 2008-05-16 8:54 ` Stefan Herbrechtsmeier 0 siblings, 1 reply; 6+ messages in thread From: Guennadi Liakhovetski @ 2008-05-06 13:45 UTC (permalink / raw) To: hbmeier@hni.uni-paderborn.de; +Cc: video4linux-list@redhat.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Add x_skip_left to soc_camera_device 2008-05-06 13:45 ` Guennadi Liakhovetski @ 2008-05-16 8:54 ` Stefan Herbrechtsmeier 2008-05-16 9:29 ` Guennadi Liakhovetski 0 siblings, 1 reply; 6+ messages in thread From: Stefan Herbrechtsmeier @ 2008-05-16 8:54 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: video4linux-list@redhat.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Add x_skip_left to soc_camera_device 2008-05-16 8:54 ` Stefan Herbrechtsmeier @ 2008-05-16 9:29 ` Guennadi Liakhovetski 2008-05-16 14:12 ` Stefan Herbrechtsmeier 0 siblings, 1 reply; 6+ messages in thread From: Guennadi Liakhovetski @ 2008-05-16 9:29 UTC (permalink / raw) To: Stefan Herbrechtsmeier; +Cc: video4linux-list@redhat.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Add x_skip_left to soc_camera_device 2008-05-16 9:29 ` Guennadi Liakhovetski @ 2008-05-16 14:12 ` Stefan Herbrechtsmeier 2008-05-19 9:05 ` Guennadi Liakhovetski 0 siblings, 1 reply; 6+ messages in thread From: Stefan Herbrechtsmeier @ 2008-05-16 14:12 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: video4linux-list@redhat.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Add x_skip_left to soc_camera_device 2008-05-16 14:12 ` Stefan Herbrechtsmeier @ 2008-05-19 9:05 ` Guennadi Liakhovetski 0 siblings, 0 replies; 6+ messages in thread From: Guennadi Liakhovetski @ 2008-05-19 9:05 UTC (permalink / raw) To: Stefan Herbrechtsmeier; +Cc: video4linux-list@redhat.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-05-19 9:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-05-06 13:00 [PATCH] Add x_skip_left to soc_camera_device hbmeier 2008-05-06 13:45 ` Guennadi Liakhovetski 2008-05-16 8:54 ` Stefan Herbrechtsmeier 2008-05-16 9:29 ` Guennadi Liakhovetski 2008-05-16 14:12 ` Stefan Herbrechtsmeier 2008-05-19 9:05 ` Guennadi Liakhovetski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.