How to add support for the em2765 webcam Speedlink VAD Laplace to the kernel ?

Hi,

after a break of 2 1/2 kernel releases (sorry, I was busy with another project), I would like to bring up again the question how to add support for this device to the kernel.
See
http://www.mail-archive.com/[email protected]/msg44417.html
("Move em27xx/em28xx webcams to a gspca subdriver ?") for the previous discussion.

Current status is, that I've reverse-engineered the Windows driver and written a new gspca-subdriver for testing, which is feature complete and working stable (will send a patch shortly !).

The device uses an em2765-bridge, so my first idea was of course to modify/extend the em28xx-driver.
But during the reverse-engineering-process, it turned out that writing a new gspca-subdriver was much easier than modifying the em28xx-driver.

The device has the following special characteristics:
- supports only bulk transfers (em28xx driver supports ISOC only)
- uses "proprietary" read/write procedures for the sensor
- uses 16bit eeprom
- em25xx-eeprom with different layout
- sensor OV2640
- different frame processing
- 3 buttons (snapshot, mute, light) which need special treatment (GPIO-polling, status-reseting, ...)

Another important point to mention: you can see from the USB-logs (sensor probing) that there must be at least 3 other webcam devices.

Some pros and cons for both solutions:

em28xx:
+ one driver for all 25xx/26xx/27xx/28xx devices
+ no duplicate code (bridge register defines, bridge read/write fcns)
+ other devices COULD benefit from the new functions/code
- big task/lots of work
- code gets bloated with stuff, which is only needed by a few special devices

gspca:
+ driver already exists (see patch)
+ default driver for webcams
+ much easier to understand and extend
+ same or even less amount of new code lines
+ keeps em28xx-code "simple"
- code duplication
- support for em28xx-webcams spread over to 2 different drivers

