Re: [coreboot] r3705 - in trunk/coreboot-v2/src: include/device northbridge/intel/i945 southbridge/intel/i82801gx
svn@coreboot.org wrote:
static const struct pci_driver mc_driver __pci_driver = { .ops = &mc_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27a0,
- .device = PCI_DEVICE_ID_INTEL_945_HOST_BRIDGE,
Sorry, but I don't really agree with these changes.
static const struct pci_driver i82801gx_ide __pci_driver = { .ops = &ide_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27df,
- .device = PCI_DEVICE_ID_INTEL_82801GB_IDE,
};
My reasoning is that #defines should add information to the code and not be an end in itself.
-static const struct pci_driver i82801ex_usb_ehci __pci_driver = { .ops = &usb_ehci_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27cc,
- .device = PCI_DEVICE_ID_INTEL_82801GB_EHCI,
};
In all these cases I quote above it is already perfectly clear from the struct name which device is refered to, in which case I think it would be more informative to have the literal device PCI id.
//Peter
Peter Stuge wrote:
svn@coreboot.org wrote:
static const struct pci_driver mc_driver __pci_driver = { .ops = &mc_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27a0,
- .device = PCI_DEVICE_ID_INTEL_945_HOST_BRIDGE,
Sorry, but I don't really agree with these changes.
static const struct pci_driver i82801gx_ide __pci_driver = { .ops = &ide_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27df,
- .device = PCI_DEVICE_ID_INTEL_82801GB_IDE,
};
My reasoning is that #defines should add information to the code and not be an end in itself.
-static const struct pci_driver i82801ex_usb_ehci __pci_driver = { .ops = &usb_ehci_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27cc,
- .device = PCI_DEVICE_ID_INTEL_82801GB_EHCI,
};
In all these cases I quote above it is already perfectly clear from the struct name which device is refered to, in which case I think it would be more informative to have the literal device PCI id.
I agree with Peter here. Using the #defines makes the code quite harder to understand / follow in case new PCI IDs are required for the drivers. In fact, I started using the numeric values because some wrong values went undetected while the macro names looked fine on the first look.
On Wed, Oct 29, 2008 at 05:02:17PM +0100, Peter Stuge wrote:
svn@coreboot.org wrote:
static const struct pci_driver mc_driver __pci_driver = { .ops = &mc_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27a0,
- .device = PCI_DEVICE_ID_INTEL_945_HOST_BRIDGE,
Sorry, but I don't really agree with these changes.
Yes, we really seems to have a disagreement here. I think the device names from pci_ids.h make perfect sense, that's why we (and Linux for that matters) has such a file in the first place, and why we use the #defines instead of hard-coded PCI device numbers in the code.
static const struct pci_driver i82801gx_ide __pci_driver = { .ops = &ide_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27df,
- .device = PCI_DEVICE_ID_INTEL_82801GB_IDE,
};
My reasoning is that #defines should add information to the code and not be an end in itself.
That's exactly my reasoning as well. The number 0x27df is utterly useless and conveys not information at all. The #define PCI_DEVICE_ID_INTEL_82801GB_IDE however, where-ever in the code I stumble upon it, I immediately know that it's an Intel device, it belongs to the 82801GB chipset/southbridge, and it refers to the IDE device (not audio, not USB, not anything else).
Yes, if you really want to know the hex number you'll have to look it up in the pci_ids.h file but that's the case for all other PCI device numbers we use all over the place (and for all #defines in general). That shouldn't be a reason to _not_ use them, IMO.
Or, to put it in another way, if we all agree to not use #defines for PCI IDs (or no #defines at all), we should just drop pci_ids.h (or all *.h files with #defines in them) entirely. I cannot image we'd want that.
Uwe.
Uwe Hermann wrote:
static const struct pci_driver i82801gx_ide __pci_driver = { .ops = &ide_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27df,
- .device = PCI_DEVICE_ID_INTEL_82801GB_IDE,
};
My reasoning is that #defines should add information to the code and not be an end in itself.
That's exactly my reasoning as well. The number 0x27df is utterly useless and conveys not information at all.
It _is_ the information. 0x27df is the number encoded in harware.
The #define PCI_DEVICE_ID_INTEL_82801GB_IDE however, where-ever in the code I stumble upon it,
I'll try to clarify. My point is that context is very important for whether a define makes sense or not.
I immediately know that it's an Intel device, it belongs to the 82801GB chipset/southbridge, and it refers to the IDE device (not audio, not USB, not anything else).
Not really. The only way to know that is to look at hardware documentation (and hope it's correct) and compare that to the number you get from hardware.
up in the pci_ids.h file but that's the case for all other PCI device numbers we use all over the place (and for all #defines in general). That shouldn't be a reason to _not_ use them, IMO.
Again; the motivation is that in this context the number is more useful than the name.
just drop pci_ids.h entirely. I cannot image we'd want that.
Let's do it! :) No, of course defines have their uses, but I think readability is a function of more than individual lines of code. Context is important.
//Peter
On Wed, Oct 29, 2008 at 1:26 PM, Uwe Hermann uwe@hermann-uwe.de wrote:
On Wed, Oct 29, 2008 at 05:02:17PM +0100, Peter Stuge wrote:
svn@coreboot.org wrote:
static const struct pci_driver mc_driver __pci_driver = { .ops = &mc_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27a0,
- .device = PCI_DEVICE_ID_INTEL_945_HOST_BRIDGE,
Sorry, but I don't really agree with these changes.
Yes, we really seems to have a disagreement here. I think the device names from pci_ids.h make perfect sense, that's why we (and Linux for that matters) has such a file in the first place, and why we use the #defines instead of hard-coded PCI device numbers in the code.
static const struct pci_driver i82801gx_ide __pci_driver = { .ops = &ide_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27df,
- .device = PCI_DEVICE_ID_INTEL_82801GB_IDE,
};
My reasoning is that #defines should add information to the code and not be an end in itself.
That's exactly my reasoning as well. The number 0x27df is utterly useless and conveys not information at all. The #define PCI_DEVICE_ID_INTEL_82801GB_IDE however, where-ever in the code I stumble upon it, I immediately know that it's an Intel device, it belongs to the 82801GB chipset/southbridge, and it refers to the IDE device (not audio, not USB, not anything else).
Yes, if you really want to know the hex number you'll have to look it up in the pci_ids.h file but that's the case for all other PCI device numbers we use all over the place (and for all #defines in general). That shouldn't be a reason to _not_ use them, IMO.
Or, to put it in another way, if we all agree to not use #defines for PCI IDs (or no #defines at all), we should just drop pci_ids.h (or all *.h files with #defines in them) entirely. I cannot image we'd want that.
I'm in agreement with Peter on this one. The pci_driver is named i82801gx_ide, that should clarify exactly what device this is working on. I can agree that having the #define anywhere else in code (where it may not be as clear what device you're aiming for) makes sense, but here it could be much more useful to know the device id here, ex. if you wanted to compare a register set by the driver to a stock bios' lspci -nxxx.
-Corey
Corey Osgood wrote:
On Wed, Oct 29, 2008 at 1:26 PM, Uwe Hermann <uwe@hermann-uwe.de mailto:uwe@hermann-uwe.de> wrote:
On Wed, Oct 29, 2008 at 05:02:17PM +0100, Peter Stuge wrote: > svn@coreboot.org <mailto:svn@coreboot.org> wrote: > > static const struct pci_driver mc_driver __pci_driver = { > > .ops = &mc_ops, > > .vendor = PCI_VENDOR_ID_INTEL, > > - .device = 0x27a0, > > + .device = PCI_DEVICE_ID_INTEL_945_HOST_BRIDGE, > > Sorry, but I don't really agree with these changes. that.I'm in agreement with Peter on this one. The pci_driver is named i82801gx_ide, that should clarify exactly what device this is working on. I can agree that having the #define anywhere else in code (where it may not be as clear what device you're aiming for) makes sense, but here it could be much more useful to know the device id here, ex. if you wanted to compare a register set by the driver to a stock bios' lspci -nxxx.
And I will disagree with you and Peter. The #define makes the code self documenting. Without it you must have the documentation to know what device is being accessed.
Marc
Marc Jones wrote:
The #define makes the code self documenting. Without it you must have the documentation to know what device is being accessed.
That's needed anyway to make sure the #define is in fact correct..
//Peter
Uwe Hermann wrote:
On Wed, Oct 29, 2008 at 05:02:17PM +0100, Peter Stuge wrote:
svn@coreboot.org wrote:
static const struct pci_driver mc_driver __pci_driver = { .ops = &mc_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27a0,
- .device = PCI_DEVICE_ID_INTEL_945_HOST_BRIDGE,
Sorry, but I don't really agree with these changes.
Yes, we really seems to have a disagreement here. I think the device names from pci_ids.h make perfect sense, that's why we (and Linux for that matters) has such a file in the first place, and why we use the #defines instead of hard-coded PCI device numbers in the code.
static const struct pci_driver i82801gx_ide __pci_driver = { .ops = &ide_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27df,
- .device = PCI_DEVICE_ID_INTEL_82801GB_IDE,
};
My reasoning is that #defines should add information to the code and not be an end in itself.
That's exactly my reasoning as well. The number 0x27df is utterly useless and conveys not information at all. The #define PCI_DEVICE_ID_INTEL_82801GB_IDE however, where-ever in the code I stumble upon it, I immediately know that it's an Intel device, it belongs to the 82801GB chipset/southbridge, and it refers to the IDE device (not audio, not USB, not anything else).
What other PCI IDs would you expect in southbridge/intel/i82801gx/i82801gx_ide.c
Yes, if you really want to know the hex number you'll have to look it up in the pci_ids.h file but that's the case for all other PCI device numbers we use all over the place (and for all #defines in general). That shouldn't be a reason to _not_ use them, IMO.
When looking at that part of the code, you always want to know the number, so you basically add another indirection to the code. Especially since the code contains comments on what devices are associated with those IDs
Or, to put it in another way, if we all agree to not use #defines for PCI IDs (or no #defines at all), we should just drop pci_ids.h (or all *.h files with #defines in them) entirely. I cannot image we'd want that.
I think they're fine in the code, but they hurt in the driver ID tables.
On 29.10.2008 18:26, Uwe Hermann wrote:
On Wed, Oct 29, 2008 at 05:02:17PM +0100, Peter Stuge wrote:
svn@coreboot.org wrote:
static const struct pci_driver mc_driver __pci_driver = { .ops = &mc_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27a0,
- .device = PCI_DEVICE_ID_INTEL_945_HOST_BRIDGE,
Sorry, but I don't really agree with these changes.
Yes, we really seems to have a disagreement here. I think the device names from pci_ids.h make perfect sense, that's why we (and Linux for that matters) has such a file in the first place, and why we use the #defines instead of hard-coded PCI device numbers in the code.
I have to side with Uwe here. The name is even human readable, so anybody reading the code saves a lookup for the meaning of a magic number. The only issue is getting the number right in the first place.
static const struct pci_driver i82801gx_ide __pci_driver = { .ops = &ide_ops, .vendor = PCI_VENDOR_ID_INTEL,
- .device = 0x27df,
- .device = PCI_DEVICE_ID_INTEL_82801GB_IDE,
};
My reasoning is that #defines should add information to the code and not be an end in itself.
That's exactly my reasoning as well. The number 0x27df is utterly useless and conveys not information at all. The #define PCI_DEVICE_ID_INTEL_82801GB_IDE however, where-ever in the code I stumble upon it, I immediately know that it's an Intel device, it belongs to the 82801GB chipset/southbridge, and it refers to the IDE device (not audio, not USB, not anything else).
Yes, if you really want to know the hex number you'll have to look it up in the pci_ids.h file but that's the case for all other PCI device numbers we use all over the place (and for all #defines in general). That shouldn't be a reason to _not_ use them, IMO.
Well said.
Or, to put it in another way, if we all agree to not use #defines for PCI IDs (or no #defines at all), we should just drop pci_ids.h (or all *.h files with #defines in them) entirely. I cannot image we'd want that.
I prefer meaning over numbers.
Basically, it boils down to a choice between: - Code which is easy to read. - Code which is easy to check against the numbers in a data sheet.
Converting #defines to numbers is easy to do automatically (the C preprocessor does that). The reverse action is hard and can't be automated due to ambiguities. That's why I want to keep the #defines.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Corey Osgood -
Marc Jones -
Peter Stuge -
Stefan Reinauer -
Uwe Hermann