Message ID | 20180314102933.21367-3-dev@g0hl1n.net |
---|---|
State | Changes Requested |
Headers | show |
Series | [1/3] usb: host: pci: use existing Intel PCI ID macros | expand |
On 03/14/2018 11:48 AM, Greg KH wrote: > On Wed, Mar 14, 2018 at 11:29:32AM +0100, Richard Leitner wrote: >> From: Richard Leitner <richard.leitner@skidata.com> >> >> Replace the hardcoded PCI vendor ID of Netlogic with a definition in >> pci_ids.h > > Why? It's only being used in one file, so it should not be in > pci_ids.h, right? It's also used as PCI_VENDOR_NETLOGIC in arch/mips/netlogic/xlp/. Should this be replaced with PCI_VENDOR_ID_NETLOGIC from pci_ids.h? > > thanks, > > greg k-h > thank you! regards;Richard.L
On Wed, Mar 14, 2018 at 12:36:17PM +0100, Richard Leitner wrote: > > On 03/14/2018 11:48 AM, Greg KH wrote: > > On Wed, Mar 14, 2018 at 11:29:32AM +0100, Richard Leitner wrote: > >> From: Richard Leitner <richard.leitner@skidata.com> > >> > >> Replace the hardcoded PCI vendor ID of Netlogic with a definition in > >> pci_ids.h > > > > Why? It's only being used in one file, so it should not be in > > pci_ids.h, right? > > It's also used as PCI_VENDOR_NETLOGIC in arch/mips/netlogic/xlp/. > > Should this be replaced with PCI_VENDOR_ID_NETLOGIC from pci_ids.h? Yes, if you are going to add it to pci_ids.h, it had better be used by multiple files, otherwise it does not belong in there. thanks, greg k-h
Am Mittwoch, den 14.03.2018, 11:29 +0100 schrieb Richard Leitner: > From: Richard Leitner <richard.leitner@skidata.com> > > Replace the hardcoded PCI vendor ID of Netlogic with a definition in > pci_ids.h Hi, in general, why? Does this patch generate any benefit for any developer reading the source? I don't see it. Does it cause an issue for anybody who has a log file with the nummerical ID and needs to grep for it? Yes it does. Where is the point of this patch? Regards Oliver
Hi Oliver, thank you for your feedback! On 03/14/2018 01:17 PM, Oliver Neukum wrote: > Am Mittwoch, den 14.03.2018, 11:29 +0100 schrieb Richard Leitner: >> From: Richard Leitner <richard.leitner@skidata.com> >> >> Replace the hardcoded PCI vendor ID of Netlogic with a definition in >> pci_ids.h > > Hi, > > in general, why? > Does this patch generate any benefit for any developer > reading the source? I don't see it. Does it cause an > issue for anybody who has a log file with the nummerical > ID and needs to grep for it? Yes it does. I'll send a v2 where I also use this definition in arch/mips/netlogic/xlp/ instead of PCI_VENDOR_NETLOGIC from arch/mips/include/asm/netlogic/xlp-hal/iomap.h. Therefore it will remove this definition from the iomap.h and move it to pci_ids.h This will IMHO be a clear benefit as it removes a redundant definition. > > Where is the point of this patch? > > Regards > Oliver > regards;Richard.L
Am Mittwoch, den 14.03.2018, 14:31 +0100 schrieb Richard Leitner: > Hi Oliver, > thank you for your feedback! > > On 03/14/2018 01:17 PM, Oliver Neukum wrote: > > Am Mittwoch, den 14.03.2018, 11:29 +0100 schrieb Richard Leitner: > > > From: Richard Leitner <richard.leitner@skidata.com> > > > > > > Replace the hardcoded PCI vendor ID of Netlogic with a definition in > > > pci_ids.h > > > > Hi, > > > > in general, why? > > Does this patch generate any benefit for any developer > > reading the source? I don't see it. Does it cause an > > issue for anybody who has a log file with the nummerical > > ID and needs to grep for it? Yes it does. > > I'll send a v2 where I also use this definition in > arch/mips/netlogic/xlp/ instead of PCI_VENDOR_NETLOGIC from > arch/mips/include/asm/netlogic/xlp-hal/iomap.h. > > Therefore it will remove this definition from the iomap.h > and move it to pci_ids.h > > This will IMHO be a clear benefit as it removes a redundant > definition. Well, but it does not. Removing a redundant definition is a clear benefit. But you are not removing a definition. You are introducing a preprocessor constant. Why? What is its benefit? Regards Oliver
On 03/14/2018 04:27 PM, Oliver Neukum wrote: > Am Mittwoch, den 14.03.2018, 14:31 +0100 schrieb Richard Leitner: >> Hi Oliver, >> thank you for your feedback! >> >> On 03/14/2018 01:17 PM, Oliver Neukum wrote: >>> Am Mittwoch, den 14.03.2018, 11:29 +0100 schrieb Richard Leitner: >>>> From: Richard Leitner <richard.leitner@skidata.com> >>>> >>>> Replace the hardcoded PCI vendor ID of Netlogic with a definition in >>>> pci_ids.h >>> >>> Hi, >>> >>> in general, why? >>> Does this patch generate any benefit for any developer >>> reading the source? I don't see it. Does it cause an >>> issue for anybody who has a log file with the nummerical >>> ID and needs to grep for it? Yes it does. >> >> I'll send a v2 where I also use this definition in >> arch/mips/netlogic/xlp/ instead of PCI_VENDOR_NETLOGIC from >> arch/mips/include/asm/netlogic/xlp-hal/iomap.h. >> >> Therefore it will remove this definition from the iomap.h >> and move it to pci_ids.h >> >> This will IMHO be a clear benefit as it removes a redundant >> definition. > > Well, but it does not. Removing a redundant definition is a clear > benefit. But you are not removing a definition. You are introducing > a preprocessor constant. Why? > What is its benefit? AFAIK pci_ids.h collects PCI vendor and device IDs in one single point. As the PCI vendor ID of Netlogic is used in multiple files IMHO it would be a good idea to add it to pci_ids.h and furthermore remove it from arch/mips/include/asm/netlogic/xlp-hal/iomap.h (where it's currently defined). Or am I getting things wrong? regards;richard.l
Am Mittwoch, den 14.03.2018, 16:44 +0100 schrieb Richard Leitner: > On 03/14/2018 04:27 PM, Oliver Neukum wrote: > > Am Mittwoch, den 14.03.2018, 14:31 +0100 schrieb Richard Leitner: > > > > > Well, but it does not. Removing a redundant definition is a clear > > benefit. But you are not removing a definition. You are introducing > > a preprocessor constant. Why? > > What is its benefit? > > AFAIK pci_ids.h collects PCI vendor and device IDs in one single > point. As the PCI vendor ID of Netlogic is used in multiple files > IMHO it would be a good idea to add it to pci_ids.h and furthermore > remove it from arch/mips/include/asm/netlogic/xlp-hal/iomap.h (where > it's currently defined). > > Or am I getting things wrong? I think so, yes. We are giving names to constants as a form of comment or to change them at multiple places at once and consistently. So #define XYZ_NETDEV_RESET_RETRIES 2 makes clearly sense. So does #define XYZ_MAGIC_VALUE1 0xab4e because it tells you that you have a magic value. But you will never redefine a PCI vendor ID. In fact you must not. And if you have a comparison like dev->vID == 0x1234 if you change this to dev->vID == SOME_VENDOR_ID what good does this to you? You already knew it was a vendor ID. Now you can name it at a glance. So what? If you have a device you will have to check whether you have some OEM version. You will always go and check the raw number. And if you have a log and need to check whether the check will be true, you will have a number. Using a constant there is nothing but trouble. Yet one more grep. Regards Oliver
On 03/15/2018 10:26 AM, Oliver Neukum wrote: > Am Mittwoch, den 14.03.2018, 16:44 +0100 schrieb Richard Leitner: >> On 03/14/2018 04:27 PM, Oliver Neukum wrote: >>> Am Mittwoch, den 14.03.2018, 14:31 +0100 schrieb Richard Leitner: >>>> >>> Well, but it does not. Removing a redundant definition is a clear >>> benefit. But you are not removing a definition. You are introducing >>> a preprocessor constant. Why? >>> What is its benefit? >> >> AFAIK pci_ids.h collects PCI vendor and device IDs in one single >> point. As the PCI vendor ID of Netlogic is used in multiple files >> IMHO it would be a good idea to add it to pci_ids.h and furthermore >> remove it from arch/mips/include/asm/netlogic/xlp-hal/iomap.h (where >> it's currently defined). >> >> Or am I getting things wrong? > > I think so, yes. We are giving names to constants as a form > of comment or to change them at multiple places at once and > consistently. > > So > > #define XYZ_NETDEV_RESET_RETRIES 2 > > makes clearly sense. So does > > #define XYZ_MAGIC_VALUE1 0xab4e > > because it tells you that you have a magic value. > But you will never redefine a PCI vendor ID. In fact you > must not. And if you have a comparison like > > dev->vID == 0x1234 > > if you change this to > > dev->vID == SOME_VENDOR_ID > > what good does this to you? You already knew it was a vendor ID. > Now you can name it at a glance. So what? If you have a device > you will have to check whether you have some OEM version. You > will always go and check the raw number. And if you have a log > and need to check whether the check will be true, you will have > a number. > Using a constant there is nothing but trouble. Yet one more grep. Thank you for that explanation. But IMHO it was clearer with a human-readable name in such comparisons... For example in the following I see at the first glance which device from which vendor is affected and I don't need any additional comments or ID databases... if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ...but if that's not the preferred way of doing things I'm perfectly fine with that. Furthermore to me it sounds you are saying that the complete pci_ids.h should be thrown over-board?!? regards;richard.l
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c index 4f4a9f36a68e..39d163729b89 100644 --- a/drivers/usb/host/pci-quirks.c +++ b/drivers/usb/host/pci-quirks.c @@ -1243,7 +1243,7 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev) /* Skip Netlogic mips SoC's internal PCI USB controller. * This device does not need/support EHCI/OHCI handoff */ - if (pdev->vendor == 0x184e) /* vendor Netlogic */ + if (pdev->vendor == PCI_VENDOR_ID_NETLOGIC) return; if (pdev->class != PCI_CLASS_SERIAL_USB_UHCI && pdev->class != PCI_CLASS_SERIAL_USB_OHCI && diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index e8d1af82a688..d23a97868dee 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -3067,4 +3067,6 @@ #define PCI_VENDOR_ID_OCZ 0x1b85 +#define PCI_VENDOR_ID_NETLOGIC 0x184e + #endif /* _LINUX_PCI_IDS_H */