[QUESTION] staging/easycap fix

[QUESTION] staging/easycap fix

[Ezequiel García]

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.

[QUESTION] staging/easycap fix

[Greg KH]

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

[QUESTION] staging/easycap fix

[Ezequiel García]

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.

[QUESTION] staging/easycap fix

[Manavendra Nath Manav]

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

[QUESTION] staging/easycap fix

[Greg KH]

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

[QUESTION] staging/easycap fix

[Ezequiel García]

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.

[QUESTION] staging/easycap fix

[Greg KH]

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

[QUESTION] staging/easycap fix

[Peter Senna Tschudin]

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 at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies



-- 
Peter Senna Tschudin
peter.senna at gmail.com
gpg id: 48274C36

[QUESTION] staging/easycap fix

[Ezequiel García]

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.