Re: [PATCH 4/5] sil24: add ATAPI support

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> +    case ATA_PROT_ATAPI:
>> +    case ATA_PROT_ATAPI_DMA:
>> +    case ATA_PROT_ATAPI_NODATA:
>> +        prb = &cb->atapi.prb;
>> +        sge = cb->atapi.sge;
>> +        memset(cb->atapi.cdb, 0, 32);
>> +        memcpy(cb->atapi.cdb, qc->cdb, ap->cdb_len);
>> +
>> +        if (unlikely((ap->cdb_len == 16) ^ pp->cdb16)) {
>> +            pp->cdb16 = ap->cdb_len == 16;
>> +            if (pp->cdb16)
>> +                writel(PORT_CS_CDB16, port + PORT_CTRL_STAT);
>> +            else
>> +                writel(PORT_CS_CDB16, port + PORT_CTRL_CLR);
>> +        }
> 
> 
> NAK:
> * you should set PORT_CS_CDB16 from ->dev_config(), not needlessly test 
> inside the fast path
> * thus, no need to cache cdb16 flag.  just tell the hardware once, and 
> then everybody agrees on the correct value.
> 

Oh.. I didn't know about ->dev_config().  Thanks for pointing out.

> 
>> @@ -772,6 +802,9 @@ static int sil24_port_start(struct ata_p
>>  
>>      ap->private_data = pp;
>>  
>> +    /* default to 12byte CDB */
>> +    writel(PORT_CS_CDB16, port + PORT_CTRL_CLR);
>> +
>>      return 0;
> 
> 
> If you set it in ->dev_config(), this should go away.

Sure.

-- 
tejun