On Sat, Sep 12, 2009 at 8:12 PM, Jeff Garzik <jgarzik@xxxxxxxxx> wrote: > 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? I agree in that it's hard to measure/signify the additional overhead, since those io insts are already too slow. Anyways, the two extra "if"s and one PIO_MASK on every ioread/iowrite are pure overhead on top of in/out insts. Thanks, -John > > >> >> >>> >>> * 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