[PATCH] to add support for certain Jeilin dual-mode cameras.

[PATCH] to add support for certain Jeilin dual-mode cameras.

[Theodore Kilgore]

Several cameras are supported here, which all share the same USB 
Vendor:Product number when started up in streaming mode. All of these 
cameras use bulk transport for streaming, and all of them produce frames 
in JPEG format.

Patch follows.

Signed-off-by Theodore Kilgore <kilgota@auburn.edu>

------
diff -r d189fb4be712 linux/Documentation/video4linux/gspca.txt
--- a/linux/Documentation/video4linux/gspca.txt	Wed Jul 29 10:01:54 2009 +0200
+++ b/linux/Documentation/video4linux/gspca.txt	Sat Aug 01 15:42:02 2009 -0500
@@ -239,6 +239,9 @@
  pac7311		093a:2629	Genious iSlim 300
  pac7311		093a:262a	Webcam 300k
  pac7311		093a:262c	Philips SPC 230 NC
+jeilinj		0979:0280	Cobra DMC 300
+jeilinj		0979:0280	FlyCamOne
+jeilinj		0979:0280	Sakar 57379
  zc3xx		0ac8:0302	Z-star Vimicro zc0302
  vc032x		0ac8:0321	Vimicro generic vc0321
  vc032x		0ac8:0323	Vimicro Vc0323
diff -r d189fb4be712 linux/drivers/media/video/gspca/Kconfig
--- a/linux/drivers/media/video/gspca/Kconfig	Wed Jul 29 10:01:54 2009 +0200
+++ b/linux/drivers/media/video/gspca/Kconfig	Sat Aug 01 15:42:02 2009 -0500
@@ -47,6 +47,15 @@
  	  To compile this driver as a module, choose M here: the
  	  module will be called gspca_finepix.

+config USB_GSPCA_JEILINJ
+	tristate "Jeilin JPEG USB V4L2 driver"
+	depends on VIDEO_V4L2 && USB_GSPCA
+	help
+	  Say Y here if you want support for cameras based on this Jeilin chip.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gspca_jeilinj.
+
  config USB_GSPCA_MARS
  	tristate "Mars USB Camera Driver"
  	depends on VIDEO_V4L2 && USB_GSPCA
diff -r d189fb4be712 linux/drivers/media/video/gspca/Makefile
--- a/linux/drivers/media/video/gspca/Makefile	Wed Jul 29 10:01:54 2009 +0200
+++ b/linux/drivers/media/video/gspca/Makefile	Sat Aug 01 15:42:02 2009 -0500
@@ -2,6 +2,7 @@
  obj-$(CONFIG_USB_GSPCA_CONEX)    += gspca_conex.o
  obj-$(CONFIG_USB_GSPCA_ETOMS)    += gspca_etoms.o
  obj-$(CONFIG_USB_GSPCA_FINEPIX)  += gspca_finepix.o
+obj-$(CONFIG_USB_GSPCA_JEILINJ)  += gspca_jeilinj.o
  obj-$(CONFIG_USB_GSPCA_MARS)     += gspca_mars.o
  obj-$(CONFIG_USB_GSPCA_MR97310A) += gspca_mr97310a.o
  obj-$(CONFIG_USB_GSPCA_OV519)    += gspca_ov519.o
@@ -30,6 +31,7 @@
  gspca_conex-objs    := conex.o
  gspca_etoms-objs    := etoms.o
  gspca_finepix-objs  := finepix.o
+gspca_jeilinj-objs  := jeilinj.o
  gspca_mars-objs     := mars.o
  gspca_mr97310a-objs := mr97310a.o
  gspca_ov519-objs    := ov519.o
diff -r d189fb4be712 linux/drivers/media/video/gspca/jeilinj.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/video/gspca/jeilinj.c	Sat Aug 01 15:42:02 2009 -0500
@@ -0,0 +1,387 @@
+/*
+ * Jeilinj subdriver
+ *
+ * Supports some Jeilin dual-mode cameras which use bulk transport and
+ * download raw JPEG data.
+ *
+ * Copyright (C) 2009 Theodore Kilgore
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#define MODULE_NAME "jeilinj"
+
+#include <linux/workqueue.h>
+#include "gspca.h"
+#include "jpeg.h"
+
+MODULE_AUTHOR("Theodore Kilgore <kilgota@auburn.edu>");
+MODULE_DESCRIPTION("GSPCA/JEILINJ USB Camera Driver");
+MODULE_LICENSE("GPL");
+
+/* Default timeouts, in ms */
+#define JEILINJ_CMD_TIMEOUT 500
+#define JEILINJ_DATA_TIMEOUT 1000
+
+/* Maximum transfer size to use. */
+#define JEILINJ_MAX_TRANSFER 0x200
+
+#define FRAME_HEADER_LEN 0x10
+
+/* Structure to hold all of our device specific stuff */
+struct sd {
+	struct gspca_dev gspca_dev;	/* !! must be the first item */
+	const struct v4l2_pix_format *cap_mode;
+	/* Driver stuff */
+	struct work_struct work_struct;
+	struct workqueue_struct *work_thread;
+	u8 quality;				 /* image quality */
+	u8 jpegqual;				/* webcam quality */
+	u8 *jpeg_hdr;
+};
+
+	struct jlj_command {
+		unsigned char instruction[2];
+		unsigned char ack_wanted;
+	};
+
+/* AFAICT these cameras will only do 320x240. */
+static struct v4l2_pix_format jlj_mode[] = {
+	{ 320, 240, V4L2_PIX_FMT_JPEG, V4L2_FIELD_NONE,
+		.bytesperline = 320,
+		.sizeimage = 320 * 240,
+		.colorspace = V4L2_COLORSPACE_JPEG,
+		.priv = 0}
+};
+
+/*
+ * cam uses endpoint 0x03 to send commands, 0x84 for read commands,
+ * and 0x82 for bulk transfer.
+ */
+
+/* All commands are two bytes only */
+static int jlj_write2(struct gspca_dev *gspca_dev, unsigned char *command)
+{
+	int retval;
+
+	memcpy(gspca_dev->usb_buf, command, 2);
+	retval = usb_bulk_msg(gspca_dev->dev,
+			usb_sndbulkpipe(gspca_dev->dev, 3),
+			gspca_dev->usb_buf, 2, NULL, 500);
+	if (retval < 0)
+		PDEBUG(D_ERR, "command write [%02x] error %d",
+				gspca_dev->usb_buf[0], retval);
+	return retval;
+}
+
+/* Responses are one byte only */
+static int jlj_read1(struct gspca_dev *gspca_dev, unsigned char response)
+{
+	int retval;
+
+	retval = usb_bulk_msg(gspca_dev->dev,
+	usb_rcvbulkpipe(gspca_dev->dev, 0x84),
+				gspca_dev->usb_buf, 1, NULL, 500);
+	response = gspca_dev->usb_buf[0];
+	if (retval < 0)
+		PDEBUG(D_ERR, "read command [%02x] error %d",
+				gspca_dev->usb_buf[0], retval);
+	return retval;
+}
+
+static int jlj_start(struct gspca_dev *gspca_dev)
+{
+	int i;
+	int retval = -1;
+	u8 response = 0xff;
+	struct jlj_command start_commands[] = {
+		{{0x71, 0x81}, 0},
+		{{0x70, 0x05}, 0},
+		{{0x95, 0x70}, 1},
+		{{0x71, 0x81}, 0},
+		{{0x70, 0x04}, 0},
+		{{0x95, 0x70}, 1},
+		{{0x71, 0x00}, 0},
+		{{0x70, 0x08}, 0},
+		{{0x95, 0x70}, 1},
+		{{0x94, 0x02}, 0},
+		{{0xde, 0x24}, 0},
+		{{0x94, 0x02}, 0},
+		{{0xdd, 0xf0}, 0},
+		{{0x94, 0x02}, 0},
+		{{0xe3, 0x2c}, 0},
+		{{0x94, 0x02}, 0},
+		{{0xe4, 0x00}, 0},
+		{{0x94, 0x02}, 0},
+		{{0xe5, 0x00}, 0},
+		{{0x94, 0x02}, 0},
+		{{0xe6, 0x2c}, 0},
+		{{0x94, 0x03}, 0},
+		{{0xaa, 0x00}, 0},
+		{{0x71, 0x1e}, 0},
+		{{0x70, 0x06}, 0},
+		{{0x71, 0x80}, 0},
+		{{0x70, 0x07}, 0}
+	};
+	for (i = 0; i < ARRAY_SIZE(start_commands); i++) {
+		retval = jlj_write2(gspca_dev, start_commands[i].instruction);
+		if (retval < 0)
+			return retval;
+		if (start_commands[i].ack_wanted)
+			retval = jlj_read1(gspca_dev, response);
+		if (retval < 0)
+			return retval;
+	}
+	PDEBUG(D_ERR, "jlj_start retval is %d", retval);
+	return retval;
+}
+
+static int jlj_stop(struct gspca_dev *gspca_dev)
+{
+	int i;
+	int retval;
+	struct jlj_command stop_commands[] = {
+		{{0x71, 0x00}, 0},
+		{{0x70, 0x09}, 0},
+		{{0x71, 0x80}, 0},
+		{{0x70, 0x05}, 0}
+	};
+	for (i = 0; i < ARRAY_SIZE(stop_commands); i++) {
+		retval = jlj_write2(gspca_dev, stop_commands[i].instruction);
+		if (retval < 0)
+			return retval;
+	}
+	return retval;
+}
+
+/* This function is called as a workqueue function and runs whenever the camera
+ * is streaming data. Because it is a workqueue function it is allowed to sleep
+ * so we can use synchronous USB calls. To avoid possible collisions with other
+ * threads attempting to use the camera's USB interface the gspca usb_lock is
+ * used when performing the one USB control operation inside the workqueue,
+ * which tells the camera to close the stream. In practice the only thing
+ * which needs to be protected against is the usb_set_interface call that
+ * gspca makes during stream_off. Otherwise the camera doesn't provide any
+ * controls that the user could try to change.
+ */
+
+static void jlj_dostream(struct work_struct *work)
+{
+	struct sd *dev = container_of(work, struct sd, work_struct);
+	struct gspca_dev *gspca_dev = &dev->gspca_dev;
+	struct gspca_frame *frame;
+	int blocks_left; /* 0x200-sized blocks remaining in current frame. */
+	int size_in_blocks;
+	int act_len;
+	int discarding = 0; /* true if we failed to get space for frame. */
+	int packet_type;
+	int ret;
+	u8 *buffer;
+
+	buffer = kmalloc(JEILINJ_MAX_TRANSFER, GFP_KERNEL | GFP_DMA);
+	if (!buffer) {
+		PDEBUG(D_ERR, "Couldn't allocate USB buffer");
+		goto quit_stream;
+	}
+	while (gspca_dev->present && gspca_dev->streaming) {
+		if (!gspca_dev->present)
+			goto quit_stream;
+		/* Start a new frame, and add the JPEG header, first thing */
+		frame = gspca_get_i_frame(gspca_dev);
+		if (frame && !discarding)
+			gspca_frame_add(gspca_dev, FIRST_PACKET, frame,
+					dev->jpeg_hdr, JPEG_HDR_SZ);
+		 else
+			discarding = 1;
+		/*
+		 * Now request data block 0. Line 0 reports the size
+		 * to download, in blocks of size 0x200, and also tells the
+		 * "actual" data size, in bytes, which seems best to ignore.
+		 */
+		ret = usb_bulk_msg(gspca_dev->dev,
+				usb_rcvbulkpipe(gspca_dev->dev, 0x82),
+				buffer, JEILINJ_MAX_TRANSFER, &act_len,
+				JEILINJ_DATA_TIMEOUT);
+		PDEBUG(D_STREAM,
+			"Got %d bytes out of %d for Block 0",
+			act_len, JEILINJ_MAX_TRANSFER);
+		if (ret < 0 || act_len < FRAME_HEADER_LEN)
+			goto quit_stream;
+		size_in_blocks = buffer[0x0a];
+		blocks_left = buffer[0x0a] - 1;
+		PDEBUG(D_STREAM, "blocks_left = 0x%x", blocks_left);
+		packet_type = INTER_PACKET;
+		if (frame && !discarding)
+			/* Toss line 0 of data block 0, keep the rest. */
+			gspca_frame_add(gspca_dev, packet_type,
+				frame, buffer + FRAME_HEADER_LEN,
+				JEILINJ_MAX_TRANSFER - FRAME_HEADER_LEN);
+			else
+				discarding = 1;
+		while (blocks_left > 0) {
+			if (!gspca_dev->present)
+				goto quit_stream;
+			ret = usb_bulk_msg(gspca_dev->dev,
+				usb_rcvbulkpipe(gspca_dev->dev, 0x82),
+				buffer, JEILINJ_MAX_TRANSFER, &act_len,
+				JEILINJ_DATA_TIMEOUT);
+			if (ret < 0 || act_len < JEILINJ_MAX_TRANSFER)
+				goto quit_stream;
+			PDEBUG(D_STREAM,
+				"%d blocks remaining for frame", blocks_left);
+			blocks_left -= 1;
+			if (blocks_left == 0)
+				packet_type = LAST_PACKET;
+			else
+				packet_type = INTER_PACKET;
+			if (frame && !discarding)
+				gspca_frame_add(gspca_dev, packet_type,
+						frame, buffer,
+						JEILINJ_MAX_TRANSFER);
+			else
+				discarding = 1;
+		}
+	}
+quit_stream:
+	mutex_lock(&gspca_dev->usb_lock);
+	if (gspca_dev->present)
+		jlj_stop(gspca_dev);
+	mutex_unlock(&gspca_dev->usb_lock);
+	kfree(buffer);
+}
+
+/* This function is called at probe time just before sd_init */
+static int sd_config(struct gspca_dev *gspca_dev,
+		const struct usb_device_id *id)
+{
+	struct cam *cam = &gspca_dev->cam;
+	struct sd *dev  = (struct sd *) gspca_dev;
+
+	dev->quality  = 85;
+	dev->jpegqual = 85;
+	PDEBUG(D_PROBE,
+		"JEILINJ camera detected"
+		" (vid/pid 0x%04X:0x%04X)", id->idVendor, id->idProduct);
+	cam->cam_mode = jlj_mode;
+	cam->nmodes = 1;
+	cam->bulk = 1;
+	/* We don't use the buffer gspca allocates so make it small. */
+	cam->bulk_size = 32;
+	INIT_WORK(&dev->work_struct, jlj_dostream);
+	return 0;
+}
+
+/* called on streamoff with alt==0 and on disconnect */
+/* the usb_lock is held at entry - restore on exit */
+static void sd_stop0(struct gspca_dev *gspca_dev)
+{
+	struct sd *dev = (struct sd *) gspca_dev;
+
+	/* wait for the work queue to terminate */
+	mutex_unlock(&gspca_dev->usb_lock);
+	/* This waits for jlj_dostream to finish */
+	destroy_workqueue(dev->work_thread);
+	dev->work_thread = NULL;
+	mutex_lock(&gspca_dev->usb_lock);
+}
+
+/* this function is called at probe and resume time */
+static int sd_init(struct gspca_dev *gspca_dev)
+{
+	return 0;
+}
+
+/* Set up for getting frames. */
+static int sd_start(struct gspca_dev *gspca_dev)
+{
+	struct sd *dev = (struct sd *) gspca_dev;
+	int ret;
+
+	/* create the JPEG header */
+	dev->jpeg_hdr = kmalloc(JPEG_HDR_SZ, GFP_KERNEL);
+	jpeg_define(dev->jpeg_hdr, gspca_dev->height, gspca_dev->width,
+			0x21);          /* JPEG 422 */
+	jpeg_set_qual(dev->jpeg_hdr, dev->quality);
+	PDEBUG(D_STREAM, "Start streaming at 320x240");
+	ret = jlj_start(gspca_dev);
+	if (ret < 0) {
+		PDEBUG(D_ERR, "Start streaming command failed");
+		return ret;
+	}
+	/* Start the workqueue function to do the streaming */
+	dev->work_thread = create_singlethread_workqueue(MODULE_NAME);
+	queue_work(dev->work_thread, &dev->work_struct);
+
+	return 0;
+}
+
+/* Table of supported USB devices */
+static const __devinitdata struct usb_device_id device_table[] = {
+	{USB_DEVICE(0x0979, 0x0280)},
+	{}
+};
+
+MODULE_DEVICE_TABLE(usb, device_table);
+
+/* sub-driver description */
+static const struct sd_desc sd_desc = {
+	.name   = MODULE_NAME,
+	.config = sd_config,
+	.init   = sd_init,
+	.start  = sd_start,
+	.stop0  = sd_stop0,
+};
+
+/* -- device connect -- */
+static int sd_probe(struct usb_interface *intf,
+		const struct usb_device_id *id)
+{
+	return gspca_dev_probe(intf, id,
+			&sd_desc,
+			sizeof(struct sd),
+			THIS_MODULE);
+}
+
+static struct usb_driver sd_driver = {
+	.name       = MODULE_NAME,
+	.id_table   = device_table,
+	.probe      = sd_probe,
+	.disconnect = gspca_disconnect,
+#ifdef CONFIG_PM
+	.suspend = gspca_suspend,
+	.resume  = gspca_resume,
+#endif
+};
+
+/* -- module insert / remove -- */
+static int __init sd_mod_init(void)
+{
+	int ret;
+
+	ret = usb_register(&sd_driver);
+	if (ret < 0)
+		return ret;
+	PDEBUG(D_PROBE, "registered");
+	return 0;
+}
+
+static void __exit sd_mod_exit(void)
+{
+	usb_deregister(&sd_driver);
+	PDEBUG(D_PROBE, "deregistered");
+}
+
+module_init(sd_mod_init);
+module_exit(sd_mod_exit);

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Jean-Francois Moine]

