Re: Patch(?): pci_device_id tables for drivers/scsi in 2.4.0-test11
>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."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: Patch(?): pci_device_id tables for drivers/scsi in 2.4.0-test11
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."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: Patch(?): pci_device_id tables for drivers/scsi in 2.4.0-test11
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: Patch(?): pci_device_id tables for drivers/scsi in 2.4.0-test11
[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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: Patch(?): pci_device_id tables for drivers/scsi in 2.4.0-test11
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." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch(?): pci_device_id tables for drivers/scsi in 2.4.0-test11
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch(?): pci_device_id tables for drivers/scsi in 2.4.0-test11
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch(?): pci_device_id tables for drivers/scsi in 2.4.0-test11
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." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch(?): pci_device_id tables for drivers/scsi in 2.4.0-test11
[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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: Patch(?): pci_device_id tables for drivers/scsi in 2.4.0-test11
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
