video/ivtv: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)
Subsystems (2)
PATCH 24/25 video/ivtv: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)
From: Peter Huewe <[email protected]>
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.
--- drivers/media/video/ivtv/ivtv-driver.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
drivers/media/video/ivtv/ivtv-driver.c +2 -4
@@ -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 ,} };
Signature · 1 line
1.7.1
Quoted 5 lines · Peter Huewe
From: Peter Huewe <[email protected]>
This patch converts
pci_tableentries, where .subvendor=PCI_ANY_ID and .subdevice=PCI_ANY_ID, .class=0 and .class_mask=0, to use thePCI_VDEVICEmacro, 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
Quoted 22 lines
Signed-off-by Peter Huewe <[email protected]>--- drivers/media/video/ivtv/ivtv-driver.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)drivers/media/video/ivtv/ivtv-driver.c +2
-4@@ -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 ,} };
Am Donnerstag 15 Juli 2010 23:43:20 schrieb Andy Walls:
Quoted 7 lines
Quoted 1 line
... use the
PCI_VDEVICEmacro, 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 ;)
Quoted 2 lines
PCI_ANY_IDindicates 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.
Quoted 2 lines
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
Quoted 12 lines · Andy Walls
Quoted 5 lines · Peter Huewe
From: Peter Huewe <[email protected]>
This patch converts
pci_tableentries, where .subvendor=PCI_ANY_ID and .subdevice=PCI_ANY_ID, .class=0 and .class_mask=0, to use thePCI_VDEVICEmacro, and thus improves readability.Hi Peter,
I have to disagree. The patch may improve typesetting, but it degrades clarity and maintainability from my perspective.
Quoted 6 lines
a.
PCI_ANY_IDindicates to the reader a wildcard match is being performed. ThePCI_VDEVICE()macro hides that to some degree.b.
PCI_VENDOR_ID_ICOMPclearly 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).
Signature · 6 lines
Jarod Wilson [email protected] -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 7 lines · Jarod Wilson
Quoted 6 lines · Andy Walls
Quoted 1 line · Peter Huewe
From: Peter Huewe <[email protected]>
a.
PCI_ANY_IDindicates to the reader a wildcard match is being performed. ThePCI_VDEVICE()macro hides that to some degree.
Quoted 6 lines
Quoted 3 lines
b.
PCI_VENDOR_ID_ICOMPclearly 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 */
?
Quoted 4 lines
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
Quoted 12 lines · Andy Walls
Quoted 11 lines · Jarod Wilson
Quoted 9 lines · Andy Walls
Quoted 1 line · Peter Huewe
From: Peter Huewe <[email protected]>
a.
PCI_ANY_IDindicates to the reader a wildcard match is being performed. ThePCI_VDEVICE()macro hides that to some degree.b.
PCI_VENDOR_ID_ICOMPclearly 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.").
Quoted 8 lines
Quoted 2 lines
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 */
Quoted 8 lines
Quoted 4 lines
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.
Signature · 6 lines
Jarod Wilson [email protected] -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html