On Tuesday 03 April 2012, H Hartley Sweeten wrote:
> On Tuesday, April 03, 2012 7:45 AM, Rafal Prylowski wrote:
> > +
> > + /* UDMA Debug Status Register */
> > + IDEUDMADEBUG = 0x2c,
> > +};
>
> I prefer defines for these. But, the enum usage seems common in the ata drivers
> so...
I like the enum ;-)
> > +
> > +static int ep93xx_pata_check_iordy(void __iomem *base)
> > +{
> > + return !!(readl(base + IDECTRL) & IDECTRL_IORDY);
> > +}
This one could return a bool.
> > +static void ep93xx_pata_wait_for_iordy(void __iomem *base)
> > +{
> > + unsigned long deadline = jiffies + msecs_to_jiffies(1000);
> > + while (!ep93xx_pata_check_iordy(base) &&
> > + time_is_before_jiffies(deadline))
> > + cpu_relax();
> > +}
Much better for a delay than the previous version. However, it's
still a busy wait, which is bad for realtime behavior, especially
since this can potentially take many milliseconds. If possible,
it should have an msleep() or at least cond_resched() instead of
the cpu_relax(). Obviously that will only work when no spinlocks
are held.
> > + if (is_addr) {
> > + ep93xx_pata_write(drv_data, tf->feature,
> > + IDECTRL_ADDR_FEATURE, t);
> > + ep93xx_pata_write(drv_data, tf->nsect, IDECTRL_ADDR_NSECT, t);
> > + ep93xx_pata_write(drv_data, tf->lbal, IDECTRL_ADDR_LBAL, t);
> > + ep93xx_pata_write(drv_data, tf->lbam, IDECTRL_ADDR_LBAM, t);
> > + ep93xx_pata_write(drv_data, tf->lbah, IDECTRL_ADDR_LBAH, t);
> > + VPRINTK("feat 0x%X nsect 0x%X lba 0x%X 0x%X 0x%X\n",
> > + tf->feature, tf->nsect, tf->lbal, tf->lbam, tf->lbah);
> > + }
> > +
> > + if (tf->flags & ATA_TFLAG_DEVICE) {
> > + ep93xx_pata_write(drv_data, tf->device, IDECTRL_ADDR_DEVICE, t);
> > + VPRINTK("device 0x%X\n", tf->device);
> > + }
Although VPRINTK is used in other ATA drivers, I would not recommend it
in new drivers. Instead, I suggest you use dev_err() and similar helpers
that are not specific to one subsystem.
> > +/* Note: original code is ata_sff_data_xfer */
> > +unsigned int ep93xx_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
> > + unsigned int buflen, int rw)
This should be a static function. A device driver with just a single file should
not need any non-static symbols. Same thing for ep93xx_pata_drain_fifo and
possibly a few others.
> > +static int __devinit ep93xx_pata_probe(struct platform_device *pdev)
> > +{
> > + struct ep93xx_pata_data *drv_data;
> > + struct ata_host *host;
> > + struct ata_port *ap;
> > + unsigned int irq;
> > + struct resource *mem_res;
> > + void __iomem *ide_base;
> > + int err;
> > +
> > + err = ep93xx_ide_acquire_gpio(pdev);
> > + if (err)
> > + return err;
Calling into platform code like this is going to make the transition to
device tree harder. I would suggest moving the code from patch 2 here,
and subsequently putting the gpio lines into device tree properties.
when the driver gets converted.
> > @@ -43,6 +43,7 @@ obj-$(CONFIG_PATA_CS5535) += pata_cs5535
> > obj-$(CONFIG_PATA_CS5536) += pata_cs5536.o
> > obj-$(CONFIG_PATA_CYPRESS) += pata_cypress.o
> > obj-$(CONFIG_PATA_EFAR) += pata_efar.o
> > +obj-$(CONFIG_PATA_EP93XX) += pata_ep93xx.o
> > obj-$(CONFIG_PATA_HPT366) += pata_hpt366.o
> > obj-$(CONFIG_PATA_HPT37X) += pata_hpt37x.o
> > obj-$(CONFIG_PATA_HPT3X2N) += pata_hpt3x2n.o
>
> Overall, this looks pretty good. I'll try to examine it more closely later.
I agree. The driver is ok on the whole, my comments are largely nitpicking.
Arnd
--
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
