* [PATCH] 2.6.10 Altix : ioc4 serial driver support @ 2004-12-16 22:24 Pat Gefre 2004-12-16 22:43 ` Matthew Wilcox 2004-12-16 23:15 ` Christoph Hellwig 0 siblings, 2 replies; 6+ messages in thread From: Pat Gefre @ 2004-12-16 22:24 UTC (permalink / raw) To: linux-kernel, linux-ia64 I have a serial driver for Altix I'd like to submit. The code is at: ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support Signed-off-by: Patrick Gefre <pfg@sgi.com> -- Pat Patrick Gefre Silicon Graphics, Inc. (E-Mail) pfg@sgi.com 2750 Blue Water Rd (Voice) (651) 683-3127 Eagan, MN 55121-1400 (FAX) (651) 683-3054 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support 2004-12-16 22:24 [PATCH] 2.6.10 Altix : ioc4 serial driver support Pat Gefre @ 2004-12-16 22:43 ` Matthew Wilcox 2004-12-16 23:15 ` Christoph Hellwig 1 sibling, 0 replies; 6+ messages in thread From: Matthew Wilcox @ 2004-12-16 22:43 UTC (permalink / raw) To: Pat Gefre; +Cc: linux-kernel, linux-ia64 On Thu, Dec 16, 2004 at 04:24:26PM -0600, Pat Gefre wrote: > I have a serial driver for Altix I'd like to submit. Why put it in arch/ia64/sn/io/sn2/driver/ioc4_serial.c ?! drivers/serial/ioc4.c would be the right place for it. You put the Kconfig there -- that should be a clue. It seems like you're directly dereferencing pointers to io memory instead of calling readb and friends. I know, this driver doesn't need to be portable, but it helps any casual reader of this driver figure out what's going on. And you can get rid of the 'volatile' that way ;-) Linux Device Drivers, Second edition says you shouldn't use SA_INTERRUPT without good reason (http://www.xml.com/ldd/chapter/book/ch09.html#t3) -- "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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support 2004-12-16 22:24 [PATCH] 2.6.10 Altix : ioc4 serial driver support Pat Gefre 2004-12-16 22:43 ` Matthew Wilcox @ 2004-12-16 23:15 ` Christoph Hellwig 2004-12-17 16:24 ` Matthew Wilcox 2004-12-17 22:14 ` Patrick Gefre 1 sibling, 2 replies; 6+ messages in thread From: Christoph Hellwig @ 2004-12-16 23:15 UTC (permalink / raw) To: Pat Gefre; +Cc: linux-kernel, linux-ia64 On Thu, Dec 16, 2004 at 04:24:26PM -0600, Pat Gefre wrote: > I have a serial driver for Altix I'd like to submit. > > The code is at: > ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support > > Signed-off-by: Patrick Gefre <pfg@sgi.com> I took a very short look and what spring to mind first is that the device probing/remoal is rather bogus. The ->probe/->remove callbacks of a PCI driver can be called at any time, and any initialization / teardown actions must happen from those. A logical consequence of that is that a proper PCI driver should have no global state. I'd also like to second Matthews commens, please move the driver to drivers/serial and use proper readX/writeX accessors. Please run the driver through sparse to find the iomem derferences and possibly other issues. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support 2004-12-16 23:15 ` Christoph Hellwig @ 2004-12-17 16:24 ` Matthew Wilcox 2004-12-17 22:14 ` Patrick Gefre 1 sibling, 0 replies; 6+ messages in thread From: Matthew Wilcox @ 2004-12-17 16:24 UTC (permalink / raw) To: Christoph Hellwig, Pat Gefre, linux-kernel, linux-ia64 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support 2004-12-16 23:15 ` Christoph Hellwig 2004-12-17 16:24 ` Matthew Wilcox @ 2004-12-17 22:14 ` Patrick Gefre 2004-12-18 14:51 ` Christoph Hellwig 1 sibling, 1 reply; 6+ messages in thread From: Patrick Gefre @ 2004-12-17 22:14 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-kernel, linux-ia64 Christoph Hellwig wrote: > On Thu, Dec 16, 2004 at 04:24:26PM -0600, Pat Gefre wrote: > >>I have a serial driver for Altix I'd like to submit. >> >>The code is at: >>ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support >> >>Signed-off-by: Patrick Gefre <pfg@sgi.com> > > > I took a very short look and what spring to mind first is that the > device probing/remoal is rather bogus. The ->probe/->remove callbacks > of a PCI driver can be called at any time, and any initialization / > teardown actions must happen from those. A logical consequence of that > is that a proper PCI driver should have no global state. > Christoph, I'm not sure what you mean here. I don't have an entry for ->remove and the driver is self-contained. -- Pat ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support 2004-12-17 22:14 ` Patrick Gefre @ 2004-12-18 14:51 ` Christoph Hellwig 0 siblings, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2004-12-18 14:51 UTC (permalink / raw) To: Patrick Gefre; +Cc: linux-kernel, linux-ia64 > I'm not sure what you mean here. I don't have an entry for ->remove and the > driver is self-contained. In a PCI driver (well, just about any driver for a modern bus) you have an probe and an remove entry. All code to setup and teardown a device must happen in those functions, and must not use global state but only the device instance pointers. btw, no need to Cc linux-ia64, there's nothing ia64-specific in this driver. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-12-18 14:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2004-12-16 22:24 [PATCH] 2.6.10 Altix : ioc4 serial driver support Pat Gefre 2004-12-16 22:43 ` Matthew Wilcox 2004-12-16 23:15 ` Christoph Hellwig 2004-12-17 16:24 ` Matthew Wilcox 2004-12-17 22:14 ` Patrick Gefre 2004-12-18 14:51 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox