Re: Fw: ACARD SCSI driver update for Linux kernel v2.6

[Matthew Wilcox <matthew@wil.cx>]

On Fri, Jan 14, 2005 at 06:03:02PM +0800, jameshsu wrote:
> We, Acard, total understand what you suggest. (including advise from
> Christoph, James B. and Alan.)
> Therefore, we modify the internal code to create this patch file from kernel
> v2.6.10.
> However, as you see this is the smallest patch files which could be
> generated. (Sorry, unable to meet your expetation)
> The reason is : this driver could supports ATP870 as well as mutiples of
> Acard's chipsets. (this includes ATP880(AEC67160) & ATP885(AEC67162)). This
> is why the patch files is huge than what you expected.
> We did not make any change for ATP870(AEC6712) on this rev.. Besides, we
> reverse following codes according to all of your suggestions. Please review
> and let me know if this is feasible.  Welcome to any of question you may
> have.
> Thanks for your help!

The change to pci.ids needs to be handled through pciids.sourceforge.net

----

I really don't like the:
+               atp_dev.ioport[0] = base_io + 0x80;
+               atp_dev.ioport[1] = base_io + 0xc0;

Just record base_io in your atp_dev.  I don't like the way this patch
turns everything into a 2-d array ... surely there has to be a better
way to do it than this?

----

+               if (dev->dev_id == ATP885_DEVID) {
+                       tmpcip += 2;
+                       outb(0x06, tmpcip);
+                       tmpcip -= 2;

would be much better written as:

		if (dev->dev_id == ATP885_DEVID)
			outb(0x06, tmpcip+2);

(hmm, this seems to be common coding style throughout the driver ...)

----

-       if (pci_set_dma_mask(dev, 0xFFFFFFFFUL)) {
-               printk(KERN_ERR "atp870u: 32bit DMA mask required but not availa
ble.\n");
-               return -EIO;
-       }
+        if (!pci_set_dma_mask(pdev, 0xFFFFFFUL)) {
+                printk(KERN_INFO "atp870u: use 32bit DMA mask.\n");
+        } else {
+                printk(KERN_ERR "atp870u: DMA mask required but not available.\
n");
+                return -EIO;
+        }

You've actually set a 24-bit DMA mask there.  Should use the symbolic
constant DMA_32BIT_MASK anyway.

----

+       { PCI_DEVICE(PCI_VENDOR_ID_ARTOP, ATP885_DEVID)                   },

Please use the PCI_DEVICE_ID_ name instead

----



-- 
"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