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

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

[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/linux-media@vger.kernel.org/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 ?


Regards,
Frank Schäfer

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

[Hans de Goede]

Hi,

On 08/20/2012 01:41 PM, Frank Schäfer wrote:
> 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/linux-media@vger.kernel.org/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 ?

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.

Regards,

Hans

p.s.

Frank have you seen this mail, it seems another Linux user has the same
camera, perhaps he can run some tests for you:
http://osdir.com/ml/linux-media/2009-05/msg00186.html

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

[Mauro Carvalho Chehab]

Em 20-08-2012 10:02, Hans de Goede escreveu:
> Hi,
> 
> On 08/20/2012 01:41 PM, Frank Schäfer wrote:
>> 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/linux-media@vger.kernel.org/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)

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

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

Sure about that? It doesn't use I2C?

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

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

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

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

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

>>
>> gspca:
>> + driver already exists (see patch)

Which 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

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?

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

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

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

[Hans de Goede]

Hi,

On 08/20/2012 09:21 PM, Mauro Carvalho Chehab wrote:
> Em 20-08-2012 10:02, Hans de Goede escreveu:

<snip>

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

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.

Regards,

Hans

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

[Mauro Carvalho Chehab]

Em 20-08-2012 17:46, Hans de Goede escreveu:
> Hi,
> 
> On 08/20/2012 09:21 PM, Mauro Carvalho Chehab wrote:
>> Em 20-08-2012 10:02, Hans de Goede escreveu:
> 
> <snip>
> 
>>> 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.
> 
> Right, like the one I gave to Douglas and you or Douglas (don't remember) added
> support for.

Yes. There are also some other similar cameras, including some special
ones (orthodontist usage, afaikt), that worked with the same driver, but
with a different sensor.

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

Yep. Let's see his findings before taking any decision on that.

Regards,
Mauro

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

[Frank Schäfer]

Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:
> Em 20-08-2012 10:02, Hans de Goede escreveu:
>> Hi,
>>
>> On 08/20/2012 01:41 PM, Frank Schäfer wrote:
>>> 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/linux-media@vger.kernel.org/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)
> 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...


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


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

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

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


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

>>> 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 ;)
>
>>> gspca:
>>> + driver already exists (see patch)
> Which patch?

See above.

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

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

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

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

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

Regards,
Frank

> Regards,
> Mauro

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

[Mauro Carvalho Chehab]

Em 21-08-2012 08:35, Frank Schäfer escreveu:
> Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:
>> Em 20-08-2012 10:02, Hans de Goede escreveu:
>>> Hi,
>>>
>>> On 08/20/2012 01:41 PM, Frank Schäfer wrote:
>>>> 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/linux-media@vger.kernel.org/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)
>> 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...

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

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

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

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

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

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

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

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.

> I've sent it to the list 3 minutes after I started this thread and also
> CC'ed you.
> 
> 
>>>> 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.

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.

>>>> 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 ;)
>>
>>>> gspca:
>>>> + driver already exists (see patch)
>> Which patch?
> 
> See above.
> 
>>>> + 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.
> 
> 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.

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

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

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

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

Regards,
Mauro

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

[Frank Schäfer]

Am 21.08.2012 14:32, schrieb Mauro Carvalho Chehab:
> Em 21-08-2012 08:35, Frank Schäfer escreveu:
>> Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:
>>> Em 20-08-2012 10:02, Hans de Goede escreveu:
>>>> Hi,
>>>>
>>>> On 08/20/2012 01:41 PM, Frank Schäfer wrote:
>>>>> 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/linux-media@vger.kernel.org/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)
>>> 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...
> Perhaps I'm tricked by tm6000 code... both codes are similar.
> There are a few differences with regards to isoc/bulk hanlding
> there.
>
>>>>> - 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

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


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

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

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 ?


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

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

>
>> I've sent it to the list 3 minutes after I started this thread and also
>> CC'ed you.
>>
>>
>>>>> 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.
> 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

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

