to add support for certain Jeilin dual-mode cameras.

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.

Quoted 4 lines · On Sat, 1 Aug 2009 16:56:06 -0500 (CDT) 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.

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.

Quoted 1 line

- 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?

On Sun, 2 Aug 2009 14:12:28 -0500 (CDT) Theodore Kilgore <[email protected]> wrote:

	[snip]

Quoted 9 lines

Quoted 1 line

- 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?

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

Quoted 16 lines

On Sun, 2 Aug 2009 14:12:28 -0500 (CDT) Theodore Kilgore <[email protected]> wrote:

	[snip]

Quoted 9 lines

Quoted 1 line

- 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]

Quoted 8 lines

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

(quoted diff) +0 -0

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

Quoted 3 lines

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

Quoted 8 lines

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

Quoted 1 line

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.

Quoted 6 lines

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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Em Mon, 3 Aug 2009 11:01:56 -0500 (CDT)
Theodore Kilgore <[email protected]> escreveu:

Quoted 24 lines · Jean-Francois Moine

Quoted 23 lines

On Sun, 2 Aug 2009 14:12:28 -0500 (CDT) Theodore Kilgore <[email protected]> wrote:

	[snip]

Quoted 9 lines

Quoted 1 line

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

Quoted 9 lines

Quoted 1 line

with the vend:prod ID, she will know which driver she has to generate

So, from this the general advice to users is

(quoted diff) +0 -0

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

Quoted 3 lines

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

Quoted 11 lines

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.

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 3 lines

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?

Quoted 11 lines

I think that the webcam names should be only

Quoted 1 line

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.

Quoted 9 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 26 lines

Quoted 1 line

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:

Quoted 6 lines

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

Quoted 6 lines

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.

Quoted 53 lines

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;

Quoted 6 lines

  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.

As I said already:

Quoted 8 lines

Quoted 8 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

On Mon, 3 Aug 2009, Theodore Kilgore wrote:

Quoted 9 lines

Quoted 9 lines

 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},
         {}

};

Quoted 33 lines

Quoted 33 lines

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)
{

Quoted 4 lines

Quoted 4 lines

...
int board_nr = id->driver_info;

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

Quoted 6 lines

Quoted 5 lines

...
}

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

Quoted 1 line

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

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 4 lines · On Sat, 1 Aug 2009 16:56:06 -0500 (CDT) 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.

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.

Signature · 6 lines

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

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

(quoted diff) +0 -0

@@ -239,6 +239,9 @@

 pac7311                093a:2629       Genious iSlim 300 pac7311                093a:262a       Webcam 300k pac7311                093a:262c       Philips SPC 230 NC

linux/Documentation/video4linux/gspca.txt +3 -0

+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

(quoted diff) +0 -0

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

linux/drivers/media/video/gspca/Kconfig +4 -0

+
+         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

(quoted diff) +0 -0

@@ -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

(quoted diff) +0 -0

@@ -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

linux/drivers/media/video/gspca/jeilinj.c Sat Aug 01 15:42:02 2009 renamed from dev/null   Thu Jan 01 00:00:00 1970 +0000 +139 -1

-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 <[email protected]>");
+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;

linux/drivers/media/video/gspca/jeilinj.c Sat Aug 01 15:42:02 2009 renamed from dev/null   Thu Jan 01 00:00:00 1970 +0000 +14 -0

+}
+
+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;

linux/drivers/media/video/gspca/jeilinj.c Sat Aug 01 15:42:02 2009 renamed from dev/null   Thu Jan 01 00:00:00 1970 +0000 +3 -0

+}
+
+/* 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.
+ */

linux/drivers/media/video/gspca/jeilinj.c Sat Aug 01 15:42:02 2009 renamed from dev/null   Thu Jan 01 00:00:00 1970 +0000 +7 -0

+
+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;
+       }

Quoted 6 lines

+       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:

linux/drivers/media/video/gspca/jeilinj.c line 262

Quoted 6 lines

linux/drivers/media/video/gspca/jeilinj.c new file +6 -0

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

Quoted 16 lines · On Sun, 2 Aug 2009 17:25:29 +0400 Alexey Klimov <[email protected]> wrote:

Quoted 6 lines

+       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:

Quoted 6 lines

linux/drivers/media/video/gspca/jeilinj.c new file +6 -0

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

Quoted 2 lines

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.

Quoted 16 lines · On Sun, 2 Aug 2009 17:25:29 +0400 Alexey Klimov <[email protected]> wrote:

Quoted 6 lines

+       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:

Quoted 6 lines

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

Quoted 2 lines

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.