Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope

2010-09-05 Thread Andy Walls
On Sun, 2010-09-05 at 10:24 +0200, Hans de Goede wrote:
> Hi,
> 
> p.s. (forgot to mention this in my previous mail)
> 
> On 09/03/2010 03:09 AM, Andy Walls wrote:
> 
> 
> 
> > @@ -447,6 +449,20 @@
> > .set = sd_setcomptarget,
> > .get = sd_getcomptarget,
> > },
> > +   {
> > +   {
> > +#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1)
> > +   .id  = V4L2_CID_LAMPS,
> > +   .type= V4L2_CTRL_TYPE_MENU,
> > +   .name= "Lamps",
> > +   .minimum = 0,
> > +   .maximum = 3,
> > +   .step= 1,
> > +   .default_value = 0,
> > +   },
> > +   .set = sd_setlamps,
> > +   .get = sd_getlamps,
> > +   },
> >   };
> >
> >   static const struct v4l2_pix_format mode[] = {
> 
> We only want this control to be available on the qx3 and not on
> all cpia1 devices,

Yes, I though about that, but couldn't think up a clean way of doing it
in the short amount of time I had available.  I did know that the
control was essentially a NoOp, so I wasn't too concerned at the time. 


>  so you need to add something like the following to
> sd_config:
> 
>   if (!(id->idVendor == 0x0813 && id->idProduct == 0x0001))
>   gspca_dev->ctrl_dis = 1 << LAMPS_IDX;
> 
> Where LAMPS_IDX is a define giving the index of V4L2_CID_LAMPS in the
> sd_ctrls array, see the ov519 gspca driver for example.

Thanks for the pointer, I'll have a look.

Regards,
Andy

> Regards,
> 
> Hans


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope

2010-09-05 Thread Andy Walls
On Sun, 2010-09-05 at 10:03 +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/03/2010 03:09 AM, Andy Walls wrote:
> > # HG changeset patch
> > # User Andy Walls
> > # Date 1283475832 14400
> > # Node ID 0d251a2976b46e11cc817207de191896718b93a3
> > # Parent  a4c762698bcb138982b81cf59e5bc4b7155866a9
> > gspca_cpia1: Add lamp cotrol for Intel Play QX3 microscope
> >
> > From: Andy Walls
> >
> > Add a v4l2 control to get the lamp control code working for the Intel Play
> > QX3 microscope.  My daughter in middle school thought it was cool, and is 
> > now
> > examining the grossest specimens she can find.
> >
> 
> Hehe, cool I'm very happy to hear the cpia1 driver actually being used in a
> "productive" manner, that shows it is worth all the time and effort I've put 
> into
> cleaning up / rewriting old v4l1 drivers :)

Yes, thank you.

Now I have 10x, 60x, and 200x images of pencils, hair, dead skin,
fingernails, and insects.  As soon as my kids figure out the microscope
body still works when removed from the base, I'm sure to have 10x, 60x,
and 200x images of eyes, mouths, ears, and nostrils... :P




> About the patch: first of all thanks. wrt lamps versus lights I'm indifferent.

I don't care much either.  Illuminator appears to be the correct term
for microscopy.  The cpia2 (QX5) and gspca_cpia1 (QX3) drivers should
probably try to match on any user visible terminology such as the string
presented as the name of the control.  The cpia2 driver has presented
the user with a "Lights" control.

I don't know much about photography or film, so I don't know what
artificial light sources that illuminate subject matter are called in
those fields.



> The only thing I've notices is that you've made the controls instand apply. 
> Normally
> controls setting changes when not streaming are just remembered and then 
> applied
> when the stream is initialized.
> 
> However your code sends the lamp settings to the device as soon as they are
> changed, and does not send them on sd_start. The sending as soon as changes
> makes sense. But did you check that this actually works,

After a few minutes of playing with the microscope one realizes that one
must be able to change the illumination on-the-fly.  Trying to get the
best image possible from a microscope is easiest when one can tweak as
much as possible in real-time: lighting, positioning, focus, etc.