I have no strong opinion whether support for this device should finally be added to em28xx or gspca and
I'm willing to continue working on both solutions as much as my time permits and as long as I'm having fun (I'm doing this as a hobby !). Anyway, the em28xx driver is very complex and I really think it would take several further kernel releases to get the job done... I would also be willing to spend some time for moving the em28xx-webcam code to a gspca subdriver, but I don't have any of these devices for testing.

What do you think ?

Hi,

Quoted 21 lines · Frank Schäfer

Hi,

after a break of 2 1/2 kernel releases (sorry, I was busy with another project), I would like to bring up again the question how to add support for this device to the kernel.
See
http://www.mail-archive.com/[email protected]/msg44417.html
("Move em27xx/em28xx webcams to a gspca subdriver ?") for the previous discussion.

Current status is, that I've reverse-engineered the Windows driver and written a new gspca-subdriver for testing, which is feature complete and working stable (will send a patch shortly !).

The device uses an em2765-bridge, so my first idea was of course to modify/extend the em28xx-driver.
But during the reverse-engineering-process, it turned out that writing a new gspca-subdriver was much easier than modifying the em28xx-driver.

The device has the following special characteristics:
- supports only bulk transfers (em28xx driver supports ISOC only)

Quoted 1 line

- uses "proprietary" read/write procedures for the sensor

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

Quoted 1 line

- sensor OV2640

Quoted 3 lines

- different frame processing
- 3 buttons (snapshot, mute, light) which need special treatment (GPIO-polling, status-reseting, ...)

Quoted 3 lines

Another important point to mention: you can see from the USB-logs (sensor probing) that there must be at least 3 other webcam devices.

Quoted 10 lines

Some pros and cons for both solutions:

em28xx:
+ one driver for all 25xx/26xx/27xx/28xx devices
+ no duplicate code (bridge register defines, bridge read/write fcns)
+ other devices COULD benefit from the new functions/code
- big task/lots of work
- code gets bloated with stuff, which is only needed by a few special devices

Quoted 3 lines

gspca:
+ driver already exists (see patch)

Quoted 6 lines

+ default driver for webcams
+ much easier to understand and extend
+ same or even less amount of new code lines
+ keeps em28xx-code "simple"
- code duplication
- support for em28xx-webcams spread over to 2 different drivers

Quoted 11 lines

I have no strong opinion whether support for this device should finally be added to em28xx or gspca and
I'm willing to continue working on both solutions as much as my time permits and as long as I'm having fun (I'm doing this as a hobby !). Anyway, the em28xx driver is very complex and I really think it would take several further kernel releases to get the job done... I would also be willing to spend some time for moving the em28xx-webcam code to a gspca subdriver, but I don't have any of these devices for testing.

What do you think ?

I think given the special way this camera uses the bridge (not using standard i2c interface, weird button layout, etc.). That it is likely server better by a specialized driver. As the (new) gspca maintainer I'm fine with taking it as a gspca sub-driver, but given the code duplication issue, that is a call Mauro should make.

Note that luckily these devices do use a unique USB id and not one of the generic em28xx ids so from that pov having a specialized driver for them is not an issue.

Em 20-08-2012 10:02, Hans de Goede escreveu:

Quoted 3 lines

Note that luckily these devices do use a unique USB id and not one of the generic em28xx ids so from that pov having a specialized driver for them is not an issue.

Hans,

Not sure if all em2765 cameras will have unique USB id's: at em28xx, the known em2710/em2750 cameras that don't have unique ID's; detecting between them requires to probe for the type of sensor.

Hi,

Quoted 1 line · Mauro Carvalho Chehab

Em 20-08-2012 10:02, Hans de Goede escreveu:

<snip>

Quoted 4 lines

Quoted 3 lines

Note that luckily these devices do use a unique USB id and not one of the generic em28xx ids so from that pov having a specialized driver for them is not an issue.

Quoted 5 lines

Hans,

Not sure if all em2765 cameras will have unique USB id's: at em28xx, the known em2710/em2750 cameras that don't have unique ID's; detecting between them requires to probe for the type of sensor.

Right, like the one I gave to Douglas and you or Douglas (don't remember) added support for.

But that one was a "regular" em28xx using camera, and this one appears to be a bit funky in places...

I'll let Frank answer your other remarks.

Em 20-08-2012 10:02, Hans de Goede escreveu:

Quoted 22 lines

Hi,

Quoted 19 lines · Frank Schäfer

Hi,

after a break of 2 1/2 kernel releases (sorry, I was busy with another project), I would like to bring up again the question how to add support for this device to the kernel.
See
http://www.mail-archive.com/[email protected]/msg44417.html
("Move em27xx/em28xx webcams to a gspca subdriver ?") for the previous discussion.

Current status is, that I've reverse-engineered the Windows driver and written a new gspca-subdriver for testing, which is feature complete and working stable (will send a patch shortly !).

The device uses an em2765-bridge, so my first idea was of course to modify/extend the em28xx-driver.
But during the reverse-engineering-process, it turned out that writing a new gspca-subdriver was much easier than modifying the em28xx-driver.

Quoted 2 lines

Quoted 2 lines

The device has the following special characteristics:
- supports only bulk transfers (em28xx driver supports ISOC only)

Em28xx driver supports both isoc and bulk transfers, as bulk is required by DVB.

Quoted 2 lines

Quoted 2 lines

- uses "proprietary" read/write procedures for the sensor

Sure about that? It doesn't use I2C?

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Quoted 1 line

Quoted 1 line

- sensor OV2640

There is a driver for it at:

	drivers/media/i2c/soc_camera/ov2640.c

The better is to use it (even if this got mapped via gspca).

Quoted 3 lines

Quoted 3 lines

- different frame processing
- 3 buttons (snapshot, mute, light) which need special treatment (GPIO-polling, status-reseting, ...)

Need to see the code to better understand that.

Quoted 2 lines

Quoted 2 lines

Another important point to mention: you can see from the USB-logs (sensor probing) that there must be at least 3 other webcam devices.

What do you mean?

Quoted 9 lines

Quoted 9 lines

Some pros and cons for both solutions:

em28xx:
+ one driver for all 25xx/26xx/27xx/28xx devices
+ no duplicate code (bridge register defines, bridge read/write fcns)
+ other devices COULD benefit from the new functions/code
- big task/lots of work
- code gets bloated with stuff, which is only needed by a few special devices

It all depends on what's needed ;)

Quoted 2 lines

Quoted 2 lines

gspca:
+ driver already exists (see patch)

Which patch?

Quoted 6 lines

Quoted 6 lines

+ default driver for webcams
+ much easier to understand and extend
+ same or even less amount of new code lines
+ keeps em28xx-code "simple"
- code duplication
- support for em28xx-webcams spread over to 2 different drivers

The spread of em27xx support on 2 different drivers can lead into problems for the users to know what driver implements support for their device.

So, if we're going to do that, then the better is to move support for all em27xx devices out of em28xx driver, but I think that this will end by creating duplicated stuff.

Btw, how is audio with your em2765 device? Is it provided via snd-usb-audio, or does it require some code like the one inside em28xx?

Quoted 17 lines

Quoted 11 lines

I have no strong opinion whether support for this device should finally be added to em28xx or gspca and
I'm willing to continue working on both solutions as much as my time permits and as long as I'm having fun (I'm doing this as a hobby !). Anyway, the em28xx driver is very complex and I really think it would take several further kernel releases to get the job done... I would also be willing to spend some time for moving the em28xx-webcam code to a gspca subdriver, but I don't have any of these devices for testing.

What do you think ?

I think given the special way this camera uses the bridge (not using standard i2c interface, weird button layout, etc.). That it is likely server better by a specialized driver. As the (new) gspca maintainer I'm fine with taking it as a gspca sub-driver, but given the code duplication issue, that is a call Mauro should make.

Quoted 3 lines

Note that luckily these devices do use a unique USB id and not one of the generic em28xx ids so from that pov having a specialized driver for them is not an issue.

Hans,

Not sure if all em2765 cameras will have unique USB id's: at em28xx, the known em2710/em2750 cameras that don't have unique ID's; detecting between them requires to probe for the type of sensor.

Regards,
Mauro

Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:

Quoted 23 lines

Em 20-08-2012 10:02, Hans de Goede escreveu:

Quoted 22 lines

Hi,

Quoted 19 lines · Frank Schäfer

Hi,

after a break of 2 1/2 kernel releases (sorry, I was busy with another project), I would like to bring up again the question how to add support for this device to the kernel.
See
http://www.mail-archive.com/[email protected]/msg44417.html
("Move em27xx/em28xx webcams to a gspca subdriver ?") for the previous discussion.

Current status is, that I've reverse-engineered the Windows driver and written a new gspca-subdriver for testing, which is feature complete and working stable (will send a patch shortly !).

The device uses an em2765-bridge, so my first idea was of course to modify/extend the em28xx-driver.
But during the reverse-engineering-process, it turned out that writing a new gspca-subdriver was much easier than modifying the em28xx-driver.

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

The device has the following special characteristics:
- supports only bulk transfers (em28xx driver supports ISOC only)

Em28xx driver supports both isoc and bulk transfers, as bulk is required by DVB.

Hmm... are you 100% sure ? Must have been added recently then...

I did a quick check of the current code, but can't find anything. Could you please give me a pointer to the code parts ?
Btw, if I'm not understanding the code wrong, em28xx_usb_probe() still seems to return -ENODEV if no isoc-in endpoint is found, so bulk-ep-only devices should not work...

Quoted 2 lines

Quoted 1 line

Quoted 1 line

- uses "proprietary" read/write procedures for the sensor

Sure about that? It doesn't use I2C?

According to the datasheet of the OV2640 it should be SCCB.

Anyway, I'm referring to how communication works on the USB level. Take a look at http://linuxtv.org/wiki/index.php/VAD_Laplace section "Reverse Engineering (evaluation of USB-logs)" to see how it is working.

Interestingly, the standard I2C reads are used, too, for reading the EEPROM. So maybe there is a "physical" difference.

"Proprietary" is probably not the best name, but I don't have e better one at the moment (suggestions ?).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Yes, I've read the comment in em28xx_i2c_eeprom():
"...there is the risk that we could corrupt the eeprom (since a 16-bit read call is interpreted as a write call by 8-bit eeproms)..." How can we know if a device uses an 8bit or 16bit EEPROM ? Can we derive that from the uses em27xx/28xx-chip ?

Anyway, this problem is common to both solutions (gspca or em28xx-driver).

Quoted 5 lines

Quoted 1 line

Quoted 1 line

- sensor OV2640

There is a driver for it at:

	drivers/media/i2c/soc_camera/ov2640.c

The better is to use it (even if this got mapped via gspca).

Yes, I know. This is already on my ToDo list.

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines

- different frame processing
- 3 buttons (snapshot, mute, light) which need special treatment (GPIO-polling, status-reseting, ...)

Need to see the code to better understand that.

Take a look at the patch.

http://www.spinics.net/lists/linux-media/msg52084.html

I've sent it to the list 3 minutes after I started this thread and also CC'ed you.

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

Another important point to mention: you can see from the USB-logs (sensor probing) that there must be at least 3 other webcam devices.

What do you mean?

The Windows driver probes 3 other sensor addresses (using the same "proprietary" reads). I've included them in my patch.
The INF-file does not contain any other USB IDs, but I think it is unlikely that they are used by this device.
SpeedLink spent different USB IDs even for identical devices with different body colors, so I think they would have done they same for variants with different sensors.
It is more likely that the Windows driver is a kind of universal em2765 driver.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Some pros and cons for both solutions:

em28xx:
+ one driver for all 25xx/26xx/27xx/28xx devices
+ no duplicate code (bridge register defines, bridge read/write fcns)
+ other devices COULD benefit from the new functions/code
- big task/lots of work
- code gets bloated with stuff, which is only needed by a few special devices

Quoted 5 lines

It all depends on what's needed ;)

Quoted 2 lines

Quoted 2 lines

gspca:
+ driver already exists (see patch)

Which patch?

See above.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

+ default driver for webcams
+ much easier to understand and extend
+ same or even less amount of new code lines
+ keeps em28xx-code "simple"
- code duplication
- support for em28xx-webcams spread over to 2 different drivers

Quoted 3 lines

The spread of em27xx support on 2 different drivers can lead into problems for the users to know what driver implements support for their device.

Ok, but that's what dmesg is for, isn't it ?
And people willing to add support for a new device have to contact the linux-media ML anyway.

Btw: there is another "non-technical" argument for a gspca subdriver:
For the remaining unsupported em2xxx webcams, we probably depend on people doing reverse-engineering of their device. People which might not be that skilled (like me ;-) ).
A gspca subdriver is MUCH easier to understand and to modify, than the em28xx-driver. I really tried and I think I didn't give up to early, but...

Quoted 3 lines

So, if we're going to do that, then the better is to move support for all em27xx devices out of em28xx driver, but I think that this will end by creating duplicated stuff.

You are probably right.
I don't like the idea of having two different drivers for the same webcam family, but I think separate drivers for webcams and TV-devices would be ok.

Concerning code duplication: I think it isn't too much. We should also take the total number of code lines into account. For example, we could get rid of field is_webcam in the em28xx device struct, which saves some lines of code.
But form your own opinion by comparing the code.

Quoted 3 lines

Btw, how is audio with your em2765 device? Is it provided via snd-usb-audio, or does it require some code like the one inside em28xx?

It works out of the box with snd-usb-audio.
But, of course, at least in theory, there could be webcams requiring the special code.
I'm not sure how likely that is, given that the audio part of webcam is just a microphone.

Quoted 20 lines

Quoted 20 lines

Quoted 11 lines

I have no strong opinion whether support for this device should finally be added to em28xx or gspca and
I'm willing to continue working on both solutions as much as my time permits and as long as I'm having fun (I'm doing this as a hobby !). Anyway, the em28xx driver is very complex and I really think it would take several further kernel releases to get the job done... I would also be willing to spend some time for moving the em28xx-webcam code to a gspca subdriver, but I don't have any of these devices for testing.

What do you think ?

I think given the special way this camera uses the bridge (not using standard i2c interface, weird button layout, etc.). That it is likely server better by a specialized driver. As the (new) gspca maintainer I'm fine with taking it as a gspca sub-driver, but given the code duplication issue, that is a call Mauro should make.

Note that luckily these devices do use a unique USB id and not one of the generic em28xx ids so from that pov having a specialized driver for them is not an issue.

Quoted 5 lines

Hans,

Not sure if all em2765 cameras will have unique USB id's: at em28xx, the known em2710/em2750 cameras that don't have unique ID's; detecting between them requires to probe for the type of sensor.

Hmmm... I'm aware of non-unique-id problem, but is it really possible that an ID is used for a webcam and a for example a DVB-T-device ?

Em 21-08-2012 08:35, Frank Schäfer escreveu:

Quoted 28 lines

Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:

Quoted 23 lines

Em 20-08-2012 10:02, Hans de Goede escreveu:

Quoted 22 lines

Hi,

Quoted 19 lines · Frank Schäfer

Hi,

after a break of 2 1/2 kernel releases (sorry, I was busy with another project), I would like to bring up again the question how to add support for this device to the kernel.
See
http://www.mail-archive.com/[email protected]/msg44417.html
("Move em27xx/em28xx webcams to a gspca subdriver ?") for the previous discussion.

Current status is, that I've reverse-engineered the Windows driver and written a new gspca-subdriver for testing, which is feature complete and working stable (will send a patch shortly !).

The device uses an em2765-bridge, so my first idea was of course to modify/extend the em28xx-driver.
But during the reverse-engineering-process, it turned out that writing a new gspca-subdriver was much easier than modifying the em28xx-driver.

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

The device has the following special characteristics:
- supports only bulk transfers (em28xx driver supports ISOC only)

Em28xx driver supports both isoc and bulk transfers, as bulk is required by DVB.

Quoted 7 lines

Hmm... are you 100% sure ? Must have been added recently then...

I did a quick check of the current code, but can't find anything. Could you please give me a pointer to the code parts ?
Btw, if I'm not understanding the code wrong, em28xx_usb_probe() still seems to return -ENODEV if no isoc-in endpoint is found, so bulk-ep-only devices should not work...

Perhaps I'm tricked by tm6000 code... both codes are similar. There are a few differences with regards to isoc/bulk hanlding there.

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

Quoted 1 line

- uses "proprietary" read/write procedures for the sensor

Sure about that? It doesn't use I2C?

According to the datasheet of the OV2640 it should be SCCB.

SCCB is a variant of I2C.

Quoted 4 lines

Anyway, I'm referring to how communication works on the USB level. Take a look at http://linuxtv.org/wiki/index.php/VAD_Laplace section "Reverse Engineering (evaluation of USB-logs)" to see how it is working.

Ok. From your logs, it seems that em2765 uses a different bus for sensor communication. It is not unusual to have more than one bus on some modern devices (cx231xx has 3 I2C buses, plus one extra bus that can be switched).

A proper mapping for it to use ov2640 driver is to create two i2c buses, one used by eeprom access, and another one for sensor.

Quoted 6 lines

Interestingly, the standard I2C reads are used, too, for reading the EEPROM. So maybe there is a "physical" difference.

"Proprietary" is probably not the best name, but I don't have e better one at the moment (suggestions ?).

It is just another bus: instead of using req 3/4 for read/write, it uses req 6 for both reads/writes at the i2c-like sensor bus.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Yes, I've read the comment in em28xx_i2c_eeprom():
"...there is the risk that we could corrupt the eeprom (since a 16-bit read call is interpreted as a write call by 8-bit eeproms)..." How can we know if a device uses an 8bit or 16bit EEPROM ? Can we derive that from the uses em27xx/28xx-chip ?

I don't know any other way to check it than to read the chip ID, at register 0x0a. Those are the chip ID's that we currently know:

enum em28xx_chip_id {
	CHIP_ID_EM2800 = 7,
	CHIP_ID_EM2710 = 17,
	CHIP_ID_EM2820 = 18,	/* Also used by some em2710 */
	CHIP_ID_EM2840 = 20,
	CHIP_ID_EM2750 = 33,
	CHIP_ID_EM2860 = 34,
	CHIP_ID_EM2870 = 35,
	CHIP_ID_EM2883 = 36,
	CHIP_ID_EM2874 = 65,
	CHIP_ID_EM2884 = 68,
	CHIP_ID_EM28174 = 113,
};

Even if we add it as a separate driver, it is likely wise to re-use the registers description at drivers/media/usb/em28xx/em28xx-reg.h, moving it to drivers/include/media, as em2765 likely uses the same registers. It also makes sense to add a chip detection at the existing driver, for it to bail out if it detects an em2765 (and the reverse on the new driver).

Quoted 2 lines

Anyway, this problem is common to both solutions (gspca or em28xx-driver).

As eeprom reading is I2C, it could make some sense to use a generic driver for reading its contents, removing the code from em28xx-i2c logic, and re-using it on both drivers (assuming that we fork it).

Quoted 10 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

- sensor OV2640

There is a driver for it at:

	drivers/media/i2c/soc_camera/ov2640.c

The better is to use it (even if this got mapped via gspca).

Yes, I know. This is already on my ToDo list.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

- different frame processing
- 3 buttons (snapshot, mute, light) which need special treatment (GPIO-polling, status-reseting, ...)

Quoted 4 lines

Quoted 1 line

Need to see the code to better understand that.

Take a look at the patch.

http://www.spinics.net/lists/linux-media/msg52084.html

Ok, let's do it via gspca, but please map both the standard I2C and the "SCCB" bus via the I2C API, and use the existing ov2640 driver.

Quoted 8 lines

I've sent it to the list 3 minutes after I started this thread and also CC'ed you.

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

Another important point to mention: you can see from the USB-logs (sensor probing) that there must be at least 3 other webcam devices.

What do you mean?

Quoted 9 lines

The Windows driver probes 3 other sensor addresses (using the same "proprietary" reads). I've included them in my patch.
The INF-file does not contain any other USB IDs, but I think it is unlikely that they are used by this device.
SpeedLink spent different USB IDs even for identical devices with different body colors, so I think they would have done they same for variants with different sensors.
It is more likely that the Windows driver is a kind of universal em2765 driver.

With means that we'll sooner or later get some devices with other sensors.

So, it is better to be prepared for that, by exposing the sensor bus via I2C.

I don't see any need to probe those other sensors right now: we should do it only if we actually get one of those other devices in hands.

Quoted 23 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Some pros and cons for both solutions:

em28xx:
+ one driver for all 25xx/26xx/27xx/28xx devices
+ no duplicate code (bridge register defines, bridge read/write fcns)
+ other devices COULD benefit from the new functions/code
- big task/lots of work
- code gets bloated with stuff, which is only needed by a few special devices

Quoted 5 lines

It all depends on what's needed ;)

Quoted 2 lines

Quoted 2 lines

gspca:
+ driver already exists (see patch)

Which patch?

See above.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

+ default driver for webcams
+ much easier to understand and extend
+ same or even less amount of new code lines
+ keeps em28xx-code "simple"
- code duplication
- support for em28xx-webcams spread over to 2 different drivers

Quoted 6 lines

Quoted 3 lines

The spread of em27xx support on 2 different drivers can lead into problems for the users to know what driver implements support for their device.

Ok, but that's what dmesg is for, isn't it ?
And people willing to add support for a new device have to contact the linux-media ML anyway.

Well, if the driver's probe can handle it and return -ENODEV if it doesn't mach a device it could work with, then we might not have much issues.

Quoted 7 lines

Btw: there is another "non-technical" argument for a gspca subdriver:
For the remaining unsupported em2xxx webcams, we probably depend on people doing reverse-engineering of their device. People which might not be that skilled (like me ;-) ).
A gspca subdriver is MUCH easier to understand and to modify, than the em28xx-driver. I really tried and I think I didn't give up to early, but...

Quoted 7 lines

Quoted 3 lines

So, if we're going to do that, then the better is to move support for all em27xx devices out of em28xx driver, but I think that this will end by creating duplicated stuff.

You are probably right.
I don't like the idea of having two different drivers for the same webcam family, but I think separate drivers for webcams and TV-devices would be ok.

Yeah, sure.

Quoted 6 lines

Concerning code duplication: I think it isn't too much. We should also take the total number of code lines into account. For example, we could get rid of field is_webcam in the em28xx device struct, which saves some lines of code.
But form your own opinion by comparing the code.

Quoted 8 lines

Quoted 3 lines

Btw, how is audio with your em2765 device? Is it provided via snd-usb-audio, or does it require some code like the one inside em28xx?

It works out of the box with snd-usb-audio.
But, of course, at least in theory, there could be webcams requiring the special code.
I'm not sure how likely that is, given that the audio part of webcam is just a microphone.

Empia has 2 "families" of chips: one with standard audio class and another one with proprietary audio class. I won't doubt that there is some variant of em2765 with proprietary audio class. If so, we may need to add some logic at the gspca driver, in order to load and bind the em28xx-alsa module.

That's one extra reason for trying to keep at least some common headers accessed by both drivers.

Quoted 23 lines

Quoted 23 lines

Quoted 18 lines

Quoted 11 lines

I have no strong opinion whether support for this device should finally be added to em28xx or gspca and
I'm willing to continue working on both solutions as much as my time permits and as long as I'm having fun (I'm doing this as a hobby !). Anyway, the em28xx driver is very complex and I really think it would take several further kernel releases to get the job done... I would also be willing to spend some time for moving the em28xx-webcam code to a gspca subdriver, but I don't have any of these devices for testing.

Quoted 1 line

What do you think ?

I think given the special way this camera uses the bridge (not using standard i2c interface, weird button layout, etc.). That it is likely server better by a specialized driver. As the (new) gspca maintainer I'm fine with taking it as a gspca sub-driver, but given the code duplication issue, that is a call Mauro should make.

Quoted 3 lines

Note that luckily these devices do use a unique USB id and not one of the generic em28xx ids so from that pov having a specialized driver for them is not an issue.

Hans,

Quoted 5 lines

Quoted 3 lines

Not sure if all em2765 cameras will have unique USB id's: at em28xx, the known em2710/em2750 cameras that don't have unique ID's; detecting between them requires to probe for the type of sensor.

Hmmm... I'm aware of non-unique-id problem, but is it really possible that an ID is used for a webcam and a for example a DVB-T-device ?

There are some USB ID's at em28xx that are used by both grabber devices and webcams.

Regards,
Mauro

Am 21.08.2012 14:32, schrieb Mauro Carvalho Chehab:

Quoted 40 lines

Em 21-08-2012 08:35, Frank Schäfer escreveu:

Quoted 28 lines

Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:

Quoted 23 lines

Em 20-08-2012 10:02, Hans de Goede escreveu:

Quoted 22 lines

Hi,

Quoted 19 lines · Frank Schäfer

Hi,

after a break of 2 1/2 kernel releases (sorry, I was busy with another project), I would like to bring up again the question how to add support for this device to the kernel.
See
http://www.mail-archive.com/[email protected]/msg44417.html
("Move em27xx/em28xx webcams to a gspca subdriver ?") for the previous discussion.

Current status is, that I've reverse-engineered the Windows driver and written a new gspca-subdriver for testing, which is feature complete and working stable (will send a patch shortly !).

The device uses an em2765-bridge, so my first idea was of course to modify/extend the em28xx-driver.
But during the reverse-engineering-process, it turned out that writing a new gspca-subdriver was much easier than modifying the em28xx-driver.

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

The device has the following special characteristics:
- supports only bulk transfers (em28xx driver supports ISOC only)

Em28xx driver supports both isoc and bulk transfers, as bulk is required by DVB.

Quoted 7 lines

Hmm... are you 100% sure ? Must have been added recently then...

I did a quick check of the current code, but can't find anything. Could you please give me a pointer to the code parts ?
Btw, if I'm not understanding the code wrong, em28xx_usb_probe() still seems to return -ENODEV if no isoc-in endpoint is found, so bulk-ep-only devices should not work...

Perhaps I'm tricked by tm6000 code... both codes are similar. There are a few differences with regards to isoc/bulk hanlding there.

Quoted 4 lines

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

Quoted 1 line

- uses "proprietary" read/write procedures for the sensor

Sure about that? It doesn't use I2C?

According to the datasheet of the OV2640 it should be SCCB.

SCCB is a variant of I2C.

I'm not sure if Omnivison would admit that. :D

Quoted 7 lines

Quoted 3 lines

Anyway, I'm referring to how communication works on the USB level. Take a look at http://linuxtv.org/wiki/index.php/VAD_Laplace section "Reverse Engineering (evaluation of USB-logs)" to see how it is working.

Ok. From your logs, it seems that em2765 uses a different bus for sensor communication. It is not unusual to have more than one bus on some modern devices (cx231xx has 3 I2C buses, plus one extra bus that can be switched).

Yes, but I'm wondering why.
Shouldn't it be possible to connect both (sensor and eeprom) to the same bus ? Or are i2c and sccb devices incompatible ?
We've seen so many em28xx devices (some of them beeing much more complex) and none of them has used two busses so far. Strange...

Quoted 2 lines

A proper mapping for it to use ov2640 driver is to create two i2c buses, one used by eeprom access, and another one for sensor.

Sure.

Quoted 36 lines

Quoted 6 lines

Interestingly, the standard I2C reads are used, too, for reading the EEPROM. So maybe there is a "physical" difference.

"Proprietary" is probably not the best name, but I don't have e better one at the moment (suggestions ?).

It is just another bus: instead of using req 3/4 for read/write, it uses req 6 for both reads/writes at the i2c-like sensor bus.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Yes, I've read the comment in em28xx_i2c_eeprom():
"...there is the risk that we could corrupt the eeprom (since a 16-bit read call is interpreted as a write call by 8-bit eeproms)..." How can we know if a device uses an 8bit or 16bit EEPROM ? Can we derive that from the uses em27xx/28xx-chip ?

I don't know any other way to check it than to read the chip ID, at register 0x0a. Those are the chip ID's that we currently know:

enum em28xx_chip_id {
	CHIP_ID_EM2800 = 7,
	CHIP_ID_EM2710 = 17,
	CHIP_ID_EM2820 = 18,	/* Also used by some em2710 */
	CHIP_ID_EM2840 = 20,
	CHIP_ID_EM2750 = 33,
	CHIP_ID_EM2860 = 34,
	CHIP_ID_EM2870 = 35,
	CHIP_ID_EM2883 = 36,
	CHIP_ID_EM2874 = 65,
	CHIP_ID_EM2884 = 68,
	CHIP_ID_EM28174 = 113,
};

Quoted 6 lines

Even if we add it as a separate driver, it is likely wise to re-use the registers description at drivers/media/usb/em28xx/em28xx-reg.h, moving it to drivers/include/media, as em2765 likely uses the same registers. It also makes sense to add a chip detection at the existing driver, for it to bail out if it detects an em2765 (and the reverse on the new driver).

em2765 has chip-id 0x36 = 54.
Do you want me to send a patch ?

Do you really think the em28xx driver should always bail out when it detects the em2765 ?

Quoted 4 lines

Quoted 1 line

Anyway, this problem is common to both solutions (gspca or em28xx-driver).

As eeprom reading is I2C, it could make some sense to use a generic driver for reading its contents, removing the code from em28xx-i2c logic, and re-using it on both drivers (assuming that we fork it).

Yes, I think that would be a good idea.

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

- sensor OV2640

There is a driver for it at:

	drivers/media/i2c/soc_camera/ov2640.c

The better is to use it (even if this got mapped via gspca).

Yes, I know. This is already on my ToDo list.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

- different frame processing
- 3 buttons (snapshot, mute, light) which need special treatment (GPIO-polling, status-reseting, ...)

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Need to see the code to better understand that.

Take a look at the patch.

http://www.spinics.net/lists/linux-media/msg52084.html

Ok, let's do it via gspca, but please map both the standard I2C and the "SCCB" bus via the I2C API, and use the existing ov2640 driver.

Wow, that was a fast decision. ;)
Could you please elaborate a bit more on that ?
How are we going to handle em27xx/em28xx webcams in the future ? Only add THIS device via gspca ?
Put all em2765 based webcams to a gspca driver ?
Put all new devices to gspca ?
Or move all webcams to gspca ?

Quoted 16 lines

Quoted 16 lines

I've sent it to the list 3 minutes after I started this thread and also CC'ed you.

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

Another important point to mention: you can see from the USB-logs (sensor probing) that there must be at least 3 other webcam devices.

What do you mean?

The Windows driver probes 3 other sensor addresses (using the same "proprietary" reads). I've included them in my patch.
The INF-file does not contain any other USB IDs, but I think it is unlikely that they are used by this device.
SpeedLink spent different USB IDs even for identical devices with different body colors, so I think they would have done they same for variants with different sensors.
It is more likely that the Windows driver is a kind of universal em2765 driver.

Quoted 1 line

With means that we'll sooner or later get some devices with other sensors.

I'm pretty sure about that. Two of them are:
eb1a:2711 V-Gear TalkCamPro

1ae7:2001 SpeedLink Snappy Microphone

Quoted 4 lines

So, it is better to be prepared for that, by exposing the sensor bus via I2C.

I don't see any need to probe those other sensors right now: we should do it only if we actually get one of those other devices in hands.

Ok. Probing the other 3 addresses doesn't hurt, and the idea behind is, that it easier for people to add support for new devices.
The first thing they usually do is to add their USB-ID to the driver and see what happens. With a bit luck, dmesg the shows something like "unknown sensor detetced at address xy" or even "unknown Omnivison sensor detected at address xy", which is (hopefully) encouraging... ;) But if you want me to not probe them, I will disable them (I'm sure you agree that we should keep the addresses in the code, at least as comments).

Quoted 61 lines

Quoted 22 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Some pros and cons for both solutions:

em28xx:
+ one driver for all 25xx/26xx/27xx/28xx devices
+ no duplicate code (bridge register defines, bridge read/write fcns)
+ other devices COULD benefit from the new functions/code
- big task/lots of work
- code gets bloated with stuff, which is only needed by a few special devices

Quoted 5 lines

It all depends on what's needed ;)

Quoted 2 lines

Quoted 2 lines

gspca:
+ driver already exists (see patch)

Which patch?

See above.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

+ default driver for webcams
+ much easier to understand and extend
+ same or even less amount of new code lines
+ keeps em28xx-code "simple"
- code duplication
- support for em28xx-webcams spread over to 2 different drivers

Quoted 6 lines

Quoted 3 lines

The spread of em27xx support on 2 different drivers can lead into problems for the users to know what driver implements support for their device.

Ok, but that's what dmesg is for, isn't it ?
And people willing to add support for a new device have to contact the linux-media ML anyway.

Well, if the driver's probe can handle it and return -ENODEV if it doesn't mach a device it could work with, then we might not have much issues.

Quoted 7 lines

Btw: there is another "non-technical" argument for a gspca subdriver:
For the remaining unsupported em2xxx webcams, we probably depend on people doing reverse-engineering of their device. People which might not be that skilled (like me ;-) ).
A gspca subdriver is MUCH easier to understand and to modify, than the em28xx-driver. I really tried and I think I didn't give up to early, but...

Quoted 7 lines

Quoted 3 lines

So, if we're going to do that, then the better is to move support for all em27xx devices out of em28xx driver, but I think that this will end by creating duplicated stuff.

You are probably right.
I don't like the idea of having two different drivers for the same webcam family, but I think separate drivers for webcams and TV-devices would be ok.

Yeah, sure.

Quoted 14 lines

Concerning code duplication: I think it isn't too much. We should also take the total number of code lines into account. For example, we could get rid of field is_webcam in the em28xx device struct, which saves some lines of code.
But form your own opinion by comparing the code.

Quoted 3 lines

Btw, how is audio with your em2765 device? Is it provided via snd-usb-audio, or does it require some code like the one inside em28xx?

It works out of the box with snd-usb-audio.
But, of course, at least in theory, there could be webcams requiring the special code.
I'm not sure how likely that is, given that the audio part of webcam is just a microphone.

Quoted 5 lines

Empia has 2 "families" of chips: one with standard audio class and another one with proprietary audio class. I won't doubt that there is some variant of em2765 with proprietary audio class. If so, we may need to add some logic at the gspca driver, in order to load and bind the em28xx-alsa module.

Ok.
Have you seen any devices with both interfaces so far ?

Quoted 3 lines

That's one extra reason for trying to keep at least some common headers accessed by both drivers.

Definitely. Using a common header for the bridge registers for both drivers is one of things on my ToDo list.
Btw, what's the right place to put this header file ?

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 18 lines

Quoted 11 lines

I have no strong opinion whether support for this device should finally be added to em28xx or gspca and
I'm willing to continue working on both solutions as much as my time permits and as long as I'm having fun (I'm doing this as a hobby !). Anyway, the em28xx driver is very complex and I really think it would take several further kernel releases to get the job done... I would also be willing to spend some time for moving the em28xx-webcam code to a gspca subdriver, but I don't have any of these devices for testing.

Quoted 1 line

What do you think ?

I think given the special way this camera uses the bridge (not using standard i2c interface, weird button layout, etc.). That it is likely server better by a specialized driver. As the (new) gspca maintainer I'm fine with taking it as a gspca sub-driver, but given the code duplication issue, that is a call Mauro should make.

Quoted 3 lines

Note that luckily these devices do use a unique USB id and not one of the generic em28xx ids so from that pov having a specialized driver for them is not an issue.

Hans,

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Not sure if all em2765 cameras will have unique USB id's: at em28xx, the known em2710/em2750 cameras that don't have unique ID's; detecting between them requires to probe for the type of sensor.

Hmmm... I'm aware of non-unique-id problem, but is it really possible that an ID is used for a webcam and a for example a DVB-T-device ?

There are some USB ID's at em28xx that are used by both grabber devices and webcams.

Great. :(

Regards,
Frank

Quoted 3 lines

Regards,
Mauro

Hmm... before reading the rest of this email... I found some doc saying that em2765 is UVC compliant. Doesn't the uvcdriver work with this device?

Em 21-08-2012 13:04, Frank Schäfer escreveu:

Quoted 41 lines

Am 21.08.2012 14:32, schrieb Mauro Carvalho Chehab:

Quoted 40 lines

Em 21-08-2012 08:35, Frank Schäfer escreveu:

Quoted 28 lines

Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:

Quoted 23 lines

Em 20-08-2012 10:02, Hans de Goede escreveu:

Quoted 22 lines

Hi,

Quoted 19 lines · Frank Schäfer

Hi,

after a break of 2 1/2 kernel releases (sorry, I was busy with another project), I would like to bring up again the question how to add support for this device to the kernel.
See
http://www.mail-archive.com/[email protected]/msg44417.html
("Move em27xx/em28xx webcams to a gspca subdriver ?") for the previous discussion.

Current status is, that I've reverse-engineered the Windows driver and written a new gspca-subdriver for testing, which is feature complete and working stable (will send a patch shortly !).

The device uses an em2765-bridge, so my first idea was of course to modify/extend the em28xx-driver.
But during the reverse-engineering-process, it turned out that writing a new gspca-subdriver was much easier than modifying the em28xx-driver.

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

The device has the following special characteristics:
- supports only bulk transfers (em28xx driver supports ISOC only)

Em28xx driver supports both isoc and bulk transfers, as bulk is required by DVB.

Quoted 7 lines

Hmm... are you 100% sure ? Must have been added recently then...

I did a quick check of the current code, but can't find anything. Could you please give me a pointer to the code parts ?
Btw, if I'm not understanding the code wrong, em28xx_usb_probe() still seems to return -ENODEV if no isoc-in endpoint is found, so bulk-ep-only devices should not work...

Perhaps I'm tricked by tm6000 code... both codes are similar. There are a few differences with regards to isoc/bulk hanlding there.

Quoted 5 lines

Quoted 4 lines

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

Quoted 1 line

- uses "proprietary" read/write procedures for the sensor

Sure about that? It doesn't use I2C?

According to the datasheet of the OV2640 it should be SCCB.

SCCB is a variant of I2C.

I'm not sure if Omnivison would admit that. :D

Maybe there are trademarks envolved ;)

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Anyway, I'm referring to how communication works on the USB level. Take a look at http://linuxtv.org/wiki/index.php/VAD_Laplace section "Reverse Engineering (evaluation of USB-logs)" to see how it is working.

Ok. From your logs, it seems that em2765 uses a different bus for sensor communication. It is not unusual to have more than one bus on some modern devices (cx231xx has 3 I2C buses, plus one extra bus that can be switched).

Quoted 6 lines

Yes, but I'm wondering why.
Shouldn't it be possible to connect both (sensor and eeprom) to the same bus ? Or are i2c and sccb devices incompatible ?
We've seen so many em28xx devices (some of them beeing much more complex) and none of them has used two busses so far. Strange...

Most devices nowadays have 2 i2c buses. TV boards typically uses an I2C switch on them. The rationale is to avoid receiving signal interference. Some newer em28xx devices have more than one bus, although, on TV chipsets, this is controlled via one register, that commands bus switch:

	/* em28xx I2C Clock Register (0x06) */
	#define EM2874_I2C_SECONDARY_BUS_SELECT	0x04 /* em2874 has two i2c busses */

On those devices (em2874/em2875, afaikt), hardware manufacturers put the TV tuner and demod at the second bus, keeping the first bus for remote controller and eeprom.

You'll see several supported devices using the secondary bus for TV and demod. As, currently, the TV eeprom is not read on those devices, nobody cared enough to add a separate I2C bus code for it, as all access used by the driver happen just on the second bus.

Quoted 45 lines

Quoted 2 lines

A proper mapping for it to use ov2640 driver is to create two i2c buses, one used by eeprom access, and another one for sensor.

Sure.

Quoted 41 lines

Quoted 5 lines

Interestingly, the standard I2C reads are used, too, for reading the EEPROM. So maybe there is a "physical" difference.

"Proprietary" is probably not the best name, but I don't have e better one at the moment (suggestions ?).

It is just another bus: instead of using req 3/4 for read/write, it uses req 6 for both reads/writes at the i2c-like sensor bus.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Yes, I've read the comment in em28xx_i2c_eeprom():
"...there is the risk that we could corrupt the eeprom (since a 16-bit read call is interpreted as a write call by 8-bit eeproms)..." How can we know if a device uses an 8bit or 16bit EEPROM ? Can we derive that from the uses em27xx/28xx-chip ?

I don't know any other way to check it than to read the chip ID, at register 0x0a. Those are the chip ID's that we currently know:

enum em28xx_chip_id {
	CHIP_ID_EM2800 = 7,
	CHIP_ID_EM2710 = 17,
	CHIP_ID_EM2820 = 18,	/* Also used by some em2710 */
	CHIP_ID_EM2840 = 20,
	CHIP_ID_EM2750 = 33,
	CHIP_ID_EM2860 = 34,
	CHIP_ID_EM2870 = 35,
	CHIP_ID_EM2883 = 36,
	CHIP_ID_EM2874 = 65,
	CHIP_ID_EM2884 = 68,
	CHIP_ID_EM28174 = 113,
};

Even if we add it as a separate driver, it is likely wise to re-use the registers description at drivers/media/usb/em28xx/em28xx-reg.h, moving it to drivers/include/media, as em2765 likely uses the same registers. It also makes sense to add a chip detection at the existing driver, for it to bail out if it detects an em2765 (and the reverse on the new driver).

Quoted 2 lines

em2765 has chip-id 0x36 = 54.
Do you want me to send a patch ?

Yes, please send it when you'll be ready for driver submission.

Quoted 2 lines

Do you really think the em28xx driver should always bail out when it detects the em2765 ?

Well, having 2 drivers for the same chipset is a very bad idea. Either one should use it.

Another option would be to have a generic em28xx dispatcher driver that would handle all of them, probe the board, and then starting either one, depending if the driver is webcam or not.

Btw, this is on my TODO list (with low priority), as there are several devices that have only DVB. So, it makes sense to split the analog TV driver, just like we did with the DVB and alsa drivers. This way, an em28xx core driver will contain only the probe and the core functions like i2c and the common helper functions, while all the rest would be on separate drivers.

IMO, doing that that could be better than coding em2765 as a completely separate driver.

It shouldn't be hard to split the code like that: most of the TV-specific code is already under em28xx-video. There are still some stuff under em28xx-core that could likely be moved into em28xx-video as well, as the code is specific for TV.

The probing code, plus the card descriptions is at em28xx-cards. Some code there, under em28xx_init_dev() are due to analog init. Moving it into em28xx-video and adding there a code like the one at the end of em28xx-dvb - the calls for em28xx_(un)register_extension() - should be enough for em28xx-video to be an independent module, that would be loaded by the core driver only if the device has analog TV.

If you want, please feel free to take this way, working on the existing em28xx code to split it like that.

Quoted 22 lines

Quoted 4 lines

Quoted 1 line

Anyway, this problem is common to both solutions (gspca or em28xx-driver).

As eeprom reading is I2C, it could make some sense to use a generic driver for reading its contents, removing the code from em28xx-i2c logic, and re-using it on both drivers (assuming that we fork it).

Yes, I think that would be a good idea.

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

- sensor OV2640

There is a driver for it at:

	drivers/media/i2c/soc_camera/ov2640.c

The better is to use it (even if this got mapped via gspca).

Yes, I know. This is already on my ToDo list.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

- different frame processing
- 3 buttons (snapshot, mute, light) which need special treatment (GPIO-polling, status-reseting, ...)

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Need to see the code to better understand that.

Take a look at the patch.

http://www.spinics.net/lists/linux-media/msg52084.html

Ok, let's do it via gspca, but please map both the standard I2C and the "SCCB" bus via the I2C API, and use the existing ov2640 driver.

Quoted 7 lines

Wow, that was a fast decision. ;)
Could you please elaborate a bit more on that ?
How are we going to handle em27xx/em28xx webcams in the future ? Only add THIS device via gspca ?
Put all em2765 based webcams to a gspca driver ?
Put all new devices to gspca ?
Or move all webcams to gspca ?

The better is to move all webcams to the new code. I suspect, however, that this can only happen if we keep just one probing code for all em28xx devices.

If you can work like that, I can certainly test the em2750 cameras I have here (and fix, if needed).

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

I've sent it to the list 3 minutes after I started this thread and also CC'ed you.

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

Another important point to mention: you can see from the USB-logs (sensor probing) that there must be at least 3 other webcam devices.

What do you mean?

The Windows driver probes 3 other sensor addresses (using the same "proprietary" reads). I've included them in my patch.
The INF-file does not contain any other USB IDs, but I think it is unlikely that they are used by this device.
SpeedLink spent different USB IDs even for identical devices with different body colors, so I think they would have done they same for variants with different sensors.
It is more likely that the Windows driver is a kind of universal em2765 driver.

Quoted 3 lines

Quoted 1 line

With means that we'll sooner or later get some devices with other sensors.

I'm pretty sure about that. Two of them are:
eb1a:2711 V-Gear TalkCamPro

This USB ID is a generic EmpiaTech ID. That means that those cameras don't have any eeprom. I'm sure you'll find other devices with different sensors sharing this very same USB ID, and maybe even some grabber devices.

Quoted 6 lines

1ae7:2001 SpeedLink Snappy Microphone

Quoted 4 lines

So, it is better to be prepared for that, by exposing the sensor bus via I2C.

I don't see any need to probe those other sensors right now: we should do it only if we actually get one of those other devices in hands.

Quoted 8 lines

Ok. Probing the other 3 addresses doesn't hurt, and the idea behind is, that it easier for people to add support for new devices.
The first thing they usually do is to add their USB-ID to the driver and see what happens. With a bit luck, dmesg the shows something like "unknown sensor detetced at address xy" or even "unknown Omnivison sensor detected at address xy", which is (hopefully) encouraging... ;) But if you want me to not probe them, I will disable them (I'm sure you agree that we should keep the addresses in the code, at least as comments).

If the sensor is Omnivision/Micron, its model typically can be identified by reading the contents of sensor register 0. The code under em28xx_hint_sensor() does that (it currently assumes a sensor at I2C address 0x5d). So, trying to read from address 0, on the possible sensor addresses may help to identify what sensor was used.

Quoted 62 lines

Quoted 62 lines

Quoted 23 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Some pros and cons for both solutions:

em28xx:
+ one driver for all 25xx/26xx/27xx/28xx devices
+ no duplicate code (bridge register defines, bridge read/write fcns)
+ other devices COULD benefit from the new functions/code
- big task/lots of work
- code gets bloated with stuff, which is only needed by a few special devices

Quoted 5 lines

It all depends on what's needed ;)

Quoted 2 lines

Quoted 2 lines

gspca:
+ driver already exists (see patch)

Which patch?

See above.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

+ default driver for webcams
+ much easier to understand and extend
+ same or even less amount of new code lines
+ keeps em28xx-code "simple"
- code duplication
- support for em28xx-webcams spread over to 2 different drivers

Quoted 6 lines

Quoted 3 lines

The spread of em27xx support on 2 different drivers can lead into problems for the users to know what driver implements support for their device.

Ok, but that's what dmesg is for, isn't it ?
And people willing to add support for a new device have to contact the linux-media ML anyway.

Well, if the driver's probe can handle it and return -ENODEV if it doesn't mach a device it could work with, then we might not have much issues.

Quoted 7 lines

Btw: there is another "non-technical" argument for a gspca subdriver:
For the remaining unsupported em2xxx webcams, we probably depend on people doing reverse-engineering of their device. People which might not be that skilled (like me ;-) ).
A gspca subdriver is MUCH easier to understand and to modify, than the em28xx-driver. I really tried and I think I didn't give up to early, but...

Quoted 7 lines

Quoted 3 lines

So, if we're going to do that, then the better is to move support for all em27xx devices out of em28xx driver, but I think that this will end by creating duplicated stuff.

You are probably right.
I don't like the idea of having two different drivers for the same webcam family, but I think separate drivers for webcams and TV-devices would be ok.

Yeah, sure.

Quoted 14 lines

Concerning code duplication: I think it isn't too much. We should also take the total number of code lines into account. For example, we could get rid of field is_webcam in the em28xx device struct, which saves some lines of code.
But form your own opinion by comparing the code.

Quoted 3 lines

Btw, how is audio with your em2765 device? Is it provided via snd-usb-audio, or does it require some code like the one inside em28xx?

It works out of the box with snd-usb-audio.
But, of course, at least in theory, there could be webcams requiring the special code.
I'm not sure how likely that is, given that the audio part of webcam is just a microphone.

Quoted 7 lines

Quoted 5 lines

Empia has 2 "families" of chips: one with standard audio class and another one with proprietary audio class. I won't doubt that there is some variant of em2765 with proprietary audio class. If so, we may need to add some logic at the gspca driver, in order to load and bind the em28xx-alsa module.

Ok.
Have you seen any devices with both interfaces so far ?

AFAIK, it is either one or the other.

Quoted 42 lines

Quoted 2 lines

That's one extra reason for trying to keep at least some common headers accessed by both drivers.

Definitely. Using a common header for the bridge registers for both drivers is one of things on my ToDo list.
Btw, what's the right place to put this header file ?

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

Quoted 17 lines

Quoted 11 lines

I have no strong opinion whether support for this device should finally be added to em28xx or gspca and
I'm willing to continue working on both solutions as much as my time permits and as long as I'm having fun (I'm doing this as a hobby !). Anyway, the em28xx driver is very complex and I really think it would take several further kernel releases to get the job done... I would also be willing to spend some time for moving the em28xx-webcam code to a gspca subdriver, but I don't have any of these devices for testing.

What do you think ?

I think given the special way this camera uses the bridge (not using standard i2c interface, weird button layout, etc.). That it is likely server better by a specialized driver. As the (new) gspca maintainer I'm fine with taking it as a gspca sub-driver, but given the code duplication issue, that is a call Mauro should make.

Quoted 3 lines

Note that luckily these devices do use a unique USB id and not one of the generic em28xx ids so from that pov having a specialized driver for them is not an issue.

Hans,

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Not sure if all em2765 cameras will have unique USB id's: at em28xx, the known em2710/em2750 cameras that don't have unique ID's; detecting between them requires to probe for the type of sensor.

Hmmm... I'm aware of non-unique-id problem, but is it really possible that an ID is used for a webcam and a for example a DVB-T-device ?

There are some USB ID's at em28xx that are used by both grabber devices and webcams.

Great. :(

Regards,
Frank

Quoted 2 lines

Regards,
Mauro

Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:

Quoted 2 lines

Hmm... before reading the rest of this email... I found some doc saying that em2765 is UVC compliant. Doesn't the uvcdriver work with this device?

Yeah, I stumbled over that, too. :D
But this device is definitely not UVC compliant. Take a look at the lsusb output.
Maybe they are using a different firmware or something like that, but I have no idea why the hell they should make a UVC compliant device non-UVC-compliant...
Another notable difference to the devices we've seen so far is the em25xx-style EEPROM. Maybe there is a connection.

Btw, do we know any em25xx devices ???

I have to admit that I didn't open my device to check which chip it uses. This information comes from the guy who created the inital wiki page. But I think we can trust this information, because he provided the full chip label content and also photos. And according to the chip id, it's none of the devices we already know.
I wouldn't hesitate to open my device to verify it, if the chance to damage the device wouldn't be that high...

Quoted 43 lines

Em 21-08-2012 13:04, Frank Schäfer escreveu:

Quoted 41 lines

Am 21.08.2012 14:32, schrieb Mauro Carvalho Chehab:

Quoted 40 lines

Em 21-08-2012 08:35, Frank Schäfer escreveu:

Quoted 28 lines

Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:

Quoted 23 lines

Em 20-08-2012 10:02, Hans de Goede escreveu:

Quoted 22 lines

Hi,

Quoted 19 lines · Frank Schäfer

Hi,

after a break of 2 1/2 kernel releases (sorry, I was busy with another project), I would like to bring up again the question how to add support for this device to the kernel.
See
http://www.mail-archive.com/[email protected]/msg44417.html
("Move em27xx/em28xx webcams to a gspca subdriver ?") for the previous discussion.

Current status is, that I've reverse-engineered the Windows driver and written a new gspca-subdriver for testing, which is feature complete and working stable (will send a patch shortly !).

The device uses an em2765-bridge, so my first idea was of course to modify/extend the em28xx-driver.
But during the reverse-engineering-process, it turned out that writing a new gspca-subdriver was much easier than modifying the em28xx-driver.

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

The device has the following special characteristics:
- supports only bulk transfers (em28xx driver supports ISOC only)

Em28xx driver supports both isoc and bulk transfers, as bulk is required by DVB.

Quoted 7 lines

Hmm... are you 100% sure ? Must have been added recently then...

I did a quick check of the current code, but can't find anything. Could you please give me a pointer to the code parts ?
Btw, if I'm not understanding the code wrong, em28xx_usb_probe() still seems to return -ENODEV if no isoc-in endpoint is found, so bulk-ep-only devices should not work...

Perhaps I'm tricked by tm6000 code... both codes are similar. There are a few differences with regards to isoc/bulk hanlding there.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

Quoted 1 line

- uses "proprietary" read/write procedures for the sensor

Sure about that? It doesn't use I2C?

According to the datasheet of the OV2640 it should be SCCB.

SCCB is a variant of I2C.

I'm not sure if Omnivison would admit that. :D

Maybe there are trademarks envolved ;)

Yes, it's a funny game. ;)

Quoted 22 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Anyway, I'm referring to how communication works on the USB level. Take a look at http://linuxtv.org/wiki/index.php/VAD_Laplace section "Reverse Engineering (evaluation of USB-logs)" to see how it is working.

Ok. From your logs, it seems that em2765 uses a different bus for sensor communication. It is not unusual to have more than one bus on some modern devices (cx231xx has 3 I2C buses, plus one extra bus that can be switched).

Quoted 6 lines

Yes, but I'm wondering why.
Shouldn't it be possible to connect both (sensor and eeprom) to the same bus ? Or are i2c and sccb devices incompatible ?
We've seen so many em28xx devices (some of them beeing much more complex) and none of them has used two busses so far. Strange...

Most devices nowadays have 2 i2c buses. TV boards typically uses an I2C switch on them. The rationale is to avoid receiving signal interference. Some newer em28xx devices have more than one bus, although, on TV chipsets, this is controlled via one register, that commands bus switch:

	/* em28xx I2C Clock Register (0x06) */
	#define EM2874_I2C_SECONDARY_BUS_SELECT	0x04 /* em2874 has two i2c busses */

Quoted 3 lines

On those devices (em2874/em2875, afaikt), hardware manufacturers put the TV tuner and demod at the second bus, keeping the first bus for remote controller and eeprom.

Ok, I didn't notice that there a two i2c busses.
I wouldn't wonder if the em2765 doesn't support this bus switch and that's why different USB reads/writes are used instead. Shouldn't be too difficult to find out...

Quoted 4 lines

You'll see several supported devices using the secondary bus for TV and demod. As, currently, the TV eeprom is not read on those devices, nobody cared enough to add a separate I2C bus code for it, as all access used by the driver happen just on the second bus.

Well, the same applies to this webcam. We do not really need to read the EEPROM at the moment.

Quoted 45 lines

Quoted 45 lines

Quoted 2 lines

A proper mapping for it to use ov2640 driver is to create two i2c buses, one used by eeprom access, and another one for sensor.

Sure.

Quoted 41 lines

Quoted 5 lines

Interestingly, the standard I2C reads are used, too, for reading the EEPROM. So maybe there is a "physical" difference.

"Proprietary" is probably not the best name, but I don't have e better one at the moment (suggestions ?).

It is just another bus: instead of using req 3/4 for read/write, it uses req 6 for both reads/writes at the i2c-like sensor bus.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Yes, I've read the comment in em28xx_i2c_eeprom():
"...there is the risk that we could corrupt the eeprom (since a 16-bit read call is interpreted as a write call by 8-bit eeproms)..." How can we know if a device uses an 8bit or 16bit EEPROM ? Can we derive that from the uses em27xx/28xx-chip ?

I don't know any other way to check it than to read the chip ID, at register 0x0a. Those are the chip ID's that we currently know:

enum em28xx_chip_id {
	CHIP_ID_EM2800 = 7,
	CHIP_ID_EM2710 = 17,
	CHIP_ID_EM2820 = 18,	/* Also used by some em2710 */
	CHIP_ID_EM2840 = 20,
	CHIP_ID_EM2750 = 33,
	CHIP_ID_EM2860 = 34,
	CHIP_ID_EM2870 = 35,
	CHIP_ID_EM2883 = 36,
	CHIP_ID_EM2874 = 65,
	CHIP_ID_EM2884 = 68,
	CHIP_ID_EM28174 = 113,
};

Even if we add it as a separate driver, it is likely wise to re-use the registers description at drivers/media/usb/em28xx/em28xx-reg.h, moving it to drivers/include/media, as em2765 likely uses the same registers. It also makes sense to add a chip detection at the existing driver, for it to bail out if it detects an em2765 (and the reverse on the new driver).

Quoted 3 lines

Quoted 2 lines

em2765 has chip-id 0x36 = 54.
Do you want me to send a patch ?

Yes, please send it when you'll be ready for driver submission.

Will do that.

Quoted 8 lines

Quoted 2 lines

Do you really think the em28xx driver should always bail out when it detects the em2765 ?

Well, having 2 drivers for the same chipset is a very bad idea. Either one should use it.

Another option would be to have a generic em28xx dispatcher driver that would handle all of them, probe the board, and then starting either one, depending if the driver is webcam or not.

Sounds good.

Quoted 6 lines

Btw, this is on my TODO list (with low priority), as there are several devices that have only DVB. So, it makes sense to split the analog TV driver, just like we did with the DVB and alsa drivers. This way, an em28xx core driver will contain only the probe and the core functions like i2c and the common helper functions, while all the rest would be on separate drivers.

Yeah, a compact bridge module providing chip info, bridge register r/w functions and access to the 2 + 1 i2c busses sounds good.
If I understand you right, this module should also do the probing and then call the right driver for the device, e.g. gspca for webcams ? Sounds complicating, because the bridge module is still needed after the handover to the other driver, but I know nearly nothing about the modules interaction possibilites (except that one module can call another ;) ).

Quoted 2 lines

IMO, doing that that could be better than coding em2765 as a completely separate driver.

Sounds like the best approach. But also lots of non-trivial work which someone has to do first. I'm afraid too much for a beginner like me... And we should keep in mind that this probably means that people will have to wait several further kernel releases before their device gets supported.

What about the GPIO/buttons stuff ? That's probably the biggest issue comparing this device with the others. Would be nice to have a more generic approach here, too.

Quoted 13 lines

It shouldn't be hard to split the code like that: most of the TV-specific code is already under em28xx-video. There are still some stuff under em28xx-core that could likely be moved into em28xx-video as well, as the code is specific for TV.

The probing code, plus the card descriptions is at em28xx-cards. Some code there, under em28xx_init_dev() are due to analog init. Moving it into em28xx-video and adding there a code like the one at the end of em28xx-dvb - the calls for em28xx_(un)register_extension() - should be enough for em28xx-video to be an independent module, that would be loaded by the core driver only if the device has analog TV.

Quoted 2 lines

If you want, please feel free to take this way, working on the existing em28xx code to split it like that.

Ok, I will a look it. But that could take some time... ;)

