Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/