Changing the illuminator settings does work during a capture, with no
apparent ill effects.  Then again, one is usually starting from bad
illumination conditions searching for good illumination conditions, so
who cares about glitching a frame that doesn't look good.



>  iow did you play with
> the lamps control while not streaming ? and then tried to stream and see if
> the settings stuck.

Yes they do.  'v4l2-ctl -c lamps=n' was used to manipulate the control,
Cheese was use for streaming.

The lamps will stay in the last state commanded, regardless of when
streaming is started and stopped.


> Also the not sending at sd_start, nor sd_init means that you assume that the
> defaults in the driver (both lamps off) ar the same as of the device as you
> never force that the device <-> driver settings are synced on driver load
> or stream start. This may not be the case when resuming from suspend or
> the driver is rmmod-ed insmod-ed.

1. manual `modprobe -r` followed by `modprobe` is not a case that the
normal end user cares about.

2. when this USB device is unplugged and plugged back in, its lights are
always off and the driver, as you note, always assumes they are off as
well.

3. suspend/resume: well, yes that case probably matters. :)



>  So assuming that the instant apply
> of this control does not cause issues, you should add a call to
> command_setlamps(gspca_dev); at the end of sd_init.

Easy enough.

Regards,
Andy

> Regards,
> 
> Hans
> 
> 
> > Priority: normal
> >
> > Signed-off-by: Andy Walls
> >
> > diff -r a4c762698bcb -r 0d251a2976b4 linux/drivers/media/video/gspca/cpia1.c
> > --- a/linux/drivers/media/video/gspca/cpia1.c   Wed Aug 25 16:13:54 
> > 2010 -0300
> > +++ b/linux/drivers/media/video/gspca/cpia1.c   Thu Sep 02 21:03:52 
> > 2010 -0400
> > @@ -333,8 +333,8 @@
> > } format;
> > struct {/* Intel QX3 specific data */
> > u8 qx3_detected;/* a QX3 is present */
> > -   u8 toplight;/* top light lit , R/W */
> > -   u8 bottomlight; /* bottom light lit, R/W */
> > +   u8 toplamp; /* top lamp lit , R/W */
> > +   u8 bottomlamp;  /* bottom lamp lit, R/W */
> > u8 button;  /* snapshot button pressed (R/O) */
> > u8 cradled; /* microscope is in cradle (R/O) */
> > } qx3;
> > @@ -373,6 +373,8 @@
> >   static int sd_getfreq(struct gspca_dev *gspca_dev, __s32 *val);
> >   static int sd_setcomptarget(struct gspca_dev *gspca_dev, __s32 val);
> >   static int sd_getcomptarget(struct gspca_

Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope

2010-09-05 Thread Hans de Goede

Hi,

p.s. (forgot to mention this in my previous mail)

On 09/03/2010 03:09 AM, Andy Walls wrote:




@@ -447,6 +449,20 @@
.set = sd_setcomptarget,
.get = sd_getcomptarget,
},
+   {
+   {
+#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1)
+   .id  = V4L2_CID_LAMPS,
+   .type= V4L2_CTRL_TYPE_MENU,
+   .name= "Lamps",
+   .minimum = 0,
+   .maximum = 3,
+   .step= 1,
+   .default_value = 0,
+   },
+   .set = sd_setlamps,
+   .get = sd_getlamps,
+   },
  };

  static const struct v4l2_pix_format mode[] = {


We only want this control to be available on the qx3 and not on
all cpia1 devices, so you need to add something like the following to
sd_config:

if (!(id->idVendor == 0x0813 && id->idProduct == 0x0001))
gspca_dev->ctrl_dis = 1 << LAMPS_IDX;

Where LAMPS_IDX is a define giving the index of V4L2_CID_LAMPS in the
sd_ctrls array, see the ov519 gspca driver for example.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope

2010-09-05 Thread Hans de Goede

Hi,

On 09/03/2010 03:09 AM, Andy Walls wrote:

# HG changeset patch
# User Andy Walls
# Date 1283475832 14400
# Node ID 0d251a2976b46e11cc817207de191896718b93a3
# Parent  a4c762698bcb138982b81cf59e5bc4b7155866a9
gspca_cpia1: Add lamp cotrol for Intel Play QX3 microscope

From: Andy Walls

Add a v4l2 control to get the lamp control code working for the Intel Play
QX3 microscope.  My daughter in middle school thought it was cool, and is now
examining the grossest specimens she can find.



Hehe, cool I'm very happy to hear the cpia1 driver actually being used in a
"productive" manner, that shows it is worth all the time and effort I've put 
into
cleaning up / rewriting old v4l1 drivers :)

