From: Richard Leitner <[email protected]>
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
From: Richard Leitner <[email protected]>
Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
the harcoded values with them.
Signed-off-by: Richard Leitner <[email protected]>
---
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
From: Richard Leitner <[email protected]>
Replace the hardcoded PCI vendor ID of Netlogic with a definition in
pci_ids.h
Signed-off-by: Richard Leitner <[email protected]>
---
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
From: Richard Leitner <[email protected]>
Instead of the hardcoded hexadecimal PCI IDs use the existing macros
from pci_ids.h for Intel IDs.
Signed-off-by: Richard Leitner <[email protected]>
---
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
On Wed, Mar 14, 2018 at 11:29:31AM +0100, Richard Leitner wrote:
> From: Richard Leitner <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Wed, Mar 14, 2018 at 11:29:33AM +0100, Richard Leitner wrote:
> From: Richard Leitner <[email protected]>
>
> Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
> the harcoded values with them.
>
> Signed-off-by: Richard Leitner <[email protected]>
> ---
> 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
On Wed, Mar 14, 2018 at 11:29:32AM +0100, Richard Leitner wrote:
> From: Richard Leitner <[email protected]>
>
> 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
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 <[email protected]>
>>
>> Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
>> the harcoded values with them.
>>
>> Signed-off-by: Richard Leitner <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>
>> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
>
> 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 <[email protected]>
>>
>> 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 <[email protected]>
> > >
> > > 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 <[email protected]>
>>>>
>>>> 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