Quoted 29 lines

Quoted 29 lines

Quoted 4 lines

Quoted 1 line

Anyway, this problem is common to both solutions (gspca or em28xx-driver).

As eeprom reading is I2C, it could make some sense to use a generic driver for reading its contents, removing the code from em28xx-i2c logic, and re-using it on both drivers (assuming that we fork it).

Yes, I think that would be a good idea.

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

- sensor OV2640

There is a driver for it at:

	drivers/media/i2c/soc_camera/ov2640.c

The better is to use it (even if this got mapped via gspca).

Yes, I know. This is already on my ToDo list.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

- different frame processing
- 3 buttons (snapshot, mute, light) which need special treatment (GPIO-polling, status-reseting, ...)

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Need to see the code to better understand that.

Take a look at the patch.

http://www.spinics.net/lists/linux-media/msg52084.html

Ok, let's do it via gspca, but please map both the standard I2C and the "SCCB" bus via the I2C API, and use the existing ov2640 driver.

Wow, that was a fast decision. ;)
Could you please elaborate a bit more on that ?
How are we going to handle em27xx/em28xx webcams in the future ? Only add THIS device via gspca ?
Put all em2765 based webcams to a gspca driver ?
Put all new devices to gspca ?
Or move all webcams to gspca ?

Quoted 5 lines

