Mailing List Archive

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,}
> };
>



_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

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












_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

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

_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

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


_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

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

_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel