Re: [PATCH 10/13] sl82c105: add ->speedproc support

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello, I wrote:

>>> Bartlomiej Zolnierkiewicz wrote:

>>>> [PATCH] sl82c105: add ->speedproc support

>>>> * add sl82c105_tunepio() wrapper for sl82c105_tune_drive()
>>>>  (just to get the error value)

>>>> * add sl82c105_tune_chipset() (->speedproc method) for setting
>>>>  transfer mode

>>>    Thanks for the patch!

>>>  > Index: b/drivers/ide/pci/sl82c105.c

>>>> ===================================================================
>>>> --- a/drivers/ide/pci/sl82c105.c
>>>> +++ b/drivers/ide/pci/sl82c105.c

> [...]

>>>> @@ -114,17 +114,45 @@ static void sl82c105_tune_drive(ide_driv
>>>>      */
>>>>     drive->io_32bit = 1;
>>>>     drive->unmask    = 1;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
>>>> +{
>>>> +    /*
>>>> +     * TODO: find best PIO mode and set device speed here
>>>> +     *       (requires adding helper function for getting PIO cycle 
>>>> time)
>>>> +     */

>>>   I thought we were doing it by calling ide_get_best_pio_mode() above...

>> We are also using ide_get_best_pio_mode() to get PIO cycle time
>> so we can't move it here ATM.

>   I've found/used quite convenient workaround for that -- return PIO 
> mode actually selected from xxx_tune_pio(), then call 
> ide_config_drive_speed() from the real tuneproc() method.

    Works like charm with ignoring the result, since ide_get_best_pio_mode() 
returns the same explicit mode as was passed to it -- so is good for the 
speedproc() methods also...

>>>> +    (void)sl82c105_tunepio(drive, pio);

>>>    Erm, I thought afterwards that I vainly folded one into another. I 
>>> think it's worth moving those io_32bit and unmask flag assignments 
>>> above back there... May also recast my patch. :-)

>> Moving them to ->init_hwif where they belong would be even better... ;-)

>    Well, I wasn't sure where they belong... :-)
>    So, OK to recast that patch?

   Recasted it and reworked your patch atop of it (adding MWDMA 0/1 support as 
a bonus! :-) -- now need to conduct some testing on a remote target...

>>>> +static int sl82c105_tune_chipset(ide_drive_t *drive, u8 mode)
>>>> +{
>>>> +    mode = ide_rate_filter(drive, mode);
>>>> +
>>>> +    if (mode >= XFER_PIO_0 && mode <= XFER_PIO_5)
>>>> +        return sl82c105_tunepio(drive, mode - XFER_PIO_0);
>>>> +
>>>> +    /*
>>>> +     * TODO: add MWDMA0/1 support
>>>> +     */
>>>> +    BUG_ON(mode != XFER_MW_DMA_2);

   I wonder is there are some W83C554 users anywhere -- that chipset also 
supports UltraDMA...

MBR, Sergei