[QUESTION] staging/easycap fix
Hi, I'm try to fix staging/easycap driver. I know it has some bugs somewhere (I've seen some panics while using the device) but I wanted to improve the code style before debugging it. In the meantime, I expect to develop a better understanding of the code. However, I want know if I am in the right direction; so I want to know if this is ok: 1. mainly I am splitting very large functions into smaller parts. For instance, xxx_probe function is +1k lines long, so I'm splitting it up to make the code cleaner, more readable. Is this ok? 2. second, I am fixing some style issues (besides checkpatch), for instance "if" syntax: - if (0 == bInterfaceNumber) { + if (bInterfaceNumber == 0) { and ugly comments like: -/*---------------------------------------------------------------------------*/ -/* - * GET PROPERTIES OF PROBED INTERFACE - */ -/*---------------------------------------------------------------------------*/ + + /* + * GET PROPERTIES OF PROBED INTERFACE + */ So, Am I on the right track? Thanks, Ezequiel.
On Tue, Feb 14, 2012 at 01:47:52AM -0300, Ezequiel García wrote:
Hi,
I'm try to fix staging/easycap driver. I know it has some bugs somewhere (I've seen some panics while using the device) but I wanted to improve the code style before debugging it. In the meantime, I expect to develop a better understanding of the code. However, I want know if I am in the right direction; so I want to know if this is ok:
1. mainly I am splitting very large functions into smaller parts. For instance, xxx_probe function is +1k lines long, so I'm splitting it up to make the code cleaner, more readable. Is this ok?
Yes.
2. second, I am fixing some style issues (besides checkpatch), for instance "if" syntax:
- if (0 == bInterfaceNumber) { + if (bInterfaceNumber == 0) {
You do know why the first style was chosen, right? That's not saying your change is incorrect, but odds are, there are bigger things that need to be fixed up first.
and ugly comments like:
-/*---------------------------------------------------------------------------*/ -/* - * GET PROPERTIES OF PROBED INTERFACE - */ -/*---------------------------------------------------------------------------*/ + + /* + * GET PROPERTIES OF PROBED INTERFACE + */
So, Am I on the right track?
Close, how about: /* Get properties of probed interface */ instead? thanks, greg k-h
El día 14 de febrero de 2012 02:06, Greg KH <greg@kroah.com> escribió:
On Tue, Feb 14, 2012 at 01:47:52AM -0300, Ezequiel García wrote:
2. second, I am fixing some style issues (besides checkpatch), for instance "if" syntax:
- if (0 == bInterfaceNumber) { + if (bInterfaceNumber == 0) {
You do know why the first style was chosen, right?
This driver code has this "if" style on every if clause. Perhaps author's taste? Don't know. Searching at kernel code I haven't seen this kind of "if" style.
That's not saying your change is incorrect, but odds are, there are bigger things that need to be fixed up first.
I know. It is difficult for me to refrain from cleaning ugly code :( Perhaps I should just stick to function split up for now?
So, Am I on the right track?
Close, how about: /* Get properties of probed interface */ instead?
Ok. Thanks a lot, Ezequiel.
2012/2/14 Ezequiel García <elezegarcia@gmail.com>:
El día 14 de febrero de 2012 02:06, Greg KH <greg@kroah.com> escribió:
On Tue, Feb 14, 2012 at 01:47:52AM -0300, Ezequiel García wrote:
2. second, I am fixing some style issues (besides checkpatch), for instance "if" syntax:
- if (0 == bInterfaceNumber) { + if (bInterfaceNumber == 0) {
You do know why the first style was chosen, right?
This driver code has this "if" style on every if clause. Perhaps author's taste? Don't know. Searching at kernel code I haven't seen this kind of "if" style.
This particular "if" style is recommended because it prevents the accidental "always true if condition" in case when the coder accidentally types "=" instead of "==". "if (0 = x)" can always save you instead of "if (x = 0)" from nasty typo bugs. Though compiler warns about it with -Wall flag. -- Manavendra Nath Manav
On Tue, Feb 14, 2012 at 11:29:54AM +0530, Manavendra Nath Manav wrote:
2012/2/14 Ezequiel García <elezegarcia@gmail.com>:
El día 14 de febrero de 2012 02:06, Greg KH <greg@kroah.com> escribió:
On Tue, Feb 14, 2012 at 01:47:52AM -0300, Ezequiel García wrote:
2. second, I am fixing some style issues (besides checkpatch), for instance "if" syntax:
- if (0 == bInterfaceNumber) { + if (bInterfaceNumber == 0) {
You do know why the first style was chosen, right?
This driver code has this "if" style on every if clause. Perhaps author's taste? Don't know. Searching at kernel code I haven't seen this kind of "if" style.
This particular "if" style is recommended because it prevents the accidental "always true if condition" in case when the coder accidentally types "=" instead of "==". "if (0 = x)" can always save you instead of "if (x = 0)" from nasty typo bugs. Though compiler warns about it with -Wall flag.
It's only "recommended" if you have a compiler that doesn't check for such foolish things. Luckily we do, so you don't have to write kernel code like this at all, so while cleaning it up would be "nice to have", it's not essencial to get it out of the staging tree at all. thanks, greg k-h
Hi Greg,
It's only "recommended" if you have a compiler that doesn't check for such foolish things.
Understood. Thanks both. Another question (hope you don't mind me asking so much): I noticed easycap does lots of (redundant?) checks as: pvideo_device = video_devdata(file); if (!pvideo_device) { return -EFAULT; } Is this bad, good or doesn't matter? (There are more examples where the check is clearly not needed but this one makes me doubt a bit) Maybe I'm focusing on small issues (as you already pointed out), do you think this kind of patches would be accepted? Of course, I would submit separate patches, one for each change: 1. split probe function 2. remove redundant checks 3. clean comment style ... and so on. Also, I'm reading driver cx231xx as it's also an usb video capture. Do you think it's a good code reference? Can you give me further reference? I can think on nothing except reading driver code itself and keep LDD3 and Love's book near. Thanks, Ezequiel.
On Tue, Feb 14, 2012 at 07:01:25PM -0300, Ezequiel García wrote:
Hi Greg,
It's only "recommended" if you have a compiler that doesn't check for such foolish things.
Understood. Thanks both.
Another question (hope you don't mind me asking so much):
I noticed easycap does lots of (redundant?) checks as:
pvideo_device = video_devdata(file); if (!pvideo_device) { return -EFAULT; }
Is this bad, good or doesn't matter? (There are more examples where the check is clearly not needed but this one makes me doubt a bit)
Depends on what video_devdata() does, and how it could ever be NULL.
Maybe I'm focusing on small issues (as you already pointed out), do you think this kind of patches would be accepted?
Of course, I would submit separate patches, one for each change: 1. split probe function 2. remove redundant checks 3. clean comment style ... and so on.
That sounds great.
Also, I'm reading driver cx231xx as it's also an usb video capture. Do you think it's a good code reference? Can you give me further reference?
I don't know, ask the linux-media developers on their list for a good reference driver to follow, they would know best. thanks, greg k-h
Ezequiel, The post: Intro to V4L2: http://www.linuxfordevices.com/c/a/Linux-For-Devices-Articles/Intro-to-V4L2/ And the post: The VIVI driver; a great starting point for V4L2 driver writers: http://lwn.net/Articles/203971/ May be useful. []'s Peter 2012/2/14 Greg KH <greg@kroah.com>:
On Tue, Feb 14, 2012 at 07:01:25PM -0300, Ezequiel García wrote:
Hi Greg,
It's only "recommended" if you have a compiler that doesn't check for such foolish things.
Understood. Thanks both.
Another question (hope you don't mind me asking so much):
I noticed easycap does lots of (redundant?) checks as:
pvideo_device = video_devdata(file); if (!pvideo_device) { return -EFAULT; }
Is this bad, good or doesn't matter? (There are more examples where the check is clearly not needed but this one makes me doubt a bit)
Depends on what video_devdata() does, and how it could ever be NULL.
Maybe I'm focusing on small issues (as you already pointed out), do you think this kind of patches would be accepted?
Of course, I would submit separate patches, one for each change: 1. split probe function 2. remove redundant checks 3. clean comment style ... and so on.
That sounds great.
Also, I'm reading driver cx231xx as it's also an usb video capture. Do you think it's a good code reference? Can you give me further reference?
I don't know, ask the linux-media developers on their list for a good reference driver to follow, they would know best.
thanks,
greg k-h
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
-- Peter Senna Tschudin peter.senna@gmail.com gpg id: 48274C36
Hi Peter, 2012/2/15, Peter Senna Tschudin <peter.senna@gmail.com>:
The post: Intro to V4L2: http://www.linuxfordevices.com/c/a/Linux-For-Devices-Articles/Intro-to-V4L2/
And the post: The VIVI driver; a great starting point for V4L2 driver writers: http://lwn.net/Articles/203971/
Thanks, those links are worth reading. Ezequiel.
participants (4)
-
Ezequiel García -
Greg KH -
Manavendra Nath Manav -
Peter Senna Tschudin