[PATCH 0/3] usb: host: pci: PCI ID consolidation

[PATCH 0/3] usb: host: pci: PCI ID consolidation

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

Centralize some hardcoded PCI IDs as definitions in the global
include/linux/pci_ids.h file. This is done to reduce the amount of
scattered PCI ID definitions and hardcoded values across the kernel.

Richard Leitner (3):
  usb: host: pci: use existing Intel PCI ID macros
  usb: host: pci: introduce PCI vendor ID for Netlogic
  usb: host: pci: replace hardcoded renesas PCI IDs

 drivers/usb/host/pci-quirks.c | 16 +++++++++-------
 drivers/usb/host/xhci-pci.c   |  7 ++++---
 include/linux/pci_ids.h       |  5 +++++
 3 files changed, 18 insertions(+), 10 deletions(-)

-- 
2.11.0

[PATCH 1/3] usb: host: pci: use existing Intel PCI ID macros

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

Instead of the hardcoded hexadecimal PCI IDs use the existing macros
from pci_ids.h for Intel IDs.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/usb/host/pci-quirks.c | 10 +++++-----
 drivers/usb/host/xhci-pci.c   |  3 ++-
 include/linux/pci_ids.h       |  1 +
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 67ad4bb6919a..4f4a9f36a68e 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -858,8 +858,8 @@ static void ehci_bios_handoff(struct pci_dev *pdev,
 	 *
 	 * The HASEE E200 hangs when the semaphore is set (bugzilla #77021).
 	 */
-	if (pdev->vendor == 0x8086 && (pdev->device == 0x283a ||
-			pdev->device == 0x27cc)) {
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+	    (pdev->device == 0x283a || pdev->device == 0x27cc)) {
 		if (dmi_check_system(ehci_dmi_nohandoff_table))
 			try_handoff = 0;
 	}
@@ -1168,9 +1168,9 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	val = readl(base + ext_cap_offset);
 
 	/* Auto handoff never worked for these devices. Force it and continue */
-	if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
-			(pdev->vendor == PCI_VENDOR_ID_RENESAS
-			 && pdev->device == 0x0014)) {
+	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
+	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
+	    (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
 		val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
 		writel(val, base + ext_cap_offset);
 	}
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 5262fa571a5d..a5bfd890190c 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -213,7 +213,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		pdev->device == PCI_DEVICE_ID_ASMEDIA_1042A_XHCI)
 		xhci->quirks |= XHCI_ASMEDIA_MODIFY_FLOWCONTROL;
 
-	if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
+	if (pdev->vendor == PCI_VENDOR_ID_TI &&
+	    pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)
 		xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
 
 	if (xhci->quirks & XHCI_RESET_ON_RESUME)
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index a6b30667a331..e8d1af82a688 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -838,6 +838,7 @@
 #define PCI_DEVICE_ID_TI_XX12		0x8039
 #define PCI_DEVICE_ID_TI_XX12_FM	0x803b
 #define PCI_DEVICE_ID_TI_XIO2000A	0x8231
+#define PCI_DEVICE_ID_TI_TUSB73X0	0x8241
 #define PCI_DEVICE_ID_TI_1130		0xac12
 #define PCI_DEVICE_ID_TI_1031		0xac13
 #define PCI_DEVICE_ID_TI_1131		0xac15
-- 
2.11.0

[1/3] usb: host: pci: use existing Intel PCI ID macros

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

Instead of the hardcoded hexadecimal PCI IDs use the existing macros
from pci_ids.h for Intel IDs.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/usb/host/pci-quirks.c | 10 +++++-----
 drivers/usb/host/xhci-pci.c   |  3 ++-
 include/linux/pci_ids.h       |  1 +
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 67ad4bb6919a..4f4a9f36a68e 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -858,8 +858,8 @@ static void ehci_bios_handoff(struct pci_dev *pdev,
 	 *
 	 * The HASEE E200 hangs when the semaphore is set (bugzilla #77021).
 	 */
-	if (pdev->vendor == 0x8086 && (pdev->device == 0x283a ||
-			pdev->device == 0x27cc)) {
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+	    (pdev->device == 0x283a || pdev->device == 0x27cc)) {
 		if (dmi_check_system(ehci_dmi_nohandoff_table))
 			try_handoff = 0;
 	}
@@ -1168,9 +1168,9 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	val = readl(base + ext_cap_offset);
 
 	/* Auto handoff never worked for these devices. Force it and continue */
-	if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
-			(pdev->vendor == PCI_VENDOR_ID_RENESAS
-			 && pdev->device == 0x0014)) {
+	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
+	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
+	    (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
 		val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
 		writel(val, base + ext_cap_offset);
 	}
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 5262fa571a5d..a5bfd890190c 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -213,7 +213,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		pdev->device == PCI_DEVICE_ID_ASMEDIA_1042A_XHCI)
 		xhci->quirks |= XHCI_ASMEDIA_MODIFY_FLOWCONTROL;
 
-	if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
+	if (pdev->vendor == PCI_VENDOR_ID_TI &&
+	    pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)
 		xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
 
 	if (xhci->quirks & XHCI_RESET_ON_RESUME)
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index a6b30667a331..e8d1af82a688 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -838,6 +838,7 @@
 #define PCI_DEVICE_ID_TI_XX12		0x8039
 #define PCI_DEVICE_ID_TI_XX12_FM	0x803b
 #define PCI_DEVICE_ID_TI_XIO2000A	0x8231
+#define PCI_DEVICE_ID_TI_TUSB73X0	0x8241
 #define PCI_DEVICE_ID_TI_1130		0xac12
 #define PCI_DEVICE_ID_TI_1031		0xac13
 #define PCI_DEVICE_ID_TI_1131		0xac15

[PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

Replace the hardcoded PCI vendor ID of Netlogic with a definition in
pci_ids.h

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 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

[2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

Replace the hardcoded PCI vendor ID of Netlogic with a definition in
pci_ids.h

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 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 */

[PATCH 3/3] usb: host: pci: replace hardcoded renesas PCI IDs

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
the harcoded values with them.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/usb/host/pci-quirks.c | 6 ++++--
 drivers/usb/host/xhci-pci.c   | 4 ++--
 include/linux/pci_ids.h       | 2 ++
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 39d163729b89..5e1ad523622e 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1170,7 +1170,8 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	/* Auto handoff never worked for these devices. Force it and continue */
 	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
 	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
-	    (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
+	    (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
+	     pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)) {
 		val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
 		writel(val, base + ext_cap_offset);
 	}
@@ -1282,7 +1283,8 @@ bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
 	 * quirk, or the system will be in a rather bad state.
 	 */
 	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-	    (pdev->device == 0x0014 || pdev->device == 0x0015))
+	    (pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201 ||
+	     pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202))
 		return true;
 
 	return false;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index a5bfd890190c..a453e4c35ac7 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -189,10 +189,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		xhci->quirks |= XHCI_BROKEN_STREAMS;
 	}
 	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-			pdev->device == 0x0014)
