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

[Jeff Garzik <jgarzik@pobox.com>]

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.


> @@ -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.

	Jeff