On Sat, 1 Aug 2009 16:56:06 -0500 (CDT)
Theodore Kilgore <kilgota@banach.math.auburn.edu> wrote:

> Several cameras are supported here, which all share the same USB 
> Vendor:Product number when started up in streaming mode. All of these 
> cameras use bulk transport for streaming, and all of them produce
> frames in JPEG format.

Hi Theodore,

Your patch seems ok, but:

- there is no kfree(sd->jpeg_hdr). Should be in stop0().

- as there is only one vend:prod, one line is enough in gspca.txt.

May you fix this and resend?

Thanks.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Theodore Kilgore]

On Sun, 2 Aug 2009, Jean-Francois Moine wrote:

> On Sat, 1 Aug 2009 16:56:06 -0500 (CDT)
> Theodore Kilgore <kilgota@banach.math.auburn.edu> wrote:
>
>> Several cameras are supported here, which all share the same USB
>> Vendor:Product number when started up in streaming mode. All of these
>> cameras use bulk transport for streaming, and all of them produce
>> frames in JPEG format.
>
> Hi Theodore,
>
> Your patch seems ok, but:
>
> - there is no kfree(sd->jpeg_hdr). Should be in stop0().

OK. Will do, and resend, as asked. But before I do that, there is the 
second item. It seems to raise questions of policy. I think it is 
appropriate to raise that question for general discussion.

>
> - as there is only one vend:prod, one line is enough in gspca.txt.

This is a question about which I have been curious for quite some time, 
and I think that now is a good time to ask it.

Just what policy do we have about this? The information which links brand 
and model to driver ought to be presented somewhere. If it does not go 
into gspca.txt then where exactly is the appropriate place to put said 
information?

As to these three particular cameras, perhaps one should take into account 
here the fact that what we actually have here is three "different" 
cameras, put on the market by three different vendors, and in at least two 
different parts of the world. The Sakar camera and the Cobra camera are 
for sale in the US. The FlyOne camera is for sale in Germany and perhaps 
in some other parts of Europe. Furthermore, in stillcam mode they are all 
three of them standard mass storage devices, but they have different IDs 
from what is listed here (good for them about that!) and the stillcam IDs 
are distinct, too. So in other words they are not the same camera, at all, 
in spite of the fact that they all use the same ID when set up in webcam 
mode.

A more general consideration is that the buyer of a camera is not helped 
at all by knowing that a particular chipset combination is supported. No. 
The buyer can only go by the make and model which are printed on the 
outside of the plastic bubble pack. So what exactly are we, the 
developers, to do in order to make the relevant information available to 
the public? There is some wiki, of course. But it seems to me the wiki 
itself ought to refer to the gspca.txt file, among other things.

I do think that one of the easy ways to address the above issue of helping 
the camera purchaser would be to agree that gspca.txt ought to contain the 
information for each individual camera which is supported. This would make 
the file longer, but it is in my opinion a small price to pay, when the 
goal is to have complete information, put in a central place.

In going over the gspca.txt file, I also see that the jeilinj entries are 
the only place where there is a "duplicate" entry, so that I am at this 
time in non-conformity. But, on checking, I also see that the SQ905 
cameras (2770:9120) and the SQ905C cameras (2770:905c) are not listed at 
all in gspca.txt. This could be claimed as an example of my ignorance that 
I am supposed to put something there. But it is also related to the fact 
that

-- there are twenty-five (25) cameras listed in 
libgphoto2/camlibs/sq905/library.c, which were reported to me as working 
with that particular stillcam driver. This list does not include several 
other cameras that I only heard about vaguely, or that I do not even know 
about at all because nobody ever reported them to me. So if I am supposed 
to pick just one of these, then which one do I list in gspca.txt, when as 
far as I know they all ought to work with the gspca sq905 driver?

and, as for the SQ905C cameras

-- there are seventeen (17) cameras listed in 
libgphoto2/camlibs/digigr8/library.c, which were reported to me as working 
with that particular stillcam driver. This list does not include several 
other cameras that I only heard about vaguely, or that I do not even know 
about at all because nobody ever reported them to me. If I am supposed to 
pick just one of these, then which one do I list in gspca.txt, when as far 
as I know they all ought to work with the gspca sq905c driver?

For the above reasons, I have up to this point not listed any of these 
cameras in gspca.txt at all, as supported. Probably that is not the right 
thing to do, either?

Now, in addition to the above, there is another problem which will hit 
very soon, probably soon after the time that Thomas Kaiser gets back from 
his vacation and starts to work again. This is the matter of the mr97310a 
driver, which is almost finished now. What we have there is a list of 
cameras which are all functionally identical as still cameras, but as 
webcams the functionality differs in minor details. Here is what happens:

08ca:0111	Aiptek Pencam VGA+	(supported, listed in gspca.txt)

093a:010f 	several cameras, some functionally identical to the Aiptek 
camera, and some which as webcams require a different init procedure and 
use different control procedures! There are two different basic cameras 
with the same USB number, and it has been necessary to create undocumented 
commands to be able to tell which kind of camera we have! This has been 
done, at this point. But as to documentation the question arises, that if 
we choose just one of these to list in gspca.txt, should we pick one of 
"Type 1" or one of "Type 2" to list?

093a:010e	several cameras with this ID number, which also fall into 
two disjoint sets when run in webcam mode, again requiring the development 
of previously undocumented commands to be able to detect and classify 
which camera it is, before initializing it. Again, which one of these 
cameras is one supposed to list in gspca.txt?

Also the above account of the unexpected complications of the mr97310a 
driver raises the natural problem that the same thing might happen yet 
again. The differences in behavior of the different cameras seem to be 
connected to such external characteristics as the type of LCD display used 
on the outside of the camera, or other minor details in the construction. 
Who is to say that there can not be a "Type 3" in either of the two cases 
where two types have already been discovered? What is the safe thing to 
do? Well, it seems to me, the only thing appropriate is to list explicitly 
all the mr97310a cameras currently supported, and in the future to list 
explicitly any other one which is reported as working.

As I said, this question of what to do with the mr97310a documentation 
will come up very soon, unless one merely avoids the question by refusing 
to make a choice. So the question of documentation becomes rather serious.

I hope that the occasion is now appropriate one deciding this issue of 
documentation. My opinion is that gspca.txt ought to list individually 
each camera which is known to work, as well as the driver used and the USB 
ID.

Theodore Kilgore

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Jean-Francois Moine]

On Sun, 2 Aug 2009 14:12:28 -0500 (CDT)
Theodore Kilgore <kilgota@banach.math.auburn.edu> wrote:

	[snip]
