Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

[Jeff Garzik]

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