>
>>>>> 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 ;)
>>>
>>>>> gspca:
>>>>> + driver already exists (see patch)
>>> Which patch?
>> See above.
>>
>>>>> + 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.
>> 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.
>
>> 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...
>>
>>> 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.
>
>> 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.
>>
>>> 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.
Ok.
Have you seen any devices with both interfaces so far ?

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

>
>>>>> 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.
>>> 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 ?
> There are some USB ID's at em28xx that are used by both grabber devices and
> webcams.

Great. :(

Regards,
Frank

>
> Regards,
> Mauro

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

[Mauro Carvalho Chehab]

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:
> Am 21.08.2012 14:32, schrieb Mauro Carvalho Chehab:
>> Em 21-08-2012 08:35, Frank Schäfer escreveu:
>>> Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:
>>>> Em 20-08-2012 10:02, Hans de Goede escreveu:
>>>>> Hi,
>>>>>
>>>>> On 08/20/2012 01:41 PM, Frank Schäfer wrote:
>>>>>> 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/linux-media@vger.kernel.org/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)
>>>> 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...
>> Perhaps I'm tricked by tm6000 code... both codes are similar.
>> There are a few differences with regards to isoc/bulk hanlding
>> there.
>>
>>>>>> - 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 ;)

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

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.

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

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

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

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

>>> I've sent it to the list 3 minutes after I started this thread and also
>>> CC'ed you.
>>>
>>>
>>>>>> 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.
>> 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.

> 1ae7:2001    SpeedLink Snappy Microphone
> 
>> 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).

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.

>>>>>> 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 ;)
>>>>
>>>>>> gspca:
>>>>>> + driver already exists (see patch)
>>>> Which patch?
>>> See above.
>>>
>>>>>> + 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.
>>> 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.
>>
>>> 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...
>>>
>>>> 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.
>>
>>> 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.
>>>
>>>> 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.
> Ok.
> Have you seen any devices with both interfaces so far ?

AFAIK, it is either one or the other.

>>
>> 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 ?
> 
>>
>>>>>> 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.
>>>> 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 ?
>> There are some USB ID's at em28xx that are used by both grabber devices and
>> webcams.
> 
> Great. :(
> 
> Regards,
> Frank
> 
>>
>> Regards,
>> Mauro
> 

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

[Frank Schäfer]

Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:
> 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...


>
> Em 21-08-2012 13:04, Frank Schäfer escreveu:
>> Am 21.08.2012 14:32, schrieb Mauro Carvalho Chehab:
>>> Em 21-08-2012 08:35, Frank Schäfer escreveu:
>>>> Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:
>>>>> Em 20-08-2012 10:02, Hans de Goede escreveu:
>>>>>> Hi,
>>>>>>
>>>>>> On 08/20/2012 01:41 PM, Frank Schäfer wrote:
>>>>>>> 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/linux-media@vger.kernel.org/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)
>>>>> 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...
>>> Perhaps I'm tricked by tm6000 code... both codes are similar.
>>> There are a few differences with regards to isoc/bulk hanlding
>>> there.
>>>
>>>>>>> - 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. ;)

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

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

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


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

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

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


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

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

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

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

>>>> I've sent it to the list 3 minutes after I started this thread and also
>>>> CC'ed you.
>>>>
>>>>
>>>>>>> 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.
>>> 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.
>
>> 1ae7:2001    SpeedLink Snappy Microphone
>>
>>> 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).
> 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.

Regards,
Frank

