Messages in this thread | | | Date | Wed, 17 Dec 2003 00:14:34 +0200 | From | Vladimir Kondratiev <> | Subject | Re: PCI Express support for 2.4 kernel |
| |
Greg KH wrote:
>Minor code comments below: > >On Tue, Dec 16, 2003 at 12:20:39PM +0200, Vladimir Kondratiev wrote: > > >>+ >>+#ifdef CONFIG_PCI_EXPRESS >>+#define PCI_PROBE_EXP 0x0008 >>+#endif >>+ >> >> > >If you change this to: >#ifdef CONFIG_PCI_EXPRESS >#define PCI_PROBE_EXP 0x0008 >#else >#define PCI_PROBE_EXP 0x0000 >#endif > >You can get rid of a number of "#ifdef CONFIG_PCI_EXPRESS" statements >later on in the .c files by just always using the PCI_PROBE_EXP value. >That cleans up the patch a bit. > > Good point. Agree.
>>+/** >>+ * RRBAR (memory base for PCI-E config space) resides here. >>+ * Initialized to default address. Actually, it is platform specific, and >>+ * value may vary. >>+ * I don't know how to detect it properly, it is chipset specific. >>+ */ >>+static u32 rrbar_phys = CONFIG_PCI_EXPRESS_BASE * 0x10000000UL; >> >> > ><snip> > > > >>+/** >>+ * I don't know how to detect it properly. >>+ * assume it is PCI-E, sanity_check will >>+ * stop me if it is not. >>+ * >>+ * Also, this function supposed to set rrbar_phys >>+ */ >>+static int is_pcie_platform(void) >>+{ return 1; } >> >> > >Shouldn't your comments at least match your code for the rrbar_phys >statement for your first release? :) > > Default moved to CONFIG_PCI_EXPRESS_BASE. Idea is some time later we fill replace is_pcie_platform with real auto detection...
>>+static int pci_express_init(void) >>+{ >>+ rrbar_window_virt = (void*)fix_to_virt(FIX_PCIE_CONFIG); >>+ if (!is_pcie_platform()) >>+ return 0; >> >> > >Call fix_to_virt() after you do the check and not before? > > > ... and I assume virtual address may be used by auto detection code.
> > >>+/** >>+ * Shuts down PCI-E resources. >>+ */ >>+static inline void pci_express_fini(void) >>+{} >> >> > >If this isn't needed, why have it anymore? > > Placeholder, to not forget. It is called in case auto detection says it is not PCI-E.
> > >>+static struct pci_ops pci_express_conf = { >>+ pci_exp_read_config_byte, >>+ pci_exp_read_config_word, >>+ pci_exp_read_config_dword, >>+ pci_exp_write_config_byte, >>+ pci_exp_write_config_word, >>+ pci_exp_write_config_dword >>+}; >> >> > >C99 initializers here? > > cut'n'paste from other methods, to keep style. If change, let's change all.
> > >>+ printk(KERN_INFO "PCI-Express config at 0x%08x\n", rrbar_phys); >> >> > >"%p" to show the address might be nicer. > > > I print phys. address, it is u32. Do you mean (void*)rrbar_phys? Don't see why not to change, I have no strong opinion for which format is better.
- 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/
| |