From: Oliver Neukum <oneukum@suse.com>
To: Richard Leitner <richard.leitner@skidata.com>,
Richard Leitner <dev@g0hl1n.net>,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
linux-usb@vger.kernel.org
Cc: bhelgaas@google.com, mathias.nyman@intel.com, gregkh@linuxfoundation.org
Subject: Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
Date: Thu, 15 Mar 2018 10:26:32 +0100 [thread overview]
Message-ID: <1521105992.18237.2.camel@suse.com> (raw)
In-Reply-To: <377fb9d5-1cc9-7ab8-c965-2fb0a4dc20f3@skidata.com>
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
WARNING: multiple messages have this Message-ID (diff)
From: Oliver Neukum <oneukum@suse.com>
To: Richard Leitner <richard.leitner@skidata.com>,
Richard Leitner <dev@g0hl1n.net>,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
linux-usb@vger.kernel.org
Cc: bhelgaas@google.com, mathias.nyman@intel.com, gregkh@linuxfoundation.org
Subject: [2/3] usb: host: pci: introduce PCI vendor ID for Netlogic
Date: Thu, 15 Mar 2018 10:26:32 +0100 [thread overview]
Message-ID: <1521105992.18237.2.camel@suse.com> (raw)
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
next prev parent reply other threads:[~2018-03-15 9:26 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-14 10:29 [PATCH 0/3] usb: host: pci: PCI ID consolidation Richard Leitner
2018-03-14 10:29 ` [PATCH 1/3] usb: host: pci: use existing Intel PCI ID macros Richard Leitner
2018-03-14 10:29 ` [1/3] " Richard Leitner
2018-03-14 10:48 ` [PATCH 1/3] " Greg KH
2018-03-14 10:48 ` [1/3] " Greg Kroah-Hartman
2018-03-14 10:29 ` [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic Richard Leitner
2018-03-14 10:29 ` [2/3] " Richard Leitner
2018-03-14 10:48 ` [PATCH 2/3] " Greg KH
2018-03-14 10:48 ` [2/3] " Greg Kroah-Hartman
2018-03-14 11:36 ` [PATCH 2/3] " Richard Leitner
2018-03-14 11:36 ` [2/3] " Richard Leitner
2018-03-14 11:49 ` [PATCH 2/3] " Greg KH
2018-03-14 11:49 ` [2/3] " Greg Kroah-Hartman
2018-03-14 12:17 ` [PATCH 2/3] " Oliver Neukum
2018-03-14 12:17 ` [2/3] " Oliver Neukum
2018-03-14 13:31 ` [PATCH 2/3] " Richard Leitner
2018-03-14 13:31 ` [2/3] " Richard Leitner
2018-03-14 15:27 ` [PATCH 2/3] " Oliver Neukum
2018-03-14 15:27 ` [2/3] " Oliver Neukum
2018-03-14 15:44 ` [PATCH 2/3] " Richard Leitner
2018-03-14 15:44 ` [2/3] " Richard Leitner
2018-03-15 9:26 ` Oliver Neukum [this message]
2018-03-15 9:26 ` Oliver Neukum
2018-03-15 9:47 ` [PATCH 2/3] " Richard Leitner
2018-03-15 9:47 ` [2/3] " Richard Leitner
2018-03-14 10:29 ` [PATCH 3/3] usb: host: pci: replace hardcoded renesas PCI IDs Richard Leitner
2018-03-14 10:29 ` [3/3] " Richard Leitner
2018-03-14 10:49 ` [PATCH 3/3] " Greg KH
2018-03-14 10:49 ` [3/3] " Greg Kroah-Hartman
2018-03-14 11:38 ` [PATCH 3/3] " Richard Leitner
2018-03-14 11:38 ` [3/3] " Richard Leitner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1521105992.18237.2.camel@suse.com \
--to=oneukum@suse.com \
--cc=bhelgaas@google.com \
--cc=dev@g0hl1n.net \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mathias.nyman@intel.com \
--cc=richard.leitner@skidata.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.