getting things ready
Josua Grawitter
As one would expect I want this branch to contain all changes we pushed away
because "they will only be done when we go into the kernel". I started with
removeing all compatibility for older kernels and the doxygen stuff.
Unfortunatly Fedora needs another month to give me a stable way of using
2.6.29 which means I find myself quite unable to test anything from now on.
So please test and get everything ready:
- Makefile
- Kconfig
- .config
are what comes to mind.
Lets get this thing over with.
GWater
Brian Johnson
issues reported by checkpatch mostly just lines is greater then 80
character warnings. There is still one error in sn9c20x.h but it
really a false positive.
Also quick patch to remove the module param and sysfs code for dealing
with fps since this was never really implemented and is unused.
Josua Grawitter
> Lets make microdia webcams plug'n play, (currently plug'n pray)
> To post to this group, send email to micr...@googlegroups.com
> Visit us online https://groups.google.com/group/microdia
> -~----------~----~----~----~------~----~------~--~---
Hey that's great. What kind of magic did you apply to fix all these warnings?
GWater
Brian Johnson
There were also a bunch of lines that had doxygen comments following
variable names or defines that i removed which nixed a lot of the over
80 character warnings as well.
I've also pushed a patch that fixes up Kconfig and Makefile to allow
correct compilation against the latest kernel from git. As of now i
can simply drop the files from our source repository into a directory
called sn9c20x under drivers/media/video/ and add the appriorate lines
to the parent Kconfig and Makefiles under drivers/media/video and i'm
able to configure and compile a kernel that includes the sn9c20x
driver
On Thu, Apr 30, 2009 at 3:51 PM, Josua Grawitter
Josua Grawitter
> Lets make microdia webcams plug'n play, (currently plug'n pray)
> To post to this group, send email to micr...@googlegroups.com
> Visit us online https://groups.google.com/group/microdia
> -~----------~----~----~----~------~----~------~--~---
Great. So who mails a patch/git-pull-request to the respnsible maintainer
(GKH, isn't it?) ?
GWater
Vasily Khoruzhick
> Great. So who mails a patch/git-pull-request to the respnsible maintainer
> (GKH, isn't it?) ?
>
> GWater
One more thing remains:
tags file should be removed, it is not necessary for build :)
Regards
Vasily
Vasily Khoruzhick
Oops, my fault, I didn't notice that it is not under version control.
Btw, here's some patches to review
Regards
Vasily
Brian Johnson
Martin Olsson
> I just created a new "prepare-for-kernel" branch in our git repo.
>
This is really good news.. A huge "congratulations" and big thank you
to all who contributed to these bits!
Are you guys targeting the .31 merge window? Or next one?
Martin
Josua Grawitter
> Lets make microdia webcams plug'n play, (currently plug'n pray)
> To post to this group, send email to micr...@googlegroups.com
> Visit us online https://groups.google.com/group/microdia
> -~----------~----~----~----~------~----~------~--~---
IMHO ASAP.
GWater
Jonathan Michalon
>> Josua Grawitter wrote:
>>> I just created a new "prepare-for-kernel" branch in our git repo.
>>>
>>> Lets get this thing over with.
>> This is really good news.. A huge "congratulations" and big thank you
>> to all who contributed to these bits!
>>
>> Are you guys targeting the .31 merge window? Or next one?
>>
>>
>>
>> Martin
>>
>> >
>
> GWater
Hello,
I'm back, my exams are finished; may I help you get things working and finalized
? I really think kernel integration is the next step now since it works pretty
good for some models and would show the few problems that are certainly always
here for the others.
Let me know how I can do something! It's too pity to hold the driver to those
who really search it.
Johndescs
GWater
Two thingsd are keeping me personally right now:
1. Brian is still experimenting with SXGA. This is a great feature but
maybe we should submit it later.
2. I don't know LKML procedure.
3. I hoped jojo as the one who started it all would give it his
blessing.
4. Also I don't wanna compile a whole kernel to see if it is working.
Anyway the current state of affairs is here:
http://repo.or.cz/w/microdia.git?a=shortlog;h=refs/heads/prepare-for-kernel
GWater
On 28 Mai, 13:57, Jonathan Michalon <studios.chalm...@no-log.org>
wrote:
Jonathan Michalon
Hash: SHA1
GWater a écrit :
> You're probably right.
>
> Two thingsd are keeping me personally right now:
> 1. Brian is still experimenting with SXGA. This is a great feature but
> maybe we should submit it later.
Yeah I was one of them who said it was available but not implemented and should
be but I thought you said yourselves it was no true SXGA but kinda fake, isn't it ?
> 2. I don't know LKML procedure.
Neither do I but it's time to learn. I'll read some doc about that tomorrow.
> 3. I hoped jojo as the one who started it all would give it his
> blessing.
Yes?
> 4. Also I don't wanna compile a whole kernel to see if it is working.
I may compile kernels, I've already done that some times without problems.
>
> Anyway the current state of affairs is here:
> http://repo.or.cz/w/microdia.git?a=shortlog;h=refs/heads/prepare-for-kernel
>
Okay, I'll try that.
> GWater
Johndescs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkoezyQACgkQ+NW4B/ApmtkctQCgkBECHK/esm+a2kM3edTzgUHA
eOwAn1hgrN9YdveDYEN9F3r5koe16TFX
=fkar
-----END PGP SIGNATURE-----
Josua Grawitter
> GWater a écrit :
> > You're probably right.
> >
> > Two thingsd are keeping me personally right now:
> > 1. Brian is still experimenting with SXGA. This is a great feature but
> > maybe we should submit it later.
>
> Yeah I was one of them who said it was available but not implemented and
> should be but I thought you said yourselves it was no true SXGA but kinda
> fake, isn't it ?
>
> > 2. I don't know LKML procedure.
>
> Neither do I but it's time to learn. I'll read some doc about that
> tomorrow.
>
> > 3. I hoped jojo as the one who started it all would give it his
> > blessing.
>
> Yes?
>
> > 4. Also I don't wanna compile a whole kernel to see if it is working.
>
> I may compile kernels, I've already done that some times without problems.
>
> > Anyway the current state of affairs is here:
> > http://repo.or.cz/w/microdia.git?a=shortlog;h=refs/heads/prepare-for-kern
> >el
>
> Okay, I'll try that.
>
> > GWater
>
> Johndescs
>
>
> Lets make microdia webcams plug'n play, (currently plug'n pray)
> To post to this group, send email to micr...@googlegroups.com
> Visit us online https://groups.google.com/group/microdia
> -~----------~----~----~----~------~----~------~--~---
OK,
there are obviously some misunderstandings here:
1. I was wrong when I said SXGA is fake. Brian made it happen using bayer.
3. All these names are driving me crazy. (Remeber the fun with Boris and
Brian?) Is "Johndescs"=="jojo" or do you just think that argument was
pointless?
Anyway, great we're progressing again :).
GWater
Jonathan Michalon
Hash: SHA1
Josua Grawitter a écrit :
>>> 3. I hoped jojo as the one who started it all would give it his
>>> blessing.
>> Yes?
>>
>
> OK,
> there are obviously some misunderstandings here:
> 1. I was wrong when I said SXGA is fake. Brian made it happen using bayer.
OK, I must have missed something.
> 3. All these names are driving me crazy. (Remeber the fun with Boris and
> Brian?) Is "Johndescs"=="jojo" or do you just think that argument was
> pointless?
I'm not THE "jojo jojo", but my true name is Jonathan, AKA johndescs because CS
is standing for Chalmion Studios and "des" is the French of "from", so read
"Jonathan from the Chalmion Studios"! I hope this would help mnemonically ;-)
As for the "Yes?" this should have been "Yes..." but the three dots in UTF-8
have been transformed to ISO-8859-1 with a '?', so my apologies. I just wanted
to approve!
>
> Anyway, great we're progressing again :).
>
Uh, I've not done anything yet...
> GWater
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkofmPcACgkQ+NW4B/Apmtm0BwCfYhRu+xGViV8GoOdWjCNMJpLu
toUAoKiiu3BU+36/SsVsEZ7tLQBT/2SD
=FLgx
-----END PGP SIGNATURE-----
Jonathan Michalon
> GWater a écrit :
>> 2. I don't know LKML procedure.
>
> Neither do I but it's time to learn. I'll read some doc about that tomorrow.
>
I've a little investigated. This is what I've noticed.
http://www.kernel.org/doc/Documentation/SubmittingDrivers
is a good basis, I don't know in which state is your source code, but it seems
to have to be checked against these requirements.
The MAINTAINERS file in the kernel tree:
2. Try to release a few ALPHA test versions to the net. Announce
them onto the kernel channel and await results. This is especially
important for device drivers, because often that's the only way
you will find things like the fact version 3 firmware needs
a magic fix you didn't know about, or some clown changed the
chips on a board and not its name. (Don't laugh! Look at the
SMC etherpower for that.)
I think we should make a tarball ready-to-test, no?
Next, there is no maintainer for the whole media/video stuff but a mailing list:
linux...@vger.kernel.org
This list may IMHO be used to announce the ALPHA presented before.
After mailing to the list, we may be driven by people knowing exactly what to do
in the specific case of this driver.
Does all this seem reasonable to you?
What pending patches / ideas is there? If SXGA is not ready, I think this
doesn't matter since it may be part of a latter update so there is not too much
to release in one block.
Johndescs
P.S. If you think I'm too quick and going in a hurry, let me know... I just hope
to help this being available for the whole community.
Josua Grawitter
> Lets make microdia webcams plug'n play, (currently plug'n pray)
> To post to this group, send email to micr...@googlegroups.com
> Visit us online https://groups.google.com/group/microdia
> -~----------~----~----~----~------~----~------~--~---
I think most we comply with most of the things mentioned:
- Copyright and License integrity were insured from the beginning.
- coding style has been fixed
- struct usb_sn9c20x mirrors the hardware(clarity)
- we have suspend/resume
- V4L2 support
- at least x86 and x86_64 have been tested
Do we want a MAINTAINER flag for our driver?
Looking back on recent activity I don't think so.
Short: I agree.
What tarball do you want to submit - our master or our prepare-for-kernel
branch?
What happened to the famous git-pull requests?
GWater
Jonathan Michalon
> I think most we comply with most of the things mentioned:
> - Copyright and License integrity were insured from the beginning.
> - coding style has been fixed
> - struct usb_sn9c20x mirrors the hardware(clarity)
> - we have suspend/resume
> - V4L2 support
> - at least x86 and x86_64 have been tested
>
> Do we want a MAINTAINER flag for our driver?
>
> Looking back on recent activity I don't think so.
>
> Short: I agree.
> What tarball do you want to submit - our master or our prepare-for-kernel
> branch?
> What happened to the famous git-pull requests?
>
> GWater
Cool that most rules are already OK.
I think we would have a place in the MAINTAINERS file from kernel tree (there is
only one on the root directory for the whole kernel).
Isn't the prepare-for-kernel one aiming on kernel integration? Logically this
should be submitted, no?
Your "famous git-pull requests" are perhaps not-so-famous: I don't know about
what you are speaking... do you mean the code should be directly grabbed from
git to be integrated?
Anyways, are some code modifications to be submitted before the so called
"freeze"? Have enough tests been done to take the responsibility of kernel
integration tentative? We should at least wait a little for the other
contributors, IMHO.
Johndecs
Josua Grawitter
> Lets make microdia webcams plug'n play, (currently plug'n pray)
> To post to this group, send email to micr...@googlegroups.com
> Visit us online https://groups.google.com/group/microdia
> -~----------~----~----~----~------~----~------~--~---
"Control: In general if there is active maintainance of a driver by
the author then patches will be redirected to them unless
they are totally obvious and without need of checking.
If you want to be the contact and update point for the
driver it is a good idea to state this in the comments,
and include an entry in MAINTAINERS for your driver."
I think we shouldn't list ourselves as maintainers because apart from SXGA
there won't be much more to contribute and people submitting patches to this
list may have to wait years until one of us answers.
I don't know of anymore changes being necessary our planned. "prepare-fr-
kernel" has been around quite some time now so I figure everyone is fine with
this driver being submitted.
Submitting prepare-for-kernel as a tarball seems wrong to me because it can't
be build on its own. You need a full kernel-tree to compile it.
git-pull-requests:
http://github.com/guides/pull-requests
example: http://lkml.org/lkml/2009/3/11/326
GWater
Jonathan Michalon
> "Control: In general if there is active maintainance of a driver by
> the author then patches will be redirected to them unless
> they are totally obvious and without need of checking.
> If you want to be the contact and update point for the
> driver it is a good idea to state this in the comments,
> and include an entry in MAINTAINERS for your driver."
>
> I think we shouldn't list ourselves as maintainers because apart from SXGA
> there won't be much more to contribute and people submitting patches to this
> list may have to wait years until one of us answers.
group stops when submitted, I don't know who would maintain this.
>
> I don't know of anymore changes being necessary our planned. "prepare-fr-
> kernel" has been around quite some time now so I figure everyone is fine with
> this driver being submitted.
>
> Submitting prepare-for-kernel as a tarball seems wrong to me because it can't
> be build on its own. You need a full kernel-tree to compile it.
say them to clone the git repo?
>
> git-pull-requests:
> http://github.com/guides/pull-requests
> example: http://lkml.org/lkml/2009/3/11/326
>
> GWater
Daniel Ribeiro
> I think we should make a tarball ready-to-test, no?
> Next, there is no maintainer for the whole media/video stuff but a mailing list:
> linux...@vger.kernel.org
> This list may IMHO be used to announce the ALPHA presented before.
> After mailing to the list, we may be driven by people knowing exactly what to do
> in the specific case of this driver.
AFAIK, Mauro Carvalho Chehab <mch...@infradead.org> is the
drivers/media maintainer.
No tarballs. You make a patch and send it inline on a mail to the
respective list and maintainer.
--
Daniel Ribeiro
Daniel Ribeiro
> 4. Also I don't wanna compile a whole kernel to see if it is working.
You will need to if you want to submit it upstream.
--
Daniel Ribeiro
Corrado Zoccolo
I'm compiling a custom kernel with microdia driver in-tree just now,
and found that, if I disable the evdev support, I get the following
errors:
CC [M] drivers/media/video/microdia/sn9c20x-usb.o
drivers/media/video/microdia/sn9c20x-usb.c: In function
‘input_thread’:
drivers/media/video/microdia/sn9c20x-usb.c:821: error: implicit
declaration of function ‘set_freezable’
drivers/media/video/microdia/sn9c20x-usb.c:823: error: implicit
declaration of function ‘kthread_should_stop’
drivers/media/video/microdia/sn9c20x-usb.c:831: error: ‘struct
usb_sn9c20x’ has no member named ‘input_gpio’
drivers/media/video/microdia/sn9c20x-usb.c:834: error: implicit
declaration of function ‘input_report_key’
drivers/media/video/microdia/sn9c20x-usb.c:834: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:835: error: ‘BTN_0’
undeclared (first use in this function)
drivers/media/video/microdia/sn9c20x-usb.c:835: error: (Each
undeclared identifier is reported only once
drivers/media/video/microdia/sn9c20x-usb.c:835: error: for each
function it appears in.)
drivers/media/video/microdia/sn9c20x-usb.c:837: error: implicit
declaration of function ‘input_sync’
drivers/media/video/microdia/sn9c20x-usb.c:837: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:840: error: implicit
declaration of function ‘wait_event_freezable_timeout’
drivers/media/video/microdia/sn9c20x-usb.c: In function
‘sn9c20x_input_init’:
drivers/media/video/microdia/sn9c20x-usb.c:852: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:852: error: implicit
declaration of function ‘input_allocate_device’
drivers/media/video/microdia/sn9c20x-usb.c:853: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:856: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:858: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:862: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:865: error: implicit
declaration of function ‘usb_to_input_id’
drivers/media/video/microdia/sn9c20x-usb.c:865: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:866: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:868: error: ‘EV_KEY’
undeclared (first use in this function)
drivers/media/video/microdia/sn9c20x-usb.c:868: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:869: error: ‘BTN_0’
undeclared (first use in this function)
drivers/media/video/microdia/sn9c20x-usb.c:869: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:870: error: ‘BTN_1’
undeclared (first use in this function)
drivers/media/video/microdia/sn9c20x-usb.c:870: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:871: error: ‘BTN_2’
undeclared (first use in this function)
drivers/media/video/microdia/sn9c20x-usb.c:871: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:872: error: ‘BTN_3’
undeclared (first use in this function)
drivers/media/video/microdia/sn9c20x-usb.c:872: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:873: error: ‘BTN_4’
undeclared (first use in this function)
drivers/media/video/microdia/sn9c20x-usb.c:873: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:874: error: ‘BTN_5’
undeclared (first use in this function)
drivers/media/video/microdia/sn9c20x-usb.c:874: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:875: error: ‘BTN_6’
undeclared (first use in this function)
drivers/media/video/microdia/sn9c20x-usb.c:875: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:876: error: ‘BTN_7’
undeclared (first use in this function)
drivers/media/video/microdia/sn9c20x-usb.c:876: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:878: error: implicit
declaration of function ‘input_register_device’
drivers/media/video/microdia/sn9c20x-usb.c:878: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:882: error: ‘struct
usb_sn9c20x’ has no member named ‘input_task’
drivers/media/video/microdia/sn9c20x-usb.c:882: error: implicit
declaration of function ‘kthread_run’
drivers/media/video/microdia/sn9c20x-usb.c:885: error: ‘struct
usb_sn9c20x’ has no member named ‘input_task’
drivers/media/video/microdia/sn9c20x-usb.c: In function
‘sn9c20x_input_cleanup’:
drivers/media/video/microdia/sn9c20x-usb.c:893: error: ‘struct
usb_sn9c20x’ has no member named ‘input_task’
drivers/media/video/microdia/sn9c20x-usb.c:894: error: implicit
declaration of function ‘kthread_stop’
drivers/media/video/microdia/sn9c20x-usb.c:894: error: ‘struct
usb_sn9c20x’ has no member named ‘input_task’
drivers/media/video/microdia/sn9c20x-usb.c:896: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:897: error: implicit
declaration of function ‘input_unregister_device’
drivers/media/video/microdia/sn9c20x-usb.c:897: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:898: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:899: error: implicit
declaration of function ‘input_free_device’
drivers/media/video/microdia/sn9c20x-usb.c:899: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
drivers/media/video/microdia/sn9c20x-usb.c:900: error: ‘struct
usb_sn9c20x’ has no member named ‘input_dev’
make[4]: *** [drivers/media/video/microdia/sn9c20x-usb.o] Error 1
make[3]: *** [drivers/media/video/microdia] Error 2
make[2]: *** [drivers/media/video] Error 2
make[1]: *** [drivers/media] Error 2
make: *** [drivers] Error 2
Enabling evdev support, the error disappears.
BTW, to compile it in-tree, I did the following:
* cloned the 2.6.30 rc8 linus tree, and copied my kernel config into it
* cloned the prepare-for-kernel branch, and put it under drivers/media/video
* applied the attached patch, to integrate the new driver with Kconfig
and Makefiles
* run make menuconfig, and enabled the driver, to be compiled as a module
* built the kernel
Corrado
__________________________________________________________________________
dott. Corrado Zoccolo mailto:czoc...@gmail.com
PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average
man. The average man seeks certainty in the eyes of the onlooker and calls
that self-confidence. The warrior seeks impeccability in his own eyes and
calls that humbleness.
Tales of Power - C. Castaneda
Brian Johnson
several weeks. Finally got it back up an running.
I've merged a the more recent changes from master into
prepare-for-kernel as well included two previous patches from Vasily
that never got committed to this branch. This should fix the issue
with compiling with evdev support disabled that Corrado was having.
I've tested compiling this in tree both built in and as a module for
the latest git kernel and it seems to work fine for me.
Regarding support for SXGA. It still only support higher resolutions
in bayer mode since i have been unable to get it to deliver full
frames in any other format. This should be safe since the patch
currently dis allows setting any other format besides Bayer when using
SXGA resolutions. Also at the moment this is only supported for ov965x
sensors since that is what i use. but additional patches to enable
SXGA mode on other capable sensors should not be hard just need
someone who can test those and make sure they work. I propose we get
the current version of this patch committed and later other patches
can add support for other sensors and possibly eventually other
formats besides Bayer;
For submitting the patch. Documentation/SubmittingPatches section 8
seems to say that for patches exceeding 40kB(ours is around 250kB) you
should instead of attaching the patch inline post a link to
it instead. I would also be inclined myself probably to add an entry
to the MAINTAINERS file as well. I is not necessary to direct patches
to this mailing list most of the webcams that have indivdual mainters
just use the linux-media list as far as i can tell.
I'm currently working on fixing up a patch against the linux kernel
that should add our driver into it.
Vasily Khoruzhick
> For submitting the patch. Documentation/SubmittingPatches section 8
> seems to say that for patches exceeding 40kB(ours is around 250kB) you
> should instead of attaching the patch inline post a link to
> it instead. I would also be inclined myself probably to add an entry
> to the MAINTAINERS file as well. I is not necessary to direct patches
> to this mailing list most of the webcams that have indivdual mainters
> just use the linux-media list as far as i can tell.
>
> I'm currently working on fixing up a patch against the linux kernel
> that should add our driver into it.
Don't forget to add our custom format (V4L2_PIX_FMT_SN9C20X_I420) to
videodev2.h and remove it from sn9c20x.h ;)
Regards
Vasily
Ralph Spitzner
> Hi guys,
> I'm compiling a custom kernel with microdia driver in-tree just now,
> and found that, if I disable the evdev support, I get the following
> errors:
> BTW, to compile it in-tree, I did the following:
> * cloned the 2.6.30 rc8 linus tree, and copied my kernel config into it
> * cloned the prepare-for-kernel branch, and put it under drivers/media/video
> * applied the attached patch, to integrate the new driver with Kconfig
> and Makefiles
> * run make menuconfig, and enabled the driver, to be compiled as a module
> * built the kernel
>
> Corrado
I did something similar, but with different 'effects',
when compiled as a module (via make menuconfig, etc.),
everything is ok. If I compile it into the Kernel
(first overlooking that dependecy on v4l being built in aswell)
it compiles, upon restart dmesg has all the right answers BUT
theres no /dev/video[0]....
Maybe I have the wrong module version, what exactly did you tell
git to get the 'prepare-for' version ?
-rasp
--
Please do not look directly into laser with remaining eye.
-fortune
Brian Johnson
I've tried it on my system with it built in to kernel 2.6.30-rc8 and
it created video0 properly. Is dmesg actually saying it registered the
device but it doesn't seem to appear?
I'm also attaching the current version of the patch i created to add
this driver into the kernel. It removes the sn9c20x format we added
and places it in videodev2.h as well as moving the README file under
Documentation. I've tested this both as a module and built in as well
as with and without evdev support and it seems to compile and work
correctly.
Vasily Khoruzhick
> Ralph,
> I've tried it on my system with it built in to kernel 2.6.30-rc8 and
> it created video0 properly. Is dmesg actually saying it registered the
> device but it doesn't seem to appear?
>
> I'm also attaching the current version of the patch i created to add
> this driver into the kernel. It removes the sn9c20x format we added
> and places it in videodev2.h as well as moving the README file under
> Documentation. I've tested this both as a module and built in as well
> as with and without evdev support and it seems to compile and work
> correctly.
I think we should get some positive responces and then you can try to submit
driver upstream. Unfortunately I have no ability to test patch until Monday.
Regards
Vasily
Daniel Ribeiro
> For submitting the patch. Documentation/SubmittingPatches section 8
> seems to say that for patches exceeding 40kB(ours is around 250kB) you
> should instead of attaching the patch inline post a link to
> it instead. I would also be inclined myself probably to add an entry
> to the MAINTAINERS file as well. I is not necessary to direct patches
> to this mailing list most of the webcams that have indivdual mainters
> just use the linux-media list as far as i can tell.
Not really, in this case you are submitting driver(s).
What you should do is to split the code on various patches, and submit
it as a series. eg, one patch for the bus, and another for each sensor,
or something like that. :)
Big patches are boring to review, doesn't matter if its on a web server
or inlined on the mail.
--
Daniel Ribeiro
Ralph Spitzner
> Ralph,
> I've tried it on my system with it built in to kernel 2.6.30-rc8 and
> it created video0 properly. Is dmesg actually saying it registered the
> device but it doesn't seem to appear?
Ok, sorry must have been a mix-up on my side of things,
rc8+patch works for me :-)
(I'm just not using *30-rc8 because I get spurious system hangs
if memory is full and it's time to swap)
Also being bored it wrote/put together a little webcam utility to
test the cam itself.
If anyone is interested, you can clone it at
git://gate.spitzner.org/flux/storage/repo/swec
(I borrowed a sbggr8_to_rgb24 from xawtv to get rid of the
libv4lconvert stuff. Which breaks at least my system)
Maybe it would be a good idea to put that into the driver (?).
regards
Brian Johnson
> Not really, in this case you are submitting driver(s).
> What you should do is to split the code on various patches, and submit
> it as a series. eg, one patch for the bus, and another for each sensor,
> or something like that. :)
sn9c20x: 184k
omnivision: 43k
micron: 30k
hv7131r: 4.4k
I could reduce the big one my about another 30k by splitting out the
sysfs and debugfs as well which would break down as
sn9c20x: 154k
sysfs: 20k
debugfs: 10k
omnivision: 43k
micron: 30k
hv7131r: 4.4k
Daniel Ribeiro
> sn9c20x: 184k
> omnivision: 43k
> micron: 30k
> hv7131r: 4.4k
>
> I could reduce the big one my about another 30k by splitting out the
> sysfs and debugfs as well which would break down as
>
> sn9c20x: 154k
> sysfs: 20k
> debugfs: 10k
> omnivision: 43k
> micron: 30k
> hv7131r: 4.4k
Thats a start. :)
I have some comments that could help reduce review ping-pong, just small
stuff that I already know are not well seen on the kernel community...
static const int RX[]
static const int RY[]
static const int GX[]
static const int GY[]
static const int BX[]
static const int BY[]
Namespace pollution. Names like these are not good, because they may
clash with other stuff on the kernel, use a more meaningful name, and
keep it lower-case (upper case is generally for macro).
Also, it may be a good idea to move these into a header file.
__u8, __u16, __u32
Just use u8, u16, u32...
545 ret = usb_sn9c20x_control_write(dev, 0x1006, led, 2);
546
547 return ret;
Just "return usb_sn9c20x...." No need for the ret variable. Also applies
to a lot of other functions.
UDIA_INFO("Using yuv420 output format\n");
This is a show stopper. People dont like these. Use dev_err|warn|dbg()
or pr_debug().
int sn9c20x_create_sysfs_files(struct video_device *vdev)
This function lacks error handling..
Other than these minor nitpicks the driver looks good. I lack the v4l
subsystem knowledge to comment on more technical stuff.
Don't forget to run scripts/checkpatch.pl on the patches, and follow the
CodingStyle.
Mauro is always at #v4l on irc.freenode.net, nick "mchehab", it may be a
good idea to come in and present the project/driver on IRC.
It's already too late for .30, but I think you have chances to get it in
for .31 if you send it for review ASAP.
Good Luck!
--
Daniel Ribeiro
Josua Grawitter
Here are patches for the first to issues.
They don't really need review but my local git thinks they're already pushed
even though they don't show up in the repo.or.cz-git webinterface
GWater
GWater
dev_*() requires "struct device *". In most functions this is
available via &(dev->udev->dev). That's ugly but usable. Unfortunatly
there are many functions that need info and debugging output which
don't have this struct available at all. I tried very hard to get
devices from usb_interfaces, v4l_devices, usb_devices but in the end I
often had to fall back to using printk() directly. And that's not
comfortable and not really an option.
My advice: Use dev_*() but redefine the macro like UDIA_*().
Thoughts, other ideas?
GWater
If anyone wants my halfdone patch... Say so.
> < 1 KBAnzeigenHerunterladen
Brian Johnson
structure is before probe is called and early in the probe function
before we create the device structure. Where there other areas that
you found Josua? The other issue is of course that our current macros
besides just printing the message will check the log level allowing
the driver to dynamically adjust which messages get displayed.
I'm attaching the current patch-set against kernel 2.6.30-rc8 for
testing purposes
Changes:
* moved to header and renamed RX,RY,GX,GY,BX and BY arrays
* changed __u* to u*
* some error checking to create_sysfs function
* changed many ret = ... to return ... getting rid of unnecessary ret variable
* added current SXGA patches for ov965x sensors (forces bayer fmt)
* input device now returns KEY_CAMERA instead of BTN_0
GWater
queue.c. Then there are the init functions in sn9c20x-ub.c and several
functions in sn9c20x-sysfs.c (the problem can be fixed by getting the
device from the *priv thingy. However I don't think this is really
worth the effort and I think there will be performance loss.
GWater
On 8 Jun., 08:15, Brian Johnson <brij...@gmail.com> wrote:
> I think the main places i know of were you can't access the device
> structure is before probe is called and early in the probe function
> before we create the device structure. Where there other areas that
> you found Josua? The other issue is of course that our current macros
> besides just printing the message will check the log level allowing
> the driver to dynamically adjust which messages get displayed.
>
> I'm attaching the current patch-set against kernel 2.6.30-rc8 for
> testing purposes
>
> Changes:
> * moved to header and renamed RX,RY,GX,GY,BX and BY arrays
> * changed __u* to u*
> * some error checking to create_sysfs function
> * changed many ret = ... to return ... getting rid of unnecessary ret variable
> * added current SXGA patches for ov965x sensors (forces bayer fmt)
> * input device now returns KEY_CAMERA instead of BTN_0
>
> 203KAnzeigenHerunterladen
>
> 0002-sn9c20x-Add-sysfs-support.patch
> 31KAnzeigenHerunterladen
>
> 0003-sn9c20x-Add-debugfs-support.patch
> 21KAnzeigenHerunterladen
>
> 0004-sn9c20x-Add-support-for-omnivision-sensors.patch
> 56KAnzeigenHerunterladen
>
> 0005-sn9c20x-Add-support-for-micron-sensors.patch
> 35KAnzeigenHerunterladen
>
> 0006-sn9c20x-Add-hv7131r-sensor-support.patch
> 6KAnzeigenHerunterladen
Daniel Ribeiro
> The device structure is not available in many functions in sn9c20x-
> queue.c. Then there are the init functions in sn9c20x-ub.c and several
> functions in sn9c20x-sysfs.c (the problem can be fixed by getting the
> device from the *priv thingy. However I don't think this is really
> worth the effort and I think there will be performance loss.
There is no performance loss on dereferencing a pointer. A printk costs
more than 1000 (just guessing) pointer dereferences.
All sysfs functions take a struct device * as the first argument. Only
thing wrong in there is that you are naming it "class". But what is the
point here? There is no UDIA_* on -sysfs.c
On -usb.c just use udev->dev.
On -queue.c i see that you have a single sn9c20x_video_queue for each
each struct usb_sn9c20x. You should consider not using a struct
sn9c20x_video_queue at all, and just merge your child fields on the
parent struct. If its not an option for you, then you can get your
struct usb_sn9c20x using the container_of macro. eg:
struct usb_sn9c20x *dev = container_of(queue, struct usb_sn9c20x, queue)
> On 8 Jun., 08:15, Brian Johnson <brij...@gmail.com> wrote:
> The other issue is of course that our current macros
> besides just printing the message will check the log level allowing
> the driver to dynamically adjust which messages get displayed.
You shouldn't invent your own log level facility, the kernel already has
one and filtering is done by standard sys(k)logd.
Use dev_[info|warn|err|dbg]. dev_dbg needs #define DEBUG to actually be
on the code.
Some other minor nitpicks....
Dont: },
{
use }, { instead.
Regarding comments format, dont:
300 /*some hardcoded values are present
301 *like those for maximal/minimal exposure
302 *and exposure steps*/
/* single line (dont forget the spaces after start and before end) */
/*
* Multi-line comment
* line 2
* line 3
*/
577 if (ret < 0)
578 return ret;
579 else
580 return 0;
No need for "else" after "return".
613 udelay(delay);
Urgh! Do you really want to block all the kernel while you wait for
USB/I2C I/O? This is a MEGA performance killer. Use usleep() instead.
--
Daniel Ribeiro
Brian Johnson
I've changed most of the UDIA_* to dev_* however currently there are
some messages being printed from the drivers init and exit functions
which do not have the dev structure available. Should i replace those
with just a printk or remove them completely?
Also the kernel doesn't have a usleep function as far as i can tell.
2009/6/9 Daniel Ribeiro <drw...@gmail.com>:
GWater
Both gspca and sn9c1xx have fewer module paramters, none of the
related to image settings. I think we can remove the debugging
messages in the init. And maybe remove most of these parameters, too.
The only ones providing real functionality are jpeg, bulk, bandwidth
and max/min_buffers. Maybe we should cut it down to them and show
error messages when we actually act use their values for something.
GWater
PS: sn9c1xx seems to have its own debugging facility. but the code
looks strange anyway.
Daniel Ribeiro
> Daniel,
> I've changed most of the UDIA_* to dev_* however currently there are
> some messages being printed from the drivers init and exit functions
> which do not have the dev structure available. Should i replace those
> with just a printk or remove them completely?
I would just remove them completely. But if you want to keep, replace
for printk() if they are informational message, or for pr_debug() if
they are debugging messages.
> Also the kernel doesn't have a usleep function as far as i can tell.
Sorry, you are right. Consider using msleep() instead. It may have an
impact on your driver performance tough. But at least it will not steal
the kernel and block it from scheduling everything else, so overall
impact on system should be smaller.
--
Daniel Ribeiro
Brian Johnson
I:ve also removed most of the image control module parameters whcih
cuts size down some as well as taking out the same ones from teh sysfs
since you really should be setting them with v4l controls instead.
Currently the params and sysfs entries look like the following.
module params:
jpeg
bulk
bandwidth
hflip
vlfip
min_buffers
max_buffers
v4l_debug
sysfs:
release
information
videostatus
hlfip
vflip
v4l_debug replaces the old log_level and is a boolean value that will
enable ioctl debugging output from teh v4l subsystem. I left vlfip in
since some cameras by default preoduce upside down output, hflip was
left in for symmetry.
GWater
That leaves some style thingsan the error handling in
sn9c20x_create_sysfs_files().
I'll get on the style stuff if you commit the patches we already
have.
(Rebasing massive amounts of small changes fails sometimes and it
would be a
shame to waste time on this boring stuff.)
GWater
Brian Johnson
I don't really have individual patches against our repository since
all these things i've been fixing are based in the six patches against
the kernel I'm preparing. I am attaching a patch you can apply to the
HEAD of prepare-for-kernel that should sync it against the current
kernel codebase I'm using.
Josua Grawitter
> I've already done the error handling on create_sysfs as well.
>
> I don't really have individual patches against our repository since
> all these things i've been fixing are based in the six patches against
> the kernel I'm preparing. I am attaching a patch you can apply to the
> HEAD of prepare-for-kernel that should sync it against the current
> kernel codebase I'm using.
>
It seems there really is nothing left to do ;).
Anyway upon testing I noticed several things:
1. Compiling without evdev fails because of one dev->input_gpio in the probe
function. It seems this needs an extra
#ifdef EVDEV_bla
since sn9c20x_initialize() uses dev->input_gpio directly after that. Maybe
that part of sn9c20x_initialize should be moved to input_init()?
2. dmesg says that my cam is accessable vie /dev/video1 while it is actually
using /dev/video0. Has something in the nameing conventions changed? (I use
Fedora11. They have moved some parts of HAL over to DeviceKit. However I don't
know whether this is caused by HAL at all).
Apart from that - great. Let' go fast lane for 2.6.31 ;)
GWater
Brian Johnson
to rewrite it as a
gspca subdriver. So i'm currently doing that i've got most of it done
at the moment and
will submit it to linux-media when i finish. There are some stuff that
has been removed
such as sysfs as well as currently the remaining module params. Also
i've dropped the
white-balance control since only a few sensors had that implemented
currently and there
is little reason i can see that you would want to turn it off anyways.
GWater
If you want help make a new branch in the repo. Or maybe do it anyway
just to keep things transparent ;) .
It feels a bit unfair to be forced into gspca but I understand they
want to keep things easy to maintain.
Three thoughts come to mind:
1. GSPCA was introduced with kernel 2.6.27 . We could make a branch
which focusses on supporting older kernels which lack the gspca
infrastructure. This would also keep this group useful. Anyone knows
how the driver backporting project works?
2. Should some of us subscribe o linux-media or the gspca mailing-list
to answer bugreports? After all we still have all the data on and
experience with the cams. That would require however that the code and
the changes you make are still transparent to us.
GWater
On 18 Jun., 17:54, Brian Johnson <brij...@gmail.com> wrote:
> Ok a quick update rather then submit our entire driver it is preferred
> to rewrite it as a
> gspca subdriver. So i'm currently doing that i've got most of it done
> at the moment and
> will submit it to linux-media when i finish. There are some stuff that
> has been removed
> such as sysfs as well as currently the remaining module params. Also
> i've dropped the
> white-balance control since only a few sensors had that implemented
> currently and there
> is little reason i can see that you would want to turn it off anyways.
>
> On Thu, Jun 11, 2009 at 3:47 PM, Josua Grawitter
>
Brian Johnson
so when reworking this as a gspca module so much changes in the
general structure tha from gits point of view i basically deleted all
the files and replaced them with 3 new ones. I am postin the current
patch whcih i've verified works properly with a 624f camera for people
look at before submitting to linux-media. This patch was written
against the latest kernel checkout but should be able to be applied
cleanly against the latest version of v4l found at
http://linuxtv.org/hg/v4l-dvb/ just hit enter and choose to skip
patching the MAINTAINERS file when it complains. Using the v4l
checkout is a lot quicker as well as you don't need to bother
upgrading your kernel just to try the driver.
Vasily Khoruzhick
> Well making a new branch on our current repo would be a bit odd since
> so when reworking this as a gspca module so much changes in the
> general structure tha from gits point of view i basically deleted all
> the files and replaced them with 3 new ones. I am postin the current
> patch whcih i've verified works properly with a 624f camera for people
> look at before submitting to linux-media. This patch was written
> against the latest kernel checkout but should be able to be applied
> cleanly against the latest version of v4l found at
> http://linuxtv.org/hg/v4l-dvb/ just hit enter and choose to skip
> patching the MAINTAINERS file when it complains. Using the v4l
> checkout is a lot quicker as well as you don't need to bother
> upgrading your kernel just to try the driver.
Works for me with my 624f cam, but it's missing gain control.
Anyway, thanks for your work ;)
Regards
Vasily
Brian Johnson
the patch and compile the whole v4l subsystem.
However if you want to compile it on its own the attached makefiile
should work for compiling.
You will need to copy the jpeg.h and gspca.h headers files from
v4l-dvb for it to work however.
Also make sure to add the following line to the sn9c20x.h header file
#define V4L2_PIX_FMT_SN9C20X_I420 v4l2_fourcc('S', '9', '2', '0')
Josua Grawitter
> Lets make microdia webcams plug'n play, (currently plug'n pray)
> To post to this group, send email to micr...@googlegroups.com
> Visit us online https://groups.google.com/group/microdia
> -~----------~----~----~----~------~----~------~--~---
Actually thats what I did. (downloading the whole v4l-dvb tree). It seems that
one macro from jpeg.h is missing (QUANT_whatever). Therefore not even the tree
builds. Anyway I'll try to get an older revision as I expect this is because
of my a bit older 2.6.29 kernel headers.
GWater
Brian Johnson
2.6.28 and a 2.6.30 kernel with no problems so it seems odds that it
would not compile against a 2.6.29 one as well.
Anyways i've attached an updated patch which I'm thinking will be the
one i likely submit. This one has two additional changes first i
increased the exposure setting to have a max of 6000 for abit of
increased granularity. Second i re-added code for the gain control.
the control control has a range of 0-28, which represents 1x-8x gain
with a step size of .25x
On Sat, Jun 20, 2009 at 3:13 PM, Josua Grawitter
Josua Grawitter
> Lets make microdia webcams plug'n play, (currently plug'n pray)
> To post to this group, send email to micr...@googlegroups.com
> Visit us online https://groups.google.com/group/microdia
> -~----------~----~----~----~------~----~------~--~---
So,
I got todays HEAD + Patch built and insmodded. Unfortunatly I get no only
blackness.
This means:
1. there is a stream (otherwise mplayer would show green)
2. It's to dark in my cave.
3. The manual gain is broken or insufficient for SOI968 (0c45:624e).
4. We need autogain reimplemented. Why are all these features missing anyway?
The advantage of "branching" this code in our repo is that all microdia
contributers can contribute to the gspca_sn9c20x rewrite. Otherwise you need
to reimplement all these faetures on your own.
GWater
Vasily Khoruzhick
> Strange I've compiled the latest version of v4l-dvb against both a
> 2.6.28 and a 2.6.30 kernel with no problems so it seems odds that it
> would not compile against a 2.6.29 one as well.
>
> Anyways i've attached an updated patch which I'm thinking will be the
> one i likely submit. This one has two additional changes first i
> increased the exposure setting to have a max of 6000 for abit of
> increased granularity. Second i re-added code for the gain control.
> the control control has a range of 0-28, which represents 1x-8x gain
> with a step size of .25x
Works for me.
Regards
Vasily
Vasily Khoruzhick
> The advantage of "branching" this code in our repo is that all microdia
> contributers can contribute to the gspca_sn9c20x rewrite. Otherwise you
> need to reimplement all these faetures on your own.
Actually, it makes sense to make another git repo with gspca_sn9c20x. I'm sure
that Brian has local repo, so it's time to publish it somewhere ;)
Regards
Vasily
Brian Johnson
> 1. there is a stream (otherwise mplayer would show green)
> 2. It's to dark in my cave.
> 3. The manual gain is broken or insufficient for SOI968 (0c45:624e).
> 4. We need autogain reimplemented. Why are all these features missing anyway?
even with low gain and exposure you ought to be able to see something?
Also when running the camera using mplayer can you check to see what
the exposure value is? start v4lucp turn on the update every second
and see if the exposure setting is in fact getting adjusted.
The gain control itself should work for all omnivision sensors i think
the formula should be: (bit[6] + 1) * (but[5] + 1) * (but[4] + 1) *
(bits[3:0]/16 + 1). This should give a maximum 8x gain.
Can you check the debugfs and find out what values are in the
following registers: 0x2d, 0x2e and 0x00 as well as what the v4l2
exposure and gain settings are set to?
> The advantage of "branching" this code in our repo is that all microdia
> contributers can contribute to the gspca_sn9c20x rewrite. Otherwise you need
> to reimplement all these faetures on your own.
repository. The other thing is since this relies on header file found
in gspca my only patchsets are against either the kernel or v4l-dvb
should e just clone a copy of v4l-dvb and make that available?
On Sun, Jun 21, 2009 at 5:39 PM, Josua Grawitter
Josua Grawitter
> Lets make microdia webcams plug'n play, (currently plug'n pray)
> To post to this group, send email to micr...@googlegroups.com
> Visit us online https://groups.google.com/group/microdia
> -~----------~----~----~----~------~----~------~--~---
A complete v4l-dvb tree sounds good. (I hadn't thought about this before,
actually)
I'll take a look at the gain code in a few hours. Anyway - there was enough
light to make it work with the standalone driver.
Thank you,
GWater