Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards

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

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>The Marvell bridge chips used on HighPoint SATA cards do not seem to support
>>the UltraDMA modes 1, 2, and 3 (as well as any MWDMA modes), so the driver
>>needs to account for this in the udma_filter() method.  In order to achieve
>>that, do the following changes:

>>- install the method for all chips, not only HPT36x/370 (improve code formatting
>>  by killing an extra tabs while at it);

>>- add to the end of the 'switch' statement in hpt3xx_udma_filter() case for
>>  HPT372[AN] and HPT374 chips upon which the SATA cards are based and check
>>  there whether we're dealing with SATA drive (by looking at words 80 and 93
>>  of the drive's identify data), reorder HPT370[A] cases for consistency...

>>Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

[...]

> Also now that ->udma_filter is always present the initial hwif->ultra_mask

    Aha, so this method's semantics intended to *completely override* the 
ultra_mask field?!  Wouldn't it be better to make the code behave more 
consistent, i.e. in ide_get_mode_mask() do:

         unsigned int mask = 0;

         switch(base) {
         case XFER_UDMA_0:
                 if ((id->field_valid & 4) == 0)
                         break;

                 if (hwif->udma_filter)
                         mask = hwif->udma_filter(drive);
		else
                         mask = hwif->ultra_mask;

                 mask &= id->dma_ultra;
                 if ((mask & 0x78) && (eighty_ninty_three(drive) == 0))
                         mask &= 0x07;
                 break;
         case XFER_MW_DMA_0:
                 if ((id->field_valid & 2) == 0)
                         break;

                 if (hwif->mdma_filter)
                         mask = hwif->mdma_filter(drive);
		else
                         mask = hwif->mwdma_mask;
                 mask &= id->dma_mword;
                 break;

to avoid the further confusion? ;-)

> Thanks,
> Bart

WBR, Sergei