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.
Signed-off-by: Peter Huewe <[email protected]>
---
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
On Thu, Jul 15, 2010 at 08:52:37PM +0200, Peter Huewe wrote:
> 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.
>
> Signed-off-by: Peter Huewe <[email protected]>
> ---
> 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
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
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
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
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
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"
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
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.
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.
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