The better is to move all webcams to the new code. I suspect, however, that this can only happen if we keep just one probing code for all em28xx devices.

If you can work like that, I can certainly test the em2750 cameras I have here (and fix, if needed).

That would be necessary. And also testing any other changes to the
em28xx driver, because I don't have such a device.

Quoted 37 lines

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

I've sent it to the list 3 minutes after I started this thread and also CC'ed you.

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

Another important point to mention: you can see from the USB-logs (sensor probing) that there must be at least 3 other webcam devices.

What do you mean?

The Windows driver probes 3 other sensor addresses (using the same "proprietary" reads). I've included them in my patch.
The INF-file does not contain any other USB IDs, but I think it is unlikely that they are used by this device.
SpeedLink spent different USB IDs even for identical devices with different body colors, so I think they would have done they same for variants with different sensors.
It is more likely that the Windows driver is a kind of universal em2765 driver.

Quoted 3 lines

Quoted 1 line

With means that we'll sooner or later get some devices with other sensors.

I'm pretty sure about that. Two of them are:
eb1a:2711 V-Gear TalkCamPro

This USB ID is a generic EmpiaTech ID. That means that those cameras don't have any eeprom. I'm sure you'll find other devices with different sensors sharing this very same USB ID, and maybe even some grabber devices.

Quoted 14 lines

1ae7:2001 SpeedLink Snappy Microphone

Quoted 4 lines

So, it is better to be prepared for that, by exposing the sensor bus via I2C.

I don't see any need to probe those other sensors right now: we should do it only if we actually get one of those other devices in hands.