> > - as there is only one vend:prod, one line is enough in gspca.txt.  
> 
> This is a question about which I have been curious for quite some
> time, and I think that now is a good time to ask it.
> 
> Just what policy do we have about this? The information which links
> brand and model to driver ought to be presented somewhere. If it does
> not go into gspca.txt then where exactly is the appropriate place to
> put said information?
	[snip]

Hi Theodore,

gspca.txt has been defined only to know which subdriver has to be
generated for a webcam that a user already owns.

The trade name of the webcams are often not clear enough (look at all
the Creative varieties). So, the user has just to plug her webcam and
with the vend:prod ID, she will know which driver she has to generate
(you may say that there are already tools which do the job, as easycam,
but I do not think they are often used).

The device list of the other drivers (CARDLIST.xx) are not sorted and
their format (numbered list) does not facilitate this job. So, I
prefered a list sorted by vend:prod.

In gspca.txt, the 3rd column contains the webcam names. As you can see,
it is a comma separated list, so, you may put here all the names you may
know. But, is it useful? I think that the webcam names should be only
in the file usb.ids which comes with the usbutils.

To go further, there should be a general file which should contain all
the usb (and pci) devices and their associated drivers. This
information exists in /lib/modules/`uname -r`/modules.usbmap when all
drivers are generated. So, we just need a tool (and a guy!) to maintain
this general file...

Best regards.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Theodore Kilgore]

On Mon, 3 Aug 2009, Jean-Francois Moine wrote:

> On Sun, 2 Aug 2009 14:12:28 -0500 (CDT)
> Theodore Kilgore <kilgota@banach.math.auburn.edu> wrote:
>
> 	[snip]
>>> - as there is only one vend:prod, one line is enough in gspca.txt.
>>
>> This is a question about which I have been curious for quite some
>> time, and I think that now is a good time to ask it.
>>
>> Just what policy do we have about this? The information which links
>> brand and model to driver ought to be presented somewhere. If it does
>> not go into gspca.txt then where exactly is the appropriate place to
>> put said information?
> 	[snip]
>
> Hi Theodore,
>
> gspca.txt has been defined only to know which subdriver has to be
> generated for a webcam that a user already owns.
>
> The trade name of the webcams are often not clear enough (look at all
> the Creative varieties). So, the user has just to plug her webcam and
> with the vend:prod ID, she will know which driver she has to generate

So, from this the general advice to users is

 	1. Buy a camera
 	2. Plug it in to see if it has a driver?

> (you may say that there are already tools which do the job, as easycam,
> but I do not think they are often used).

The problem is, the USB ID (and hence the driver) are not publicized by 
anyone connected to the manufacturer or the retailer. So the result is 
that there is a lot of hardware out there which is is currently unusable 
and nobody knows that, and also lots of hardware out there which is 
perfectly usable because we already supported it, but again nobody knows 
that. The corollary is that the "trade names" you refer to ought to be 
listed _somewhere_ as completely, clearly, and precisely as possible. To 
me, and obviously good place to start with that would be to list somewhere 
in the kernel documentation the devices supported by a given driver or 
module.


>
> The device list of the other drivers (CARDLIST.xx) are not sorted and
> their format (numbered list) does not facilitate this job. So, I
> prefered a list sorted by vend:prod.
>
> In gspca.txt, the 3rd column contains the webcam names. As you can see,
> it is a comma separated list, so, you may put here all the names you may
> know. But, is it useful?

I would certainly hope so. Otherwise, why bother to put any names in there 
at all?


I think that the webcam names should be only
> in the file usb.ids which comes with the usbutils.

That file is hopelessly out of date, by definition, and occasionally 
seems to me inaccurate in such details as ownership of USB ID X by 
company Y, for which, for example, see the association of 0x2770 (S&Q 
Technologies) to the Japanese camera and electronics packager and 
retailer NHL.

More relevant to the present discussion, though, is that even if the 
usb.ids file were completely up to date it serves an entirely different 
purpose from what we are discussing here. My understanding is that the 
usb.ids file is supposed to be nothing but an inventory of the devices 
that we know about. It was never intended as a list of supported devices 
and by its nature can not serve that purpose simultaneously. Also, a lot 
of USB devices come under the category of "who cares what the Vendor and 
Product number are?" such as standard mass storage pocket flash drives. So 
I suspect that no one is conscientious about listing them.

>
> To go further, there should be a general file which should contain all
> the usb (and pci) devices and their associated drivers. This
> information exists in /lib/modules/`uname -r`/modules.usbmap when all
> drivers are generated. So, we just need a tool (and a guy!) to maintain
> this general file...

Hmm. Yes. The "guy." Well, it is better to figure out a way to make 
things like that happen automatically, and then one does not need to worry 
so much about the "guy." I will mention how Gphoto handles this problem, 
for comparison. It might not be so easy to carry out here, because what 
finally comes is the command option gphoto2 --list-cameras. That command 
will print out a (very long) list of all the currently nsupported cameras, 
by name. At this point, the list has over 1000 entries. It is up to the 
writer of the camera library to list the cameras which are supported by 
that particular piece of software. So the entry instead of just looking 
like this

/* Table of supported USB devices */
static const __devinitdata struct usb_device_id device_table[] = {
         {USB_DEVICE(0x2770, 0x905c)},
         {USB_DEVICE(0x2770, 0x9050)},
         {USB_DEVICE(0x2770, 0x913d)},
         {}
};

must contain two other additional fields, describing the name in 
human-readable form (intended to identify the camera to the extent that 
one can probably pick it out from others on the shelf) and the current 
status of the support for the camera. Like this:

{"Sakar Micro Digital 2428x", GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x905c},
{"Jazz JDC9",           GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x905c},
{"Disney pix micro",    GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x9050},
{"Disney pix micro 2",  GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x9052},
{"Suprema Digital Keychain Camera", GP_DRIVER_STATUS_EXPERIMENTAL,
                                                        0x2770, 0x913d}

If something similar were done here, then perhaps it would be possible to 
provide a tool which would print out two lists. One of them could be a 
list of the known and supported devices by gspca, say. The second option 
could provide a list of the devices which the local kernel supports.

The need to rely on "the guy" is greatly reduced if such procedures are 
put into place. Then it is the responsibility of the person who writes the 
module to produce the information in the first place, and it is one of the 
items on the checklist at submission time. It is also very easy for 
someone else to add to the iist later on, if the occasion arises.

Perhaps someone else can come up with a better idea?

Theodore Kilgore

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Mauro Carvalho Chehab]

Em Mon, 3 Aug 2009 11:01:56 -0500 (CDT)
Theodore Kilgore <kilgota@banach.math.auburn.edu> escreveu:

> On Mon, 3 Aug 2009, Jean-Francois Moine wrote:
> 
> > On Sun, 2 Aug 2009 14:12:28 -0500 (CDT)
> > Theodore Kilgore <kilgota@banach.math.auburn.edu> wrote:
> >
> > 	[snip]
> >>> - as there is only one vend:prod, one line is enough in gspca.txt.
> >>
> >> This is a question about which I have been curious for quite some
> >> time, and I think that now is a good time to ask it.
> >>
> >> Just what policy do we have about this? The information which links
> >> brand and model to driver ought to be presented somewhere. If it does
> >> not go into gspca.txt then where exactly is the appropriate place to
> >> put said information?
> > 	[snip]
> >
> > Hi Theodore,
> >
> > gspca.txt has been defined only to know which subdriver has to be
> > generated for a webcam that a user already owns.
> >
> > The trade name of the webcams are often not clear enough (look at all
> > the Creative varieties). So, the user has just to plug her webcam and
> > with the vend:prod ID, she will know which driver she has to generate
> 
> So, from this the general advice to users is
> 
>  	1. Buy a camera
>  	2. Plug it in to see if it has a driver?
> 
> > (you may say that there are already tools which do the job, as easycam,
> > but I do not think they are often used).
> 
> The problem is, the USB ID (and hence the driver) are not publicized by 
> anyone connected to the manufacturer or the retailer. So the result is 
> that there is a lot of hardware out there which is is currently unusable 
> and nobody knows that, and also lots of hardware out there which is 
> perfectly usable because we already supported it, but again nobody knows 
> that. The corollary is that the "trade names" you refer to ought to be 
> listed _somewhere_ as completely, clearly, and precisely as possible. To 
> me, and obviously good place to start with that would be to list somewhere 
> in the kernel documentation the devices supported by a given driver or 
> module.
> 
> 
> >
> > The device list of the other drivers (CARDLIST.xx) are not sorted and
> > their format (numbered list) does not facilitate this job. So, I
> > prefered a list sorted by vend:prod.
> >
> > In gspca.txt, the 3rd column contains the webcam names. As you can see,
> > it is a comma separated list, so, you may put here all the names you may
> > know. But, is it useful?
> 
> I would certainly hope so. Otherwise, why bother to put any names in there 
> at all?
> 
> 
> I think that the webcam names should be only
> > in the file usb.ids which comes with the usbutils.
> 
> That file is hopelessly out of date, by definition, and occasionally 
> seems to me inaccurate in such details as ownership of USB ID X by 
> company Y, for which, for example, see the association of 0x2770 (S&Q 
> Technologies) to the Japanese camera and electronics packager and 
> retailer NHL.
> 
> More relevant to the present discussion, though, is that even if the 
> usb.ids file were completely up to date it serves an entirely different 
> purpose from what we are discussing here. My understanding is that the 
> usb.ids file is supposed to be nothing but an inventory of the devices 
> that we know about. It was never intended as a list of supported devices 
> and by its nature can not serve that purpose simultaneously. Also, a lot 
> of USB devices come under the category of "who cares what the Vendor and 
> Product number are?" such as standard mass storage pocket flash drives. So 
> I suspect that no one is conscientious about listing them.
> 
> >
> > To go further, there should be a general file which should contain all
> > the usb (and pci) devices and their associated drivers. This
> > information exists in /lib/modules/`uname -r`/modules.usbmap when all
> > drivers are generated. So, we just need a tool (and a guy!) to maintain
> > this general file...
> 
> Hmm. Yes. The "guy." Well, it is better to figure out a way to make 
> things like that happen automatically, and then one does not need to worry 
> so much about the "guy." I will mention how Gphoto handles this problem, 
> for comparison. It might not be so easy to carry out here, because what 
> finally comes is the command option gphoto2 --list-cameras. That command 
> will print out a (very long) list of all the currently nsupported cameras, 
> by name. At this point, the list has over 1000 entries. It is up to the 
> writer of the camera library to list the cameras which are supported by 
> that particular piece of software. So the entry instead of just looking 
> like this
> 
> /* Table of supported USB devices */
> static const __devinitdata struct usb_device_id device_table[] = {
>          {USB_DEVICE(0x2770, 0x905c)},
>          {USB_DEVICE(0x2770, 0x9050)},
>          {USB_DEVICE(0x2770, 0x913d)},
>          {}
> };
> 
> must contain two other additional fields, describing the name in 
> human-readable form (intended to identify the camera to the extent that 
> one can probably pick it out from others on the shelf) and the current 
> status of the support for the camera. Like this:
> 
> {"Sakar Micro Digital 2428x", GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x905c},
> {"Jazz JDC9",           GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x905c},
> {"Disney pix micro",    GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x9050},
> {"Disney pix micro 2",  GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x9052},
> {"Suprema Digital Keychain Camera", GP_DRIVER_STATUS_EXPERIMENTAL,
>                                                         0x2770, 0x913d}

The struct usb_device_id doesn't have any thing like that, and creating another
table repeating the USB ID is very ugly (yet, a few drivers do it). This also
means to add 4 extra bytes per camera, just to store a duplicated information.

The better way is to do something like:

enum {
	SAKAR_MICRO_DIGITAL_2428X,
	DISNEY_PIX_MICRO,
	DISNEY_PIX_MICRO2,
	SUPREMA_DIGITAL_KEYCHAIN_CAMERA,
};

static const __devinitdata struct usb_device_id device_table[] = {
        {USB_DEVICE(0x2770, 0x905c), .driver_info = SAKAR_MICRO_DIGITAL_2428X},
        {USB_DEVICE(0x2770, 0x9050), .driver_info = DISNEY_PIX_MICRO},
	{USB_DEVICE(0x2770, 0x9052), .driver_info = DISNEY_PIX_MICRO2},
        {USB_DEVICE(0x2770, 0x913d), .driver_info = SUPREMA_DIGITAL_KEYCHAIN_CAMERA},
         {}
};

