| Message ID | 20220513225924.1655-3-jonathan.lemon@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp: ocp: various updates | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 3 of 3 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 23 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, 13 May 2022 15:59:16 -0700 Jonathan Lemon wrote: > +#ifndef PCI_VENDOR_ID_CELESTICA > +#define PCI_VENDOR_ID_CELESTICA 0x18d4 > +#endif > + > +#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD > +#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008 > +#endif The ifdefs are unnecessary, these kind of constructs are often used out of tree when one does not control the headers, but not sure what purpose they'd serve upstream?
On Mon, May 16, 2022 at 05:43:03PM -0700, Jakub Kicinski wrote: > On Fri, 13 May 2022 15:59:16 -0700 Jonathan Lemon wrote: > > +#ifndef PCI_VENDOR_ID_CELESTICA > > +#define PCI_VENDOR_ID_CELESTICA 0x18d4 > > +#endif > > + > > +#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD > > +#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008 > > +#endif > > The ifdefs are unnecessary, these kind of constructs are often used out > of tree when one does not control the headers, but not sure what purpose > they'd serve upstream? include/linux/pci_ids.h says: * Do not add new entries to this file unless the definitions * are shared between multiple drivers. Neither FACEBOOK (0x1d9b) nor CELESTICA (0x18d4) are present in this file. This seems to a common idiom in several other drivers. Picking one at random: gve.h:#define PCI_VENDOR_ID_GOOGLE 0x1ae0 So these #defines are needed.
On Mon, 16 May 2022 18:46:44 -0700 Jonathan Lemon wrote: > On Mon, May 16, 2022 at 05:43:03PM -0700, Jakub Kicinski wrote: > > On Fri, 13 May 2022 15:59:16 -0700 Jonathan Lemon wrote: > > > +#ifndef PCI_VENDOR_ID_CELESTICA > > > +#define PCI_VENDOR_ID_CELESTICA 0x18d4 > > > +#endif > > > + > > > +#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD > > > +#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008 > > > +#endif > > > > The ifdefs are unnecessary, these kind of constructs are often used out > > of tree when one does not control the headers, but not sure what purpose > > they'd serve upstream? > > include/linux/pci_ids.h says: > > * Do not add new entries to this file unless the definitions > * are shared between multiple drivers. > > Neither FACEBOOK (0x1d9b) nor CELESTICA (0x18d4) are present > in this file. This seems to a common idiom in several other > drivers. Picking one at random: > > gve.h:#define PCI_VENDOR_ID_GOOGLE 0x1ae0 > > > So these #defines are needed. Indeed, but also I'm not complaining about defines but the ifdefs in which they are wrapped :)
On Tue, May 17, 2022 at 08:25:58AM -0700, Jakub Kicinski wrote: > On Mon, 16 May 2022 18:46:44 -0700 Jonathan Lemon wrote: > > On Mon, May 16, 2022 at 05:43:03PM -0700, Jakub Kicinski wrote: > > > On Fri, 13 May 2022 15:59:16 -0700 Jonathan Lemon wrote: > > > > +#ifndef PCI_VENDOR_ID_CELESTICA > > > > +#define PCI_VENDOR_ID_CELESTICA 0x18d4 > > > > +#endif > > > > + > > > > +#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD > > > > +#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008 > > > > +#endif > > > > > > The ifdefs are unnecessary, these kind of constructs are often used out > > > of tree when one does not control the headers, but not sure what purpose > > > they'd serve upstream? > > > > include/linux/pci_ids.h says: > > > > * Do not add new entries to this file unless the definitions > > * are shared between multiple drivers. > > > > Neither FACEBOOK (0x1d9b) nor CELESTICA (0x18d4) are present > > in this file. This seems to a common idiom in several other > > drivers. Picking one at random: > > > > gve.h:#define PCI_VENDOR_ID_GOOGLE 0x1ae0 > > > > > > So these #defines are needed. > > Indeed, but also I'm not complaining about defines but the ifdefs > in which they are wrapped :) This is standard defensive coding practice. I would vastly prefer to leave them this way, and my hard-wired fingers would also not like to be retrained. Next, you'll be telling me that all the kernel headers should be using "#pragma once".
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c index e02a0fd70d3d..d8d80138c629 100644 --- a/drivers/ptp/ptp_ocp.c +++ b/drivers/ptp/ptp_ocp.c @@ -28,6 +28,14 @@ #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400 #endif +#ifndef PCI_VENDOR_ID_CELESTICA +#define PCI_VENDOR_ID_CELESTICA 0x18d4 +#endif + +#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD +#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008 +#endif + static struct class timecard_class = { .owner = THIS_MODULE, .name = "timecard", @@ -634,7 +642,8 @@ static struct ocp_resource ocp_fb_resource[] = { static const struct pci_device_id ptp_ocp_pcidev_id[] = { { PCI_DEVICE_DATA(FACEBOOK, TIMECARD, &ocp_fb_resource) }, - { 0 } + { PCI_DEVICE_DATA(CELESTICA, TIMECARD, &ocp_fb_resource) }, + { } }; MODULE_DEVICE_TABLE(pci, ptp_ocp_pcidev_id);