>
>>>>>>> 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 ;)
>>>>>
>>>>>>> gspca:
>>>>>>> + driver already exists (see patch)
>>>>> Which patch?
>>>> See above.
>>>>
>>>>>>> + 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.
>>>> 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.
>>>
>>>> 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...
>>>>
>>>>> 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.
>>>
>>>> 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.
>>>>
>>>>> 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.
>> Ok.
>> Have you seen any devices with both interfaces so far ?
> AFAIK, it is either one or the other.
>
>>> 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 ?
>>
>>>>>>> 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.
>>>>> 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 ?
>>> There are some USB ID's at em28xx that are used by both grabber devices and
>>> webcams.
>> Great. :(
>>
>> Regards,
>> Frank
>>
>>> Regards,
>>> Mauro

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

[Mauro Carvalho Chehab]

Em 22-08-2012 04:53, Frank Schäfer escreveu:
> Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:
>> 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 ???

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

> 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...
> 
> 
>>
>> Em 21-08-2012 13:04, Frank Schäfer escreveu:
>>> Am 21.08.2012 14:32, schrieb Mauro Carvalho Chehab:
>>>> Em 21-08-2012 08:35, Frank Schäfer escreveu:
>>>>> Am 20.08.2012 21:21, schrieb Mauro Carvalho Chehab:
>>>>>> Em 20-08-2012 10:02, Hans de Goede escreveu:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 08/20/2012 01:41 PM, Frank Schäfer wrote:
>>>>>>>> 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/linux-media@vger.kernel.org/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)
>>>>>> 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...
>>>> Perhaps I'm tricked by tm6000 code... both codes are similar.
>>>> There are a few differences with regards to isoc/bulk hanlding
>>>> there.
>>>>
>>>>>>>> - 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. ;)
> 
>>>>> 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...
>> 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.
> 
> 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.

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

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.

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

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

> 
>> 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.
> 
> Ok, I will a look it. But that could take some time... ;)

Ok.

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

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've sent it to the list 3 minutes after I started this thread and also
>>>>> CC'ed you.
>>>>>
>>>>>
>>>>>>>> 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.
>>>> 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.
>>
>>> 1ae7:2001    SpeedLink Snappy Microphone
>>>
>>>> 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).
>> 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

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

[Frank Schäfer]

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


Am 22.08.2012 20:15, schrieb Mauro Carvalho Chehab:
> Em 22-08-2012 04:53, Frank Schäfer escreveu:
>> Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:
>>> 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 ???
> 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.

...
[snip]

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

A quick test failed.
I enabled EM2874_I2C_SECONDARY_BUS_SELECT and tried to read from the
sensor (and other slave addresses).
It seems reading from the bus then works for ALL slave addresses but all
data bytes are 0x00.

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

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

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

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 ?


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

>
>>
>>> 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.
>> Ok, I will a look it. But that could take some time... ;)
> Ok.
>
>>>>>> 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.
>>>>
>>>>>>>>> - 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.
>>>>>>
>>>>>>>>> - 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
>>>>> 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 ?
>>> 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.
> 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

>
>>>>>> I've sent it to the list 3 minutes after I started this thread and also
>>>>>> CC'ed you.
>>>>>>
>>>>>>
>>>>>>>>> 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.
>>>>> 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.
>>>
>>>> 1ae7:2001    SpeedLink Snappy Microphone
>>>>
>>>>> 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).
>>> 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

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

[Frank Schäfer]

Ping !

Am 26.08.2012 20:53, schrieb Frank Schäfer:
> Sorry for the delayed reply, I got distracted by something with higher
> prority.
>
>
> Am 22.08.2012 20:15, schrieb Mauro Carvalho Chehab:
>> Em 22-08-2012 04:53, Frank Schäfer escreveu:
>>> Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:
>>>> 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 ???
>> 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>
>>>> 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.
>>>
>>>
>>>>>> 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.
>>>>>
>>>>>>> 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.
>>>>>>
>>>>>>>>>> - 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).
>>>>> 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.
>>>
>>>>> 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.
>>>
>>>> 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 ;) ).
>> 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.
> Ok, thanks for your explanations.
> I will see what I can do.
>
>>>> 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.
> 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 ?

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