About the patch: first of all thanks. wrt lamps versus lights I'm indifferent.
The only thing I've notices is that you've made the controls instand apply. 
Normally
controls setting changes when not streaming are just remembered and then applied
when the stream is initialized.

However your code sends the lamp settings to the device as soon as they are
changed, and does not send them on sd_start. The sending as soon as changes
makes sense. But did you check that this actually works, iow did you play with
the lamps control while not streaming ? and then tried to stream and see if
the settings stuck.

Also the not sending at sd_start, nor sd_init means that you assume that the
defaults in the driver (both lamps off) ar the same as of the device as you
never force that the device <-> driver settings are synced on driver load
or stream start. This may not be the case when resuming from suspend or
the driver is rmmod-ed insmod-ed. So assuming that the instant apply
of this control does not cause issues, you should add a call to
command_setlamps(gspca_dev); at the end of sd_init.

Regards,

Hans



Priority: normal

Signed-off-by: Andy Walls

diff -r a4c762698bcb -r 0d251a2976b4 linux/drivers/media/video/gspca/cpia1.c
--- a/linux/drivers/media/video/gspca/cpia1.c   Wed Aug 25 16:13:54 2010 -0300
+++ b/linux/drivers/media/video/gspca/cpia1.c   Thu Sep 02 21:03:52 2010 -0400
@@ -333,8 +333,8 @@
} format;
struct {/* Intel QX3 specific data */
u8 qx3_detected;/* a QX3 is present */
-   u8 toplight;/* top light lit , R/W */
-   u8 bottomlight; /* bottom light lit, R/W */
+   u8 toplamp; /* top lamp lit , R/W */
+   u8 bottomlamp;  /* bottom lamp lit, R/W */
u8 button;  /* snapshot button pressed (R/O) */
u8 cradled; /* microscope is in cradle (R/O) */
} qx3;
@@ -373,6 +373,8 @@
  static int sd_getfreq(struct gspca_dev *gspca_dev, __s32 *val);
  static int sd_setcomptarget(struct gspca_dev *gspca_dev, __s32 val);
  static int sd_getcomptarget(struct gspca_dev *gspca_dev, __s32 *val);
+static int sd_setlamps(struct gspca_dev *gspca_dev, __s32 val);
+static int sd_getlamps(struct gspca_dev *gspca_dev, __s32 *val);

  static const struct ctrl sd_ctrls[] = {
{
@@ -447,6 +449,20 @@
.set = sd_setcomptarget,
.get = sd_getcomptarget,
},
+   {
+   {
+#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1)
+   .id  = V4L2_CID_LAMPS,
+   .type= V4L2_CTRL_TYPE_MENU,
+   .name= "Lamps",
+   .minimum = 0,
+   .maximum = 3,
+   .step= 1,
+   .default_value = 0,
+   },
+   .set = sd_setlamps,
+   .get = sd_getlamps,
+   },
  };

  static const struct v4l2_pix_format mode[] = {
@@ -766,8 +782,8 @@
params->compressionTarget.targetQ = 5; /* From windows driver */

params->qx3.qx3_detected = 0;
-   params->qx3.toplight = 0;
-   params->qx3.bottomlight = 0;
+   params->qx3.toplamp = 0;
+   params->qx3.bottomlamp = 0;
params->qx3.button = 0;
params->qx3.cradled = 0;
  }
@@ -1059,17 +1075,16 @@
  0, sd->params.streamStartLine, 0, 0);
  }

