lkml.org 
[lkml]   [2003]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: 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/

    \
     
     \ /
      Last update: 2005-03-22 13:59    [W:0.028 / U:1.308 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site