>
>>> 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.
>
>>>> 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.
>>> Ok, I will a look it. But that could take some time... ;)
>> Ok.
>>
>>>>>>> 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.
>>>>>
>>>>>>>>>> - 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.
>>>>>>>
>>>>>>>>>> - 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
>>>>>> 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 ?
>>>> 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.
>> 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
>
>>>>>>> I've sent it to the list 3 minutes after I started this thread and also
>>>>>>> CC'ed you.
>>>>>>>
>>>>>>>
>>>>>>>>>> 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.
>>>>>> 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.
>>>>
>>>>> 1ae7:2001    SpeedLink Snappy Microphone
>>>>>
>>>>>> 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).
>>>> 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

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

[Mauro Carvalho Chehab]

Em Sun, 23 Sep 2012 16:03:25 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Ping !

Sorry, too busy those days.
> 
> Am 26.08.2012 20:53, schrieb Frank Schäfer:
> > Sorry for the delayed reply, I got distracted by something with higher
> > prority.
> >
> >
> > Am 22.08.2012 20:15, schrieb Mauro Carvalho Chehab:
> >> Em 22-08-2012 04:53, Frank Schäfer escreveu:
> >>> Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:
> >>>> 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 ???
> >> 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.

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


> 
> <snip>
> >>>> 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.
> >>>
> >>>
> >>>>>> 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.
> >>>>>
> >>>>>>> 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.
> >>>>>>
> >>>>>>>>>> - 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).
> >>>>> 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.
> >>>
> >>>>> 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.
> >>>
> >>>> 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 ;) ).
> >> 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.
> > Ok, thanks for your explanations.
> > I will see what I can do.
> >
> >>>> 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.
> > 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.

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

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

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

> but also increased the code size enormously.

Why?

> 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.
> 
> Please also consider the time factor.
> 
> Regards,
> Frank
> 

Regards,
Mauro

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

[Mauro Carvalho Chehab]

Em Sat, 6 Oct 2012 08:56:24 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:

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

Just to be clearer here: we should only fork ov2640 if we can't find a way
to re-use the existing driver. Forked drivers are always a maintenance
headache.

Cheers,
Mauro

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

[Devin Heitmueller]

On Sat, Oct 6, 2012 at 7:56 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> 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

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

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

[Frank Schäfer]

Am 06.10.2012 13:56, schrieb Mauro Carvalho Chehab:
> Em Sun, 23 Sep 2012 16:03:25 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Ping !
> Sorry, too busy those days.

No problem.

>> Am 26.08.2012 20:53, schrieb Frank Schäfer:
>>> Sorry for the delayed reply, I got distracted by something with higher
>>> prority.
>>>
>>>
>>> Am 22.08.2012 20:15, schrieb Mauro Carvalho Chehab:
>>>> Em 22-08-2012 04:53, Frank Schäfer escreveu:
>>>>> Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:
>>>>>> 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 ???
>>>> 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.
>
>> 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.
>
>
>> <snip>
>>>>>> 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.
>>>>>
>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>>>>>> - 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).
>>>>>>> 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.
>>>>>
>>>>>>> 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.
>>>>>
>>>>>> 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 ;) ).
>>>> 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.
>>> Ok, thanks for your explanations.
>>> I will see what I can do.
>>>
>>>>>> 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.
>>> 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.
>
>>> - 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.

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

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


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

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

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

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

Regards,
Frank

>> Please also consider the time factor.
>>
>> Regards,
>> Frank
>>
> Regards,
> Mauro

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

[Frank Schäfer]

Am 07.10.2012 04:56, schrieb Devin Heitmueller:
> On Sat, Oct 6, 2012 at 7:56 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> 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).

Yes, I've read the comment in the code...

According to the (possibly outdated) em2580/em2585 datasheet I've found,
these chips support 16bit eeproms ONLY.
What do we know about the others ? Are there any chips which support
both 8bit and 16bit eeproms ?
Maybe we can make it depending on the chip_id.