struct camera_description {
	char *name;
	unsigned int flags;
};

static struct camera_description cameras[] = {
	[SAKAR_MICRO_DIGITAL_2428X] = {
		.name = "Sakar Micro Digital 2428x/Jazz JDC9",
		.flags = GP_DRIVER_STATUS_EXPERIMENTAL,
	},
	[DISNEY_PIX_MICRO] = {
		.name = "Disney pix micro",
		.flags = GP_DRIVER_STATUS_EXPERIMENTAL,
	},
	[DISNEY_PIX_MICRO2] = {
		.name = "Disney pix micro 2",
		.flags = GP_DRIVER_STATUS_EXPERIMENTAL,
	},
	[SUPREMA_DIGITAL_KEYCHAIN_CAMERA] = {
		.name = "Suprema Digital Keychain Camera",
		.flags = GP_DRIVER_STATUS_EXPERIMENTAL,
	},
};

The association between the two tables can easily be done at the .config ops:

/* this function is called at probe time */
static int sd_config(struct gspca_dev *gspca_dev,
                        const struct usb_device_id *id)
{

...
	int board_nr = id->driver_info;

	printf(KERN_WARN "Detected camera %s\n", cameras[board_nr]);

...
}

A small change at cx88.pl will be enough to auto-generate gspca.txt.

> 
> If something similar were done here, then perhaps it would be possible to 
> provide a tool which would print out two lists. One of them could be a 
> list of the known and supported devices by gspca, say. The second option 
> could provide a list of the devices which the local kernel supports.
> 
> The need to rely on "the guy" is greatly reduced if such procedures are 
> put into place. Then it is the responsibility of the person who writes the 
> module to produce the information in the first place, and it is one of the 
> items on the checklist at submission time. It is also very easy for 
> someone else to add to the iist later on, if the occasion arises.
> 
> Perhaps someone else can come up with a better idea?
> 
> Theodore Kilgore
> --
> 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




Cheers,
Mauro

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Theodore Kilgore]

Mauro,

Thank you for some very interesting comments. I will have some responses 
"in-line," also expanding on some of the previous points.



On Mon, 3 Aug 2009, Mauro Carvalho Chehab wrote:

> Em Mon, 3 Aug 2009 11:01:56 -0500 (CDT)
> Theodore Kilgore <kilgota@banach.math.auburn.edu> escreveu:
>
>> On Mon, 3 Aug 2009, Jean-Francois Moine wrote:
>>
>>> On Sun, 2 Aug 2009 14:12:28 -0500 (CDT)
>>> Theodore Kilgore <kilgota@banach.math.auburn.edu> wrote:
>>>
>>> 	[snip]
>>>>> - as there is only one vend:prod, one line is enough in gspca.txt.
>>>>
>>>> This is a question about which I have been curious for quite some
>>>> time, and I think that now is a good time to ask it.
>>>>
>>>> Just what policy do we have about this? The information which links
>>>> brand and model to driver ought to be presented somewhere. If it does
>>>> not go into gspca.txt then where exactly is the appropriate place to
>>>> put said information?
>>> 	[snip]
>>>
>>> Hi Theodore,
>>>
>>> gspca.txt has been defined only to know which subdriver has to be
>>> generated for a webcam that a user already owns.
>>>
>>> The trade name of the webcams are often not clear enough (look at all
>>> the Creative varieties).

My opinion here is that in the interests of general usability the wecam 
names should be as clear and unambiguous as possible, and most ideally the 
trade name, if enough of it is quoted, is "clear enough." Unfortunately, 
there is another problem I did not think of, here, which is an argument 
against myself. It is that quite often a "manufacturer" switches the chip 
inside of a device marked with a certain trade name and does not tell us 
about it. This has happened with several cameras that I myself have 
supported. But I think we still have to do our best.

So, the user has just to plug her webcam and
>>> with the vend:prod ID, she will know which driver she has to generate
>>
>> So, from this the general advice to users is
>>
>>  	1. Buy a camera
>>  	2. Plug it in to see if it has a driver?
>>
>>> (you may say that there are already tools which do the job, as easycam,
>>> but I do not think they are often used).

I did not understand this comment. Where would "tools which do the job" 
come by the information, anyway? So that is why I had no response to it.

>>
>> The problem is, the USB ID (and hence the driver) are not publicized by
>> anyone connected to the manufacturer or the retailer. So the result is
>> that there is a lot of hardware out there which is is currently unusable
>> and nobody knows that, and also lots of hardware out there which is
>> perfectly usable because we already supported it, but again nobody knows
>> that. The corollary is that the "trade names" you refer to ought to be
>> listed _somewhere_ as completely, clearly, and precisely as possible. To
>> me, and obviously good place to start with that would be to list somewhere
>> in the kernel documentation the devices supported by a given driver or
>> module.

The discussion which comes below makes me suspect (again) that the best 
way to maintain a list of the supported cameras, which includes the trade 
names of them, would be to place that information in the Documentation 
directory, rather than to dirty up the code by making it do the listing.

>>
>>
>>>
>>> The device list of the other drivers (CARDLIST.xx) are not sorted and
>>> their format (numbered list) does not facilitate this job. So, I
>>> prefered a list sorted by vend:prod.
>>>
>>> In gspca.txt, the 3rd column contains the webcam names. As you can see,
>>> it is a comma separated list, so, you may put here all the names you may
>>> know. But, is it useful?
>>
>> I would certainly hope so. Otherwise, why bother to put any names in there
>> at all?

I certainly stand by this statement. It is not logical to put in one name 
willy-nilly and not to put in a dozen or so others. Moreover, I do not 
claim it has happened, but if it has happened it should not have happened, 
that a name is put there but is incomplete, making it not possible 
actually to identify the device.

Nonetheless, I do agree that a comma-separated list could get 
quite long. But I think this is the only feasible approach in the long 
run.

>>
>>
>> I think that the webcam names should be only
>>> in the file usb.ids which comes with the usbutils.
>>
>> That file is hopelessly out of date, by definition, and occasionally
>> seems to me inaccurate in such details as ownership of USB ID X by
>> company Y, for which, for example, see the association of 0x2770 (S&Q
>> Technologies) to the Japanese camera and electronics packager and
>> retailer NHL.
>>
>> More relevant to the present discussion, though, is that even if the
>> usb.ids file were completely up to date it serves an entirely different
>> purpose from what we are discussing here. My understanding is that the
>> usb.ids file is supposed to be nothing but an inventory of the devices
>> that we know about. It was never intended as a list of supported devices
>> and by its nature can not serve that purpose simultaneously. Also, a lot
>> of USB devices come under the category of "who cares what the Vendor and
>> Product number are?" such as standard mass storage pocket flash drives. So
>> I suspect that no one is conscientious about listing them.

Again, the usb.ids file seems to me to serve entirely another purpose. It 
lists those IDs which are known, reported, and entered into said file. 
This is important, but it is not the same thing at all as listing 
_supported_ devices. On the contrary, it has historically listed 
unsupported devices, too.

Also somehow connected with the usb.ids file, as I understand, is the 
website at qbik.ch which lists all devices reported to it, along with the 
status of their support. I have an account there and have reported a 
number of devices. There is a problem there, though. It is the problem 
that you can not go there and edit an entry unless you yourself have 
created that entry. There are several devices on that site marked with a 
big red X (will not work on Linux) for which I myself later on provided 
the support. I can not change the big red X. I can go and leave a comment 
which someone can read if curious enough to go past the big red X and 
click on "show" and I can try to contact the original poster of the report 
about the device. But the original poster may have disappeared from the 
face of the earth, and mail to the OP gets bounced, and nevertheless one 
can not change the original entry. Only the OP can do that. So here is a 
problem, which in its fundamentals is the problem of "the guy." Who is the 
"guy"? I have tried to find out, and I still do not know. So one thing I 
think we all agree about is that it is really not a good idea to rely on 
"the guy."

>>
>>>
>>> To go further, there should be a general file which should contain all
>>> the usb (and pci) devices and their associated drivers. This
>>> information exists in /lib/modules/`uname -r`/modules.usbmap when all
>>> drivers are generated.

Come again? If the information was never there in the first place, how 
does it now magically get generated. To take the most recent example of 
the jeilinj driver, I strongly suspect that it can not list all three 
(known) supported cameras by name. Nor, perhaps, should it. If mixed into 
module code, such gory details could probably be classified as crud.


So, we just need a tool (and a guy!) to maintain
>>> this general file...
>>
>> Hmm. Yes. The "guy." Well, it is better to figure out a way to make
>> things like that happen automatically, and then one does not need to worry
>> so much about the "guy." I will mention how Gphoto handles this problem,
>> for comparison. It might not be so easy to carry out here, because what
>> finally comes is the command option gphoto2 --list-cameras. That command
>> will print out a (very long) list of all the currently nsupported cameras,
>> by name. At this point, the list has over 1000 entries. It is up to the
>> writer of the camera library to list the cameras which are supported by
>> that particular piece of software. So the entry instead of just looking
>> like this
>>
>> /* Table of supported USB devices */
>> static const __devinitdata struct usb_device_id device_table[] = {
>>          {USB_DEVICE(0x2770, 0x905c)},
>>          {USB_DEVICE(0x2770, 0x9050)},
>>          {USB_DEVICE(0x2770, 0x913d)},
>>          {}
>> };
>>
>> must contain two other additional fields, describing the name in
>> human-readable form (intended to identify the camera to the extent that
>> one can probably pick it out from others on the shelf) and the current
>> status of the support for the camera. Like this:
>>
>> {"Sakar Micro Digital 2428x", GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x905c},
>> {"Jazz JDC9",           GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x905c},
>> {"Disney pix micro",    GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x9050},
>> {"Disney pix micro 2",  GP_DRIVER_STATUS_EXPERIMENTAL, 0x2770, 0x9052},
>> {"Suprema Digital Keychain Camera", GP_DRIVER_STATUS_EXPERIMENTAL,
>>                                                         0x2770, 0x913d}
>
> The struct usb_device_id doesn't have any thing like that, and creating another
> table repeating the USB ID is very ugly (yet, a few drivers do it). This also
> means to add 4 extra bytes per camera, just to store a duplicated information.
>
> The better way is to do something like:
>
> enum {
> 	SAKAR_MICRO_DIGITAL_2428X,
> 	DISNEY_PIX_MICRO,
> 	DISNEY_PIX_MICRO2,
> 	SUPREMA_DIGITAL_KEYCHAIN_CAMERA,
> };
>
> static const __devinitdata struct usb_device_id device_table[] = {
>        {USB_DEVICE(0x2770, 0x905c), .driver_info = SAKAR_MICRO_DIGITAL_2428X},
>        {USB_DEVICE(0x2770, 0x9050), .driver_info = DISNEY_PIX_MICRO},
> 	{USB_DEVICE(0x2770, 0x9052), .driver_info = DISNEY_PIX_MICRO2},
>        {USB_DEVICE(0x2770, 0x913d), .driver_info = SUPREMA_DIGITAL_KEYCHAIN_CAMERA},
>         {}
> };
>
> struct camera_description {
> 	char *name;
> 	unsigned int flags;
> };
>
> static struct camera_description cameras[] = {
> 	[SAKAR_MICRO_DIGITAL_2428X] = {
> 		.name = "Sakar Micro Digital 2428x/Jazz JDC9",
> 		.flags = GP_DRIVER_STATUS_EXPERIMENTAL,
> 	},
> 	[DISNEY_PIX_MICRO] = {
> 		.name = "Disney pix micro",
> 		.flags = GP_DRIVER_STATUS_EXPERIMENTAL,
> 	},
> 	[DISNEY_PIX_MICRO2] = {
> 		.name = "Disney pix micro 2",
> 		.flags = GP_DRIVER_STATUS_EXPERIMENTAL,
> 	},
> 	[SUPREMA_DIGITAL_KEYCHAIN_CAMERA] = {
> 		.name = "Suprema Digital Keychain Camera",
> 		.flags = GP_DRIVER_STATUS_EXPERIMENTAL,
> 	},
> };
>
> The association between the two tables can easily be done at the .config ops:
>
> /* this function is called at probe time */
> static int sd_config(struct gspca_dev *gspca_dev,
>                        const struct usb_device_id *id)
> {
>
> ...
> 	int board_nr = id->driver_info;
>
> 	printf(KERN_WARN "Detected camera %s\n", cameras[board_nr]);
>
> ...
> }
>
> A small change at cx88.pl will be enough to auto-generate gspca.txt.

