Re: [PATCH] 2.5.13 IDE 52

[Jens Axboe]

On Sun, May 05 2002, Martin Dalecki wrote:
> - Split up the TCQ UDMA handling stuff in to proper functions. Jens must has
>   been dreaming as he introduced them ;-).

This was pending stuff, and why I wrote that the start command tcq = 1
stuff was a gross hack.

> -		int tcq = 0;
>  
>  		if (!drive->using_dma)
>  			return ide_started;
>  
>  		/* for dma commands we don't set the handler */
> -		if (args->taskfile.command == WIN_WRITEDMA || args->taskfile.command == WIN_WRITEDMA_EXT)
> +		if (args->taskfile.command == WIN_WRITEDMA
> +		 || args->taskfile.command == WIN_WRITEDMA_EXT)
>  			dma_act = ide_dma_write;
> -		else if (args->taskfile.command == WIN_READDMA || args->taskfile.command == WIN_READDMA_EXT)
> +		else if (args->taskfile.command == WIN_READDMA
> +		      || args->taskfile.command == WIN_READDMA_EXT)
>  			dma_act = ide_dma_read;
> -		else if (args->taskfile.command == WIN_WRITEDMA_QUEUED || args->taskfile.command == WIN_WRITEDMA_QUEUED_EXT) {
> -			tcq = 1;
> -			dma_act = ide_dma_write_queued;
> -		} else if (args->taskfile.command == WIN_READDMA_QUEUED || args->taskfile.command == WIN_READDMA_QUEUED_EXT) {
> -			tcq = 1;
> -			dma_act = ide_dma_read_queued;
> -		} else {
> +#ifdef CONFIG_BLK_DEV_IDE_TCQ
> +		else if (args->taskfile.command == WIN_WRITEDMA_QUEUED
> +		      || args->taskfile.command == WIN_WRITEDMA_QUEUED_EXT
> +		      || args->taskfile.command == WIN_READDMA_QUEUED
> +		      || args->taskfile.command == WIN_READDMA_QUEUED_EXT)
> +			return udma_tcq_taskfile(drive, rq);
> +#endif
> +		else {
>  			printk("ata_taskfile: unknown command %x\n", args->taskfile.command);
>  			return ide_stopped;
>  		}
>  
> -		/*
> -		 * FIXME: this is a gross hack, need to unify tcq dma proc and
> -		 * regular dma proc -- basically split stuff that needs to act
> -		 * on a request from things like ide_dma_check etc.
> -		 */
> -		if (tcq)
> -			return drive->channel->udma(dma_act, drive, rq);
> -		else {
> -			if (drive->channel->udma(dma_act, drive, rq))
> -				return ide_stopped;
> -		}
> +
> +		if (drive->channel->udma(dma_act, drive, rq))
> +			return ide_stopped;

This is still ugly, IMHO. What I wanted was to split the udma->
function into two parts, one that acts on a request (ide_dma_read,
ide_dma_being, etc -- and then also ide_dma_read_queued) ad one that
does the silly stuff like ide_dma_check etc. Then unify the tcq and
non-tcq stuff so that udma_rw->(dma_act, drive, rq) always returns
ide_started or ide_stopped (or ide_released) and kill the #ifdef above.

I don't think udma_tcq_taskfile() should be public like this, and btw I
think the name really SUCKS! :-). ata_tcq_dma() is much better for
instance, even though it should be a private stratey. BTW, this goes for
all your tcq.c renaming.

-- 
Jens Axboe