With regards to eeprom type probing:
I've made some experiments to find out what happens when trying to
access the 16bit eeprom in my device as 8bit eeprom.
My hope was to get a clear result like an i2c error, no data or all
bytes beeing 0x00 or 0xff.
Unfortunately, there is no error and I'm getting random data (would have
to cerify if it's really "random").
So probing will be difficult.

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

Indeed, we should be very careful.

Regards,
Frank

> Devin
>

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

[Mauro Carvalho Chehab]

Em Sun, 07 Oct 2012 15:41:50 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 06.10.2012 13:56, schrieb Mauro Carvalho Chehab:
> > Em Sun, 23 Sep 2012 16:03:25 +0200
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Ping !
> > Sorry, too busy those days.
> 
> No problem.
> 
> >> Am 26.08.2012 20:53, schrieb Frank Schäfer:
> >>> Sorry for the delayed reply, I got distracted by something with higher
> >>> prority.
> >>>
> >>>
> >>> Am 22.08.2012 20:15, schrieb Mauro Carvalho Chehab:
> >>>> Em 22-08-2012 04:53, Frank Schäfer escreveu:
> >>>>> Am 21.08.2012 19:29, schrieb Mauro Carvalho Chehab:
> >>>>>> 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 ???
> >>>> 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.
> >
> >> 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.
> >
> >
> >> <snip>
> >>>>>> 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.
> >>>>>
> >>>>>
> >>>>>>>> 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.
> >>>>>>>
> >>>>>>>>> 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.
> >>>>>>>>
> >>>>>>>>>>>> - 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).
> >>>>>>> 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.
> >>>>>
> >>>>>>> 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.
> >>>>>
> >>>>>> 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 ;) ).
> >>>> 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.
> >>> Ok, thanks for your explanations.
> >>> I will see what I can do.
> >>>
> >>>>>> 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.
> >>> 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.
> >
> >>> - 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.
> 
> >>> 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.
> 
> Ok, but then there is not much left in the gspca driver. ;)

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

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

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

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

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

We'll never know that ;)

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

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

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

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

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

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

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

[Frank Schäfer]

Am 07.10.2012 18:08, schrieb Mauro Carvalho Chehab:

<snip>
>>>>> 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.
>> Ok, but then there is not much left in the gspca driver. ;)
> Yes.
> That's what I said at the beginning of those discussions ;)
>
>> 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.
>
>> 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).
>
>> - 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.

In theory, that's the best solution.

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

The problem is, that many drivers are not written written from scratch
based on a complete datasheet / knowledge of the device.
If that would be the case, everything would be fine.
But for drivers based on reverse-engineering and with incomplete or even
missing hardware specs, the usage of sub-drivers means a time-intensive
and often problematic extra conversion step.
This is also why the gspca-conversion will probably never happen. Even
if we could test all those devices, the amount of work would be enormous
and there will likely always be any "magic register sequences" needed
for one driver but not working with another one.
And forcing a new driver to be converted to using subdrivers before
getting accepted for kernel inclusion can easily result in several
further kernel releases before users get their devices working.

Of course that doesn't mean we should give up the goal ! But we should
also be aware of the drawbacks.
The trick will be to find the right balance between ideology and pragmatism.


Don't get me wrong, I don't want to complain about this special case.
But maybe it's a good example for the problems.
Some people might give up earlier than me... ;)


>
>> The big question is, which devices can we expect to appear in the future ?
> We'll never know that ;)
>
>> 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.
>
>> 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.

I agree.
At the moment, it looks like the em25xx uses a subset of the em28xx
registers.
IIRC, there is only a single register (related to audio mute) which the
em28xx driver doesn't know/use yet and I wouldn't wonder if it works
with the em27xx/em28xx, too.

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

Uff... so yet another big point on the todo list. :(
I'm not sure that we can expect the device beeing supported by the
kernel before 2015...

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

Last time I checked it were >200 lines of extra code. Cleaning up the
code will reduce this, but not that much.

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

Hmm... might be a good idea.

Regards,
Frank

>
> Regards,
> Mauro