Mauro, this is impressive. First, you point out that Gphoto could have 
done the job more efficiently, and it seems to me that you are right. Then 
you point out that to make such a change would break compatibility with 
the Linux kernel's struct usb_device_id so it cannot be done that way. 
Then you provide a workaround. It is very clever.

However, another side of me says that all of this workaround would have to 
get compiled into the code for every gspca driver, making the code for 
each driver, and the resulting binary output, too, in turn longer and more 
complicated. Therefore, the excursion ends up convincing me that in the 
first place the documentation about which devices are supported, listed by 
trade name as well as USB ID, really ought to be in some kind of place 
like gspca.txt. It is after all much easier to edit a text file than it is 
to write code, and the contents of the text file do not contribute to the 
growth of size of the resulting binary. And it would be at least as easy, 
at least from this point forward, to place the responsibility on the 
author/maintainer of a gspca_* module to provide such a list as part of 
the procedure for submitting code and patches. Then a perl script could, 
for example, parse that one file and put the entries into alphabetical 
order, or any other reasonable and desired order.

<snip>

As I said already:

>>
>> The need to rely on "the guy" is greatly reduced if such procedures are
>> put into place. Then it is the responsibility of the person who writes the
>> module to produce the information in the first place, and it is one of the
>> items on the checklist at submission time. It is also very easy for
>> someone else to add to the iist later on, if the occasion arises.

So this is my suggestion. I think the proper place for the information is 
a text file, and one already exists which is along those lines but is 
incomplete. The way to make sure the information gets into the file is 
then to make that step to be part of the patch procedure for adding 
drivers.

Theodore Kilgore

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Mauro Carvalho Chehab]

On Mon, 3 Aug 2009, Theodore Kilgore wrote:

>>
>>  enum {
>>   SAKAR_MICRO_DIGITAL_2428X,
>>   DISNEY_PIX_MICRO,
>>   DISNEY_PIX_MICRO2,
>>   SUPREMA_DIGITAL_KEYCHAIN_CAMERA,
>>  };
>>
>>  static const __devinitdata struct usb_device_id device_table[] = {
>>         {USB_DEVICE(0x2770, 0x905c), .driver_info =
>>         SAKAR_MICRO_DIGITAL_2428X},
>>         {USB_DEVICE(0x2770, 0x9050), .driver_info = DISNEY_PIX_MICRO},
>>   {USB_DEVICE(0x2770, 0x9052), .driver_info = DISNEY_PIX_MICRO2},
>>         {USB_DEVICE(0x2770, 0x913d), .driver_info =
>>         SUPREMA_DIGITAL_KEYCHAIN_CAMERA},
>>          {}
>>  };
>>
>>  struct camera_description {
>>   char *name;
>>   unsigned int flags;
>>  };
>>
>>  static struct camera_description cameras[] = {
>>   [SAKAR_MICRO_DIGITAL_2428X] = {
>>    .name = "Sakar Micro Digital 2428x/Jazz JDC9",
>>    .flags = GP_DRIVER_STATUS_EXPERIMENTAL,
>>   },
>>   [DISNEY_PIX_MICRO] = {
>>    .name = "Disney pix micro",
>>    .flags = GP_DRIVER_STATUS_EXPERIMENTAL,
>>   },
>>   [DISNEY_PIX_MICRO2] = {
>>    .name = "Disney pix micro 2",
>>    .flags = GP_DRIVER_STATUS_EXPERIMENTAL,
>>   },
>>   [SUPREMA_DIGITAL_KEYCHAIN_CAMERA] = {
>>    .name = "Suprema Digital Keychain Camera",
>>    .flags = GP_DRIVER_STATUS_EXPERIMENTAL,
>>  	},
>>  };
>>
>>  The association between the two tables can easily be done at the .config
>>  ops:
>>
>>  /* this function is called at probe time */
>>  static int sd_config(struct gspca_dev *gspca_dev,
>>                         const struct usb_device_id *id)
>>  {
>>
>>  ...
>>   int board_nr = id->driver_info;
>>
>>   printf(KERN_WARN "Detected camera %s\n", cameras[board_nr]);
>>
>>  ...
>>  }
>>
>>  A small change at cx88.pl will be enough to auto-generate gspca.txt.
>
> Mauro, this is impressive. First, you point out that Gphoto could have done 
> the job more efficiently, and it seems to me that you are right. Then you 
> point out that to make such a change would break compatibility with the Linux 
> kernel's struct usb_device_id so it cannot be done that way. Then you provide 
> a workaround. It is very clever.

This kind of trick is used already on several other drivers: cx88, bttv, 
em28xx, usbvision, saa7134, ...

> However, another side of me says that all of this workaround would have to 
> get compiled into the code for every gspca driver, making the code for each 
> driver, and the resulting binary output, too, in turn longer and more 
> complicated.

Longer: yes. It will generate a longer data segment with the names of all 
supported webcams.

However, This won't increase the complextiy of the driver. As I showed, 
you can know what device you have by just looking at id.driver_info. Also, 
as this standard on other drivers, you aren't adding any uncommon weird 
behavior.

Yet, this change would be a big patch, since several drivers already use 
device_info for something else. The patch would be trivial though, since 
we just need to move the current driver_info information to something 
inside the webcam struct.

> Therefore, the excursion ends up convincing me that in the first 
> place the documentation about which devices are supported, listed by trade 
> name as well as USB ID, really ought to be in some kind of place like 
> gspca.txt. It is after all much easier to edit a text file than it is to 
> write code, and the contents of the text file do not contribute to the growth 
> of size of the resulting binary. And it would be at least as easy, at least 
> from this point forward, to place the responsibility on the author/maintainer 
> of a gspca_* module to provide such a list as part of the procedure for 
> submitting code and patches. Then a perl script could, for example, parse 
> that one file and put the entries into alphabetical order, or any other 
> reasonable and desired order.

This will only work fine if there are some sanity check script to validate 
if all USB ID's are present at gspca.txt. Otherwise, we'll always have the 
risk of not having it properly updated.

There is another alternative: add a comment before each new board at 
gspca's USB ID's entries. Something like:

static const __devinitdata struct usb_device_id device_table[] = {
 	/* Webcam: Sakar Micro Digital 2428x / Jazz JDC9 */
 	{USB_DEVICE(0x2770, 0x905c)},
 	/* Webcam: Disney pix micro */
 	{USB_DEVICE(0x2770, 0x9050)},
 	/* Webcam: Suprema Digital Keychain Camera */
 	{USB_DEVICE(0x2770, 0x913d)},
 	{}
};

This could be easily parseable by a script that would generate the 
gspca.txt. Also, the patch for it can be generated by some script.

> As I said already:
>
>> > 
>> >  The need to rely on "the guy" is greatly reduced if such procedures are
>> >  put into place. Then it is the responsibility of the person who writes 
>> >  the
>> >  module to produce the information in the first place, and it is one of 
>> >  the
>> >  items on the checklist at submission time. It is also very easy for
>> >  someone else to add to the iist later on, if the occasion arises.
>
> So this is my suggestion. I think the proper place for the information is a 
> text file, and one already exists which is along those lines but is 
> incomplete. The way to make sure the information gets into the file is then 
> to make that step to be part of the patch procedure for adding drivers.
>
> Theodore Kilgore
> --
> 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
>
>

-- 
Cheers,
Mauro Carvalho Chehab
http://linuxtv.org
mchehab@infradead.org

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Theodore Kilgore]

On Mon, 3 Aug 2009, Mauro Carvalho Chehab wrote:

> On Mon, 3 Aug 2009, Theodore Kilgore wrote:

<snip>

>>>  static const __devinitdata struct usb_device_id device_table[] = {
>>>         {USB_DEVICE(0x2770, 0x905c), .driver_info =
>>>         SAKAR_MICRO_DIGITAL_2428X},
>>>         {USB_DEVICE(0x2770, 0x9050), .driver_info = DISNEY_PIX_MICRO},
>>>   {USB_DEVICE(0x2770, 0x9052), .driver_info = DISNEY_PIX_MICRO2},
>>>         {USB_DEVICE(0x2770, 0x913d), .driver_info =
>>>         SUPREMA_DIGITAL_KEYCHAIN_CAMERA},
>>>          {}
>>>  };

Incidentally, the above was only a small snippet from the list given in 
libgphoto2/camlibs/digigr8/library.c. The entire list is quite a bit 
longer. As I mentioned in a post yesterday, there are seventeen entries in 
all. That seventeen is a sample of the numbers that one often has to deal 
with. The reason I say that the entire list ought to be available and 
publicized somehow is that people simply would not know that these 
seventeen cameras are all supported in webcam mode (and by the same driver 
module, too!) unless someone informs them.


OK, so here was your first suggestion:

>>>
>>>  struct camera_description {
>>>   char *name;
>>>   unsigned int flags;
>>>  };
>>>
>>>  static struct camera_description cameras[] = {
>>>   [SAKAR_MICRO_DIGITAL_2428X] = {
>>>    .name = "Sakar Micro Digital 2428x/Jazz JDC9",
>>>    .flags = GP_DRIVER_STATUS_EXPERIMENTAL,
>>>   },
>>>   [DISNEY_PIX_MICRO] = {
>>>    .name = "Disney pix micro",
>>>    .flags = GP_DRIVER_STATUS_EXPERIMENTAL,
>>>   },
>>>   [DISNEY_PIX_MICRO2] = {
>>>    .name = "Disney pix micro 2",
>>>    .flags = GP_DRIVER_STATUS_EXPERIMENTAL,
>>>   },
>>>   [SUPREMA_DIGITAL_KEYCHAIN_CAMERA] = {
>>>    .name = "Suprema Digital Keychain Camera",
>>>    .flags = GP_DRIVER_STATUS_EXPERIMENTAL,
>>>  	},
>>>  };
>>>
>>>  The association between the two tables can easily be done at the .config
>>>  ops:
>>>
>>>  /* this function is called at probe time */
>>>  static int sd_config(struct gspca_dev *gspca_dev,
>>>                         const struct usb_device_id *id)
>>>  {
>>>
>>>  ...
>>>   int board_nr = id->driver_info;
>>>
>>>   printf(KERN_WARN "Detected camera %s\n", cameras[board_nr]);

Incidentally, if two cameras have the same USB ID _and_ are otherwise 
functionally identical (these are not necessarily the same thing) then I 
think the physical camera will not always be the same as the detected 
camera. So, while it is always nice to have debug messages, that is not 
otherwise solving very much.

>>>
>>>  ...
>>>  }
>>>
>>>  A small change at cx88.pl will be enough to auto-generate gspca.txt.
>> 
>> Mauro, this is impressive. First, you point out that Gphoto could have done 
>> the job more efficiently, and it seems to me that you are right. Then you 
>> point out that to make such a change would break compatibility with the 
>> Linux kernel's struct usb_device_id so it cannot be done that way. Then you 
>> provide a workaround. It is very clever.
>
> This kind of trick is used already on several other drivers: cx88, bttv, 
> em28xx, usbvision, saa7134, ...
>
>> However, another side of me says that all of this workaround would have to 
>> get compiled into the code for every gspca driver, making the code for each 
>> driver, and the resulting binary output, too, in turn longer and more 
>> complicated.
>
> Longer: yes. It will generate a longer data segment with the names of all 
> supported webcams.


Could it be that some people who are writing code for RTS systems and 
small single-purpose systems might scream about unnecessary bloat?

>
> However, This won't increase the complextiy of the driver. As I showed, you 
> can know what device you have by just looking at id.driver_info. Also, as 
> this standard on other drivers, you aren't adding any uncommon weird 
> behavior.
>
> Yet, this change would be a big patch, since several drivers already use 
> device_info for something else. The patch would be trivial though, since we 
> just need to move the current driver_info information to something inside the 
> webcam struct.

This looks to me like an excellent opportunity to get everyone tied in 
knots at the same time. Gee. That sounds like fun. Perhaps we could get 
"the guy" to do it :) ?

