[05/11] ov534: Fix setting manual exposure
Commit Message
Exposure is now a u16 value, both MSB and LSB are set, but values in the v4l2
control are limited to the interval [0,506] as 0x01fa (506) is the maximum
observed value with AEC enabled.
Skip setting exposure when AEC is enabled.
Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
linux/drivers/media/video/gspca/ov534.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
--
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
Comments
On Sat, 27 Feb 2010 21:20:22 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:
> Exposure is now a u16 value, both MSB and LSB are set, but values in
> the v4l2 control are limited to the interval [0,506] as 0x01fa (506)
> is the maximum observed value with AEC enabled.
[snip]
> .type = V4L2_CTRL_TYPE_INTEGER,
> .name = "Exposure",
> .minimum = 0,
> - .maximum = 255,
> + .maximum = 506,
> .step = 1,
> #define EXPO_DEF 120
> .default_value = EXPO_DEF,
Hi Antonio,
Do we need a so high precision for the exposure? Just setting the
maximum value to 253 should solve the problem.
Cheers.
On Sun, 28 Feb 2010 19:38:14 +0100
Jean-Francois Moine <moinejf@free.fr> wrote:
> On Sat, 27 Feb 2010 21:20:22 +0100
> Antonio Ospite <ospite@studenti.unina.it> wrote:
>
> > Exposure is now a u16 value, both MSB and LSB are set, but values in
> > the v4l2 control are limited to the interval [0,506] as 0x01fa (506)
> > is the maximum observed value with AEC enabled.
> [snip]
> > .type = V4L2_CTRL_TYPE_INTEGER,
> > .name = "Exposure",
> > .minimum = 0,
> > - .maximum = 255,
> > + .maximum = 506,
> > .step = 1,
> > #define EXPO_DEF 120
> > .default_value = EXPO_DEF,
>
> Hi Antonio,
>
> Do we need a so high precision for the exposure? Just setting the
> maximum value to 253 should solve the problem.
>
JF, the intent here is to cover all the range of values available in
Auto Exposure mode too, doesn't this make sense to you?
I could set .maximum to 253 to limit the "UI" control precision but then
I should use 2*value when setting the registers in order to cover the
actual max value, this looks a little unclean.
Anyhow, let me know what you prefer, I have no strong feelings on that.
If you want to save a byte, I'll agree.
Regards,
Antonio
On Sun, 28 Feb 2010 19:54:25 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:
> JF, the intent here is to cover all the range of values available in
> Auto Exposure mode too, doesn't this make sense to you?
>
> I could set .maximum to 253 to limit the "UI" control precision but
> then I should use 2*value when setting the registers in order to
> cover the actual max value, this looks a little unclean.
>
> Anyhow, let me know what you prefer, I have no strong feelings on
> that. If you want to save a byte, I'll agree.
Looking at the rare chip documents I got, I often saw internal control
values on 3 bytes (16 million values). Could anybody see the difference
between values v and (v+1)?
So, often, even when the internal controls are stored in only one byte,
the ms-win drivers propose only ranges as 0..30 or 0..100.
On Sun, 28 Feb 2010 19:54:25 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:
> On Sun, 28 Feb 2010 19:38:14 +0100
> Jean-Francois Moine <moinejf@free.fr> wrote:
>
> > On Sat, 27 Feb 2010 21:20:22 +0100
> > Antonio Ospite <ospite@studenti.unina.it> wrote:
> >
> > > Exposure is now a u16 value, both MSB and LSB are set, but values in
> > > the v4l2 control are limited to the interval [0,506] as 0x01fa (506)
> > > is the maximum observed value with AEC enabled.
> > [snip]
> > > .type = V4L2_CTRL_TYPE_INTEGER,
> > > .name = "Exposure",
> > > .minimum = 0,
> > > - .maximum = 255,
> > > + .maximum = 506,
> > > .step = 1,
> > > #define EXPO_DEF 120
> > > .default_value = EXPO_DEF,
> >
> > Hi Antonio,
> >
> > Do we need a so high precision for the exposure? Just setting the
> > maximum value to 253 should solve the problem.
> >
>
> JF, the intent here is to cover all the range of values available in
> Auto Exposure mode too, doesn't this make sense to you?
>
> I could set .maximum to 253 to limit the "UI" control precision but then
> I should use 2*value when setting the registers in order to cover the
> actual max value, this looks a little unclean.
>
Ok, I now see that that's exactly what current code does, sorry for the
noise. The patch then degenerates to a simpler one with some
documentation added, so others don't overlook the code like I did.
Sending it in a min.
Regards,
Antonio
===================================================================
@@ -59,7 +59,7 @@
u8 brightness;
u8 contrast;
u8 gain;
- u8 exposure;
+ u16 exposure;
u8 agc;
u8 awb;
u8 aec;
@@ -140,7 +140,7 @@
.type = V4L2_CTRL_TYPE_INTEGER,
.name = "Exposure",
.minimum = 0,
- .maximum = 255,
+ .maximum = 506,
.step = 1,
#define EXPO_DEF 120
.default_value = EXPO_DEF,
@@ -684,11 +684,15 @@
static void setexposure(struct gspca_dev *gspca_dev)
{
struct sd *sd = (struct sd *) gspca_dev;
- u8 val;
+ u16 val;
+
+ if (sd->aec)
+ return;
val = sd->exposure;
- sccb_reg_write(gspca_dev, 0x08, val >> 7);
- sccb_reg_write(gspca_dev, 0x10, val << 1);
+ sccb_reg_write(gspca_dev, 0x08, val >> 8);
+ sccb_reg_write(gspca_dev, 0x10, val & 0xff);
+
}
static void setagc(struct gspca_dev *gspca_dev)