-#if 0 /* Currently unused */ /* keep */
-static int command_setlights(struct gspca_dev *gspca_dev)
+static int command_setlamps(struct gspca_dev *gspca_dev)
  {
struct sd *sd = (struct sd *) gspca_dev;
-   int ret, p1, p2;
+   int ret, p;

if (!sd->params.qx3.qx3_detected)
return 0;

-   p1 = (sd->params.qx3.bottomlight == 0)<<  1;
-   p2 = (sd->params.qx3.toplight == 0)<<  3;
+   p  = (sd->params.qx3.toplamp== 0) ? 0x8 : 0;
+   p |= (sd->params.qx3.bottomlamp == 0) ? 0x2 : 0;

ret = do_command(gspca_dev, CPIA_COMMAND_WriteVCReg,
 0x90, 0x8F, 0x50, 0);
@@ -1077,9 +1092,8 @@
return ret;

return do_command(gspca_dev, CPIA_COMMAND_WriteMCPort, 2, 0,
- p1 

Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope

2010-09-03 Thread Andy Walls
On Fri, 2010-09-03 at 10:38 +0200, Jean-Francois Moine wrote:
> On Thu, 02 Sep 2010 21:09:42 -0400
> Andy Walls  wrote:
>   [snip]
> > Add a v4l2 control to get the lamp control code working for the Intel
> > Play QX3 microscope.  My daughter in middle school thought it was
> > cool, and is now examining the grossest specimens she can find.
>   [snip]
> > -   u8 toplight;/* top light lit , R/W */
> > -   u8 bottomlight; /* bottom light lit, R/W */
> > +   u8 toplamp; /* top lamp lit , R/W */
> > +   u8 bottomlamp;  /* bottom lamp lit, R/W */
>   [snip]
> > +#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1)
>   [snip]
> 
> Hi Andy,
> 
> First, I do not see why you changed the name 'light' to 'lamp' while
> 'light' is used in the other cpia driver (cpia2).

Hi Jean-Francois,

My primary reason was slightly easier maintenance of gspca_cpia1.

A case-insensitive search for "lamp" will find only the code related to
controlling the lamps.  The string "light" is used in at least one other
context in the driver.

Other reasons for using "lamp":

The QX3 actually uses incandescent bulbs with slow turn on circuitry to
meet USB surge limit requirements.  The light sources really are lamps
in that sense, not just lights.

"Illuminator" seems to be the proper microscopy term for the assembly
that provides light from below/behind the specimen, but it is harder to
type than "lamp". ;)   I'm not sure what term applies to light sources
above/afore the specimen.




> Then, you used a private control ID, and linux-media people don't like
> that.

I knew that going in, however:

The gspca_cpia module already uses a private control.

The cpia2 driver uses 7 private controls, one of which is
CPIA2_CID_LIGHTS for controlling the lamps, er, lights. ;)

The cpia2 driver used the same menu labels I did -
"Off", "Top", "Bottom", "Both" - just in a slightly different order.

So the QX5 microscope can use a private control, but the QX3 microscope
can't use a private control?

It seems like there's a strong precedent here, since such an API is
already presented to user-space for controlling a device, the QX5, that
is functionally and physically nearly identical to the QX3.


I don't quite understand the aversion to well written private controls.
A private control under V4L2 appears to mean "not used by another bridge
driver", but what end user cares about that? 

The V4L2 Control API is so well specified, that user space needs no
apriori knowledge of a driver's controls, and yet applications can still
present a GUI for every control useful for a user to manipulate. I put
forward 'v4l2-ctl -L' and 'qv4l2' as examples of how users need not ever
care if a control is private or not.

V4L2 applications that would be capturing video or stills from
microscopes already know how to manipulate V4L2 controls, so why not let
them control the illumination with the same interface.  Why make them
rummage around with some other API?
 
Using V4L2 controls to control the image processing, but prohibiting
applications from using them to control the subject matter environment,
doesn't make a lot of sense from an application writing perspective.

We're tying our hands behind our back for the sake of "API idealism".
So we have a flexible, well specified V4L2 control API and then don't
let people use it, because we haven't figured out how to make lighting
controls common across 200 video capture implementations and a crappy
LED interface exists somewhere else in the kernel?  Does that make sense
to anyone?  What are the benefits?