>
>> Therefore, the excursion ends up convincing me that in the first place the 
>> documentation about which devices are supported, listed by trade name as 
>> well as USB ID, really ought to be in some kind of place like gspca.txt. It 
>> is after all much easier to edit a text file than it is to write code, and 
>> the contents of the text file do not contribute to the growth of size of 
>> the resulting binary. And it would be at least as easy, at least from this 
>> point forward, to place the responsibility on the author/maintainer of a 
>> gspca_* module to provide such a list as part of the procedure for 
>> submitting code and patches. Then a perl script could, for example, parse 
>> that one file and put the entries into alphabetical order, or any other 
>> reasonable and desired order.
>
> This will only work fine if there are some sanity check script to validate if 
> all USB ID's are present at gspca.txt. Otherwise, we'll always have the risk 
> of not having it properly updated.

The solution for that is simple. From this day on, anybody who adds a new 
device to any gspca driver provides the entry for gspca.txt or else the 
patch is not accepted without suitable revision. Just, first, it has to be 
decided what kind of thing goes in that file. What is missing is a policy 
and a clear-cut guideline.

>
> There is another alternative: add a comment before each new board at gspca's 
> USB ID's entries. Something like:
>
> static const __devinitdata struct usb_device_id device_table[] = {
> 	/* Webcam: Sakar Micro Digital 2428x / Jazz JDC9 */
> 	{USB_DEVICE(0x2770, 0x905c)},
> 	/* Webcam: Disney pix micro */
> 	{USB_DEVICE(0x2770, 0x9050)},
> 	/* Webcam: Suprema Digital Keychain Camera */
> 	{USB_DEVICE(0x2770, 0x913d)},
> 	{}
> };
>
> This could be easily parseable by a script that would generate the gspca.txt.

Yes. Existing documentation tools certainly can do something like that, 
and in fact already do, on fairly routine basis.

I don't know about you, but I like this much better. It is simpler, 
does not screw with the code, and is in plain text.

But, alas, it still does not resolve the question of exactly what should 
go into gspca.txt, and what format that ought to have.

> Also, the patch for it can be generated by some script.

What I said already. A clear-cut policy and a clear-cut procedure and a 
clear-cut format for the information to go into gspca.txt (or somewhere 
else, if that is more appropriate) are what is missing. The job has to get 
done. Very nice if it is done by a script, too, but that seems to me to be 
quite secondary. Also, to rely totally on a script might make upset anyone 
who has been a "good citizen" and has already listed his/her device in 
gspca.txt in a manner which is acceptable, but now gets asked to put the 
information in a comment in the source file instead.


Theodore Kilgore

Re: [PATCH] to add support for certain Jeilin dual-mode cameras. (resubmit)

[Theodore Kilgore]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 14835 bytes --]



On Sun, 2 Aug 2009, Jean-Francois Moine wrote:

> On Sat, 1 Aug 2009 16:56:06 -0500 (CDT)
> Theodore Kilgore <kilgota@banach.math.auburn.edu> wrote:
>
>> Several cameras are supported here, which all share the same USB
>> Vendor:Product number when started up in streaming mode. All of these
>> cameras use bulk transport for streaming, and all of them produce
>> frames in JPEG format.
>
> Hi Theodore,
>
> Your patch seems ok, but:
>
> - there is no kfree(sd->jpeg_hdr). Should be in stop0().
>
> - as there is only one vend:prod, one line is enough in gspca.txt.
>
> May you fix this and resend?
>
> Thanks.
>
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Jean-Francois,

The version below should meet your objections. The memory for the header 
is freed, and I keep only one entry for the USB ID in gspca.txt. After 
some deliberation, I decided to keep the entry for the Sakar camera 
(belongs to Andy Walls), as that seems to be the one which was most 
recently purchased.

At the same time, I hope very much that the points which I have raised 
in my previous response in regard to documentation can be seriously 
discussed. As I see it, there is a problem about documentation.

Theodore Kilgore

Patch follows.

Signed-off-by: Theodore Kilgore <kilgota@auburn.edu>

----------------
diff -r d189fb4be712 linux/Documentation/video4linux/gspca.txt
--- a/linux/Documentation/video4linux/gspca.txt	Wed Jul 29 10:01:54 2009 +0200
+++ b/linux/Documentation/video4linux/gspca.txt	Sun Aug 02 14:12:25 2009 -0500
@@ -239,6 +239,7 @@
  pac7311		093a:2629	Genious iSlim 300
  pac7311		093a:262a	Webcam 300k
  pac7311		093a:262c	Philips SPC 230 NC
+jeilinj		0979:0280	Sakar 57379
  zc3xx		0ac8:0302	Z-star Vimicro zc0302
  vc032x		0ac8:0321	Vimicro generic vc0321
  vc032x		0ac8:0323	Vimicro Vc0323
diff -r d189fb4be712 linux/drivers/media/video/gspca/Kconfig
--- a/linux/drivers/media/video/gspca/Kconfig	Wed Jul 29 10:01:54 2009 +0200
+++ b/linux/drivers/media/video/gspca/Kconfig	Sun Aug 02 14:12:25 2009 -0500
@@ -47,6 +47,15 @@
  	  To compile this driver as a module, choose M here: the
  	  module will be called gspca_finepix.

+config USB_GSPCA_JEILINJ
+	tristate "Jeilin JPEG USB V4L2 driver"
+	depends on VIDEO_V4L2 && USB_GSPCA
+	help
+	  Say Y here if you want support for cameras based on this Jeilin chip.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gspca_jeilinj.
+
  config USB_GSPCA_MARS
  	tristate "Mars USB Camera Driver"
  	depends on VIDEO_V4L2 && USB_GSPCA
diff -r d189fb4be712 linux/drivers/media/video/gspca/Makefile
--- a/linux/drivers/media/video/gspca/Makefile	Wed Jul 29 10:01:54 2009 +0200
+++ b/linux/drivers/media/video/gspca/Makefile	Sun Aug 02 14:12:25 2009 -0500
@@ -2,6 +2,7 @@
  obj-$(CONFIG_USB_GSPCA_CONEX)    += gspca_conex.o
  obj-$(CONFIG_USB_GSPCA_ETOMS)    += gspca_etoms.o
  obj-$(CONFIG_USB_GSPCA_FINEPIX)  += gspca_finepix.o
+obj-$(CONFIG_USB_GSPCA_JEILINJ)  += gspca_jeilinj.o
  obj-$(CONFIG_USB_GSPCA_MARS)     += gspca_mars.o
  obj-$(CONFIG_USB_GSPCA_MR97310A) += gspca_mr97310a.o
  obj-$(CONFIG_USB_GSPCA_OV519)    += gspca_ov519.o
@@ -30,6 +31,7 @@
  gspca_conex-objs    := conex.o
  gspca_etoms-objs    := etoms.o
  gspca_finepix-objs  := finepix.o
+gspca_jeilinj-objs  := jeilinj.o
  gspca_mars-objs     := mars.o
  gspca_mr97310a-objs := mr97310a.o
  gspca_ov519-objs    := ov519.o
