[PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)

[PATCH 12/25] char: 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/char/epca.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/char/epca.c b/drivers/char/epca.c
index d9df46a..5dafcdb 100644
--- a/drivers/char/epca.c
+++ b/drivers/char/epca.c
@@ -2762,10 +2762,10 @@ err_out:
 
 
 static struct pci_device_id epca_pci_tbl[] = {
-	{ PCI_VENDOR_DIGI, PCI_DEVICE_XR, PCI_ANY_ID, PCI_ANY_ID, 0, 0, brd_xr },
-	{ PCI_VENDOR_DIGI, PCI_DEVICE_XEM, PCI_ANY_ID, PCI_ANY_ID, 0, 0, brd_xem },
-	{ PCI_VENDOR_DIGI, PCI_DEVICE_CX, PCI_ANY_ID, PCI_ANY_ID, 0, 0, brd_cx },
-	{ PCI_VENDOR_DIGI, PCI_DEVICE_XRJ, PCI_ANY_ID, PCI_ANY_ID, 0, 0, brd_xrj },
+	{ PCI_VDEVICE(DIGI, PCI_DEVICE_XR), brd_xr },
+	{ PCI_VDEVICE(DIGI, PCI_DEVICE_XEM), brd_xem },
+	{ PCI_VDEVICE(DIGI, PCI_DEVICE_CX), brd_cx },
+	{ PCI_VDEVICE(DIGI, PCI_DEVICE_XRJ), brd_xrj },
 	{ 0, }
 };
 
-- 
1.7.1

Re: [PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE

[Greg KH]

On Thu, Jul 15, 2010 at 08:52:37PM +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.
> 
> Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> ---
>  drivers/char/epca.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/epca.c b/drivers/char/epca.c
> index d9df46a..5dafcdb 100644
> --- a/drivers/char/epca.c
> +++ b/drivers/char/epca.c
> @@ -2762,10 +2762,10 @@ err_out:
>  
>  
>  static struct pci_device_id epca_pci_tbl[] = {
> -	{ PCI_VENDOR_DIGI, PCI_DEVICE_XR, PCI_ANY_ID, PCI_ANY_ID, 0, 0, brd_xr },
> -	{ PCI_VENDOR_DIGI, PCI_DEVICE_XEM, PCI_ANY_ID, PCI_ANY_ID, 0, 0, brd_xem },
> -	{ PCI_VENDOR_DIGI, PCI_DEVICE_CX, PCI_ANY_ID, PCI_ANY_ID, 0, 0, brd_cx },
> -	{ PCI_VENDOR_DIGI, PCI_DEVICE_XRJ, PCI_ANY_ID, PCI_ANY_ID, 0, 0, brd_xrj },
> +	{ PCI_VDEVICE(DIGI, PCI_DEVICE_XR), brd_xr },
> +	{ PCI_VDEVICE(DIGI, PCI_DEVICE_XEM), brd_xem },
> +	{ PCI_VDEVICE(DIGI, PCI_DEVICE_CX), brd_cx },
> +	{ PCI_VDEVICE(DIGI, PCI_DEVICE_XRJ), brd_xrj },

The main reason I hate this macro, is that it now makes it almost
impossible to grep for any users of the PCI_VENDOR_DIGI pci vendor id.
I much prefer the PCI_DEVICE() macro instead, and as such, I'm not
willing to take any of these patches, sorry.

greg k-h

Re: [PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE

[Joe Perches]

On Thu, 2010-07-15 at 13:45 -0700, Greg KH wrote:
> I much prefer the PCI_DEVICE() macro instead, and as such, I'm not
> willing to take any of these patches, sorry.

grepping for pci device ids using constants and
expecting the result to be comprehensive isn't
sensible.
 
$ grep -rwP --include=*.[ch] -w PCI_VDEVICE drivers/char | wc -l
32

The current drivers/ use of PCI_VDEVICE to PCI_DEVICE is ~50/50

$ grep --include=*.[ch] -rwP PCI_DEVICE drivers | wc -l
866
$ grep --include=*.[ch] -rwP PCI_VDEVICE drivers | wc -l
768

Re: [PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)

[Peter Hüwe]

Am Donnerstag 15 Juli 2010 22:45:40 schrieb Greg KH:
> The main reason I hate this macro, is that it now makes it almost
> impossible to grep for any users of the PCI_VENDOR_DIGI pci vendor id.
> I much prefer the PCI_DEVICE() macro instead, and as such, I'm not
> willing to take any of these patches, sorry.

No problem ;)
Patches are just proposals - nothing else.

The only  question that remains is, do you see any point in converting the 
patches to use PCI_DEVICE?
Since you have to address/set the .driver_data explicitly I guess there's no 
point in doing it.

It's
{ PCI_VENDOR_DIGI, PCI_DEVICE_XRJ, PCI_ANY_ID, PCI_ANY_ID, 0, 0, brd_xrj },
vs.
 { PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_XR), .driver_data=brd_xrj },
and I guess it isn't really an improvement.

Maybe there should be a version of PCI_DEVICE that addresses this issue? 
But I have to admit, something like:
{ PCI_DEVICE_DD(PCI_VENDOR_ID_DIGI, PCI_DEVICE_XR), brd_xrj }, 
doesn't look that much better.

Thanks,
Peter

Re: [PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE

[Greg KH]

On Thu, Jul 15, 2010 at 11:07:34PM +0200, Peter H?we wrote:
> Am Donnerstag 15 Juli 2010 22:45:40 schrieb Greg KH:
> > The main reason I hate this macro, is that it now makes it almost
> > impossible to grep for any users of the PCI_VENDOR_DIGI pci vendor id.
> > I much prefer the PCI_DEVICE() macro instead, and as such, I'm not
> > willing to take any of these patches, sorry.
> 
> No problem ;)
> Patches are just proposals - nothing else.
> 
> The only  question that remains is, do you see any point in converting the 
> patches to use PCI_DEVICE?
> Since you have to address/set the .driver_data explicitly I guess there's no 
> point in doing it.
> 
> It's
> { PCI_VENDOR_DIGI, PCI_DEVICE_XRJ, PCI_ANY_ID, PCI_ANY_ID, 0, 0, brd_xrj },
> vs.
>  { PCI_DEVICE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_XR), .driver_data=brd_xrj },
> and I guess it isn't really an improvement.

