On 09/12/2009 10:41 PM, Jung-Ik (John) Lee wrote:
On Sat, Sep 12, 2009 at 5:55 PM, Jeff Garzik<jgarzik@xxxxxxxxx> wrote:
General comment:
* since you use iomap to map the region, you should use ioread{8,16,32} /
iowrite{8,16,32} accessors. Do not use inb/outb/inl/outl/etc.
.
I used them for runtime hot registers by separately mapping them
simply to avoid an extra overhead of ioread/iowrite, over the
portability.
I know it's not a good idea but in this case for these hot ports can
in/out be used?
It is _highly_ unlikely that the overhead is even measureable above the
noise, I would think. Do you have data showing that ioread/iowrite
impose a noticeable penalty?
* run through scripts/checkpatch.pl
Weird. I don't see any WS issues you pointed below in my source code
or git diff file, except UT = T/4 below.
My apologies; most of those appear to be problems with Thunderbird. I
think it renders <tab> incorrectly.
+static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device
*adev)
+{
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ struct atp867x_priv *dp = ap->private_data;
+ u8 speed = adev->dma_mode;
+ u8 b;
+ u8 mode;
+
+
+ switch (speed) {
+ case XFER_UDMA_6:
+ mode = ATP867X_IO_DMAMODE_UDMA_6;
+ break;
+ case XFER_UDMA_5:
+ mode = ATP867X_IO_DMAMODE_UDMA_5;
+ break;
+ case XFER_UDMA_4:
+ mode = ATP867X_IO_DMAMODE_UDMA_4;
+ break;
+ case XFER_UDMA_3:
+ mode = ATP867X_IO_DMAMODE_UDMA_3;
+ break;
+ case XFER_UDMA_2:
+ mode = ATP867X_IO_DMAMODE_UDMA_2;
+ break;
+ case XFER_UDMA_1:
+ mode = ATP867X_IO_DMAMODE_UDMA_1;
+ break;
+ case XFER_UDMA_0:
+ mode = ATP867X_IO_DMAMODE_UDMA_0;
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
+ " Default to XFER_UDMA_0.\n", (unsigned)speed);
+ mode = ATP867X_IO_DMAMODE_UDMA_0;
a table would be nice, preferred over a switch statement. You may use
ARRAY_SIZE() macro to generate a constant at compile time for number of
elements in array.
OK. I had it in a pure math like mode = speed - XFER_UDMA_0 +1;
That's fine too.
+ /*
+ * Broken BIOS might not set latency high enough
+ */
+ pci_read_config_byte(pdev, PCI_LATENCY_TIMER,&v);
+ if (v< 0x80) {
+ v = 0x80;
+ pci_write_config_byte(pdev, PCI_LATENCY_TIMER, v);
+ printk(KERN_DEBUG "ATP867X: set latency timer of device
%s"
+ " to %d\n", pci_name(pdev), v);
+ }
this seems pointless - pci_set_master() already does this
pci_set_master won't re-set it if BIOS set it to somewhere between 16
and 256. This controller wants 0x80.
so, if BIOS set to less than 0x80, like 0x20, pci_set_master will keep
the value.
I could do this via pci fixup or quirks but that seems too much for
this simple setting.
Given your explanation, that's fine.
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