+			pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)
 		xhci->quirks |= XHCI_TRUST_TX_LENGTH;
 	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-			pdev->device == 0x0015)
+			pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202)
 		xhci->quirks |= XHCI_RESET_ON_RESUME;
 	if (pdev->vendor == PCI_VENDOR_ID_VIA)
 		xhci->quirks |= XHCI_RESET_ON_RESUME;
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index d23a97868dee..eb52f0e9b651 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2427,6 +2427,8 @@
 #define PCI_DEVICE_ID_RENESAS_SH7763	0x0004
 #define PCI_DEVICE_ID_RENESAS_SH7785	0x0007
 #define PCI_DEVICE_ID_RENESAS_SH7786	0x0010
+#define PCI_DEVICE_ID_RENESAS_UPD720201	0x0014
+#define PCI_DEVICE_ID_RENESAS_UPD720202	0x0015
 
 #define PCI_VENDOR_ID_SOLARFLARE	0x1924
 #define PCI_DEVICE_ID_SOLARFLARE_SFC4000A_0	0x0703
-- 
2.11.0

[3/3] usb: host: pci: replace hardcoded renesas PCI IDs

[Richard Leitner]

From: Richard Leitner <richard.leitner@skidata.com>

Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
the harcoded values with them.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/usb/host/pci-quirks.c | 6 ++++--
 drivers/usb/host/xhci-pci.c   | 4 ++--
 include/linux/pci_ids.h       | 2 ++
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 39d163729b89..5e1ad523622e 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1170,7 +1170,8 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	/* Auto handoff never worked for these devices. Force it and continue */
 	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
 	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
-	    (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
+	    (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
+	     pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)) {
 		val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
 		writel(val, base + ext_cap_offset);
 	}
@@ -1282,7 +1283,8 @@ bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
 	 * quirk, or the system will be in a rather bad state.
 	 */
 	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-	    (pdev->device == 0x0014 || pdev->device == 0x0015))
