Re: [PATCH v2 1/3] PATA host controller driver for ep93xx

[Arnd Bergmann <arnd@arndb.de>]

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