[PATCH 24/25] video/ivtv: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)

[PATCH 24/25] video/ivtv: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)

[Peter Huewe]

From: Peter Huewe <peterhuewe@gmx.de>

This patch converts pci_table entries, where .subvendor=PCI_ANY_ID and
.subdevice=PCI_ANY_ID, .class=0 and .class_mask=0, to use the
PCI_VDEVICE macro, and thus improves readability.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/media/video/ivtv/ivtv-driver.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/ivtv/ivtv-driver.c b/drivers/media/video/ivtv/ivtv-driver.c
index 90daa6e..8e73ab9 100644
--- a/drivers/media/video/ivtv/ivtv-driver.c
+++ b/drivers/media/video/ivtv/ivtv-driver.c
@@ -69,10 +69,8 @@ int ivtv_first_minor;
 
 /* add your revision and whatnot here */
 static struct pci_device_id ivtv_pci_tbl[] __devinitdata = {
-	{PCI_VENDOR_ID_ICOMP, PCI_DEVICE_ID_IVTV15,
-	 PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
-	{PCI_VENDOR_ID_ICOMP, PCI_DEVICE_ID_IVTV16,
-	 PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
+	{PCI_VDEVICE(ICOMP, PCI_DEVICE_ID_IVTV15), 0},
+	{PCI_VDEVICE(ICOMP, PCI_DEVICE_ID_IVTV16), 0},
 	{0,}
 };
 
-- 
1.7.1

Re: [PATCH 24/25] video/ivtv: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)

[Andy Walls]

On Thu, 2010-07-15 at 21:08 +0200, Peter Huewe wrote:
> From: Peter Huewe <peterhuewe@gmx.de>
> 
> This patch converts pci_table entries, where .subvendor=PCI_ANY_ID and
> .subdevice=PCI_ANY_ID, .class=0 and .class_mask=0, to use the
> PCI_VDEVICE macro, and thus improves readability.

Hi Peter,

I have to disagree.  The patch may improve typesetting, but it degrades
clarity and maintainability from my perspective.

a. PCI_ANY_ID indicates to the reader a wildcard match is being
performed.  The PCI_VDEVICE() macro hides that to some degree.

b. PCI_VENDOR_ID_ICOMP clearly indicates that ICOMP is a vendor.
"ICOMP" alone does not hint to the reader that is stands for a company
(the now defunct "Internext Compression, Inc.").


IMO, macros, for things other than named constants, should only be used
for constructs that the C language does not express clearly or compactly
in the context.  This structure initialization being done in file scope,
where white space and line feeds are cheap, will only be obfuscated by
macros, not clarified.

So I'm going to NAK this for ivtv, unless someone can help me understand
any big picture benefit that I may not see from my possibly myopic
perspective.


BTW, I have not seen a similar patch come in my mailbox for
cx18-driver.c.  Why propose the change for ivtv and not cx18?

Regards,
Andy

> Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> ---
>  drivers/media/video/ivtv/ivtv-driver.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/video/ivtv/ivtv-driver.c b/drivers/media/video/ivtv/ivtv-driver.c
> index 90daa6e..8e73ab9 100644
> --- a/drivers/media/video/ivtv/ivtv-driver.c
> +++ b/drivers/media/video/ivtv/ivtv-driver.c
> @@ -69,10 +69,8 @@ int ivtv_first_minor;
>  
>  /* add your revision and whatnot here */
>  static struct pci_device_id ivtv_pci_tbl[] __devinitdata = {
> -	{PCI_VENDOR_ID_ICOMP, PCI_DEVICE_ID_IVTV15,
> -	 PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> -	{PCI_VENDOR_ID_ICOMP, PCI_DEVICE_ID_IVTV16,
> -	 PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> +	{PCI_VDEVICE(ICOMP, PCI_DEVICE_ID_IVTV15), 0},
> +	{PCI_VDEVICE(ICOMP, PCI_DEVICE_ID_IVTV16), 0},
>  	{0,}
>  };
>  

Re: [PATCH 24/25] video/ivtv: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)

[Peter Hüwe]

Am Donnerstag 15 Juli 2010 23:43:20 schrieb Andy Walls:
> > ... use the PCI_VDEVICE macro, and thus improves readability.
> I have to disagree.  The patch may improve typesetting, but it degrades
> clarity and maintainability from my perspective.
> ...
> So I'm going to NAK this for ivtv, unless someone can help me understand
> any big picture benefit that I may not see from my possibly myopic
> perspective.

Hi Andy,

absolutely no problem ;) - I thing that is one of the great things about open 
source that people can have different opinions and discuss them.

Patches are proposals only, and if I spark a discussion which brings the 
development process further or from which I can learn something, I'm a lucky 
guy ;)

> PCI_ANY_ID indicates to the reader a wildcard match is being
> performed. 
Yeah, but I get absolutely no information out of the 0, 0, parameters either - 
unless I look up the function - but then I could also look up the Macro.

> BTW, I have not seen a similar patch come in my mailbox for
> cx18-driver.c.  Why propose the change for ivtv and not cx18?