+	    (pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201 ||
+	     pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202))
 		return true;
 
 	return false;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index a5bfd890190c..a453e4c35ac7 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -189,10 +189,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		xhci->quirks |= XHCI_BROKEN_STREAMS;
 	}
 	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-			pdev->device == 0x0014)
+			pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)
 		xhci->quirks |= XHCI_TRUST_TX_LENGTH;
 	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-			pdev->device == 0x0015)
+			pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202)
 		xhci->quirks |= XHCI_RESET_ON_RESUME;
 	if (pdev->vendor == PCI_VENDOR_ID_VIA)
 		xhci->quirks |= XHCI_RESET_ON_RESUME;
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index d23a97868dee..eb52f0e9b651 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2427,6 +2427,8 @@
 #define PCI_DEVICE_ID_RENESAS_SH7763	0x0004
 #define PCI_DEVICE_ID_RENESAS_SH7785	0x0007
 #define PCI_DEVICE_ID_RENESAS_SH7786	0x0010
+#define PCI_DEVICE_ID_RENESAS_UPD720201	0x0014
+#define PCI_DEVICE_ID_RENESAS_UPD720202	0x0015
 
 #define PCI_VENDOR_ID_SOLARFLARE	0x1924
 #define PCI_DEVICE_ID_SOLARFLARE_SFC4000A_0	0x0703

Re: [PATCH 1/3] usb: host: pci: use existing Intel PCI ID macros

[Greg KH]

On Wed, Mar 14, 2018 at 11:29:31AM +0100, Richard Leitner wrote:
> From: Richard Leitner <richard.leitner@skidata.com>
> 
> Instead of the hardcoded hexadecimal PCI IDs use the existing macros
> from pci_ids.h for Intel IDs.


You also did something else in this patch, yet failed to mention it
here:

> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> ---
>  drivers/usb/host/pci-quirks.c | 10 +++++-----
>  drivers/usb/host/xhci-pci.c   |  3 ++-
>  include/linux/pci_ids.h       |  1 +
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 67ad4bb6919a..4f4a9f36a68e 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -858,8 +858,8 @@ static void ehci_bios_handoff(struct pci_dev *pdev,
>  	 *
>  	 * The HASEE E200 hangs when the semaphore is set (bugzilla #77021).
>  	 */
> -	if (pdev->vendor == 0x8086 && (pdev->device == 0x283a ||
> -			pdev->device == 0x27cc)) {
> +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +	    (pdev->device == 0x283a || pdev->device == 0x27cc)) {
>  		if (dmi_check_system(ehci_dmi_nohandoff_table))
>  			try_handoff = 0;
>  	}
> @@ -1168,9 +1168,9 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
>  	val = readl(base + ext_cap_offset);
>  
>  	/* Auto handoff never worked for these devices. Force it and continue */
> -	if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
> -			(pdev->vendor == PCI_VENDOR_ID_RENESAS
> -			 && pdev->device == 0x0014)) {
> +	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
> +	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
> +	    (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
>  		val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
>  		writel(val, base + ext_cap_offset);
>  	}
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 5262fa571a5d..a5bfd890190c 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -213,7 +213,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>  		pdev->device == PCI_DEVICE_ID_ASMEDIA_1042A_XHCI)
>  		xhci->quirks |= XHCI_ASMEDIA_MODIFY_FLOWCONTROL;
>  
> -	if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
> +	if (pdev->vendor == PCI_VENDOR_ID_TI &&
> +	    pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)
>  		xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
>  
>  	if (xhci->quirks & XHCI_RESET_ON_RESUME)
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index a6b30667a331..e8d1af82a688 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -838,6 +838,7 @@
>  #define PCI_DEVICE_ID_TI_XX12		0x8039
>  #define PCI_DEVICE_ID_TI_XX12_FM	0x803b
>  #define PCI_DEVICE_ID_TI_XIO2000A	0x8231
> +#define PCI_DEVICE_ID_TI_TUSB73X0	0x8241

You shared a TI device id.

Please break this up into 2 patches.

thanks,

greg k-h

[1/3] usb: host: pci: use existing Intel PCI ID macros

[Greg Kroah-Hartman]

On Wed, Mar 14, 2018 at 11:29:31AM +0100, Richard Leitner wrote:
> From: Richard Leitner <richard.leitner@skidata.com>
> 
> Instead of the hardcoded hexadecimal PCI IDs use the existing macros
> from pci_ids.h for Intel IDs.


You also did something else in this patch, yet failed to mention it
here:

> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> ---
>  drivers/usb/host/pci-quirks.c | 10 +++++-----
>  drivers/usb/host/xhci-pci.c   |  3 ++-
>  include/linux/pci_ids.h       |  1 +
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 67ad4bb6919a..4f4a9f36a68e 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -858,8 +858,8 @@ static void ehci_bios_handoff(struct pci_dev *pdev,
>  	 *
>  	 * The HASEE E200 hangs when the semaphore is set (bugzilla #77021).
>  	 */
> -	if (pdev->vendor == 0x8086 && (pdev->device == 0x283a ||
> -			pdev->device == 0x27cc)) {
> +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +	    (pdev->device == 0x283a || pdev->device == 0x27cc)) {
>  		if (dmi_check_system(ehci_dmi_nohandoff_table))
>  			try_handoff = 0;
>  	}
> @@ -1168,9 +1168,9 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
>  	val = readl(base + ext_cap_offset);
>  
>  	/* Auto handoff never worked for these devices. Force it and continue */
> -	if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
> -			(pdev->vendor == PCI_VENDOR_ID_RENESAS
> -			 && pdev->device == 0x0014)) {
> +	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
> +	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
> +	    (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
>  		val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
>  		writel(val, base + ext_cap_offset);
>  	}
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 5262fa571a5d..a5bfd890190c 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -213,7 +213,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>  		pdev->device == PCI_DEVICE_ID_ASMEDIA_1042A_XHCI)
>  		xhci->quirks |= XHCI_ASMEDIA_MODIFY_FLOWCONTROL;
>  
> -	if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
> +	if (pdev->vendor == PCI_VENDOR_ID_TI &&
> +	    pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)
>  		xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
>  
>  	if (xhci->quirks & XHCI_RESET_ON_RESUME)
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index a6b30667a331..e8d1af82a688 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -838,6 +838,7 @@
>  #define PCI_DEVICE_ID_TI_XX12		0x8039
>  #define PCI_DEVICE_ID_TI_XX12_FM	0x803b
>  #define PCI_DEVICE_ID_TI_XIO2000A	0x8231
> +#define PCI_DEVICE_ID_TI_TUSB73X0	0x8241

You shared a TI device id.

Please break this up into 2 patches.

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@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

[Greg KH]

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?

thanks,

greg k-h

[2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Greg Kroah-Hartman]

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?

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: host: pci: replace hardcoded renesas PCI IDs

[Greg KH]

On Wed, Mar 14, 2018 at 11:29:33AM +0100, Richard Leitner wrote:
> From: Richard Leitner <richard.leitner@skidata.com>
> 
> Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
> the harcoded values with them.
> 
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> ---
>  drivers/usb/host/pci-quirks.c | 6 ++++--
>  drivers/usb/host/xhci-pci.c   | 4 ++--
>  include/linux/pci_ids.h       | 2 ++
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 39d163729b89..5e1ad523622e 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -1170,7 +1170,8 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
>  	/* Auto handoff never worked for these devices. Force it and continue */
>  	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
>  	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
> -	    (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
> +	    (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
> +	     pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)) {
>  		val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
>  		writel(val, base + ext_cap_offset);
>  	}
> @@ -1282,7 +1283,8 @@ bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
>  	 * quirk, or the system will be in a rather bad state.
>  	 */
>  	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
> -	    (pdev->device == 0x0014 || pdev->device == 0x0015))
> +	    (pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201 ||
> +	     pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202))
>  		return true;
>  
>  	return false;
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index a5bfd890190c..a453e4c35ac7 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -189,10 +189,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>  		xhci->quirks |= XHCI_BROKEN_STREAMS;
>  	}
>  	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
> -			pdev->device == 0x0014)
> +			pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)
>  		xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>  	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
> -			pdev->device == 0x0015)
> +			pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202)
>  		xhci->quirks |= XHCI_RESET_ON_RESUME;
>  	if (pdev->vendor == PCI_VENDOR_ID_VIA)
>  		xhci->quirks |= XHCI_RESET_ON_RESUME;
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index d23a97868dee..eb52f0e9b651 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2427,6 +2427,8 @@
>  #define PCI_DEVICE_ID_RENESAS_SH7763	0x0004
>  #define PCI_DEVICE_ID_RENESAS_SH7785	0x0007
>  #define PCI_DEVICE_ID_RENESAS_SH7786	0x0010
> +#define PCI_DEVICE_ID_RENESAS_UPD720201	0x0014
> +#define PCI_DEVICE_ID_RENESAS_UPD720202	0x0015