Ok. Probing the other 3 addresses doesn't hurt, and the idea behind is, that it easier for people to add support for new devices.
The first thing they usually do is to add their USB-ID to the driver and see what happens. With a bit luck, dmesg the shows something like "unknown sensor detetced at address xy" or even "unknown Omnivison sensor detected at address xy", which is (hopefully) encouraging... ;) But if you want me to not probe them, I will disable them (I'm sure you agree that we should keep the addresses in the code, at least as comments).

Quoted 5 lines

If the sensor is Omnivision/Micron, its model typically can be identified by reading the contents of sensor register 0. The code under em28xx_hint_sensor() does that (it currently assumes a sensor at I2C address 0x5d). So, trying to read from address 0, on the possible sensor addresses may help to identify what sensor was used.

Yes I'm doing this for Omnivision sensors in the gspca subdriver. And the the em28xx sensor probing function does it for Micron sensors.

Em 22-08-2012 04:53, Frank Schäfer escreveu:

Quoted 12 lines

Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:

Quoted 2 lines

Hmm... before reading the rest of this email... I found some doc saying that em2765 is UVC compliant. Doesn't the uvcdriver work with this device?

Yeah, I stumbled over that, too. :D
But this device is definitely not UVC compliant. Take a look at the lsusb output.
Maybe they are using a different firmware or something like that, but I have no idea why the hell they should make a UVC compliant device non-UVC-compliant...
Another notable difference to the devices we've seen so far is the em25xx-style EEPROM. Maybe there is a connection.

Quoted 1 line

Btw, do we know any em25xx devices ???

No, I never heard about em25xx. It seems that there are some new em275xx chips, but I don't have any technical data.

Quoted 22 lines

Quoted 22 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Anyway, I'm referring to how communication works on the USB level. Take a look at http://linuxtv.org/wiki/index.php/VAD_Laplace section "Reverse Engineering (evaluation of USB-logs)" to see how it is working.

Ok. From your logs, it seems that em2765 uses a different bus for sensor communication. It is not unusual to have more than one bus on some modern devices (cx231xx has 3 I2C buses, plus one extra bus that can be switched).

Quoted 6 lines

Yes, but I'm wondering why.
Shouldn't it be possible to connect both (sensor and eeprom) to the same bus ? Or are i2c and sccb devices incompatible ?
We've seen so many em28xx devices (some of them beeing much more complex) and none of them has used two busses so far. Strange...

Most devices nowadays have 2 i2c buses. TV boards typically uses an I2C switch on them. The rationale is to avoid receiving signal interference. Some newer em28xx devices have more than one bus, although, on TV chipsets, this is controlled via one register, that commands bus switch:

	/* em28xx I2C Clock Register (0x06) */
	#define EM2874_I2C_SECONDARY_BUS_SELECT	0x04 /* em2874 has two i2c busses */

Quoted 7 lines

Quoted 3 lines

On those devices (em2874/em2875, afaikt), hardware manufacturers put the TV tuner and demod at the second bus, keeping the first bus for remote controller and eeprom.

Ok, I didn't notice that there a two i2c busses.
I wouldn't wonder if the em2765 doesn't support this bus switch and that's why different USB reads/writes are used instead. Shouldn't be too difficult to find out...

Perhaps it accepts both ways. IMHO, a separate req for the other bus is better than needing to change a register for every single read/write (If my memories are not betraying me, from some USB logs, I remind I saw that kind of thing: for every single I2C operation, it first sets the bus, and then reads/writes - even when the previous operation were at the same bus).

Well, you may try to change register 6 to use the secondary bus and see if the "standard" I2C code will work there for the sensor.

Quoted 75 lines

Quoted 5 lines

You'll see several supported devices using the secondary bus for TV and demod. As, currently, the TV eeprom is not read on those devices, nobody cared enough to add a separate I2C bus code for it, as all access used by the driver happen just on the second bus.

Well, the same applies to this webcam. We do not really need to read the EEPROM at the moment.

Quoted 45 lines

Quoted 45 lines

Quoted 2 lines

A proper mapping for it to use ov2640 driver is to create two i2c buses, one used by eeprom access, and another one for sensor.

Sure.

Quoted 41 lines

Quoted 5 lines

Interestingly, the standard I2C reads are used, too, for reading the EEPROM. So maybe there is a "physical" difference.

"Proprietary" is probably not the best name, but I don't have e better one at the moment (suggestions ?).

It is just another bus: instead of using req 3/4 for read/write, it uses req 6 for both reads/writes at the i2c-like sensor bus.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Yes, I've read the comment in em28xx_i2c_eeprom():
"...there is the risk that we could corrupt the eeprom (since a 16-bit read call is interpreted as a write call by 8-bit eeproms)..." How can we know if a device uses an 8bit or 16bit EEPROM ? Can we derive that from the uses em27xx/28xx-chip ?

I don't know any other way to check it than to read the chip ID, at register 0x0a. Those are the chip ID's that we currently know:

enum em28xx_chip_id {
	CHIP_ID_EM2800 = 7,
	CHIP_ID_EM2710 = 17,
	CHIP_ID_EM2820 = 18,	/* Also used by some em2710 */
	CHIP_ID_EM2840 = 20,
	CHIP_ID_EM2750 = 33,
	CHIP_ID_EM2860 = 34,
	CHIP_ID_EM2870 = 35,
	CHIP_ID_EM2883 = 36,
	CHIP_ID_EM2874 = 65,
	CHIP_ID_EM2884 = 68,
	CHIP_ID_EM28174 = 113,
};

Even if we add it as a separate driver, it is likely wise to re-use the registers description at drivers/media/usb/em28xx/em28xx-reg.h, moving it to drivers/include/media, as em2765 likely uses the same registers. It also makes sense to add a chip detection at the existing driver, for it to bail out if it detects an em2765 (and the reverse on the new driver).

Quoted 3 lines

Quoted 2 lines

em2765 has chip-id 0x36 = 54.
Do you want me to send a patch ?

Yes, please send it when you'll be ready for driver submission.

Will do that.

Quoted 8 lines

Quoted 2 lines

Do you really think the em28xx driver should always bail out when it detects the em2765 ?

Well, having 2 drivers for the same chipset is a very bad idea. Either one should use it.

Another option would be to have a generic em28xx dispatcher driver that would handle all of them, probe the board, and then starting either one, depending if the driver is webcam or not.

Sounds good.

Quoted 6 lines

Btw, this is on my TODO list (with low priority), as there are several devices that have only DVB. So, it makes sense to split the analog TV driver, just like we did with the DVB and alsa drivers. This way, an em28xx core driver will contain only the probe and the core functions like i2c and the common helper functions, while all the rest would be on separate drivers.

Quoted 8 lines

Yeah, a compact bridge module providing chip info, bridge register r/w functions and access to the 2 + 1 i2c busses sounds good.
If I understand you right, this module should also do the probing and then call the right driver for the device, e.g. gspca for webcams ? Sounds complicating, because the bridge module is still needed after the handover to the other driver, but I know nearly nothing about the modules interaction possibilites (except that one module can call another ;) ).

It is not complicated.

At the main driver, take a look at request_module_async(), at em28xx-cards:
it basically schedules a deferred work that will load the needed drivers, after em28xx finishes probing.

At the sub-drivers, they use em28xx_register_extension() on their init code. This function uses a table with 4 parameters, like this one:

static struct em28xx_ops audio_ops = {
	.id   = EM28XX_AUDIO,
	.name = "Em28xx Audio Extension",
	.init = em28xx_audio_init,
	.fini = em28xx_audio_fini,
};

The .init() function will then do everything that it is needed for the device to run. It is called when the main driver detects a new card, and it is ready for that (the main driver has some code there to avoid race conditions, serializing the extensions load).

The .fini() is called when the device got removed, or when the driver calls em28xx_unregister_extension().

The struct em28xx *dev is passed as a parameter on both calls, to allow the several drivers to share common data.

This logic works pretty well, even on SMP environments with lots of CPU. The only care needed there (with won't affect a webcam driver) is to properly lock the driver to avoid two different sub-drivers to access the same device resources at the same time (this is needed between DVB and video parts of the driver).

By moving the TV part to a separate driver, it makes sense to also create an em28xx_video structure, moving there everything that it is not common, but this is an optimization that could be done anytime.

I suggest you to create an em28xx_webcam struct and put there data that it is specifics to the webcam driver there, if any.

Quoted 8 lines

Quoted 3 lines

IMO, doing that that could be better than coding em2765 as a completely separate driver.

Sounds like the best approach. But also lots of non-trivial work which someone has to do first. I'm afraid too much for a beginner like me... And we should keep in mind that this probably means that people will have to wait several further kernel releases before their device gets supported.

Well, it is not that hard. If I had any time, I would do it right now. It probably won't take more than a few hours of work to split the video part into a separate driver, as 99% of the work is to move a few functions between the .c files, and move the init code from em28xx-cards.c to em28xx-video.

I can seek for doing that after the media workshop that will happen next week.

Quoted 3 lines

What about the GPIO/buttons stuff ? That's probably the biggest issue comparing this device with the others. Would be nice to have a more generic approach here, too.

See em28xx_query_sbutton(), at em28xx-input.c. It makes sense to move the button code to a separate file, as this is needed only by the webcam driver.

Quoted 14 lines

Quoted 14 lines

It shouldn't be hard to split the code like that: most of the TV-specific code is already under em28xx-video. There are still some stuff under em28xx-core that could likely be moved into em28xx-video as well, as the code is specific for TV.

The probing code, plus the card descriptions is at em28xx-cards. Some code there, under em28xx_init_dev() are due to analog init. Moving it into em28xx-video and adding there a code like the one at the end of em28xx-dvb - the calls for em28xx_(un)register_extension() - should be enough for em28xx-video to be an independent module, that would be loaded by the core driver only if the device has analog TV.

Quoted 3 lines

Quoted 2 lines

If you want, please feel free to take this way, working on the existing em28xx code to split it like that.

Ok, I will a look it. But that could take some time... ;)

Ok.

Quoted 32 lines

Quoted 32 lines

Quoted 22 lines

Quoted 4 lines

Quoted 1 line

Anyway, this problem is common to both solutions (gspca or em28xx-driver).

As eeprom reading is I2C, it could make some sense to use a generic driver for reading its contents, removing the code from em28xx-i2c logic, and re-using it on both drivers (assuming that we fork it).

Yes, I think that would be a good idea.

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

- sensor OV2640

There is a driver for it at:

	drivers/media/i2c/soc_camera/ov2640.c

The better is to use it (even if this got mapped via gspca).

Yes, I know. This is already on my ToDo list.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

- different frame processing
- 3 buttons (snapshot, mute, light) which need special treatment (GPIO-polling, status-reseting, ...)

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Need to see the code to better understand that.

Take a look at the patch.

http://www.spinics.net/lists/linux-media/msg52084.html

Ok, let's do it via gspca, but please map both the standard I2C and the "SCCB" bus via the I2C API, and use the existing ov2640 driver.

Quoted 7 lines

Wow, that was a fast decision. ;)
Could you please elaborate a bit more on that ?
How are we going to handle em27xx/em28xx webcams in the future ? Only add THIS device via gspca ?
Put all em2765 based webcams to a gspca driver ?
Put all new devices to gspca ?
Or move all webcams to gspca ?

The better is to move all webcams to the new code. I suspect, however, that this can only happen if we keep just one probing code for all em28xx devices.

Quoted 4 lines

Quoted 2 lines

If you can work like that, I can certainly test the em2750 cameras I have here (and fix, if needed).

That would be necessary. And also testing any other changes to the
em28xx driver, because I don't have such a device.

Yeah, sure. I'm sure that there are also other people with em28xx hardware that can help testing, in order to avoid regressions there.

Quoted 38 lines

Quoted 38 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

I've sent it to the list 3 minutes after I started this thread and also CC'ed you.

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

Another important point to mention: you can see from the USB-logs (sensor probing) that there must be at least 3 other webcam devices.

What do you mean?

The Windows driver probes 3 other sensor addresses (using the same "proprietary" reads). I've included them in my patch.
The INF-file does not contain any other USB IDs, but I think it is unlikely that they are used by this device.
SpeedLink spent different USB IDs even for identical devices with different body colors, so I think they would have done they same for variants with different sensors.
It is more likely that the Windows driver is a kind of universal em2765 driver.

Quoted 3 lines

Quoted 1 line

With means that we'll sooner or later get some devices with other sensors.

I'm pretty sure about that. Two of them are:
eb1a:2711 V-Gear TalkCamPro

This USB ID is a generic EmpiaTech ID. That means that those cameras don't have any eeprom. I'm sure you'll find other devices with different sensors sharing this very same USB ID, and maybe even some grabber devices.

Quoted 14 lines

1ae7:2001 SpeedLink Snappy Microphone

Quoted 4 lines

So, it is better to be prepared for that, by exposing the sensor bus via I2C.

I don't see any need to probe those other sensors right now: we should do it only if we actually get one of those other devices in hands.

Ok. Probing the other 3 addresses doesn't hurt, and the idea behind is, that it easier for people to add support for new devices.
The first thing they usually do is to add their USB-ID to the driver and see what happens. With a bit luck, dmesg the shows something like "unknown sensor detetced at address xy" or even "unknown Omnivison sensor detected at address xy", which is (hopefully) encouraging... ;) But if you want me to not probe them, I will disable them (I'm sure you agree that we should keep the addresses in the code, at least as comments).

Quoted 7 lines

Quoted 5 lines

If the sensor is Omnivision/Micron, its model typically can be identified by reading the contents of sensor register 0. The code under em28xx_hint_sensor() does that (it currently assumes a sensor at I2C address 0x5d). So, trying to read from address 0, on the possible sensor addresses may help to identify what sensor was used.

Yes I'm doing this for Omnivision sensors in the gspca subdriver. And the the em28xx sensor probing function does it for Micron sensors.

Ok.

Regards,
Mauro

Sorry for the delayed reply, I got distracted by something with higher prority.

Am 22.08.2012 20:15, schrieb Mauro Carvalho Chehab:

Quoted 13 lines

Em 22-08-2012 04:53, Frank Schäfer escreveu:

Quoted 12 lines

Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:

Quoted 2 lines

Hmm... before reading the rest of this email... I found some doc saying that em2765 is UVC compliant. Doesn't the uvcdriver work with this device?

Yeah, I stumbled over that, too. :D
But this device is definitely not UVC compliant. Take a look at the lsusb output.
Maybe they are using a different firmware or something like that, but I have no idea why the hell they should make a UVC compliant device non-UVC-compliant...
Another notable difference to the devices we've seen so far is the em25xx-style EEPROM. Maybe there is a connection.

Quoted 3 lines

Quoted 1 line

Btw, do we know any em25xx devices ???

No, I never heard about em25xx. It seems that there are some new em275xx chips, but I don't have any technical data.

Maybe they changed the name and there was never a em2580/em2585. But I assume this is an older chip design.

Quoted 115 lines

Quoted 74 lines

Quoted 4 lines

You'll see several supported devices using the secondary bus for TV and demod. As, currently, the TV eeprom is not read on those devices, nobody cared enough to add a separate I2C bus code for it, as all access used by the driver happen just on the second bus.

Well, the same applies to this webcam. We do not really need to read the EEPROM at the moment.

Quoted 45 lines

Quoted 45 lines

Quoted 2 lines

A proper mapping for it to use ov2640 driver is to create two i2c buses, one used by eeprom access, and another one for sensor.

Sure.

Quoted 41 lines

Quoted 5 lines

Interestingly, the standard I2C reads are used, too, for reading the EEPROM. So maybe there is a "physical" difference.

"Proprietary" is probably not the best name, but I don't have e better one at the moment (suggestions ?).

It is just another bus: instead of using req 3/4 for read/write, it uses req 6 for both reads/writes at the i2c-like sensor bus.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Yes, I've read the comment in em28xx_i2c_eeprom():
"...there is the risk that we could corrupt the eeprom (since a 16-bit read call is interpreted as a write call by 8-bit eeproms)..." How can we know if a device uses an 8bit or 16bit EEPROM ? Can we derive that from the uses em27xx/28xx-chip ?

I don't know any other way to check it than to read the chip ID, at register 0x0a. Those are the chip ID's that we currently know:

enum em28xx_chip_id {
	CHIP_ID_EM2800 = 7,
	CHIP_ID_EM2710 = 17,
	CHIP_ID_EM2820 = 18,	/* Also used by some em2710 */
	CHIP_ID_EM2840 = 20,
	CHIP_ID_EM2750 = 33,
	CHIP_ID_EM2860 = 34,
	CHIP_ID_EM2870 = 35,
	CHIP_ID_EM2883 = 36,
	CHIP_ID_EM2874 = 65,
	CHIP_ID_EM2884 = 68,
	CHIP_ID_EM28174 = 113,
};

Even if we add it as a separate driver, it is likely wise to re-use the registers description at drivers/media/usb/em28xx/em28xx-reg.h, moving it to drivers/include/media, as em2765 likely uses the same registers. It also makes sense to add a chip detection at the existing driver, for it to bail out if it detects an em2765 (and the reverse on the new driver).

Quoted 3 lines

Quoted 2 lines

em2765 has chip-id 0x36 = 54.
Do you want me to send a patch ?

Yes, please send it when you'll be ready for driver submission.

Will do that.

Quoted 8 lines

Quoted 2 lines

Do you really think the em28xx driver should always bail out when it detects the em2765 ?

Well, having 2 drivers for the same chipset is a very bad idea. Either one should use it.

Another option would be to have a generic em28xx dispatcher driver that would handle all of them, probe the board, and then starting either one, depending if the driver is webcam or not.

Sounds good.

Quoted 6 lines

Btw, this is on my TODO list (with low priority), as there are several devices that have only DVB. So, it makes sense to split the analog TV driver, just like we did with the DVB and alsa drivers. This way, an em28xx core driver will contain only the probe and the core functions like i2c and the common helper functions, while all the rest would be on separate drivers.

Quoted 8 lines

Yeah, a compact bridge module providing chip info, bridge register r/w functions and access to the 2 + 1 i2c busses sounds good.
If I understand you right, this module should also do the probing and then call the right driver for the device, e.g. gspca for webcams ? Sounds complicating, because the bridge module is still needed after the handover to the other driver, but I know nearly nothing about the modules interaction possibilites (except that one module can call another ;) ).

