....
 

Guardian Digital Inc. > InfoCenter > Mailing List Archives > Linux Kernel

Linux Kernel Mailing List Archive

From: Matthew Wilcox (matthew@wil.cx)
Date: Fri Dec 17 2004 - 11:24:11 EST


On Thu, Dec 16, 2004 at 11:15:19PM +0000, Christoph Hellwig wrote:
> I took a very short look and what spring to mind first is that the

Rats, I'd hoped you'd have time to do a more thorough review. Here's
some more comments:

Don't define your own names for standard PCI config space -- use the
ones in linux/pci.h instead. This whole section should be deleted:

+/*
+ * PCI Configuration Space Register Address Map, use offset from IOC4 PCI
+ * configuration base such that this can be used for multiple IOC4s
+ */
+#define IOC4_PCI_SCR 0x4 /* Status/Command */
+#define IOC4_PCI_REV 0x8 /* Revision */
+#define IOC4_PCI_LAT 0xC /* Latency Timer */
+#define IOC4_PCI_BAR0 0x10 /* IOC4 base address 0 */
+#define IOC4_PCI_SIDV 0x2c /* Subsys ID and vendor */
+#define IOC4_PCI_CAP 0x34 /* Capability pointer */
+#define IOC4_PCI_LATGNTINT 0x3c /* Max_lat, min_gnt, int_pin, int_line */

Calling a pci_dev a "pci_handle" is confusing; most code uses "pdev".

+ pci_read_config_dword(pci_handle, IOC4_PCI_SCR, &tmp);
+ pci_write_config_dword(pci_handle, IOC4_PCI_SCR,
+ tmp | PCI_COMMAND_MASTER |
+ PCI_COMMAND_MEMORY |
+ PCI_COMMAND_PARITY | PCI_COMMAND_SERR);

You call pci_set_master() before this which takes care of PCI_COMMAND_MASTER.
You also call pci_enable_device() which calls pcibios_enable_device()
which ensures PCI_COMMAND_MEMORY is set if it needs to be.

So the code above should be:

        pci_read_config_dword(pdev, PCI_COMMAND, &cmd);
        pci_write_config_dword(pdev, PCI_COMMAND, cmd | \
                        PCI_COMMAND_PARITY | PCI_COMMAND_SERR);

Personally, I believe we should be setting PCI_COMMAND_PARITY and
PCI_COMMAND_SERR on all devices by default in pcibios_enable_device,
if not in pci_enable_device(). But we don't, so it's fine to do it
in your driver for the moment.

You don't need the:

+ if (!ia64_platform_is("sn2"))
+ return -ENODEV;

since this code will only ever be called if someone has an ioc4 in
their system. If it's not an sn2, something's very strange ;-)

+struct pci_driver ioc4_s_driver = {
+ name :"IOC4 Serial",
+ id_table :ioc4_s_id_table,
+ probe :ioc4_attach,
+};

please use C99 initialisers instead

+ {PCI_VENDOR_ID_SGI, PCI_DEVICE_ID_SGI_IOC4, PCI_ANY_ID, PCI_ANY_ID, 0,0,0},
you don't need the trailing zeroes

I don't see why you need valid_icount_path(). Everywhere it's called,
you seem to have been handed an ioc4_port back by the kernel core.
Are you just checking for data corruption elsewhere, or is this masking
a bug elsewhere in the driver?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[ About Guardian Digital ] - [ Press Center ] - [ Contact Us ] - [ System Activation ] - [ Reseller Info ] - [ Online Store ] - [ Site Map ]
Copyright (c) 2000 - 2004 Guardian Digital, Inc. Linux Lockbox and EnGarde are Trademarks of Guardian Digital, Inc.