It's a bit nicer, yes.  But only do it if you want to.

> Maybe there should be a version of PCI_DEVICE that addresses this issue? 
> But I have to admit, something like:
> { PCI_DEVICE_DD(PCI_VENDOR_ID_DIGI, PCI_DEVICE_XR), brd_xrj }, 
> doesn't look that much better.

You can do:
	{ PCI_DEVICE_TABLE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_XR), brd_xrj },
just fine today, so do that if you want to.

thanks,

greg k-h

Re: [PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE

[Greg KH]

On Thu, Jul 15, 2010 at 02:00:15PM -0700, Joe Perches wrote:
> On Thu, 2010-07-15 at 13:45 -0700, Greg KH wrote:
> > I much prefer the PCI_DEVICE() macro instead, and as such, I'm not
> > willing to take any of these patches, sorry.
> 
> grepping for pci device ids using constants and
> expecting the result to be comprehensive isn't
> sensible.

But it's a nice goal :)

> $ grep -rwP --include=*.[ch] -w PCI_VDEVICE drivers/char | wc -l
> 32

drivers/char is not exactly a large collection of PCI drivers, only some
old serial port ones.

> The current drivers/ use of PCI_VDEVICE to PCI_DEVICE is ~50/50
> 
> $ grep --include=*.[ch] -rwP PCI_DEVICE drivers | wc -l
> 866
> $ grep --include=*.[ch] -rwP PCI_VDEVICE drivers | wc -l
> 768

Hey, anything to increase that ratio is good :)

thanks,

greg k-h

