Re: [PATCH v3] Resurrect IT8172 IDE controller driver

["Shane McDonald" <mcdonald.shane@gmail.com>]

I have made the changes you have suggested.  See resolution below.
A v4 patch follows shortly.

On Sun, Dec 28, 2008 at 4:44 PM, Sergei Shtylyov
<sshtylyov@ru.mvista.com> wrote:
> Shane McDonald wrote:
>
>  Well, you're close. :-)

Just 3 more iterations... :-)

>> diff -uprN a/drivers/ide/it8172.c b/drivers/ide/it8172.c
>> --- a/drivers/ide/it8172.c      1969-12-31 18:00:00.000000000 -0600
>> +++ b/drivers/ide/it8172.c      2008-12-22 01:23:15.000000000 -0600
>> @@ -0,0 +1,188 @@
>> +                                    /* RTx  PWx */
>> +       static const u8 timings[][2] = {
>> +                                       { 7, 7 },
>> +                                       { 3, 4 },
>
>  This is not enough for PIO mode 1, as this only gives 150 ns active + 120
> ns recovery = 270 ns where minimum is 383 ns

OK, I think I understand the required timings now.  I will update PIO mode 1
to set RTx to 7 and PWx to 4, giving 150 ns active + 240 ns recovery
= 390 ns (> minimum 383 ns).

>> +       if (drive->dn) {
>
>  Would have been good to collapse both branches...

Yes, the code is quite repetitive.  I've collapsed it into a single
chunk of code.

>> +               if (pio > 2 || ata_id_has_iordy(drive->id))
>
>  You can actually skip "pio > 2" test.

Agreed.

>> +       if (speed >= XFER_UDMA_0) {
>> +               u8 udma = speed - XFER_UDMA_0;
>> +               u_speed = min_t(u8, 2 - (udma & 1), udma) << (drive->dn *
>> 4);
>
>  There's no need to use such complex math when the chip only supports
> UltraDMA modes 0 thru 2. Be simple:
>
>                u_speed = udma << (drive->dn * 4);

Suggested change has been implemented.

>> +               if (speed >= XFER_MW_DMA_0)
>
>  We dropped support for SWDMA, so the check  should not be needed...

Yes.  Check will be removed.

>> +                       pio = mwdma_to_pio[speed - XFER_MW_DMA_0];
>> +               else
>> +                       pio = 2;
>>
>
>  ... and *else* branch is pointless.

and is now removed.

>> +               it8172_set_pio_mode(drive, pio);
>
>  Well, not the best place in the PIIX driver -- DMA modes shouldn't affect
> prefetch and IORDY settings (regardless of what the Intel's programming
> manuals say :-).
> Àlthough, it's probably OK to continue abusing the set_pio_mode() method.

I will continue to abuse set_pio_mode().

>
>> +static const struct ide_port_info it8172_port_info __devinitdata = {
>> +       .name           = DRV_NAME,
>> +       .port_ops       = &it8172_port_ops,
>> +       .enablebits     = {{0x41, 0x80, 0x80}, {0x00, 0x00, 0x00} },
>>
>
>  Inconsistent spacing, add space after first {...

I will correct the spacing.

> MBR, Sergei

Shane