> As many gspca users are waiting for a light/LED/illuminator/lamp
> control, I tried to define a standard one in March 2009:
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/3095
> 
> A second, but more restrictive, attempt was done by Németh Márton in
> February 2010:
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/16705
> 
> The main objection to that proposals was that the sysfs LED interface
> should be used instead:
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/3114
> 
> A patch in this way was done by Németh Márton in February 2010:
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/16670
> 
> but it was rather complex, and there was no consensus
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/17111

I see I'm preaching to the choir then.

I do not have time, nor desire to play the "bring m a rock" game for the
API for controlling lights - especially when multiple people have been
playing it already.

It's a shame.  The Intel Play QX3 is a nice microscope and is readily
found on eBay.  My daughter absolutely loves it.  However, the unit is
close to useless without control of the lights.

If users are losing out because of inability to agree on an
implementation, then something is wrong with the process.




Re

Re: [PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope

2010-09-03 Thread Jean-Francois Moine
On Thu, 02 Sep 2010 21:09:42 -0400
Andy Walls  wrote:
[snip]
> Add a v4l2 control to get the lamp control code working for the Intel
> Play QX3 microscope.  My daughter in middle school thought it was
> cool, and is now examining the grossest specimens she can find.
[snip]
> - u8 toplight;/* top light lit , R/W */
> - u8 bottomlight; /* bottom light lit, R/W */
> + u8 toplamp; /* top lamp lit , R/W */
> + u8 bottomlamp;  /* bottom lamp lit, R/W */
[snip]
> +#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1)
[snip]

Hi Andy,

First, I do not see why you changed the name 'light' to 'lamp' while
'light' is used in the other cpia driver (cpia2).

Then, you used a private control ID, and linux-media people don't like
that.

As many gspca users are waiting for a light/LED/illuminator/lamp
control, I tried to define a standard one in March 2009:
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/3095

A second, but more restrictive, attempt was done by Németh Márton in
February 2010:
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/16705

The main objection to that proposals was that the sysfs LED interface
should be used instead:
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/3114

A patch in this way was done by Németh Márton in February 2010:
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/16670

but it was rather complex, and there was no consensus
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/17111

So, I don't think that your patch could be accepted...

Best regards.

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] gspca_cpia1: Add lamp control for Intel Play QX3 microscope

2010-09-02 Thread Andy Walls
# HG changeset patch
# User Andy Walls 
# Date 1283475832 14400
# Node ID 0d251a2976b46e11cc817207de191896718b93a3
# Parent  a4c762698bcb138982b81cf59e5bc4b7155866a9
gspca_cpia1: Add lamp cotrol for Intel Play QX3 microscope

From: Andy Walls 

Add a v4l2 control to get the lamp control code working for the Intel Play
QX3 microscope.  My daughter in middle school thought it was cool, and is now
examining the grossest specimens she can find.

Priority: normal

Signed-off-by: Andy Walls 

diff -r a4c762698bcb -r 0d251a2976b4 linux/drivers/media/video/gspca/cpia1.c
--- a/linux/drivers/media/video/gspca/cpia1.c   Wed Aug 25 16:13:54 2010 -0300
+++ b/linux/drivers/media/video/gspca/cpia1.c   Thu Sep 02 21:03:52 2010 -0400
@@ -333,8 +333,8 @@
} format;
struct {/* Intel QX3 specific data */
u8 qx3_detected;/* a QX3 is present */
-   u8 toplight;/* top light lit , R/W */
-   u8 bottomlight; /* bottom light lit, R/W */
+   u8 toplamp; /* top lamp lit , R/W */
+   u8 bottomlamp;  /* bottom lamp lit, R/W */
u8 button;  /* snapshot button pressed (R/O) */
u8 cradled; /* microscope is in cradle (R/O) */
} qx3;
@@ -373,6 +373,8 @@
 static int sd_getfreq(struct gspca_dev *gspca_dev, __s32 *val);
 static int sd_setcomptarget(struct gspca_dev *gspca_dev, __s32 val);
 static int sd_getcomptarget(struct gspca_dev *gspca_dev, __s32 *val);
