Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

From: Matthew Wilcox
Date: Fri Dec 17 2004 - 11:25:53 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,

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 | \

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

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@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at