Here is my first pass at adding pci_device_id tables to all
PCI scsi drivers in linux-2.4.0-test11. It implements a compromise
regarding named initializers for pci_device_id table entries: shorter
tables or tables that contain anonymous constants use the named fields,
but the few longer tables where the purpose of the constants are more
clearly labelled do not use named fields, because those tables would
be really big otherwise (in terms of lines of source code, not what
they compile into). The theory in this is that the number of tables
that use the unlabelled structure initializers is still kept relatively
small, so a change to pci_device_id that would require changing the
initializers would involve changing only a relatively small number
of drivers, as opposed to potentially all ~150 PCI drivers.
--
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."
In article <[email protected]> you wrote:
> --opJtzjQTFsWo+cga
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Here is my first pass at adding pci_device_id tables to all
> PCI scsi drivers in linux-2.4.0-test11. It implements a compromise
> regarding named initializers for pci_device_id table entries: shorter
> tables or tables that contain anonymous constants use the named fields,
> but the few longer tables where the purpose of the constants are more
> clearly labelled do not use named fields, because those tables would
> be really big otherwise (in terms of lines of source code, not what
> they compile into).
IMHO the pci tables look much cleaner without named initiliters.
They are really ugly if we nest structures and arrays.
The other argument for named initilizers don't aren't true in this
case too. Neither there are lots of NULL-initilized fields nor is
there any reason to add new fields (the pci tables are external
API, because of MODULE_DEVICE_TABLE).
Christoph
--
Always remember that you are unique. Just like everyone else.
Christoph Hellwig <[email protected]> writes:
>Neither there are lots of NULL-initilized fields nor is
>there any reason to add new fields (the pci tables are external
>API, because of MODULE_DEVICE_TABLE).
PCI ID matching relies on the zeros being filled in for
an empty value in the case of class_mask. depmod deliberately
includes a format comment at the start of modules.pcimap so that the
structure can be changed in the future.
However, thanks for your feedback. I will take it into
consideration.
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."
[Adam J. Richter]
> +static struct pci_device_id atp870u_pci_tbl[] __initdata = {
> +{vendor: 0x1191, device: 0x8002, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
> +{vendor: 0x1191, device: 0x8010, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
> +{vendor: 0x1191, device: 0x8020, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
> +{vendor: 0x1191, device: 0x8030, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
> +{vendor: 0x1191, device: 0x8040, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
> +{vendor: 0x1191, device: 0x8050, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
> +{vendor: 0x1191, device: 0x8060, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
You should add the ID numbers to pci_ids.h instead of hardcoding them
here. That would make the structure a lot more readable than using
member name tags.
Peter
--- include/linux/pci_ids.h.orig Mon Nov 13 01:43:49 2000
+++ include/linux/pci_ids.h Wed Nov 22 23:57:51 2000
@@ -864,6 +864,13 @@
#define PCI_DEVICE_ID_ARTOP_ATP850UF 0x0005
#define PCI_DEVICE_ID_ARTOP_ATP860 0x0006
#define PCI_DEVICE_ID_ARTOP_ATP860R 0x0007
+#define PCI_DEVICE_ID_ARTOP_AEC7610 0x8002
+#define PCI_DEVICE_ID_ARTOP_AEC7612UW 0x8010
+#define PCI_DEVICE_ID_ARTOP_AEC7612U 0x8020
+#define PCI_DEVICE_ID_ARTOP_AEC7612S 0x8030
+#define PCI_DEVICE_ID_ARTOP_AEC7612D 0x8040
+#define PCI_DEVICE_ID_ARTOP_AEC7612SUW 0x8050
+//#define PCI_DEVICE_ID_ARTOP_??? 0x8060
#define PCI_VENDOR_ID_ZEITNET 0x1193
#define PCI_DEVICE_ID_ZEITNET_1221 0x0001
[Adam J. Richter]
> +static struct pci_device_id atp870u_pci_tbl[] __initdata = {
> +{vendor: 0x1191, device: 0x8002, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
> +{vendor: 0x1191, device: 0x8010, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
It would make it easier to read and safer to type if you used a macro
to generate the fields.
#define PCITBL(v,d,sv,sd) \
{ PCI_VENDOR_ID_##v, PCI_DEVICE_ID_##d, \
PCI_VENDOR_ID_##sv, PCI_DEVICE_ID_##sd }
#define PCITBL_END {0,0,0,0}
static struct pci_device_id foo_pci_tbl[] __initdata = {
PCITBL(INTEL, INTEL_82437VX, ANY, ANY),
PCITBL_END
}
Shorter is easier to read. Using a prefix on the fields makes it much
harder for somebody to accidentally swap device and vendor codes. If
they swap the parameters and type
PCITBL(INTEL_82437VX, INTEL, ANY, ANY),
by mistake then they get compile errors, PCI_VENDOR_ID_INTEL_82437VX
and PCI_DEVICE_ID_INTEL are undefined.
Keith Owens wrote:
>
> [Adam J. Richter]
> > +static struct pci_device_id atp870u_pci_tbl[] __initdata = {
> > +{vendor: 0x1191, device: 0x8002, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
> > +{vendor: 0x1191, device: 0x8010, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
>
> It would make it easier to read and safer to type if you used a macro
> to generate the fields.
>
> #define PCITBL(v,d,sv,sd) \
> { PCI_VENDOR_ID_##v, PCI_DEVICE_ID_##d, \
> PCI_VENDOR_ID_##sv, PCI_DEVICE_ID_##sd }
>
> #define PCITBL_END {0,0,0,0}
>
> static struct pci_device_id foo_pci_tbl[] __initdata = {
> PCITBL(INTEL, INTEL_82437VX, ANY, ANY),
> PCITBL_END
> }
* your macro fails for the 'ANY' case, because the proper macro is
PCI_ANY_ID not PCI_{VENDOR,DEVICE}_ID_ANY.
* many drivers need to set the driver_data field
That said, the general idea presented above is quite good. The PCITBL
macro will probably change on a per-driver basis... I don't think it
belongs in a common header. For example, ethernet drivers might want to
have a macro that always checks for PCI-CLASS-ETHERNET under the hood,
while visibly looking like
PCITBL(INTEL, 82437VX, {a driver_data value}),
PCITBL(INTEL, 82987VX, {another driver_data value}),
PCITBL_END
Jeff
--
Jeff Garzik |
Building 1024 | The chief enemy of creativity is "good" sense
MandrakeSoft | -- Picasso
On Thu, 23 Nov 2000 02:25:33 -0500,
Jeff Garzik <[email protected]> wrote:
>Keith Owens wrote:
>> #define PCITBL(v,d,sv,sd) \
>> { PCI_VENDOR_ID_##v, PCI_DEVICE_ID_##d, \
>> PCI_VENDOR_ID_##sv, PCI_DEVICE_ID_##sd }
>
>* your macro fails for the 'ANY' case, because the proper macro is
>PCI_ANY_ID not PCI_{VENDOR,DEVICE}_ID_ANY.
It was just a suggestion. Actually getting it working was left as an
exercise for the reader ;).
#define PCI_VENDOR_ID_ANY PCI_ANY_ID
#define PCI_DEVICE_ID_ANY PCI_ANY_ID
>Keith Owens wrote:
>>
>> [Adam J. Richter]
>> > +static struct pci_device_id atp870u_pci_tbl[] __initdata = {
>> > +{vendor: 0x1191, device: 0x8002, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
>> > +{vendor: 0x1191, device: 0x8010, subvendor: PCI_ANY_ID, subdevice: PCI_ANY_ID},
>>
>> It would make it easier to read and safer to type if you used a macro
>> to generate the fields.
>>
>> #define PCITBL(v,d,sv,sd) \
>> { PCI_VENDOR_ID_##v, PCI_DEVICE_ID_##d, \
>> PCI_VENDOR_ID_##sv, PCI_DEVICE_ID_##sd }
>>
>> #define PCITBL_END {0,0,0,0}
>>
>> static struct pci_device_id foo_pci_tbl[] __initdata = {
>> PCITBL(INTEL, INTEL_82437VX, ANY, ANY),
>> PCITBL_END
>> }
>* your macro fails for the 'ANY' case, because the proper macro is
>PCI_ANY_ID not PCI_{VENDOR,DEVICE}_ID_ANY.
>* many drivers need to set the driver_data field
>That said, the general idea presented above is quite good. The PCITBL
>macro will probably change on a per-driver basis... I don't think it
>belongs in a common header. For example, ethernet drivers might want to
>have a macro that always checks for PCI-CLASS-ETHERNET under the hood,
>while visibly looking like
>PCITBL(INTEL, 82437VX, {a driver_data value}),
>PCITBL(INTEL, 82987VX, {another driver_data value}),
>PCITBL_END
> Jeff
Jaroslav Kysela's isapnp MODULE_DEVICE_TABLE support uses a
a similar scheme (see Documentation/isapnp.txt and
include/linux/isapnp.h). I also used similar macros to add
support usb MODULE_DEVICE_TABLE support for usb/storage/
(not yet in 2.4.0-test11 yet) and usb/pegasus.c.
Note that, in all of these cases, the macros expand to field:value
form, which addresses my concerns about modifiability.
The cases where I implemented this sort of thing in the USB code
were where there was a big tables of values that included device
ID information plus more information than could comfortably fit in
in id->driver_data. Using a trick used in the gcc sources, I changed
the table entries into a calls to a macro and moved them into a
separate .h file, which was included multiple times with different
definitions of the macro to construct the usb_device_id table and a
parallel table of additional driver specific data. There are a bunch
of cases in the code that I have been adding pci_device_id tables
to where an approach like this will probably end up being used
when somebody gets around to eliminating the duplication of data
between the pci_device_id table and the "old" way the driver
organized the PCI ID information.
In the specific case of the current drivers/scsi/atp870u.c,
I think a macro would not help yet becuase the pci entries
still fit on one line without the macro. So, it would just
make the code longer and perhaps less clear. However, when the
devid[] array in that driver is eliminated and perhaps the
driver is ported to the new PCI interface, then the
vendor_data field will probably be used and it will probably be
worthwhile to define a macro as you describe (at least that's
my prediction; I don't claim to be the maintainer of any of these
drivers).
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."