It is not complicated.

At the main driver, take a look at request_module_async(), at em28xx-cards:
it basically schedules a deferred work that will load the needed drivers, after em28xx finishes probing.

At the sub-drivers, they use em28xx_register_extension() on their init code. This function uses a table with 4 parameters, like this one:

static struct em28xx_ops audio_ops = {
	.id   = EM28XX_AUDIO,
	.name = "Em28xx Audio Extension",
	.init = em28xx_audio_init,
	.fini = em28xx_audio_fini,
};

The .init() function will then do everything that it is needed for the device to run. It is called when the main driver detects a new card, and it is ready for that (the main driver has some code there to avoid race conditions, serializing the extensions load).

The .fini() is called when the device got removed, or when the driver calls em28xx_unregister_extension().

The struct em28xx *dev is passed as a parameter on both calls, to allow the several drivers to share common data.

This logic works pretty well, even on SMP environments with lots of CPU. The only care needed there (with won't affect a webcam driver) is to properly lock the driver to avoid two different sub-drivers to access the same device resources at the same time (this is needed between DVB and video parts of the driver).

Quoted 6 lines

By moving the TV part to a separate driver, it makes sense to also create an em28xx_video structure, moving there everything that it is not common, but this is an optimization that could be done anytime.

I suggest you to create an em28xx_webcam struct and put there data that it is specifics to the webcam driver there, if any.

Ok, thanks for your explanations.
I will see what I can do.

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

IMO, doing that that could be better than coding em2765 as a completely separate driver.

Sounds like the best approach. But also lots of non-trivial work which someone has to do first. I'm afraid too much for a beginner like me... And we should keep in mind that this probably means that people will have to wait several further kernel releases before their device gets supported.

Quoted 8 lines

Well, it is not that hard. If I had any time, I would do it right now. It probably won't take more than a few hours of work to split the video part into a separate driver, as 99% of the work is to move a few functions between the .c files, and move the init code from em28xx-cards.c to em28xx-video.

I can seek for doing that after the media workshop that will happen next week.

Ok.
Trying to summarize the plan, I'm not sure that I understand the driver layout completely yet..
We are going to
- separate the video part of the em28xx driver and create a compact module providing just the core fucntions
- let this module do the probing an then either call "the rest" of the em28xx driver for DVB-devices or a gspca module for all em27xx/em28xx based webcams
- use sub-modules for the sensors and possibly other commonly used features (e.g. an eeprom module) in the webcam module

Ist that correct ?

Quoted 7 lines

Quoted 4 lines

What about the GPIO/buttons stuff ? That's probably the biggest issue comparing this device with the others. Would be nice to have a more generic approach here, too.

See em28xx_query_sbutton(), at em28xx-input.c. It makes sense to move the button code to a separate file, as this is needed only by the webcam driver.

The buttons/LED-stuff works different for this webcam.
It has 3 buttons (snapshot, mute and illumination) and two LEDs (capturing, illumination).
The snapshot button uses GPIO register set 2 (0x81/0x85), the two other buttons and the LEDs use GPIO register set 1 (0x80/0x84). The state of the mute and illumination buttons need to be reset by the driver, the bit of the snapshot button clears istself automatically when the button is depressed.

So for a generic approach, we would need something like

struct gpio_line {
reg_r
reg_w
bit
inverted
needs_reset
poll
callback
}

and corresponding functions.

Seems to be too much overhead at the moment.

Quoted 49 lines

Quoted 12 lines

Quoted 12 lines

It shouldn't be hard to split the code like that: most of the TV-specific code is already under em28xx-video. There are still some stuff under em28xx-core that could likely be moved into em28xx-video as well, as the code is specific for TV.

The probing code, plus the card descriptions is at em28xx-cards. Some code there, under em28xx_init_dev() are due to analog init. Moving it into em28xx-video and adding there a code like the one at the end of em28xx-dvb - the calls for em28xx_(un)register_extension() - should be enough for em28xx-video to be an independent module, that would be loaded by the core driver only if the device has analog TV.

Quoted 3 lines

Quoted 2 lines

If you want, please feel free to take this way, working on the existing em28xx code to split it like that.

Ok, I will a look it. But that could take some time... ;)

Ok.

Quoted 32 lines

Quoted 32 lines

Quoted 22 lines

Quoted 4 lines

Quoted 1 line

Anyway, this problem is common to both solutions (gspca or em28xx-driver).

As eeprom reading is I2C, it could make some sense to use a generic driver for reading its contents, removing the code from em28xx-i2c logic, and re-using it on both drivers (assuming that we fork it).

Yes, I think that would be a good idea.

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

- sensor OV2640

There is a driver for it at:

	drivers/media/i2c/soc_camera/ov2640.c

The better is to use it (even if this got mapped via gspca).

Yes, I know. This is already on my ToDo list.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

- different frame processing
- 3 buttons (snapshot, mute, light) which need special treatment (GPIO-polling, status-reseting, ...)

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Need to see the code to better understand that.

Take a look at the patch.

http://www.spinics.net/lists/linux-media/msg52084.html

Ok, let's do it via gspca, but please map both the standard I2C and the "SCCB" bus via the I2C API, and use the existing ov2640 driver.

Quoted 7 lines

Wow, that was a fast decision. ;)
Could you please elaborate a bit more on that ?
How are we going to handle em27xx/em28xx webcams in the future ? Only add THIS device via gspca ?
Put all em2765 based webcams to a gspca driver ?
Put all new devices to gspca ?
Or move all webcams to gspca ?

The better is to move all webcams to the new code. I suspect, however, that this can only happen if we keep just one probing code for all em28xx devices.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

If you can work like that, I can certainly test the em2750 cameras I have here (and fix, if needed).

That would be necessary. And also testing any other changes to the
em28xx driver, because I don't have such a device.

Yeah, sure. I'm sure that there are also other people with em28xx hardware that can help testing, in order to avoid regressions there.

I hope so.
Do you know an affordable DVB-T devices with isoc and bulk endpoints using an 16bit-eeprom ? ;)

Regards,
Frank

Quoted 48 lines

Quoted 37 lines

Quoted 37 lines

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

I've sent it to the list 3 minutes after I started this thread and also CC'ed you.

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

Another important point to mention: you can see from the USB-logs (sensor probing) that there must be at least 3 other webcam devices.

What do you mean?

The Windows driver probes 3 other sensor addresses (using the same "proprietary" reads). I've included them in my patch.
The INF-file does not contain any other USB IDs, but I think it is unlikely that they are used by this device.
SpeedLink spent different USB IDs even for identical devices with different body colors, so I think they would have done they same for variants with different sensors.
It is more likely that the Windows driver is a kind of universal em2765 driver.

Quoted 3 lines

Quoted 1 line

With means that we'll sooner or later get some devices with other sensors.

I'm pretty sure about that. Two of them are:
eb1a:2711 V-Gear TalkCamPro

This USB ID is a generic EmpiaTech ID. That means that those cameras don't have any eeprom. I'm sure you'll find other devices with different sensors sharing this very same USB ID, and maybe even some grabber devices.

Quoted 14 lines

1ae7:2001 SpeedLink Snappy Microphone

Quoted 4 lines

So, it is better to be prepared for that, by exposing the sensor bus via I2C.

I don't see any need to probe those other sensors right now: we should do it only if we actually get one of those other devices in hands.

Ok. Probing the other 3 addresses doesn't hurt, and the idea behind is, that it easier for people to add support for new devices.
The first thing they usually do is to add their USB-ID to the driver and see what happens. With a bit luck, dmesg the shows something like "unknown sensor detetced at address xy" or even "unknown Omnivison sensor detected at address xy", which is (hopefully) encouraging... ;) But if you want me to not probe them, I will disable them (I'm sure you agree that we should keep the addresses in the code, at least as comments).

Quoted 7 lines

Quoted 5 lines

If the sensor is Omnivision/Micron, its model typically can be identified by reading the contents of sensor register 0. The code under em28xx_hint_sensor() does that (it currently assumes a sensor at I2C address 0x5d). So, trying to read from address 0, on the possible sensor addresses may help to identify what sensor was used.

Yes I'm doing this for Omnivision sensors in the gspca subdriver. And the the em28xx sensor probing function does it for Micron sensors.

Ok.

Regards,
Mauro

Ping !

Am 26.08.2012 20:53, schrieb Frank Schäfer:

Quoted 19 lines

Sorry for the delayed reply, I got distracted by something with higher prority.

Am 22.08.2012 20:15, schrieb Mauro Carvalho Chehab:

Quoted 14 lines

Em 22-08-2012 04:53, Frank Schäfer escreveu:

Quoted 13 lines

Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:

Quoted 2 lines

Hmm... before reading the rest of this email... I found some doc saying that em2765 is UVC compliant. Doesn't the uvcdriver work with this device?

Yeah, I stumbled over that, too. :D
But this device is definitely not UVC compliant. Take a look at the lsusb output.
Maybe they are using a different firmware or something like that, but I have no idea why the hell they should make a UVC compliant device non-UVC-compliant...
Another notable difference to the devices we've seen so far is the em25xx-style EEPROM. Maybe there is a connection.

Btw, do we know any em25xx devices ???

Quoted 4 lines

Quoted 2 lines

No, I never heard about em25xx. It seems that there are some new em275xx chips, but I don't have any technical data.

Maybe they changed the name and there was never a em2580/em2585. But I assume this is an older chip design.

In the mean time I was told that em2580/em2585 devices really exists. They are used for example in intraoral cameras for dentists. The em2765 seems to be a kind of relabled em25xx.

Both chips have two i2c busses and work only with 16 bit address eeproms, which have to be connected to bus A.
The sensor read/write procedure used for this webcam is very likely the standard method for accessing i2c bus B of these chips.
It COULD also be vendor specific procedure, but I don't think 3 other slave addresses would be probed in that case...

<snip>

Quoted 139 lines

Quoted 115 lines

Quoted 74 lines

Quoted 4 lines

You'll see several supported devices using the secondary bus for TV and demod. As, currently, the TV eeprom is not read on those devices, nobody cared enough to add a separate I2C bus code for it, as all access used by the driver happen just on the second bus.

Well, the same applies to this webcam. We do not really need to read the EEPROM at the moment.

Quoted 45 lines

Quoted 45 lines

Quoted 2 lines

A proper mapping for it to use ov2640 driver is to create two i2c buses, one used by eeprom access, and another one for sensor.

Sure.

Quoted 41 lines

Quoted 5 lines

Interestingly, the standard I2C reads are used, too, for reading the EEPROM. So maybe there is a "physical" difference.

"Proprietary" is probably not the best name, but I don't have e better one at the moment (suggestions ?).

It is just another bus: instead of using req 3/4 for read/write, it uses req 6 for both reads/writes at the i2c-like sensor bus.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Yes, I've read the comment in em28xx_i2c_eeprom():
"...there is the risk that we could corrupt the eeprom (since a 16-bit read call is interpreted as a write call by 8-bit eeproms)..." How can we know if a device uses an 8bit or 16bit EEPROM ? Can we derive that from the uses em27xx/28xx-chip ?

I don't know any other way to check it than to read the chip ID, at register 0x0a. Those are the chip ID's that we currently know:

enum em28xx_chip_id {
	CHIP_ID_EM2800 = 7,
	CHIP_ID_EM2710 = 17,
	CHIP_ID_EM2820 = 18,	/* Also used by some em2710 */
	CHIP_ID_EM2840 = 20,
	CHIP_ID_EM2750 = 33,
	CHIP_ID_EM2860 = 34,
	CHIP_ID_EM2870 = 35,
	CHIP_ID_EM2883 = 36,
	CHIP_ID_EM2874 = 65,
	CHIP_ID_EM2884 = 68,
	CHIP_ID_EM28174 = 113,
};

Even if we add it as a separate driver, it is likely wise to re-use the registers description at drivers/media/usb/em28xx/em28xx-reg.h, moving it to drivers/include/media, as em2765 likely uses the same registers. It also makes sense to add a chip detection at the existing driver, for it to bail out if it detects an em2765 (and the reverse on the new driver).

Quoted 3 lines

Quoted 2 lines

em2765 has chip-id 0x36 = 54.
Do you want me to send a patch ?

Yes, please send it when you'll be ready for driver submission.

Will do that.

Quoted 8 lines

Quoted 2 lines

Do you really think the em28xx driver should always bail out when it detects the em2765 ?

Well, having 2 drivers for the same chipset is a very bad idea. Either one should use it.

Another option would be to have a generic em28xx dispatcher driver that would handle all of them, probe the board, and then starting either one, depending if the driver is webcam or not.

Sounds good.

Quoted 6 lines

Btw, this is on my TODO list (with low priority), as there are several devices that have only DVB. So, it makes sense to split the analog TV driver, just like we did with the DVB and alsa drivers. This way, an em28xx core driver will contain only the probe and the core functions like i2c and the common helper functions, while all the rest would be on separate drivers.

Quoted 8 lines

Yeah, a compact bridge module providing chip info, bridge register r/w functions and access to the 2 + 1 i2c busses sounds good.
If I understand you right, this module should also do the probing and then call the right driver for the device, e.g. gspca for webcams ? Sounds complicating, because the bridge module is still needed after the handover to the other driver, but I know nearly nothing about the modules interaction possibilites (except that one module can call another ;) ).

It is not complicated.

At the main driver, take a look at request_module_async(), at em28xx-cards:
it basically schedules a deferred work that will load the needed drivers, after em28xx finishes probing.

At the sub-drivers, they use em28xx_register_extension() on their init code. This function uses a table with 4 parameters, like this one:

static struct em28xx_ops audio_ops = {
	.id   = EM28XX_AUDIO,
	.name = "Em28xx Audio Extension",
	.init = em28xx_audio_init,
	.fini = em28xx_audio_fini,
};

The .init() function will then do everything that it is needed for the device to run. It is called when the main driver detects a new card, and it is ready for that (the main driver has some code there to avoid race conditions, serializing the extensions load).

The .fini() is called when the device got removed, or when the driver calls em28xx_unregister_extension().