+static int sd_setlamps(struct gspca_dev *gspca_dev, __s32 val);
+static int sd_getlamps(struct gspca_dev *gspca_dev, __s32 *val);
 
 static const struct ctrl sd_ctrls[] = {
{
@@ -447,6 +449,20 @@
.set = sd_setcomptarget,
.get = sd_getcomptarget,
},
+   {
+   {
+#define V4L2_CID_LAMPS (V4L2_CID_PRIVATE_BASE+1)
+   .id  = V4L2_CID_LAMPS,
+   .type= V4L2_CTRL_TYPE_MENU,
+   .name= "Lamps",
+   .minimum = 0,
+   .maximum = 3,
+   .step= 1,
+   .default_value = 0,
+   },
+   .set = sd_setlamps,
+   .get = sd_getlamps,
+   },
 };
 
 static const struct v4l2_pix_format mode[] = {
@@ -766,8 +782,8 @@
params->compressionTarget.targetQ = 5; /* From windows driver */
 
params->qx3.qx3_detected = 0;
-   params->qx3.toplight = 0;
-   params->qx3.bottomlight = 0;
+   params->qx3.toplamp = 0;
+   params->qx3.bottomlamp = 0;
params->qx3.button = 0;
params->qx3.cradled = 0;
 }
@@ -1059,17 +1075,16 @@
  0, sd->params.streamStartLine, 0, 0);
 }
 
-#if 0 /* Currently unused */ /* keep */
-static int command_setlights(struct gspca_dev *gspca_dev)
+static int command_setlamps(struct gspca_dev *gspca_dev)
 {
struct sd *sd = (struct sd *) gspca_dev;
-   int ret, p1, p2;
+   int ret, p;
 
if (!sd->params.qx3.qx3_detected)
return 0;
 
-   p1 = (sd->params.qx3.bottomlight == 0) << 1;
-   p2 = (sd->params.qx3.toplight == 0) << 3;
+   p  = (sd->params.qx3.toplamp== 0) ? 0x8 : 0;
+   p |= (sd->params.qx3.bottomlamp == 0) ? 0x2 : 0;
 
ret = do_command(gspca_dev, CPIA_COMMAND_WriteVCReg,
 0x90, 0x8F, 0x50, 0);
@@ -1077,9 +1092,8 @@
return ret;
 
return do_command(gspca_dev, CPIA_COMMAND_WriteMCPort, 2, 0,
- p1 | p2 | 0xE0, 0);
+ p | 0xE0, 0);
 }
-#endif
 
 static int set_flicker(struct gspca_dev *gspca_dev, int on, int apply)
 {
@@ -1932,6 +1946,27 @@
return 0;
 }
 
+static int sd_setlamps(struct gspca_dev *gspca_dev, __s32 val)
+{
+   struct sd *sd = (struct sd *) gspca_dev;
+
+   sd->params.qx3.toplamp= (val & 0x2) ? 1 : 0;
+   sd->params.qx3.bottomlamp = (val & 0x1) ? 1 : 0;
+
+   if (sd->params.qx3.qx3_detected)
+   return command_setlamps(gspca_dev);
+
+   return 0;
+}
+
+static int sd_getlamps(struct gspca_dev *gspca_dev, __s32 *val)
+{
+   struct sd *sd = (struct sd *) gspca_dev;
+
+   *val = (sd->params.qx3.toplamp << 1) | (sd->params.qx3.bottomlamp << 0);
+   return 0;
+}
+
 static int sd_querymenu(struct gspca_dev *gspca_dev,
struct v4l2_querymenu *menu)
 {
@@ -1959,6 +1994,22 @@
return 0;
}
break;
+   case V4L2_CID_LAMPS:
+   switch (menu->index) {
+   case 0:
+   strcpy((char *) menu->name, "Off");
+   return 0;
+   case 1:
+   strcpy((char *) menu->name, "Bottom");
+   return 0;
+   case 2:
+   strcpy((char *) menu->name, "Top")