diff -r d189fb4be712 linux/drivers/media/video/gspca/jeilinj.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/video/gspca/jeilinj.c	Sun Aug 02 14:12:25 2009 -0500
@@ -0,0 +1,388 @@
+/*
+ * Jeilinj subdriver
+ *
+ * Supports some Jeilin dual-mode cameras which use bulk transport and
+ * download raw JPEG data.
+ *
+ * Copyright (C) 2009 Theodore Kilgore
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#define MODULE_NAME "jeilinj"
+
+#include <linux/workqueue.h>
+#include "gspca.h"
+#include "jpeg.h"
+
+MODULE_AUTHOR("Theodore Kilgore <kilgota@auburn.edu>");
+MODULE_DESCRIPTION("GSPCA/JEILINJ USB Camera Driver");
+MODULE_LICENSE("GPL");
+
+/* Default timeouts, in ms */
+#define JEILINJ_CMD_TIMEOUT 500
+#define JEILINJ_DATA_TIMEOUT 1000
+
+/* Maximum transfer size to use. */
+#define JEILINJ_MAX_TRANSFER 0x200
+
+#define FRAME_HEADER_LEN 0x10
+
+/* Structure to hold all of our device specific stuff */
+struct sd {
+	struct gspca_dev gspca_dev;	/* !! must be the first item */
+	const struct v4l2_pix_format *cap_mode;
+	/* Driver stuff */
+	struct work_struct work_struct;
+	struct workqueue_struct *work_thread;
+	u8 quality;				 /* image quality */
+	u8 jpegqual;				/* webcam quality */
+	u8 *jpeg_hdr;
+};
+
+	struct jlj_command {
+		unsigned char instruction[2];
+		unsigned char ack_wanted;
+	};
+
+/* AFAICT these cameras will only do 320x240. */
+static struct v4l2_pix_format jlj_mode[] = {
+	{ 320, 240, V4L2_PIX_FMT_JPEG, V4L2_FIELD_NONE,
+		.bytesperline = 320,
+		.sizeimage = 320 * 240,
+		.colorspace = V4L2_COLORSPACE_JPEG,
+		.priv = 0}
+};
+
+/*
+ * cam uses endpoint 0x03 to send commands, 0x84 for read commands,
+ * and 0x82 for bulk transfer.
+ */
+
+/* All commands are two bytes only */
+static int jlj_write2(struct gspca_dev *gspca_dev, unsigned char *command)
+{
+	int retval;
+
+	memcpy(gspca_dev->usb_buf, command, 2);
+	retval = usb_bulk_msg(gspca_dev->dev,
+			usb_sndbulkpipe(gspca_dev->dev, 3),
+			gspca_dev->usb_buf, 2, NULL, 500);
+	if (retval < 0)
+		PDEBUG(D_ERR, "command write [%02x] error %d",
+				gspca_dev->usb_buf[0], retval);
+	return retval;
+}
+
+/* Responses are one byte only */
+static int jlj_read1(struct gspca_dev *gspca_dev, unsigned char response)
+{
+	int retval;
+
+	retval = usb_bulk_msg(gspca_dev->dev,
+	usb_rcvbulkpipe(gspca_dev->dev, 0x84),
+				gspca_dev->usb_buf, 1, NULL, 500);
+	response = gspca_dev->usb_buf[0];
+	if (retval < 0)
+		PDEBUG(D_ERR, "read command [%02x] error %d",
+				gspca_dev->usb_buf[0], retval);
+	return retval;
+}
+
+static int jlj_start(struct gspca_dev *gspca_dev)
+{
+	int i;
+	int retval = -1;
+	u8 response = 0xff;
+	struct jlj_command start_commands[] = {
+		{{0x71, 0x81}, 0},
+		{{0x70, 0x05}, 0},
+		{{0x95, 0x70}, 1},
+		{{0x71, 0x81}, 0},
+		{{0x70, 0x04}, 0},
+		{{0x95, 0x70}, 1},
+		{{0x71, 0x00}, 0},
+		{{0x70, 0x08}, 0},
+		{{0x95, 0x70}, 1},
+		{{0x94, 0x02}, 0},
+		{{0xde, 0x24}, 0},
+		{{0x94, 0x02}, 0},
+		{{0xdd, 0xf0}, 0},
+		{{0x94, 0x02}, 0},
+		{{0xe3, 0x2c}, 0},
+		{{0x94, 0x02}, 0},
+		{{0xe4, 0x00}, 0},
+		{{0x94, 0x02}, 0},
+		{{0xe5, 0x00}, 0},
+		{{0x94, 0x02}, 0},
+		{{0xe6, 0x2c}, 0},
+		{{0x94, 0x03}, 0},
+		{{0xaa, 0x00}, 0},
+		{{0x71, 0x1e}, 0},
+		{{0x70, 0x06}, 0},
+		{{0x71, 0x80}, 0},
+		{{0x70, 0x07}, 0}
+	};
+	for (i = 0; i < ARRAY_SIZE(start_commands); i++) {
+		retval = jlj_write2(gspca_dev, start_commands[i].instruction);
+		if (retval < 0)
+			return retval;
+		if (start_commands[i].ack_wanted)
+			retval = jlj_read1(gspca_dev, response);
+		if (retval < 0)
+			return retval;
+	}
+	PDEBUG(D_ERR, "jlj_start retval is %d", retval);
+	return retval;
+}
+
+static int jlj_stop(struct gspca_dev *gspca_dev)
+{
+	int i;
+	int retval;
+	struct jlj_command stop_commands[] = {
+		{{0x71, 0x00}, 0},
+		{{0x70, 0x09}, 0},
+		{{0x71, 0x80}, 0},
+		{{0x70, 0x05}, 0}
+	};
+	for (i = 0; i < ARRAY_SIZE(stop_commands); i++) {
+		retval = jlj_write2(gspca_dev, stop_commands[i].instruction);
+		if (retval < 0)
+			return retval;
+	}
+	return retval;
+}
+
+/* This function is called as a workqueue function and runs whenever the camera
+ * is streaming data. Because it is a workqueue function it is allowed to sleep
+ * so we can use synchronous USB calls. To avoid possible collisions with other
+ * threads attempting to use the camera's USB interface the gspca usb_lock is
+ * used when performing the one USB control operation inside the workqueue,
+ * which tells the camera to close the stream. In practice the only thing
+ * which needs to be protected against is the usb_set_interface call that
+ * gspca makes during stream_off. Otherwise the camera doesn't provide any
+ * controls that the user could try to change.
+ */
+
+static void jlj_dostream(struct work_struct *work)
+{
+	struct sd *dev = container_of(work, struct sd, work_struct);
+	struct gspca_dev *gspca_dev = &dev->gspca_dev;
+	struct gspca_frame *frame;
+	int blocks_left; /* 0x200-sized blocks remaining in current frame. */
+	int size_in_blocks;
+	int act_len;
+	int discarding = 0; /* true if we failed to get space for frame. */
+	int packet_type;
+	int ret;
+	u8 *buffer;
+
+	buffer = kmalloc(JEILINJ_MAX_TRANSFER, GFP_KERNEL | GFP_DMA);
+	if (!buffer) {
+		PDEBUG(D_ERR, "Couldn't allocate USB buffer");
+		goto quit_stream;
+	}
+	while (gspca_dev->present && gspca_dev->streaming) {
+		if (!gspca_dev->present)
+			goto quit_stream;
+		/* Start a new frame, and add the JPEG header, first thing */
+		frame = gspca_get_i_frame(gspca_dev);
+		if (frame && !discarding)
+			gspca_frame_add(gspca_dev, FIRST_PACKET, frame,
+					dev->jpeg_hdr, JPEG_HDR_SZ);
+		 else
+			discarding = 1;
+		/*
+		 * Now request data block 0. Line 0 reports the size
+		 * to download, in blocks of size 0x200, and also tells the
+		 * "actual" data size, in bytes, which seems best to ignore.
+		 */
+		ret = usb_bulk_msg(gspca_dev->dev,
+				usb_rcvbulkpipe(gspca_dev->dev, 0x82),
+				buffer, JEILINJ_MAX_TRANSFER, &act_len,
+				JEILINJ_DATA_TIMEOUT);
+		PDEBUG(D_STREAM,
+			"Got %d bytes out of %d for Block 0",
+			act_len, JEILINJ_MAX_TRANSFER);
+		if (ret < 0 || act_len < FRAME_HEADER_LEN)
+			goto quit_stream;
+		size_in_blocks = buffer[0x0a];
+		blocks_left = buffer[0x0a] - 1;
+		PDEBUG(D_STREAM, "blocks_left = 0x%x", blocks_left);
+		packet_type = INTER_PACKET;
+		if (frame && !discarding)
+			/* Toss line 0 of data block 0, keep the rest. */
+			gspca_frame_add(gspca_dev, packet_type,
+				frame, buffer + FRAME_HEADER_LEN,
+				JEILINJ_MAX_TRANSFER - FRAME_HEADER_LEN);
+			else
+				discarding = 1;
+		while (blocks_left > 0) {
+			if (!gspca_dev->present)
+				goto quit_stream;
+			ret = usb_bulk_msg(gspca_dev->dev,
+				usb_rcvbulkpipe(gspca_dev->dev, 0x82),
+				buffer, JEILINJ_MAX_TRANSFER, &act_len,
+				JEILINJ_DATA_TIMEOUT);
+			if (ret < 0 || act_len < JEILINJ_MAX_TRANSFER)
+				goto quit_stream;
+			PDEBUG(D_STREAM,
+				"%d blocks remaining for frame", blocks_left);
+			blocks_left -= 1;
+			if (blocks_left == 0)
+				packet_type = LAST_PACKET;
+			else
+				packet_type = INTER_PACKET;
+			if (frame && !discarding)
+				gspca_frame_add(gspca_dev, packet_type,
+						frame, buffer,
+						JEILINJ_MAX_TRANSFER);
+			else
+				discarding = 1;
+		}
+	}
+quit_stream:
+	mutex_lock(&gspca_dev->usb_lock);
+	if (gspca_dev->present)
+		jlj_stop(gspca_dev);
+	mutex_unlock(&gspca_dev->usb_lock);
+	kfree(buffer);
+}
+
+/* This function is called at probe time just before sd_init */
+static int sd_config(struct gspca_dev *gspca_dev,
+		const struct usb_device_id *id)
+{
+	struct cam *cam = &gspca_dev->cam;
+	struct sd *dev  = (struct sd *) gspca_dev;
+
+	dev->quality  = 85;
+	dev->jpegqual = 85;
+	PDEBUG(D_PROBE,
+		"JEILINJ camera detected"
+		" (vid/pid 0x%04X:0x%04X)", id->idVendor, id->idProduct);
+	cam->cam_mode = jlj_mode;
+	cam->nmodes = 1;
+	cam->bulk = 1;
+	/* We don't use the buffer gspca allocates so make it small. */
+	cam->bulk_size = 32;
+	INIT_WORK(&dev->work_struct, jlj_dostream);
+	return 0;
+}
+
+/* called on streamoff with alt==0 and on disconnect */
+/* the usb_lock is held at entry - restore on exit */
+static void sd_stop0(struct gspca_dev *gspca_dev)
+{
+	struct sd *dev = (struct sd *) gspca_dev;
+
+	/* wait for the work queue to terminate */
+	mutex_unlock(&gspca_dev->usb_lock);
+	/* This waits for jlj_dostream to finish */
+	destroy_workqueue(dev->work_thread);
+	dev->work_thread = NULL;
+	mutex_lock(&gspca_dev->usb_lock);
+	kfree(dev->jpeg_hdr);
+}
+
+/* this function is called at probe and resume time */
+static int sd_init(struct gspca_dev *gspca_dev)
+{
+	return 0;
+}
+
+/* Set up for getting frames. */
+static int sd_start(struct gspca_dev *gspca_dev)
+{
+	struct sd *dev = (struct sd *) gspca_dev;
+	int ret;
+
+	/* create the JPEG header */
+	dev->jpeg_hdr = kmalloc(JPEG_HDR_SZ, GFP_KERNEL);
+	jpeg_define(dev->jpeg_hdr, gspca_dev->height, gspca_dev->width,
+			0x21);          /* JPEG 422 */
+	jpeg_set_qual(dev->jpeg_hdr, dev->quality);
+	PDEBUG(D_STREAM, "Start streaming at 320x240");
+	ret = jlj_start(gspca_dev);
+	if (ret < 0) {
+		PDEBUG(D_ERR, "Start streaming command failed");
+		return ret;
+	}
+	/* Start the workqueue function to do the streaming */
+	dev->work_thread = create_singlethread_workqueue(MODULE_NAME);
+	queue_work(dev->work_thread, &dev->work_struct);
+
+	return 0;
+}
+
+/* Table of supported USB devices */
+static const __devinitdata struct usb_device_id device_table[] = {
+	{USB_DEVICE(0x0979, 0x0280)},
+	{}
+};
+
+MODULE_DEVICE_TABLE(usb, device_table);
+
+/* sub-driver description */
+static const struct sd_desc sd_desc = {
+	.name   = MODULE_NAME,
+	.config = sd_config,
+	.init   = sd_init,
+	.start  = sd_start,
+	.stop0  = sd_stop0,
+};
+
+/* -- device connect -- */
+static int sd_probe(struct usb_interface *intf,
+		const struct usb_device_id *id)
+{
+	return gspca_dev_probe(intf, id,
+			&sd_desc,
+			sizeof(struct sd),
+			THIS_MODULE);
+}
+
+static struct usb_driver sd_driver = {
+	.name       = MODULE_NAME,
+	.id_table   = device_table,
+	.probe      = sd_probe,
+	.disconnect = gspca_disconnect,
+#ifdef CONFIG_PM
+	.suspend = gspca_suspend,
+	.resume  = gspca_resume,
+#endif
+};
+
+/* -- module insert / remove -- */
+static int __init sd_mod_init(void)
+{
+	int ret;
+
+	ret = usb_register(&sd_driver);
+	if (ret < 0)
+		return ret;
+	PDEBUG(D_PROBE, "registered");
+	return 0;
+}
+
+static void __exit sd_mod_exit(void)
+{
+	usb_deregister(&sd_driver);
+	PDEBUG(D_PROBE, "deregistered");
+}
+
+module_init(sd_mod_init);
+module_exit(sd_mod_exit);

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Alexey Klimov]

Hello, Theodore

