Just to avoid duplication of effort, I am posting this preliminary
patch which adds PCI MODULE_DEVICE_TABLE declarations to the three PCI
drivers in linux-2.4.0-test11/drivers/block. In response to input from
Christoph Hellwig, I have reduced my threshhold on using named initializers
to three entries, although I think that may be going to far, as I would
really like to keep the number of files that initialize the pci_device_id
arrays this way low so that changing struct pci_device_id remains feasible.
Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
[email protected] \ / San Jose, California 95129-1034
+1 408 261-6630 | g g d r a s i l United States of America
fax +1 408 261-6631 "Free Software For The Rest Of Us."
--- linux-2.4.0-test11/drivers/block/DAC960.c Thu Oct 26 23:35:47 2000
+++ linux/drivers/block/DAC960.c Wed Nov 22 12:42:23 2000
@@ -40,11 +40,22 @@
#include <linux/spinlock.h>
#include <linux/timer.h>
#include <linux/pci.h>
+#include <linux/init.h>
#include <asm/io.h>
#include <asm/segment.h>
#include <asm/uaccess.h>
#include "DAC960.h"
+
+static struct pci_device_id DAC960_pci_tbl[] __initdata = {
+ { PCI_VENDOR_ID_MYLEX, PCI_DEVICE_ID_MYLEX_DAC960_BA, PCI_ANY_ID, PCI_ANY_ID},
+ { PCI_VENDOR_ID_MYLEX, PCI_DEVICE_ID_MYLEX_DAC960_LP, PCI_ANY_ID, PCI_ANY_ID},
+ { PCI_VENDOR_ID_DEC, PCI_DEVICE_ID_DEC_21285, PCI_ANY_ID, PCI_ANY_ID},
+ { PCI_VENDOR_ID_MYLEX, PCI_DEVICE_ID_MYLEX_DAC960_PG, PCI_ANY_ID, PCI_ANY_ID},
+ { PCI_VENDOR_ID_MYLEX, PCI_DEVICE_ID_MYLEX_DAC960_PD, PCI_ANY_ID, PCI_ANY_ID},
+ { } /* Terminating entry */
+};
+MODULE_DEVICE_TABLE(pci, DAC960_pci_tbl);
/*
DAC960_ControllerCount is the number of DAC960 Controllers detected.
--- linux-2.4.0-test11/drivers/block/cciss.c Thu Oct 26 23:35:47 2000
+++ linux/drivers/block/cciss.c Wed Nov 22 12:29:27 2000
@@ -50,6 +50,17 @@
/* Embedded module documentation macros - see modules.h */
MODULE_AUTHOR("Charles M. White III - Compaq Computer Corporation");
MODULE_DESCRIPTION("Driver for Compaq Smart Array Controller 5300");
+static struct pci_device_id cciss_pci_tbl[] __initdata = {
+ {
+ vendor: PCI_VENDOR_ID_COMPAQ,
+ device: PCI_DEVICE_ID_COMPAQ_CISS,
+ subvendor: PCI_ANY_ID,
+ subdevice: PCI_ANY_ID,
+ },
+ { } /* Terminating entry */
+};
+MODULE_DEVICE_TABLE(pci, cciss_pci_tbl);
+
#include "cciss_cmd.h"
#include "cciss.h"
--- linux-2.4.0-test11/drivers/block/cpqarray.c Thu Nov 16 11:30:29 2000
+++ linux/drivers/block/cpqarray.c Wed Nov 22 12:34:53 2000
@@ -52,6 +52,16 @@
MODULE_AUTHOR("Compaq Computer Corporation");
MODULE_DESCRIPTION("Driver for Compaq Smart2 Array Controllers");
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,0)
+static struct pci_device_id cpqarray_pci_tbl[] __initdata = {
+ { PCI_VENDOR_ID_DEC, PCI_DEVICE_ID_COMPAQ_42XX, PCI_ANY_ID, PCI_ANY_ID},
+ { PCI_VENDOR_ID_NCR, PCI_DEVICE_ID_NCR_53C1510, PCI_ANY_ID, PCI_ANY_ID},
+ { PCI_VENDOR_ID_COMPAQ, PCI_DEVICE_ID_COMPAQ_SMART2P,PCI_ANY_ID, PCI_ANY_ID},
+ { } /* Terminating entry */
+};
+MODULE_DEVICE_TABLE(pci, cpqarray_pci_tbl);
+#endif
+
#define MAJOR_NR COMPAQ_SMART2_MAJOR
#include <linux/blk.h>
#include <linux/blkdev.h>
"Adam J. Richter" wrote:
> Just to avoid duplication of effort, I am posting this preliminary
> patch which adds PCI MODULE_DEVICE_TABLE declarations to the three PCI
> drivers in linux-2.4.0-test11/drivers/block. In response to input from
> Christoph Hellwig, I have reduced my threshhold on using named initializers
> to three entries, although I think that may be going to far, as I would
> really like to keep the number of files that initialize the pci_device_id
> arrays this way low so that changing struct pci_device_id remains feasible.
*This* is the over-engineering attitude I was talking about. The only
reason why you are preferring named initializers is because
pci_device_id MIGHT be changed. And if it is changed, it makes the
changeover just tad easier. For that, you ugly up the code and make it
more difficult to maintain.
_I_ am one of the people that works on maintaining these random PCI
drivers that no one gives a shit about. And if applied, your patches
make my job just a little bit harder.
We -discourage- these kind of crap design decisions in the Linux kernel.
Don't over-engineer.
> --- linux-2.4.0-test11/drivers/block/DAC960.c Thu Oct 26 23:35:47 2000
> +++ linux/drivers/block/DAC960.c Wed Nov 22 12:42:23 2000
ok
> --- linux-2.4.0-test11/drivers/block/cciss.c Thu Oct 26 23:35:47 2000
> +++ linux/drivers/block/cciss.c Wed Nov 22 12:29:27 2000
> @@ -50,6 +50,17 @@
> /* Embedded module documentation macros - see modules.h */
> MODULE_AUTHOR("Charles M. White III - Compaq Computer Corporation");
> MODULE_DESCRIPTION("Driver for Compaq Smart Array Controller 5300");
> +static struct pci_device_id cciss_pci_tbl[] __initdata = {
> + {
> + vendor: PCI_VENDOR_ID_COMPAQ,
> + device: PCI_DEVICE_ID_COMPAQ_CISS,
> + subvendor: PCI_ANY_ID,
> + subdevice: PCI_ANY_ID,
> + },
> + { } /* Terminating entry */
> +};
> +MODULE_DEVICE_TABLE(pci, cciss_pci_tbl);
hell no
> --- linux-2.4.0-test11/drivers/block/cpqarray.c Thu Nov 16 11:30:29 2000
> +++ linux/drivers/block/cpqarray.c Wed Nov 22 12:34:53 2000
ok
--
Jeff Garzik |
Building 1024 | The chief enemy of creativity is "good" sense
MandrakeSoft | -- Picasso
On Wed, Nov 22, 2000 at 05:14:38PM -0500, Jeff Garzik wrote:
> *This* is the over-engineering attitude I was talking about. The only
> reason why you are preferring named initializers is because
> pci_device_id MIGHT be changed. And if it is changed, it makes the
> changeover just tad easier. For that, you ugly up the code and make it
> more difficult to maintain.
The other reason is that it makes self documenting code -- no need to look
up the structure definition to make sense out of the code.
-Andi (who thinks easily readable code is good)
Andi Kleen wrote:
>
> On Wed, Nov 22, 2000 at 05:14:38PM -0500, Jeff Garzik wrote:
> > *This* is the over-engineering attitude I was talking about. The only
> > reason why you are preferring named initializers is because
> > pci_device_id MIGHT be changed. And if it is changed, it makes the
> > changeover just tad easier. For that, you ugly up the code and make it
> > more difficult to maintain.
>
> The other reason is that it makes self documenting code -- no need to look
> up the structure definition to make sense out of the code.
For the general case, that is true.
But note that the general case is usually a -single- structure being
initialized, not an array of structures. Unless the struct members
being initialized vary wildly from one array element to another, using
named initialized it redundant and -reduces- the ability of the
programmer to look at the pci_tbl[] and evaluate its contents at a
glance.
PCI tables do not use named initalizers on purpose. It was not an
accident or design mistake.
Jeff
--
Jeff Garzik |
Building 1024 | The chief enemy of creativity is "good" sense
MandrakeSoft | -- Picasso
On Wed, 22 Nov 2000 14:01:36 -0800,
"Adam J. Richter" <[email protected]> wrote:
> Just to avoid duplication of effort, I am posting this preliminary
>patch which adds PCI MODULE_DEVICE_TABLE declarations to the three PCI
>drivers in linux-2.4.0-test11/drivers/block. In response to input from
>Christoph Hellwig, I have reduced my threshhold on using named initializers
>to three entries, although I think that may be going to far, as I would
>really like to keep the number of files that initialize the pci_device_id
>arrays this way low so that changing struct pci_device_id remains feasible.
No need for named initializers. Always add any new fields in struct
pci_device_id at the end. It does matter if you use named or unnamed
initializers, the new fields will be zero on all existing declarations.
Introducing new fields in the middle of pci_device_id introduces
version skew problems for modutils and all code that reads
modules.pcimap.
Adam J. Richter writes:
> + { PCI_VENDOR_ID_DEC, PCI_DEVICE_ID_DEC_21285, PCI_ANY_ID, PCI_ANY_ID},
No no no no no no no.
This "device" should be identified by either the class code OR the
subsystem device/vendor IDs.
By using "PCI_VENDOR_ID_DEC" and "PCI_DEVICE_ID_DEC_21285" you are referring
to a chip which can be:
1. A host bridge
2. A non-I2O add in card performing non-I2O functions
3. An I2O card
_____
|_____| ------------------------------------------------- ---+---+-
| | Russell King [email protected] --- ---
| | | | http://www.arm.linux.org.uk/personal/aboutme.html / / |
| +-+-+ --- -+-
/ | THE developer of ARM Linux |+| /|\
/ | | | --- |
+-+-+ ------------------------------------------------- /\\\ |
>"Adam J. Richter" wrote:
>> Just to avoid duplication of effort, I am posting this preliminary
>> patch which adds PCI MODULE_DEVICE_TABLE declarations to the three PCI
>> drivers in linux-2.4.0-test11/drivers/block. In response to input from
>> Christoph Hellwig, I have reduced my threshhold on using named initializers
>> to three entries, although I think that may be going to far, as I would
>> really like to keep the number of files that initialize the pci_device_id
>> arrays this way low so that changing struct pci_device_id remains feasible.
>*This* is the over-engineering attitude I was talking about. The only
>reason why you are preferring named initializers is because
>pci_device_id MIGHT be changed. And if it is changed, it makes the
>changeover just tad easier.
It is also much easier to spot an initialization bug, if, say,
a class is being initialized with a class_mask. It also make the
code much more self-documenting. I frequently have to bring up a copy
of include/linux/pci.h to decipher usb_devicde_id tables.
>For that, you ugly up the code and make it
>more difficult to maintain.
I think this makes it easier to maintain, especially by
driver authors who want to think more about their pet hardware than
how a generic kernel data structure is ordered.
Also bear in mind that once these drivers are ported to the
new PCI interface, many will use pci_device_id->driver_data, which
will cause all of the entries that are not in "field:value" form to
no longer fit on one line anyhow.
>_I_ am one of the people that works on maintaining these random PCI
>drivers that no one gives a shit about.
I don't believe that using "field:value" format makes centralized
maintenance harder, but, if you find it that way, you should consider
the position of driver author a driver author who is more
knowledgeable about the hardware and has less repetitive need to
memorize the arrangement of an obscure kernel data structure. The
__initdata vs __devinitdata for the pci_device_id tables is the same
sort of issue. I believe that named initializers also make it easier
for developers whose focus is not on the central kernel data structures
to spot and fix bugs and develop new drivers, and that this is a more
scalable approach to kernel development.
In any case, if you want, I would be happy to send you patches
that include only the changes that do the initilization anonymously.
Until those appear in Linus's releases, I see it is a more definitely
positive contribution on my part to focus on writing pci_device_id
tables for other drivers that lack them.
Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
[email protected] \ / San Jose, California 95129-1034
+1 408 261-6630 | g g d r a s i l United States of America
fax +1 408 261-6631 "Free Software For The Rest Of Us."