That might be the case since I picked out several different locations using 
cscope (by gut instinct) - so the patchset is not comprehensive. 
There are still a lot of places that are not converted, I just picked some at 
random and stopped at 25 patches.
The reason behind this was that I almost expected some kind of controversy and 
discussion about the changes - and since I'm getting some NACKs I guess this 
was a smart move - imagine I had converted the whole kernel source just to get 
a simple NACK ;)

Thanks,
Peter

Re: [PATCH 24/25] video/ivtv: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)

[Jarod Wilson]

On Thu, Jul 15, 2010 at 5:43 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> On Thu, 2010-07-15 at 21:08 +0200, Peter Huewe wrote:
>> From: Peter Huewe <peterhuewe@gmx.de>
>>
>> This patch converts pci_table entries, where .subvendor=PCI_ANY_ID and
>> .subdevice=PCI_ANY_ID, .class=0 and .class_mask=0, to use the
>> PCI_VDEVICE macro, and thus improves readability.
>
> Hi Peter,
>
> I have to disagree.  The patch may improve typesetting, but it degrades
> clarity and maintainability from my perspective.
>
> a. PCI_ANY_ID indicates to the reader a wildcard match is being
> performed.  The PCI_VDEVICE() macro hides that to some degree.
>
> b. PCI_VENDOR_ID_ICOMP clearly indicates that ICOMP is a vendor.
> "ICOMP" alone does not hint to the reader that is stands for a company
> (the now defunct "Internext Compression, Inc.").

Personally, I'm a fan of comments around things like this to describe
*exactly* what device(s) they're referring to. Then ICOMP being all
alone without the prefix isn't really much of an issue (though it
could still be easily mistaken for something other than a pci vendor
id, I suppose).

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH 24/25] video/ivtv: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)

[Andy Walls]

On Thu, 2010-07-15 at 18:07 -0400, Jarod Wilson wrote:
> On Thu, Jul 15, 2010 at 5:43 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> > On Thu, 2010-07-15 at 21:08 +0200, Peter Huewe wrote:
> >> From: Peter Huewe <peterhuewe@gmx.de>

> > a. PCI_ANY_ID indicates to the reader a wildcard match is being
> > performed.  The PCI_VDEVICE() macro hides that to some degree.
> >
> > b. PCI_VENDOR_ID_ICOMP clearly indicates that ICOMP is a vendor.
> > "ICOMP" alone does not hint to the reader that is stands for a company
> > (the now defunct "Internext Compression, Inc.").
> 
> Personally, I'm a fan of comments around things like this to describe
> *exactly* what device(s) they're referring to.

Something like this then for ivtv: 

/* Claim every iTVC15/CX23415 or CX23416 based PCI Subsystem ever made */

?


>  Then ICOMP being all
> alone without the prefix isn't really much of an issue (though it
> could still be easily mistaken for something other than a pci vendor
> id, I suppose).

Probably not.  Another minor side effect is that it breaks a tag search
for easily jumping to the definition to see the ID value.  "ICOMP" won't
be in the tags file, but "PCI_VENDOR_ID_ICOMP" will be.

Regards,
Andy

Re: [PATCH 24/25] video/ivtv: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)

[Jarod Wilson]

On Fri, Jul 16, 2010 at 7:42 AM, Andy Walls <awalls@md.metrocast.net> wrote:
> On Thu, 2010-07-15 at 18:07 -0400, Jarod Wilson wrote:
>> On Thu, Jul 15, 2010 at 5:43 PM, Andy Walls <awalls@md.metrocast.net> wrote:
>> > On Thu, 2010-07-15 at 21:08 +0200, Peter Huewe wrote:
>> >> From: Peter Huewe <peterhuewe@gmx.de>
>
>> > a. PCI_ANY_ID indicates to the reader a wildcard match is being
>> > performed.  The PCI_VDEVICE() macro hides that to some degree.
>> >
>> > b. PCI_VENDOR_ID_ICOMP clearly indicates that ICOMP is a vendor.
>> > "ICOMP" alone does not hint to the reader that is stands for a company
>> > (the now defunct "Internext Compression, Inc.").
>>
>> Personally, I'm a fan of comments around things like this to describe
>> *exactly* what device(s) they're referring to.
>
> Something like this then for ivtv:
>
> /* Claim every iTVC15/CX23415 or CX23416 based PCI Subsystem ever made */
>
> ?

More or less. Though perhaps more succinctly, just:

/* All iTVC15/CX23415 and CX23416 based devices */

>>  Then ICOMP being all
>> alone without the prefix isn't really much of an issue (though it
>> could still be easily mistaken for something other than a pci vendor
>> id, I suppose).
>
> Probably not.  Another minor side effect is that it breaks a tag search
> for easily jumping to the definition to see the ID value.  "ICOMP" won't
> be in the tags file, but "PCI_VENDOR_ID_ICOMP" will be.

Hm. That's a fair point. I recall a time or three hunting for symbols
using cscope, and having a bitch of a time, because some of them were
obscured by macro magic.


-- 
Jarod Wilson
jarod@wilsonet.com