Re: [PATCH 1/11] qla2xxx: Add ISP24xx definitions.

[Andrew Vasquez <andrew.vasquez@qlogic.com>]

On Tue, 14 Jun 2005, Christoph Hellwig wrote:

> > +#ifndef PCI_DEVICE_ID_QLOGIC_ISP2512
> > +#define PCI_DEVICE_ID_QLOGIC_ISP2512	0x2512
> > +#endif
> > +
> > +#ifndef PCI_DEVICE_ID_QLOGIC_ISP2522
> > +#define PCI_DEVICE_ID_QLOGIC_ISP2522	0x2522
> > +#endif
> 
> It's only an historic accident that the older ids are in here, care
> to remove all and move them to pci_ids.h instead?
> 
> > +	int		(*start_scsi)(srb_t *);
> 
> umm, that's most of ->queuecommand, isn't it?  Shouldn't you define
> a separate scsi_host_template for 24xx/25xx?
> 
> > +//ISP24xx
> 
> please try to avoid C++-style comments
> 
> > +	volatile uint16_t mailbox0;
> > +	volatile uint16_t mailbox1;
> > +	volatile uint16_t mailbox2;
> 
> never use volatile, always use proper kernel primitives to access
> hardware or atomic_t for atomic in-kernel variables.

No problem.  I'll have another patchset which hopefully addresses the
concerns and with a bit of additional testing in the next week or so.

Thanks,
Andrew