The struct em28xx *dev is passed as a parameter on both calls, to allow the several drivers to share common data.

This logic works pretty well, even on SMP environments with lots of CPU. The only care needed there (with won't affect a webcam driver) is to properly lock the driver to avoid two different sub-drivers to access the same device resources at the same time (this is needed between DVB and video parts of the driver).

Quoted 6 lines

By moving the TV part to a separate driver, it makes sense to also create an em28xx_video structure, moving there everything that it is not common, but this is an optimization that could be done anytime.

I suggest you to create an em28xx_webcam struct and put there data that it is specifics to the webcam driver there, if any.

Ok, thanks for your explanations.
I will see what I can do.

Quoted 15 lines

Quoted 7 lines

Quoted 2 lines

IMO, doing that that could be better than coding em2765 as a completely separate driver.

Sounds like the best approach. But also lots of non-trivial work which someone has to do first. I'm afraid too much for a beginner like me... And we should keep in mind that this probably means that people will have to wait several further kernel releases before their device gets supported.

Well, it is not that hard. If I had any time, I would do it right now. It probably won't take more than a few hours of work to split the video part into a separate driver, as 99% of the work is to move a few functions between the .c files, and move the init code from em28xx-cards.c to em28xx-video.

I can seek for doing that after the media workshop that will happen next week.

Quoted 9 lines

Ok.
Trying to summarize the plan, I'm not sure that I understand the driver layout completely yet..
We are going to
- separate the video part of the em28xx driver and create a compact module providing just the core fucntions
- let this module do the probing an then either call "the rest" of the em28xx driver for DVB-devices or a gspca module for all em27xx/em28xx based webcams

Quoted 2 lines

- use sub-modules for the sensors and possibly other commonly used features (e.g. an eeprom module) in the webcam module

Quoted 2 lines

Ist that correct ?

Mauro, could you please elaborate your plan ?
What exactly do you want me to do to get this device supported by the kernel ?

In the mean time I have ported the gspca driver to the ic2 interface, which was necessary to experiment with the ov2640 soc-camera driver,

but also increased the code size enormously.

Unfortunately, lots of changes to the ov2640 driver would be necessary use it. It starts with the init sequence and continues with a long sequence of sensor register writes at capturing start.
My hope was, that the differences can be neglected, but unfortunately this is not the case.
Given the amount of work, the fact that most of these registers are undocumented and the high risk to break things for other users of the driver, I think we should stay with the "custom" code for this webcam at least for the moment.

Please also consider the time factor.

Regards,
Frank

Quoted 9 lines

Unfortunately, lots of changes to the ov2640 driver would be necessary use it. It starts with the init sequence and continues with a long sequence of sensor register writes at capturing start.
My hope was, that the differences can be neglected, but unfortunately this is not the case.
Given the amount of work, the fact that most of these registers are undocumented and the high risk to break things for other users of the driver, I think we should stay with the "custom" code for this webcam at least for the moment.

Well, then do a new ov2640 i2c driver. We can later try to merge both, if we can get enough spec data.

AFAIKT, newer em28xx chips are using this concept. The em28xx-i2c code require changes to support two I2C buses, and to handle 16 bit eeproms. We never cared of doing that because we never needed, so far, to read anything from those devices' eeproms.

Quoted 4 lines · Mauro Carvalho Chehab

AFAIKT, newer em28xx chips are using this concept. The em28xx-i2c code require changes to support two I2C buses, and to handle 16 bit eeproms. We never cared of doing that because we never needed, so far, to read anything from those devices' eeproms.

I actually wrote the code to read the 16-bit eeprom from the em2874, but removed it before submitting it upstream because I was afraid well-intentioned em28xx users trying to add support for their boards would trash their eeprom. This is because performing a read against a 16-bit eeprom is equivalent to a write on an 8-bit eeprom. Hence if the user didn't know what he/she was doing, and used the 16-bit eeprom code against an older eeprom, the eeprom would get trashed (this actually happened to me once when I was doing the em2874 device support originally).

If we really want that code back in the tree, I can dig it up -- but I won't be responsible for users killing their devices.

Devin

Em Sun, 23 Sep 2012 16:03:25 +0200
Frank Schäfer <[email protected]> escreveu:

Quoted 1 line

Ping !

Sorry, too busy those days.

Quoted 19 lines

Am 26.08.2012 20:53, schrieb Frank Schäfer:

Quoted 18 lines

Sorry for the delayed reply, I got distracted by something with higher prority.

Am 22.08.2012 20:15, schrieb Mauro Carvalho Chehab:

Quoted 13 lines

Em 22-08-2012 04:53, Frank Schäfer escreveu:

Quoted 12 lines

Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:

Quoted 2 lines

Hmm... before reading the rest of this email... I found some doc saying that em2765 is UVC compliant. Doesn't the uvcdriver work with this device?

Yeah, I stumbled over that, too. :D
But this device is definitely not UVC compliant. Take a look at the lsusb output.
Maybe they are using a different firmware or something like that, but I have no idea why the hell they should make a UVC compliant device non-UVC-compliant...
Another notable difference to the devices we've seen so far is the em25xx-style EEPROM. Maybe there is a connection.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Btw, do we know any em25xx devices ???

No, I never heard about em25xx. It seems that there are some new em275xx chips, but I don't have any technical data.

Maybe they changed the name and there was never a em2580/em2585. But I assume this is an older chip design.

In the mean time I was told that em2580/em2585 devices really exists. They are used for example in intraoral cameras for dentists. The em2765 seems to be a kind of relabled em25xx.

Ok.

Quoted 6 lines

Both chips have two i2c busses and work only with 16 bit address eeproms, which have to be connected to bus A.
The sensor read/write procedure used for this webcam is very likely the standard method for accessing i2c bus B of these chips.
It COULD also be vendor specific procedure, but I don't think 3 other slave addresses would be probed in that case...

AFAIKT, newer em28xx chips are using this concept. The em28xx-i2c code require changes to support two I2C buses, and to handle 16 bit eeproms. We never cared of doing that because we never needed, so far, to read anything from those devices' eeproms.

Quoted 140 lines

<snip>

Quoted 139 lines

Quoted 115 lines

Quoted 74 lines

Quoted 4 lines

You'll see several supported devices using the secondary bus for TV and demod. As, currently, the TV eeprom is not read on those devices, nobody cared enough to add a separate I2C bus code for it, as all access used by the driver happen just on the second bus.

Well, the same applies to this webcam. We do not really need to read the EEPROM at the moment.

Quoted 45 lines

Quoted 45 lines

Quoted 2 lines

A proper mapping for it to use ov2640 driver is to create two i2c buses, one used by eeprom access, and another one for sensor.

Sure.

Quoted 41 lines

Quoted 5 lines

Interestingly, the standard I2C reads are used, too, for reading the EEPROM. So maybe there is a "physical" difference.

"Proprietary" is probably not the best name, but I don't have e better one at the moment (suggestions ?).

It is just another bus: instead of using req 3/4 for read/write, it uses req 6 for both reads/writes at the i2c-like sensor bus.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Yes, I've read the comment in em28xx_i2c_eeprom():
"...there is the risk that we could corrupt the eeprom (since a 16-bit read call is interpreted as a write call by 8-bit eeproms)..." How can we know if a device uses an 8bit or 16bit EEPROM ? Can we derive that from the uses em27xx/28xx-chip ?

I don't know any other way to check it than to read the chip ID, at register 0x0a. Those are the chip ID's that we currently know:

enum em28xx_chip_id {
	CHIP_ID_EM2800 = 7,
	CHIP_ID_EM2710 = 17,
	CHIP_ID_EM2820 = 18,	/* Also used by some em2710 */
	CHIP_ID_EM2840 = 20,
	CHIP_ID_EM2750 = 33,
	CHIP_ID_EM2860 = 34,
	CHIP_ID_EM2870 = 35,
	CHIP_ID_EM2883 = 36,
	CHIP_ID_EM2874 = 65,
	CHIP_ID_EM2884 = 68,
	CHIP_ID_EM28174 = 113,
};

Even if we add it as a separate driver, it is likely wise to re-use the registers description at drivers/media/usb/em28xx/em28xx-reg.h, moving it to drivers/include/media, as em2765 likely uses the same registers. It also makes sense to add a chip detection at the existing driver, for it to bail out if it detects an em2765 (and the reverse on the new driver).

Quoted 3 lines

Quoted 2 lines

em2765 has chip-id 0x36 = 54.
Do you want me to send a patch ?

Yes, please send it when you'll be ready for driver submission.

Will do that.

Quoted 8 lines

Quoted 2 lines

Do you really think the em28xx driver should always bail out when it detects the em2765 ?

Well, having 2 drivers for the same chipset is a very bad idea. Either one should use it.

Another option would be to have a generic em28xx dispatcher driver that would handle all of them, probe the board, and then starting either one, depending if the driver is webcam or not.

Sounds good.

Quoted 6 lines

Btw, this is on my TODO list (with low priority), as there are several devices that have only DVB. So, it makes sense to split the analog TV driver, just like we did with the DVB and alsa drivers. This way, an em28xx core driver will contain only the probe and the core functions like i2c and the common helper functions, while all the rest would be on separate drivers.

Quoted 8 lines

Yeah, a compact bridge module providing chip info, bridge register r/w functions and access to the 2 + 1 i2c busses sounds good.
If I understand you right, this module should also do the probing and then call the right driver for the device, e.g. gspca for webcams ? Sounds complicating, because the bridge module is still needed after the handover to the other driver, but I know nearly nothing about the modules interaction possibilites (except that one module can call another ;) ).

It is not complicated.

At the main driver, take a look at request_module_async(), at em28xx-cards:
it basically schedules a deferred work that will load the needed drivers, after em28xx finishes probing.

At the sub-drivers, they use em28xx_register_extension() on their init code. This function uses a table with 4 parameters, like this one:

static struct em28xx_ops audio_ops = {
	.id   = EM28XX_AUDIO,
	.name = "Em28xx Audio Extension",
	.init = em28xx_audio_init,
	.fini = em28xx_audio_fini,
};

The .init() function will then do everything that it is needed for the device to run. It is called when the main driver detects a new card, and it is ready for that (the main driver has some code there to avoid race conditions, serializing the extensions load).

The .fini() is called when the device got removed, or when the driver calls em28xx_unregister_extension().

The struct em28xx *dev is passed as a parameter on both calls, to allow the several drivers to share common data.

This logic works pretty well, even on SMP environments with lots of CPU. The only care needed there (with won't affect a webcam driver) is to properly lock the driver to avoid two different sub-drivers to access the same device resources at the same time (this is needed between DVB and video parts of the driver).

Quoted 6 lines

By moving the TV part to a separate driver, it makes sense to also create an em28xx_video structure, moving there everything that it is not common, but this is an optimization that could be done anytime.

I suggest you to create an em28xx_webcam struct and put there data that it is specifics to the webcam driver there, if any.

Ok, thanks for your explanations.
I will see what I can do.

Quoted 15 lines

Quoted 7 lines

Quoted 2 lines

IMO, doing that that could be better than coding em2765 as a completely separate driver.

Sounds like the best approach. But also lots of non-trivial work which someone has to do first. I'm afraid too much for a beginner like me... And we should keep in mind that this probably means that people will have to wait several further kernel releases before their device gets supported.

Well, it is not that hard. If I had any time, I would do it right now. It probably won't take more than a few hours of work to split the video part into a separate driver, as 99% of the work is to move a few functions between the .c files, and move the init code from em28xx-cards.c to em28xx-video.

I can seek for doing that after the media workshop that will happen next week.

Quoted 9 lines

Quoted 9 lines

Ok.
Trying to summarize the plan, I'm not sure that I understand the driver layout completely yet..
We are going to
- separate the video part of the em28xx driver and create a compact module providing just the core fucntions
- let this module do the probing an then either call "the rest" of the em28xx driver for DVB-devices or a gspca module for all em27xx/em28xx based webcams

Yes.

Quoted 2 lines

Quoted 2 lines

- use sub-modules for the sensors and possibly other commonly used features (e.g. an eeprom module) in the webcam module

I think that this should also be part of the em28xx core, as 2 I2C buses and 16-bit eeproms are also used on newer em28xx chips (em2874/em2875 for example). The way request_module_async works allow the main em28xx to load a gspca-based em25xx/em27xx module that, in turn, will use symbols defined on the main em28xx module.

Quoted 4 lines

Quoted 1 line

Ist that correct ?

Mauro, could you please elaborate your plan ?
What exactly do you want me to do to get this device supported by the kernel ?

Basically, the core em28xx module will have:

	drivers/media/usb/em28xx/em28xx-cards.c
	drivers/media/usb/em28xx/em28xx-i2c.c
	drivers/media/usb/em28xx/em28xx-core.c

Eventually, part of the functions under em28xx-core could be moved to em28xx-video, as they would be used just there. For the already supported em2710/em2750 webcams, we should need to change the logic there to use the new gspca-em2xxx module, but this change can be done later.

The em28xx-alsa module already have:

	drivers/media/usb/em28xx/em28xx-audio.c

Nothing changes on it.

The em28xx-dvb module already have:

	drivers/media/usb/em28xx/em28xx-dvb.c

Nothing changes on it.

The new em28xx-v4l module will have:

	drivers/media/usb/em28xx/em28xx-vbi.c
	drivers/media/usb/em28xx/em28xx-video.c

The em28xx-rc module already have:

	drivers/media/usb/em28xx/em28xx-input.c

It makes sense to split it into two separate files, one with just the remote control stuff, and the other one with the webcam snapshot buttons.

The file with the webcam buttons support should be merged with the em28xx gspca module, together with the code you wrote.

Quoted 3 lines

In the mean time I have ported the gspca driver to the ic2 interface, which was necessary to experiment with the ov2640 soc-camera driver,

Good!

Quoted 2 lines

but also increased the code size enormously.

Why?

Quoted 10 lines

Unfortunately, lots of changes to the ov2640 driver would be necessary use it. It starts with the init sequence and continues with a long sequence of sensor register writes at capturing start.
My hope was, that the differences can be neglected, but unfortunately this is not the case.
Given the amount of work, the fact that most of these registers are undocumented and the high risk to break things for other users of the driver, I think we should stay with the "custom" code for this webcam at least for the moment.

Well, then do a new ov2640 i2c driver. We can later try to merge both, if we can get enough spec data.

Quoted 5 lines

Please also consider the time factor.

Regards,
Frank

Regards,
Mauro

Am 06.10.2012 13:56, schrieb Mauro Carvalho Chehab:

Quoted 5 lines

Em Sun, 23 Sep 2012 16:03:25 +0200
Frank Schäfer <[email protected]> escreveu:

Quoted 1 line

Ping !

Sorry, too busy those days.

No problem.

Quoted 192 lines

Quoted 19 lines

Am 26.08.2012 20:53, schrieb Frank Schäfer:

Quoted 18 lines

Sorry for the delayed reply, I got distracted by something with higher prority.

Am 22.08.2012 20:15, schrieb Mauro Carvalho Chehab:

Quoted 13 lines

Em 22-08-2012 04:53, Frank Schäfer escreveu:

Quoted 12 lines

Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:

Quoted 2 lines

Hmm... before reading the rest of this email... I found some doc saying that em2765 is UVC compliant. Doesn't the uvcdriver work with this device?

Yeah, I stumbled over that, too. :D
But this device is definitely not UVC compliant. Take a look at the lsusb output.
Maybe they are using a different firmware or something like that, but I have no idea why the hell they should make a UVC compliant device non-UVC-compliant...
Another notable difference to the devices we've seen so far is the em25xx-style EEPROM. Maybe there is a connection.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Btw, do we know any em25xx devices ???

No, I never heard about em25xx. It seems that there are some new em275xx chips, but I don't have any technical data.

Maybe they changed the name and there was never a em2580/em2585. But I assume this is an older chip design.

In the mean time I was told that em2580/em2585 devices really exists. They are used for example in intraoral cameras for dentists. The em2765 seems to be a kind of relabled em25xx.

Ok.

Quoted 6 lines

Both chips have two i2c busses and work only with 16 bit address eeproms, which have to be connected to bus A.
The sensor read/write procedure used for this webcam is very likely the standard method for accessing i2c bus B of these chips.
It COULD also be vendor specific procedure, but I don't think 3 other slave addresses would be probed in that case...

AFAIKT, newer em28xx chips are using this concept. The em28xx-i2c code require changes to support two I2C buses, and to handle 16 bit eeproms. We never cared of doing that because we never needed, so far, to read anything from those devices' eeproms.

Quoted 140 lines

<snip>

Quoted 139 lines

Quoted 115 lines

Quoted 74 lines

Quoted 4 lines

You'll see several supported devices using the secondary bus for TV and demod. As, currently, the TV eeprom is not read on those devices, nobody cared enough to add a separate I2C bus code for it, as all access used by the driver happen just on the second bus.

Well, the same applies to this webcam. We do not really need to read the EEPROM at the moment.

Quoted 45 lines

Quoted 45 lines

Quoted 2 lines

A proper mapping for it to use ov2640 driver is to create two i2c buses, one used by eeprom access, and another one for sensor.

Sure.

Quoted 41 lines

Quoted 5 lines

Interestingly, the standard I2C reads are used, too, for reading the EEPROM. So maybe there is a "physical" difference.

"Proprietary" is probably not the best name, but I don't have e better one at the moment (suggestions ?).

It is just another bus: instead of using req 3/4 for read/write, it uses req 6 for both reads/writes at the i2c-like sensor bus.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

- uses 16bit eeprom
- em25xx-eeprom with different layout

There are other supported chips with 16bit eeproms. Currently, support for 16bit eeproms is disabled just because this weren't needed so far, but I'm sure this is a need there.

Yes, I've read the comment in em28xx_i2c_eeprom():
"...there is the risk that we could corrupt the eeprom (since a 16-bit read call is interpreted as a write call by 8-bit eeproms)..." How can we know if a device uses an 8bit or 16bit EEPROM ? Can we derive that from the uses em27xx/28xx-chip ?

I don't know any other way to check it than to read the chip ID, at register 0x0a. Those are the chip ID's that we currently know:

enum em28xx_chip_id {
	CHIP_ID_EM2800 = 7,
	CHIP_ID_EM2710 = 17,
	CHIP_ID_EM2820 = 18,	/* Also used by some em2710 */
	CHIP_ID_EM2840 = 20,
	CHIP_ID_EM2750 = 33,
	CHIP_ID_EM2860 = 34,
	CHIP_ID_EM2870 = 35,
	CHIP_ID_EM2883 = 36,
	CHIP_ID_EM2874 = 65,
	CHIP_ID_EM2884 = 68,
	CHIP_ID_EM28174 = 113,
};

Even if we add it as a separate driver, it is likely wise to re-use the registers description at drivers/media/usb/em28xx/em28xx-reg.h, moving it to drivers/include/media, as em2765 likely uses the same registers. It also makes sense to add a chip detection at the existing driver, for it to bail out if it detects an em2765 (and the reverse on the new driver).

Quoted 3 lines

Quoted 2 lines

em2765 has chip-id 0x36 = 54.
Do you want me to send a patch ?

Yes, please send it when you'll be ready for driver submission.

Will do that.

Quoted 8 lines

Quoted 2 lines

Do you really think the em28xx driver should always bail out when it detects the em2765 ?

Well, having 2 drivers for the same chipset is a very bad idea. Either one should use it.

Another option would be to have a generic em28xx dispatcher driver that would handle all of them, probe the board, and then starting either one, depending if the driver is webcam or not.

Sounds good.

Quoted 6 lines

Btw, this is on my TODO list (with low priority), as there are several devices that have only DVB. So, it makes sense to split the analog TV driver, just like we did with the DVB and alsa drivers. This way, an em28xx core driver will contain only the probe and the core functions like i2c and the common helper functions, while all the rest would be on separate drivers.

Quoted 8 lines

Yeah, a compact bridge module providing chip info, bridge register r/w functions and access to the 2 + 1 i2c busses sounds good.
If I understand you right, this module should also do the probing and then call the right driver for the device, e.g. gspca for webcams ? Sounds complicating, because the bridge module is still needed after the handover to the other driver, but I know nearly nothing about the modules interaction possibilites (except that one module can call another ;) ).

