Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
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_RETRIES2 makes clearly sense. So does #define XYZ_MAGIC_VALUE10xab4e 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
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 LeitnerReplace 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
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> > > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
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>> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
Am Mittwoch, den 14.03.2018, 11:29 +0100 schrieb Richard Leitner: > From: Richard Leitner> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
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> >> > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
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>> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
On Wed, Mar 14, 2018 at 11:29:32AM +0100, Richard Leitner wrote: > From: Richard Leitner> > 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? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
From: Richard LeitnerReplace the hardcoded PCI vendor ID of Netlogic with a definition in pci_ids.h Signed-off-by: Richard Leitner --- drivers/usb/host/pci-quirks.c | 2 +- include/linux/pci_ids.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) 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 */ -- 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html