Re: [PATCH 13/14] libata-hp-prep: add prereset() method and implement ata_std_prereset()

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
>> +static void ata_wait_spinup(struct ata_port *ap)
>> +{
>> +    struct ata_eh_context *ehc = &ap->eh_context;
>> +    unsigned long end, secs;
>> +    int rc;
>> +
>> +    /* first, debounce */
>> +    rc = sata_phy_debounce(ap, sata_deb_timing_eh);
> 
> unconditionally calls a sata_xxx function, and so "sata_wait_spinup" 
> naming is preferred.

Waiting for spinup is common operation for both PATA and SATA although I 
don't know how many PATA controllers support hotplug.  The following 
code pattern is used a lot to avoid explicit support testing in the 
caller.  ie. The function which uses optional controller feature is 
responsible for testing whether it is implemented by the controller.

rc = sata_xxx(args);
if (rc && rc != -EOPNOTSUPP) {
	/* handle error */
}

Also, if we rename this function to sata_wait_spinup() because it calls 
sata_*() unconditionally then ata_std_prereset() should be renamed to 
sata_std_prereset() as it calls sata_wait_spinup() unconditionally.

>> +    /* if debounced successfully and offline, no need to wait */
>> +    if (rc == 0 && ata_port_offline(ap))
>> +        return;
> 
> ISTR ata_port_offline being SATA-specific too, but anyway...  :)

ata_port_offline() applies to both PATA and SATA.  It's just that the 
current implmentation always returns 0 for PATA.  Well, I don't know. 
Maybe we should rename all those to sata_*() and rename them again when 
actual PATA support is implemented.

>> +    /* okay, let's give the drive time to spin up */
>> +    end = ehc->i.hotplug_timestamp + ATA_SPINUP_WAIT * HZ / 1000;
>> +    secs = ((end - jiffies) + HZ - 1) / HZ;
>> +
>> +    if (time_after(jiffies, end))
>> +        return;
>> +
>> +    if (secs > 5)
>> +        ata_port_printk(ap, KERN_INFO, "waiting for device to spin up "
>> +                "(%lu secs)\n", secs);
>> +
>> +    schedule_timeout_uninterruptible(end - jiffies);
> 
> BTW:  Medium-term, re-read Hale Landis's ATADRVR probe code 
> (http://www.ata-atapi.com/) to make sure the PATA probe path is still 
> intact.

Will do.

-- 
tejun