It is not complicated.

At the main driver, take a look at request_module_async(), at em28xx-cards:
it basically schedules a deferred work that will load the needed drivers, after em28xx finishes probing.

At the sub-drivers, they use em28xx_register_extension() on their init code. This function uses a table with 4 parameters, like this one:

static struct em28xx_ops audio_ops = {
	.id   = EM28XX_AUDIO,
	.name = "Em28xx Audio Extension",
	.init = em28xx_audio_init,
	.fini = em28xx_audio_fini,
};

The .init() function will then do everything that it is needed for the device to run. It is called when the main driver detects a new card, and it is ready for that (the main driver has some code there to avoid race conditions, serializing the extensions load).

The .fini() is called when the device got removed, or when the driver calls em28xx_unregister_extension().

The struct em28xx *dev is passed as a parameter on both calls, to allow the several drivers to share common data.

This logic works pretty well, even on SMP environments with lots of CPU. The only care needed there (with won't affect a webcam driver) is to properly lock the driver to avoid two different sub-drivers to access the same device resources at the same time (this is needed between DVB and video parts of the driver).

Quoted 6 lines

By moving the TV part to a separate driver, it makes sense to also create an em28xx_video structure, moving there everything that it is not common, but this is an optimization that could be done anytime.

I suggest you to create an em28xx_webcam struct and put there data that it is specifics to the webcam driver there, if any.

Ok, thanks for your explanations.
I will see what I can do.

Quoted 15 lines

Quoted 7 lines

Quoted 2 lines

IMO, doing that that could be better than coding em2765 as a completely separate driver.

Sounds like the best approach. But also lots of non-trivial work which someone has to do first. I'm afraid too much for a beginner like me... And we should keep in mind that this probably means that people will have to wait several further kernel releases before their device gets supported.

Well, it is not that hard. If I had any time, I would do it right now. It probably won't take more than a few hours of work to split the video part into a separate driver, as 99% of the work is to move a few functions between the .c files, and move the init code from em28xx-cards.c to em28xx-video.

I can seek for doing that after the media workshop that will happen next week.

Quoted 9 lines

Quoted 9 lines

Ok.
Trying to summarize the plan, I'm not sure that I understand the driver layout completely yet..
We are going to
- separate the video part of the em28xx driver and create a compact module providing just the core fucntions
- let this module do the probing an then either call "the rest" of the em28xx driver for DVB-devices or a gspca module for all em27xx/em28xx based webcams

Yes.

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

- use sub-modules for the sensors and possibly other commonly used features (e.g. an eeprom module) in the webcam module

I think that this should also be part of the em28xx core, as 2 I2C buses and 16-bit eeproms are also used on newer em28xx chips (em2874/em2875 for example). The way request_module_async works allow the main em28xx to load a gspca-based em25xx/em27xx module that, in turn, will use symbols defined on the main em28xx module.

Ok.

Quoted 32 lines

Quoted 4 lines

Quoted 1 line

Ist that correct ?

Mauro, could you please elaborate your plan ?
What exactly do you want me to do to get this device supported by the kernel ?

Basically, the core em28xx module will have:

	drivers/media/usb/em28xx/em28xx-cards.c
	drivers/media/usb/em28xx/em28xx-i2c.c
	drivers/media/usb/em28xx/em28xx-core.c

Eventually, part of the functions under em28xx-core could be moved to em28xx-video, as they would be used just there. For the already supported em2710/em2750 webcams, we should need to change the logic there to use the new gspca-em2xxx module, but this change can be done later.

The em28xx-alsa module already have:

	drivers/media/usb/em28xx/em28xx-audio.c

Nothing changes on it.

The em28xx-dvb module already have:

	drivers/media/usb/em28xx/em28xx-dvb.c

Nothing changes on it.

The new em28xx-v4l module will have:

	drivers/media/usb/em28xx/em28xx-vbi.c
	drivers/media/usb/em28xx/em28xx-video.c

The em28xx-rc module already have:

	drivers/media/usb/em28xx/em28xx-input.c

It makes sense to split it into two separate files, one with just the remote control stuff, and the other one with the webcam snapshot buttons.

Quoted 2 lines

The file with the webcam buttons support should be merged with the em28xx gspca module, together with the code you wrote.

Ok, but then there is not much left in the gspca driver. ;)

The two main remaining things feature blocks are
- USB-bulk-support
- data/frame processing
and I think it would make sense to have them both in em28xx, too.

I would say that the main reason for a gspca subdriver would be
- the different code for i2c bus B (there is still a chance that this is something SpeedLink specific !)

- the complicating buttons stuff
- the ov2640 code (as long as no sub-module is used for that) which would require adding lots of new code to the em28xx-driver not needed by 95% of the devices.

The big question is, which devices can we expect to appear in the future ?

I'm pretty sure there are much more cameras (like the mentioned intraoral camera devices fro dentists).

As I said before, I can see the
Windows driver probing 4 ic2 slave addresses.
Tuner i2c clients are also mentioned in the em2580/em2585 datasheet, so these chips could be designed for TV stuff, too (although we haven't seen such a device yet).

Quoted 4 lines

Quoted 3 lines

In the mean time I have ported the gspca driver to the ic2 interface, which was necessary to experiment with the ov2640 soc-camera driver,

Good!

It still needs some issues to be resolved (there seems to be a nice USB-locking issue cause by the interaction with the gspca_main module...).

Quoted 4 lines

Quoted 3 lines

but also increased the code size enormously.

Why?

Additional functions (i2c_xfer, i2c_xfer, ...), structs (i2c_algorithm, i2c_adapter, i2c_client, ...).
Also some code duplication is required (e.g. i2c_check_for_device methods) And of course everything times 2 because we have two busses.

Quoted 10 lines

Quoted 10 lines

Unfortunately, lots of changes to the ov2640 driver would be necessary use it. It starts with the init sequence and continues with a long sequence of sensor register writes at capturing start.
My hope was, that the differences can be neglected, but unfortunately this is not the case.
Given the amount of work, the fact that most of these registers are undocumented and the high risk to break things for other users of the driver, I think we should stay with the "custom" code for this webcam at least for the moment.

Quoted 2 lines

Well, then do a new ov2640 i2c driver. We can later try to merge both, if we can get enough spec data.

I'm not sure we will ever get them.
The datasheet we have is already detailed and manufacturers never document everything (even for there customers).

I will see what I can do to use the existing driver, but it's definitely a bigger task...

Quoted 27 lines

Quoted 27 lines

Quoted 4 lines

Quoted 1 line

Ist that correct ?

Mauro, could you please elaborate your plan ?
What exactly do you want me to do to get this device supported by the kernel ?

Basically, the core em28xx module will have:

	drivers/media/usb/em28xx/em28xx-cards.c
	drivers/media/usb/em28xx/em28xx-i2c.c
	drivers/media/usb/em28xx/em28xx-core.c

Eventually, part of the functions under em28xx-core could be moved to em28xx-video, as they would be used just there. For the already supported em2710/em2750 webcams, we should need to change the logic there to use the new gspca-em2xxx module, but this change can be done later.

The em28xx-alsa module already have:

	drivers/media/usb/em28xx/em28xx-audio.c

Nothing changes on it.

The em28xx-dvb module already have:

	drivers/media/usb/em28xx/em28xx-dvb.c

Nothing changes on it.

The new em28xx-v4l module will have:

	drivers/media/usb/em28xx/em28xx-vbi.c
	drivers/media/usb/em28xx/em28xx-video.c

Quoted 8 lines

Quoted 7 lines

The em28xx-rc module already have:

	drivers/media/usb/em28xx/em28xx-input.c

It makes sense to split it into two separate files, one with just the remote control stuff, and the other one with the webcam snapshot buttons.

The file with the webcam buttons support should be merged with the em28xx gspca module, together with the code you wrote.

Ok, but then there is not much left in the gspca driver. ;)

Yes.
That's what I said at the beginning of those discussions ;)

Quoted 4 lines

The two main remaining things feature blocks are
- USB-bulk-support
- data/frame processing
and I think it would make sense to have them both in em28xx, too.

Ok. Well, DVB uses dvb bulk (or, at least, with some variants). So, maybe part of the code could be moved to em28xx core, if makes sense.

Quoted 3 lines

I would say that the main reason for a gspca subdriver would be
- the different code for i2c bus B (there is still a chance that this is something SpeedLink specific !)

See cx88: there's one special board that requires a special code for a second bus. It was mapped there as a separate driver (see drivers/media/pci/cx88/cx88-vp3054-i2c.c).

It could make sense to put the secondary em28xx I2C bus on a separate driver, especially if the code there are really specific for SpeedLink (I don't think so, but tests are of course needed).

Quoted 4 lines

- the complicating buttons stuff
- the ov2640 code (as long as no sub-module is used for that) which would require adding lots of new code to the em28xx-driver not needed by 95% of the devices.

Separate drivers for sensors make sense.

I would love to split the sensors
from the gspca drivers. It is still there simply because gspca driver is really old, and nobody has all devices supported there. So, changing it for old drivers will likely never happen. Newer drivers are doing the same thing just because of the inercial movement... developers there just used to not split i2c... changing to a new model requires them some time and effort.

Quoted 2 lines

The big question is, which devices can we expect to appear in the future ?

We'll never know that ;)

Quoted 2 lines

I'm pretty sure there are much more cameras (like the mentioned intraoral camera devices fro dentists).

Some of the webcams already supported by em28xx driver are intraoral ones.

Quoted 5 lines

As I said before, I can see the
Windows driver probing 4 ic2 slave addresses.
Tuner i2c clients are also mentioned in the em2580/em2585 datasheet, so these chips could be designed for TV stuff, too (although we haven't seen such a device yet).

Provided that the vast majority of registers don't change, I'm in favor of keeping just one driver. If they end to come with new concepts, a completely different register set, etc, then a new driver makes sense.

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

In the mean time I have ported the gspca driver to the ic2 interface, which was necessary to experiment with the ov2640 soc-camera driver,

Good!

It still needs some issues to be resolved (there seems to be a nice USB-locking issue cause by the interaction with the gspca_main module...).

Yeah, a locking with gspca will require a new locking schema. We'll need it anyway, in order to provide proper locking for cases where the same physical device uses more than one independent driver, like:
- snd-usb-audio em28xx;
- IR mce driver and cx231xx;
...

Greg KH once proposed to extend the devres subsystem in order to handle resource locking between different drivers. That looked promising, but, unfortunately, I got distracted by hundreds of other things, and didn't have any time yet to dig into it and write some RFC patches.

Quoted 9 lines

Quoted 5 lines

Quoted 4 lines

but also increased the code size enormously.

Why?

Additional functions (i2c_xfer, i2c_xfer, ...), structs (i2c_algorithm, i2c_adapter, i2c_client, ...).
Also some code duplication is required (e.g. i2c_check_for_device methods) And of course everything times 2 because we have two busses.

Ok. Well, this is not that big ;)

Quoted 16 lines

Quoted 10 lines

Quoted 10 lines

Unfortunately, lots of changes to the ov2640 driver would be necessary use it. It starts with the init sequence and continues with a long sequence of sensor register writes at capturing start.
My hope was, that the differences can be neglected, but unfortunately this is not the case.
Given the amount of work, the fact that most of these registers are undocumented and the high risk to break things for other users of the driver, I think we should stay with the "custom" code for this webcam at least for the moment.

Quoted 2 lines

Well, then do a new ov2640 i2c driver. We can later try to merge both, if we can get enough spec data.

I'm not sure we will ever get them.
The datasheet we have is already detailed and manufacturers never document everything (even for there customers).

Quoted 2 lines

I will see what I can do to use the existing driver, but it's definitely a bigger task...

One thing that you may do is to add a platform_data config option there that tells it to behave as expected by em28xx. This is a hack, if we don't know exactly what's different, as we generally prefer to pass parameters that enable or disable known features. Yet, this is better than duplicating the driver, as, by having it altogether, it will be easier to later fix it.

Regards,
Mauro