On Sun, Aug 2, 2009 at 1:56 AM, Theodore
Kilgore<kilgota@banach.math.auburn.edu> wrote:
> Several cameras are supported here, which all share the same USB
> Vendor:Product number when started up in streaming mode. All of these
> cameras use bulk transport for streaming, and all of them produce frames in
> JPEG format.
>
> Patch follows.
>
> Signed-off-by Theodore Kilgore <kilgota@auburn.edu>
>
> ------
> diff -r d189fb4be712 linux/Documentation/video4linux/gspca.txt
> --- a/linux/Documentation/video4linux/gspca.txt Wed Jul 29 10:01:54 2009
> +0200
> +++ b/linux/Documentation/video4linux/gspca.txt Sat Aug 01 15:42:02 2009
> -0500
> @@ -239,6 +239,9 @@
>  pac7311                093a:2629       Genious iSlim 300
>  pac7311                093a:262a       Webcam 300k
>  pac7311                093a:262c       Philips SPC 230 NC
> +jeilinj                0979:0280       Cobra DMC 300
> +jeilinj                0979:0280       FlyCamOne
> +jeilinj                0979:0280       Sakar 57379
>  zc3xx          0ac8:0302       Z-star Vimicro zc0302
>  vc032x         0ac8:0321       Vimicro generic vc0321
>  vc032x         0ac8:0323       Vimicro Vc0323
> diff -r d189fb4be712 linux/drivers/media/video/gspca/Kconfig
> --- a/linux/drivers/media/video/gspca/Kconfig   Wed Jul 29 10:01:54 2009
> +0200
> +++ b/linux/drivers/media/video/gspca/Kconfig   Sat Aug 01 15:42:02 2009
> -0500
> @@ -47,6 +47,15 @@
>          To compile this driver as a module, choose M here: the
>          module will be called gspca_finepix.
>
> +config USB_GSPCA_JEILINJ
> +       tristate "Jeilin JPEG USB V4L2 driver"
> +       depends on VIDEO_V4L2 && USB_GSPCA
> +       help
> +         Say Y here if you want support for cameras based on this Jeilin
> chip.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called gspca_jeilinj.
> +
>  config USB_GSPCA_MARS
>        tristate "Mars USB Camera Driver"
>        depends on VIDEO_V4L2 && USB_GSPCA
> diff -r d189fb4be712 linux/drivers/media/video/gspca/Makefile
> --- a/linux/drivers/media/video/gspca/Makefile  Wed Jul 29 10:01:54 2009
> +0200
> +++ b/linux/drivers/media/video/gspca/Makefile  Sat Aug 01 15:42:02 2009
> -0500
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_USB_GSPCA_CONEX)    += gspca_conex.o
>  obj-$(CONFIG_USB_GSPCA_ETOMS)    += gspca_etoms.o
>  obj-$(CONFIG_USB_GSPCA_FINEPIX)  += gspca_finepix.o
> +obj-$(CONFIG_USB_GSPCA_JEILINJ)  += gspca_jeilinj.o
>  obj-$(CONFIG_USB_GSPCA_MARS)     += gspca_mars.o
>  obj-$(CONFIG_USB_GSPCA_MR97310A) += gspca_mr97310a.o
>  obj-$(CONFIG_USB_GSPCA_OV519)    += gspca_ov519.o
> @@ -30,6 +31,7 @@
>  gspca_conex-objs    := conex.o
>  gspca_etoms-objs    := etoms.o
>  gspca_finepix-objs  := finepix.o
> +gspca_jeilinj-objs  := jeilinj.o
>  gspca_mars-objs     := mars.o
>  gspca_mr97310a-objs := mr97310a.o
>  gspca_ov519-objs    := ov519.o
> diff -r d189fb4be712 linux/drivers/media/video/gspca/jeilinj.c
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/drivers/media/video/gspca/jeilinj.c Sat Aug 01 15:42:02 2009
> -0500
> @@ -0,0 +1,387 @@
> +/*
> + * Jeilinj subdriver
> + *
> + * Supports some Jeilin dual-mode cameras which use bulk transport and
> + * download raw JPEG data.
> + *
> + * Copyright (C) 2009 Theodore Kilgore
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#define MODULE_NAME "jeilinj"
> +
> +#include <linux/workqueue.h>
> +#include "gspca.h"
> +#include "jpeg.h"
> +
> +MODULE_AUTHOR("Theodore Kilgore <kilgota@auburn.edu>");
> +MODULE_DESCRIPTION("GSPCA/JEILINJ USB Camera Driver");
> +MODULE_LICENSE("GPL");
> +
> +/* Default timeouts, in ms */
> +#define JEILINJ_CMD_TIMEOUT 500
> +#define JEILINJ_DATA_TIMEOUT 1000
> +
> +/* Maximum transfer size to use. */
> +#define JEILINJ_MAX_TRANSFER 0x200
> +
> +#define FRAME_HEADER_LEN 0x10
> +
> +/* Structure to hold all of our device specific stuff */
> +struct sd {
> +       struct gspca_dev gspca_dev;     /* !! must be the first item */
> +       const struct v4l2_pix_format *cap_mode;
> +       /* Driver stuff */
> +       struct work_struct work_struct;
> +       struct workqueue_struct *work_thread;
> +       u8 quality;                              /* image quality */
> +       u8 jpegqual;                            /* webcam quality */
> +       u8 *jpeg_hdr;
> +};
> +
> +       struct jlj_command {
> +               unsigned char instruction[2];
> +               unsigned char ack_wanted;
> +       };
> +
> +/* AFAICT these cameras will only do 320x240. */
> +static struct v4l2_pix_format jlj_mode[] = {
> +       { 320, 240, V4L2_PIX_FMT_JPEG, V4L2_FIELD_NONE,
> +               .bytesperline = 320,
> +               .sizeimage = 320 * 240,
> +               .colorspace = V4L2_COLORSPACE_JPEG,
> +               .priv = 0}
> +};
> +
> +/*
> + * cam uses endpoint 0x03 to send commands, 0x84 for read commands,
> + * and 0x82 for bulk transfer.
> + */
> +
> +/* All commands are two bytes only */
> +static int jlj_write2(struct gspca_dev *gspca_dev, unsigned char *command)
> +{
> +       int retval;
> +
> +       memcpy(gspca_dev->usb_buf, command, 2);
> +       retval = usb_bulk_msg(gspca_dev->dev,
> +                       usb_sndbulkpipe(gspca_dev->dev, 3),
> +                       gspca_dev->usb_buf, 2, NULL, 500);
> +       if (retval < 0)
> +               PDEBUG(D_ERR, "command write [%02x] error %d",
> +                               gspca_dev->usb_buf[0], retval);
> +       return retval;
> +}
> +
> +/* Responses are one byte only */
> +static int jlj_read1(struct gspca_dev *gspca_dev, unsigned char response)
> +{
> +       int retval;
> +
> +       retval = usb_bulk_msg(gspca_dev->dev,
> +       usb_rcvbulkpipe(gspca_dev->dev, 0x84),
> +                               gspca_dev->usb_buf, 1, NULL, 500);
> +       response = gspca_dev->usb_buf[0];
> +       if (retval < 0)
> +               PDEBUG(D_ERR, "read command [%02x] error %d",
> +                               gspca_dev->usb_buf[0], retval);
> +       return retval;
> +}
> +
> +static int jlj_start(struct gspca_dev *gspca_dev)
> +{
> +       int i;
> +       int retval = -1;
> +       u8 response = 0xff;
> +       struct jlj_command start_commands[] = {
> +               {{0x71, 0x81}, 0},
> +               {{0x70, 0x05}, 0},
> +               {{0x95, 0x70}, 1},
> +               {{0x71, 0x81}, 0},
> +               {{0x70, 0x04}, 0},
> +               {{0x95, 0x70}, 1},
> +               {{0x71, 0x00}, 0},
> +               {{0x70, 0x08}, 0},
> +               {{0x95, 0x70}, 1},
> +               {{0x94, 0x02}, 0},
> +               {{0xde, 0x24}, 0},
> +               {{0x94, 0x02}, 0},
> +               {{0xdd, 0xf0}, 0},
> +               {{0x94, 0x02}, 0},
> +               {{0xe3, 0x2c}, 0},
> +               {{0x94, 0x02}, 0},
> +               {{0xe4, 0x00}, 0},
> +               {{0x94, 0x02}, 0},
> +               {{0xe5, 0x00}, 0},
> +               {{0x94, 0x02}, 0},
> +               {{0xe6, 0x2c}, 0},
> +               {{0x94, 0x03}, 0},
> +               {{0xaa, 0x00}, 0},
> +               {{0x71, 0x1e}, 0},
> +               {{0x70, 0x06}, 0},
> +               {{0x71, 0x80}, 0},
> +               {{0x70, 0x07}, 0}
> +       };
> +       for (i = 0; i < ARRAY_SIZE(start_commands); i++) {
> +               retval = jlj_write2(gspca_dev,
> start_commands[i].instruction);
> +               if (retval < 0)
> +                       return retval;
> +               if (start_commands[i].ack_wanted)
> +                       retval = jlj_read1(gspca_dev, response);
> +               if (retval < 0)
> +                       return retval;
> +       }
> +       PDEBUG(D_ERR, "jlj_start retval is %d", retval);
> +       return retval;
> +}
> +
> +static int jlj_stop(struct gspca_dev *gspca_dev)
> +{
> +       int i;
> +       int retval;
> +       struct jlj_command stop_commands[] = {
> +               {{0x71, 0x00}, 0},
> +               {{0x70, 0x09}, 0},
> +               {{0x71, 0x80}, 0},
> +               {{0x70, 0x05}, 0}
> +       };
> +       for (i = 0; i < ARRAY_SIZE(stop_commands); i++) {
> +               retval = jlj_write2(gspca_dev,
> stop_commands[i].instruction);
> +               if (retval < 0)
> +                       return retval;
> +       }
> +       return retval;
> +}
> +
> +/* This function is called as a workqueue function and runs whenever the
> camera
> + * is streaming data. Because it is a workqueue function it is allowed to
> sleep
> + * so we can use synchronous USB calls. To avoid possible collisions with
> other
> + * threads attempting to use the camera's USB interface the gspca usb_lock
> is
> + * used when performing the one USB control operation inside the workqueue,
> + * which tells the camera to close the stream. In practice the only thing
> + * which needs to be protected against is the usb_set_interface call that
> + * gspca makes during stream_off. Otherwise the camera doesn't provide any
> + * controls that the user could try to change.
> + */
> +
> +static void jlj_dostream(struct work_struct *work)
> +{
> +       struct sd *dev = container_of(work, struct sd, work_struct);
> +       struct gspca_dev *gspca_dev = &dev->gspca_dev;
> +       struct gspca_frame *frame;
> +       int blocks_left; /* 0x200-sized blocks remaining in current frame.
> */
> +       int size_in_blocks;
> +       int act_len;
> +       int discarding = 0; /* true if we failed to get space for frame. */
> +       int packet_type;
> +       int ret;
> +       u8 *buffer;
> +
> +       buffer = kmalloc(JEILINJ_MAX_TRANSFER, GFP_KERNEL | GFP_DMA);
> +       if (!buffer) {
> +               PDEBUG(D_ERR, "Couldn't allocate USB buffer");
> +               goto quit_stream;
> +       }

This clean up on error path looks bad. On quit_stream you have:

> +quit_stream:
> +       mutex_lock(&gspca_dev->usb_lock);
> +       if (gspca_dev->present)
> +               jlj_stop(gspca_dev);
> +       mutex_unlock(&gspca_dev->usb_lock);
> +       kfree(buffer);

kfree() tries to free null buffer after kmalloc for buffer failed.
Please, check if i'm not wrong.


-- 
Best regards, Klimov Alexey

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Jean-Francois Moine]

On Sun, 2 Aug 2009 17:25:29 +0400
Alexey Klimov <klimov.linux@gmail.com> wrote:

> > +       buffer = kmalloc(JEILINJ_MAX_TRANSFER, GFP_KERNEL |
> > GFP_DMA);
> > +       if (!buffer) {
> > +               PDEBUG(D_ERR, "Couldn't allocate USB buffer");
> > +               goto quit_stream;
> > +       }  
> 
> This clean up on error path looks bad. On quit_stream you have:
> 
> > +quit_stream:
> > +       mutex_lock(&gspca_dev->usb_lock);
> > +       if (gspca_dev->present)
> > +               jlj_stop(gspca_dev);
> > +       mutex_unlock(&gspca_dev->usb_lock);
> > +       kfree(buffer);  
> 
> kfree() tries to free null buffer after kmalloc for buffer failed.
> Please, check if i'm not wrong.

Hi Alexey,

AFAIK, kfree() checks the pointer.

Cheers.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Mauro Carvalho Chehab]

Em Mon, 3 Aug 2009 08:30:12 +0200
Jean-Francois Moine <moinejf@free.fr> escreveu:

> On Sun, 2 Aug 2009 17:25:29 +0400
> Alexey Klimov <klimov.linux@gmail.com> wrote:
> 
> > > +       buffer = kmalloc(JEILINJ_MAX_TRANSFER, GFP_KERNEL |
> > > GFP_DMA);
> > > +       if (!buffer) {
> > > +               PDEBUG(D_ERR, "Couldn't allocate USB buffer");
> > > +               goto quit_stream;
> > > +       }  
> > 
> > This clean up on error path looks bad. On quit_stream you have:
> > 
> > > +quit_stream:
> > > +       mutex_lock(&gspca_dev->usb_lock);
> > > +       if (gspca_dev->present)
> > > +               jlj_stop(gspca_dev);
> > > +       mutex_unlock(&gspca_dev->usb_lock);
> > > +       kfree(buffer);  
> > 
> > kfree() tries to free null buffer after kmalloc for buffer failed.
> > Please, check if i'm not wrong.
> 
> Hi Alexey,
> 
> AFAIK, kfree() checks the pointer.

Yeah. Theodore's code is ok. kfree(NULL) is legal.

> 
> Cheers.
> 




Cheers,
Mauro

Re: [PATCH] to add support for certain Jeilin dual-mode cameras.

[Alexey Klimov]

On Mon, Aug 3, 2009 at 10:30 AM, Jean-Francois Moine<moinejf@free.fr> wrote:
> On Sun, 2 Aug 2009 17:25:29 +0400
> Alexey Klimov <klimov.linux@gmail.com> wrote:
>
>> > +       buffer = kmalloc(JEILINJ_MAX_TRANSFER, GFP_KERNEL |
>> > GFP_DMA);
>> > +       if (!buffer) {
>> > +               PDEBUG(D_ERR, "Couldn't allocate USB buffer");
>> > +               goto quit_stream;
>> > +       }
>>
>> This clean up on error path looks bad. On quit_stream you have:
>>
>> > +quit_stream:
>> > +       mutex_lock(&gspca_dev->usb_lock);
>> > +       if (gspca_dev->present)
>> > +               jlj_stop(gspca_dev);
>> > +       mutex_unlock(&gspca_dev->usb_lock);
>> > +       kfree(buffer);
>>
>> kfree() tries to free null buffer after kmalloc for buffer failed.
>> Please, check if i'm not wrong.
>
> Hi Alexey,
>
> AFAIK, kfree() checks the pointer.
>
> Cheers.

Yes, you're right. I checked the code in kfree().
Sorry for doubts.

-- 
Best regards, Klimov Alexey