Re: [PATCH] Re: [PATCH][IDE] update qd65xx driver

[Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl>]

On Thursday 11 of September 2003 23:51, Andries.Brouwer@cwi.nl wrote:
> That reminds me, did I ever send you this?
>
> Andries

No, only similar patch for hpt366.c.

I think the (almost) correct scheme is following (some drivers get it wrong):

config_chipset_pio() style functions are used for driver auto-tuning
and  "255, pio" is used to find best PIO mode supported by device.
"pio" argument should be set to highest mode allowed by controller.

tune_proc()/tune_drive() style functions are used for direct mode setting
and "pio, max_pio" is used to set desired mode.
"pio" is desired mode, "max_pio" is max mode supported by controller.

It's almost correct because we should also check if desired PIO mode
is supported by device, currently we let user shoot herself/himself.

IDE driver itself calls ->tuneproc() with argument == 255, so always
highest supported PIO mode is chosen.  User-space can call ->tuneproc()
with smaller argument.  With you patch smaller than highest possible
PIO mode can't be chosen.

Your patch for hpt366.c has also this side effect that you can now only
set highest possible PIO mode.  You should revert this change
and fix hpt3xx_tune_drive() call inside hpt366_config_drive_xfer_rate()
to pass 255 instead of 5.

--bartlomiej

> diff -u --recursive --new-file -X /linux/dontdiff
> a/drivers/ide/legacy/qd65xx.c b/drivers/ide/legacy/qd65xx.c ---
> a/drivers/ide/legacy/qd65xx.c	Mon Sep  8 23:44:59 2003
> +++ b/drivers/ide/legacy/qd65xx.c	Thu Sep 11 23:20:26 2003
> @@ -261,7 +261,7 @@
>  	int recovery_time = 415; /* worst case values from the dos driver */
>
>  	if (drive->id && !qd_find_disk_type(drive, &active_time, &recovery_time))
> { -		pio = ide_get_best_pio_mode(drive, pio, 255, &d);
> +		pio = ide_get_best_pio_mode(drive, 255, pio, &d);
>  		pio = IDE_MIN(pio,4);
>
>  		switch (pio) {