Now this patch was fine :)

Care to redo this series?

thanks,

greg k-h

[3/3] usb: host: pci: replace hardcoded renesas PCI IDs

[Greg Kroah-Hartman]

On Wed, Mar 14, 2018 at 11:29:33AM +0100, Richard Leitner wrote:
> From: Richard Leitner <richard.leitner@skidata.com>
> 
> Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
> the harcoded values with them.
> 
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> ---
>  drivers/usb/host/pci-quirks.c | 6 ++++--
>  drivers/usb/host/xhci-pci.c   | 4 ++--
>  include/linux/pci_ids.h       | 2 ++
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 39d163729b89..5e1ad523622e 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -1170,7 +1170,8 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
>  	/* Auto handoff never worked for these devices. Force it and continue */
>  	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
>  	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
> -	    (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
> +	    (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
> +	     pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)) {
>  		val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
>  		writel(val, base + ext_cap_offset);
>  	}
> @@ -1282,7 +1283,8 @@ bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
>  	 * quirk, or the system will be in a rather bad state.
>  	 */
>  	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
> -	    (pdev->device == 0x0014 || pdev->device == 0x0015))
> +	    (pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201 ||
> +	     pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202))
>  		return true;
>  
>  	return false;
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index a5bfd890190c..a453e4c35ac7 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -189,10 +189,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>  		xhci->quirks |= XHCI_BROKEN_STREAMS;
>  	}
>  	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
> -			pdev->device == 0x0014)
> +			pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)
>  		xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>  	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
> -			pdev->device == 0x0015)
> +			pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202)
>  		xhci->quirks |= XHCI_RESET_ON_RESUME;
>  	if (pdev->vendor == PCI_VENDOR_ID_VIA)
>  		xhci->quirks |= XHCI_RESET_ON_RESUME;
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index d23a97868dee..eb52f0e9b651 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2427,6 +2427,8 @@
>  #define PCI_DEVICE_ID_RENESAS_SH7763	0x0004
>  #define PCI_DEVICE_ID_RENESAS_SH7785	0x0007
>  #define PCI_DEVICE_ID_RENESAS_SH7786	0x0010
> +#define PCI_DEVICE_ID_RENESAS_UPD720201	0x0014
> +#define PCI_DEVICE_ID_RENESAS_UPD720202	0x0015

Now this patch was fine :)

Care to redo this series?

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@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

[Richard Leitner]

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

[2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: host: pci: replace hardcoded renesas PCI IDs

[Richard Leitner]

On 03/14/2018 11:49 AM, Greg KH wrote:
> On Wed, Mar 14, 2018 at 11:29:33AM +0100, Richard Leitner wrote:
>> From: Richard Leitner <richard.leitner@skidata.com>
>>
>> Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
>> the harcoded values with them.
>>
>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>> ---
>>  drivers/usb/host/pci-quirks.c | 6 ++++--
>>  drivers/usb/host/xhci-pci.c   | 4 ++--
>>  include/linux/pci_ids.h       | 2 ++
>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>

> Now this patch was fine :)
> 
> Care to redo this series?

Sure. Will send a v2 soon.

> 
> thanks,
> 
> greg k-h
> 

thank you!

regards;Richard.L

[3/3] usb: host: pci: replace hardcoded renesas PCI IDs

[Richard Leitner]

On 03/14/2018 11:49 AM, Greg KH wrote:
> On Wed, Mar 14, 2018 at 11:29:33AM +0100, Richard Leitner wrote:
>> From: Richard Leitner <richard.leitner@skidata.com>
>>
>> Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
>> the harcoded values with them.
>>
>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>> ---
>>  drivers/usb/host/pci-quirks.c | 6 ++++--
>>  drivers/usb/host/xhci-pci.c   | 4 ++--
>>  include/linux/pci_ids.h       | 2 ++
>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>

> Now this patch was fine :)
> 
> Care to redo this series?

Sure. Will send a v2 soon.

> 
> 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 majordomo@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

[Greg KH]

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

[2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Greg Kroah-Hartman]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@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

[Oliver Neukum]

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

[2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Oliver Neukum]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@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

[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.

> 
> Where is the point of this patch?
> 
> 	Regards
> 		Oliver
> 

regards;Richard.L

[2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[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.

> 
> 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 majordomo@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

[Oliver Neukum]

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

[2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Oliver Neukum]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@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

[Richard Leitner]

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

[2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@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

[Oliver Neukum]

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

[2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Oliver Neukum]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@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

[Richard Leitner]

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

[2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html