Re: [PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE

[Joe Perches]

On Thu, 2010-07-15 at 21:29 -0700, Greg KH wrote:
> On Thu, Jul 15, 2010 at 02:00:15PM -0700, Joe Perches wrote:
> > On Thu, 2010-07-15 at 13:45 -0700, Greg KH wrote:
> > > I much prefer the PCI_DEVICE() macro instead, and as such, I'm not
> > > willing to take any of these patches, sorry.
> > grepping for pci device ids using constants and
> > expecting the result to be comprehensive isn't
> > sensible.
> But it's a nice goal :)

I think your goal is not a good one.

For instance:

$ grep -rP --include=*.[ch] "\bPCI_VDEVICE\s*\(\s*INTEL" drivers | wc -l
201
$ grep -rP --include=*.[ch] "\bPCI_DEVICE\s*\(PCI_VENDOR_ID_INTEL" drivers | wc -l
45
$ grep -rP --include=*.[ch] "\bPCI_DEVICE\s*\(\s*0x8086" drivers | wc -l
38

I'd much rather do a search for "PCI_VDEVICE.*INTEL"

Re: [PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE

[Greg KH]

On Thu, Jul 15, 2010 at 09:44:23PM -0700, Joe Perches wrote:
> On Thu, 2010-07-15 at 21:29 -0700, Greg KH wrote:
> > On Thu, Jul 15, 2010 at 02:00:15PM -0700, Joe Perches wrote:
> > > On Thu, 2010-07-15 at 13:45 -0700, Greg KH wrote:
> > > > I much prefer the PCI_DEVICE() macro instead, and as such, I'm not
> > > > willing to take any of these patches, sorry.
> > > grepping for pci device ids using constants and
> > > expecting the result to be comprehensive isn't
> > > sensible.
> > But it's a nice goal :)
> 
> I think your goal is not a good one.
> 
> For instance:
> 
> $ grep -rP --include=*.[ch] "\bPCI_VDEVICE\s*\(\s*INTEL" drivers | wc -l
> 201
> $ grep -rP --include=*.[ch] "\bPCI_DEVICE\s*\(PCI_VENDOR_ID_INTEL" drivers | wc -l
> 45
> $ grep -rP --include=*.[ch] "\bPCI_DEVICE\s*\(\s*0x8086" drivers | wc -l
> 38
> 
> I'd much rather do a search for "PCI_VDEVICE.*INTEL"

I'd much rather use 'cscope' or 'ctags' than trying to remember regular
expressions like the above.

greg k-h

Re: [PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE

[Joe Perches]

On Thu, 2010-07-15 at 22:29 -0700, Greg KH wrote:
> On Thu, Jul 15, 2010 at 09:44:23PM -0700, Joe Perches wrote:
> > On Thu, 2010-07-15 at 21:29 -0700, Greg KH wrote:
> > > On Thu, Jul 15, 2010 at 02:00:15PM -0700, Joe Perches wrote:
> > > > On Thu, 2010-07-15 at 13:45 -0700, Greg KH wrote:
> > > > > I much prefer the PCI_DEVICE() macro instead, and as such, I'm not
> > > > > willing to take any of these patches, sorry.
> > > > grepping for pci device ids using constants and
> > > > expecting the result to be comprehensive isn't
> > > > sensible.
> > > But it's a nice goal :)
> > I think your goal is not a good one.
> > 
> > For instance:
> > 
> > $ grep -rP --include=*.[ch] "\bPCI_VDEVICE\s*\(\s*INTEL" drivers | wc -l
> > 201
> > $ grep -rP --include=*.[ch] "\bPCI_DEVICE\s*\(PCI_VENDOR_ID_INTEL" drivers | wc -l
> > 45
> > $ grep -rP --include=*.[ch] "\bPCI_DEVICE\s*\(\s*0x8086" drivers | wc -l
> > 38
> > I'd much rather do a search for "PCI_VDEVICE.*INTEL"
> I'd much rather use 'cscope' or 'ctags' than trying to remember regular
> expressions like the above.

Then it appears your original argument doesn't have much merit.

Re: [PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE

[Greg KH]

On Thu, Jul 15, 2010 at 10:37:20PM -0700, Joe Perches wrote:
> On Thu, 2010-07-15 at 22:29 -0700, Greg KH wrote:
> > On Thu, Jul 15, 2010 at 09:44:23PM -0700, Joe Perches wrote:
> > > On Thu, 2010-07-15 at 21:29 -0700, Greg KH wrote:
> > > > On Thu, Jul 15, 2010 at 02:00:15PM -0700, Joe Perches wrote:
> > > > > On Thu, 2010-07-15 at 13:45 -0700, Greg KH wrote:
> > > > > > I much prefer the PCI_DEVICE() macro instead, and as such, I'm not
> > > > > > willing to take any of these patches, sorry.
> > > > > grepping for pci device ids using constants and
> > > > > expecting the result to be comprehensive isn't
> > > > > sensible.
> > > > But it's a nice goal :)
> > > I think your goal is not a good one.
> > > 
> > > For instance:
> > > 
> > > $ grep -rP --include=*.[ch] "\bPCI_VDEVICE\s*\(\s*INTEL" drivers | wc -l
> > > 201
> > > $ grep -rP --include=*.[ch] "\bPCI_DEVICE\s*\(PCI_VENDOR_ID_INTEL" drivers | wc -l
> > > 45
> > > $ grep -rP --include=*.[ch] "\bPCI_DEVICE\s*\(\s*0x8086" drivers | wc -l
> > > 38
> > > I'd much rather do a search for "PCI_VDEVICE.*INTEL"
> > I'd much rather use 'cscope' or 'ctags' than trying to remember regular
> > expressions like the above.
> 
> Then it appears your original argument doesn't have much merit.

I'd rather people not use PCI_VDEVICE() as then you can't easily scan
for the PCI_VENDOR_ID_FOO value usage with a tool like cscope, so I
think my original point stands.

nevermind.

Re: [PATCH 12/25] char: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)

[Peter Hüwe]

Am Freitag 16 Juli 2010 06:28:45 schrieb Greg KH:
> > Maybe there should be a version of PCI_DEVICE that addresses this issue?
> > But I have to admit, something like:
> > { PCI_DEVICE_DD(PCI_VENDOR_ID_DIGI, PCI_DEVICE_XR), brd_xrj },
> > doesn't look that much better.
> 
> You can do:
> 	{ PCI_DEVICE_TABLE(PCI_VENDOR_ID_DIGI, PCI_DEVICE_XR), brd_xrj },
> just fine today, so do that if you want to.


Hi Greg,

unfortunately I can't find the definition of this macro - can you perhaps 
pinpoint me to it? And maybe an example in the kernel?

Thanks,
Peter