Upstreaming syntek driver

Upstreaming syntek driver

[Luca Tettamanti]

Hello,
I'm a "lucky" owner of a Syntek USB webcam (embedded on my Asus
laptop); as you might know Nicolas (CC) wrote a driver for these
cams[1][2], but it's still not included in mainline kernel.
Since I'd rather save myself and the other users the pain of compiling
an out-of-tree driver I'm offering my help to make the changes
necessary to see this driver upstreamed; I'm already a maintainer of
another driver (in hwmon), so I'm familiar with the development
process.
>From a quick overview of the code I've spotted a few problems:
- minor style issues, trivially dealt with
- missing cleanups in error paths, idem
- possible memory leak, reported on the bug tracker - requires investigation
- big switch statements for all the models, could be simplified with
function pointers

Another objection could be that the initialization is basically
writing magic numbers into magic registers... I guess that Nicolas
recorded the initialization sequence with a USB sniffer. No solution
for this one; does anybody have a contact inside Syntek?

Are there other issues blocking the inclusion of this driver?

Luca
[1] http://syntekdriver.sourceforge.net/
[2] http://syntekdriver.svn.sourceforge.net/viewvc/syntekdriver/trunk/

Re: Upstreaming syntek driver

[Hans Verkuil]

On Tuesday, January 18, 2011 23:17:11 Luca Tettamanti wrote:
> Hello,
> I'm a "lucky" owner of a Syntek USB webcam (embedded on my Asus
> laptop); as you might know Nicolas (CC) wrote a driver for these
> cams[1][2], but it's still not included in mainline kernel.
> Since I'd rather save myself and the other users the pain of compiling
> an out-of-tree driver I'm offering my help to make the changes
> necessary to see this driver upstreamed; I'm already a maintainer of
> another driver (in hwmon), so I'm familiar with the development
> process.
> From a quick overview of the code I've spotted a few problems:
> - minor style issues, trivially dealt with
> - missing cleanups in error paths, idem
> - possible memory leak, reported on the bug tracker - requires investigation
> - big switch statements for all the models, could be simplified with
> function pointers
> 
> Another objection could be that the initialization is basically
> writing magic numbers into magic registers... I guess that Nicolas
> recorded the initialization sequence with a USB sniffer. No solution
> for this one; does anybody have a contact inside Syntek?
> 
> Are there other issues blocking the inclusion of this driver?

After a quick scan through the sources in svn I found the following (in no
particular order):

- Supports easycap model with ID 05e1:0408: a driver for this model is now
  in driver/staging/easycap.

- format conversion must be moved to libv4lconvert (if that can't already be
  used out of the box). Ditto for software brightness correction.

- kill off the sysfs bits

- kill off V4L1

- use the new control framework for the control handling

- use video_ioctl2 instead of the current ioctl function

- use unlocked_ioctl instead of ioctl

But probably the first step should be to see if this can't be made part of the
gspca driver. I can't help thinking that that would be the best approach. But
I guess the gspca developers can give a better idea of how hard that is.

Regards,

       Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: Upstreaming syntek driver

[Luca Tettamanti]

On Tue, Jan 18, 2011 at 11:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
[...]
> After a quick scan through the sources in svn I found the following (in no
> particular order):
>
> - Supports easycap model with ID 05e1:0408: a driver for this model is now
>  in driver/staging/easycap.

Can you elaborate? Is this the same hardware?

> - format conversion must be moved to libv4lconvert (if that can't already be
>  used out of the box). Ditto for software brightness correction.
>
> - kill off the sysfs bits
>
> - kill off V4L1
>
> - use the new control framework for the control handling
>
> - use video_ioctl2 instead of the current ioctl function
>
> - use unlocked_ioctl instead of ioctl

Ok, major surgery then :)

> But probably the first step should be to see if this can't be made part of the
> gspca driver. I can't help thinking that that would be the best approach. But
> I guess the gspca developers can give a better idea of how hard that is.

I've looked at the framework provided by gspca, it would probably
allow to drop most of the USB support code from the driver. I'm
looking into frame handling.

Luca

Re: Upstreaming syntek driver

[Hans de Goede]

Hi,

On 01/20/2011 12:35 PM, Luca Tettamanti wrote:
> On Tue, Jan 18, 2011 at 11:45 PM, Hans Verkuil<hverkuil@xs4all.nl>  wrote:
> [...]
>> After a quick scan through the sources in svn I found the following (in no
>> particular order):
>>
>> - Supports easycap model with ID 05e1:0408: a driver for this model is now
>>   in driver/staging/easycap.
>
> Can you elaborate? Is this the same hardware?
>
>> - format conversion must be moved to libv4lconvert (if that can't already be
>>   used out of the box). Ditto for software brightness correction.
>>
>> - kill off the sysfs bits
>>
>> - kill off V4L1
>>
>> - use the new control framework for the control handling
>>
>> - use video_ioctl2 instead of the current ioctl function
>>
>> - use unlocked_ioctl instead of ioctl
>
> Ok, major surgery then :)
>
>> But probably the first step should be to see if this can't be made part of the
>> gspca driver. I can't help thinking that that would be the best approach. But
>> I guess the gspca developers can give a better idea of how hard that is.
>
> I've looked at the framework provided by gspca, it would probably
> allow to drop most of the USB support code from the driver.

Yeah, that is the whole idea :) I give a big +1 to the Hans' suggestion
to convert this to a gspca driver!

> I'm looking into frame handling.

Let me know if you need any help / explanation about how certain things
are done in gspca.

Regards,

Hans G (aka the other Hans).

Re: Upstreaming syntek driver

[Luca Tettamanti]

On Wed, Jan 19, 2011 at 8:21 AM, Nicolas <nicolas@vivien.fr> wrote:
[...]
> Now I have a new laptop. So I make only in sort that stk11xx works with new kernel releases.
>
> Of course I can help you, provide USB logs... But I haven't the time to do new developments.

Annotated logs might be useful, but for the moment I don't intend to
engage into a full scale reverse engineering effort.

Luca