lkml.org 
[lkml]   [2007]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 13/15] ide: fix UDMA/MWDMA/SWDMA masks

Hi,

Sergei Shtylyov wrote:
>
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
>> [PATCH] ide: fix UDMA/MWDMA/SWDMA masks
>
>> * use 0x00 instead of 0x80 to disable ->{ultra,mwdma,swdma}_mask
>> * add udma_mask field to ide_pci_device_t and use it to initialize
>> ->ultra_mask in aec62xx, pdc202xx_new and pdc202xx_old drivers
>> * fix UDMA masks to match with chipset specific *_ratemask()
>> (alim15x3, hpt366, serverworks and siimage drivers need UDMA mask
>> filtering method - done in the next patch)
>
> More nit picking (-:
>
>> Index: b/drivers/ide/pci/cmd64x.c
>> ===================================================================
>> --- a/drivers/ide/pci/cmd64x.c
>> +++ b/drivers/ide/pci/cmd64x.c
>> @@ -695,9 +695,10 @@ static void __devinit init_hwif_cmd64x(i
>> hwif->swdma_mask = 0x07;
>>
>> if (dev->device == PCI_DEVICE_ID_CMD_643)
>> - hwif->ultra_mask = 0x80;
>> + hwif->ultra_mask = 0x00;
>> if (dev->device == PCI_DEVICE_ID_CMD_646)
>> - hwif->ultra_mask = (class_rev > 0x04) ? 0x07 : 0x80;
>> + hwif->ultra_mask =
>> + (class_rev == 0x05 || class_rev == 0x07) ? 0x07 : 0x00;
>> if (dev->device == PCI_DEVICE_ID_CMD_648)
>> hwif->ultra_mask = 0x1f;
>
> Hm, well, this doesn't look consistent with the changes in other drivers.
> This driver asks for explicit hwif->cds->ultra_mask initializers, IMO...
> You'd only have to check for PCI-646 revisions < 5 then...

reworked

>> Index: b/drivers/ide/pci/piix.c
>> ===================================================================
>> --- a/drivers/ide/pci/piix.c
>> +++ b/drivers/ide/pci/piix.c
>> @@ -493,7 +493,7 @@ static void __devinit init_hwif_piix(ide
>> case PCI_DEVICE_ID_INTEL_82371FB_0:
>> case PCI_DEVICE_ID_INTEL_82371FB_1:
>> case PCI_DEVICE_ID_INTEL_82371SB_1:
>> - hwif->ultra_mask = 0x80;
>> + hwif->ultra_mask = 0x00;
>> break;
>> case PCI_DEVICE_ID_INTEL_82371AB:
>> case PCI_DEVICE_ID_INTEL_82443MX_1:
>> @@ -501,6 +501,10 @@ static void __devinit init_hwif_piix(ide
>> case PCI_DEVICE_ID_INTEL_82801AB_1:
>> hwif->ultra_mask = 0x07;
>> break;
>> + case PCI_DEVICE_ID_INTEL_82801AA_1:
>> + case PCI_DEVICE_ID_INTEL_82372FB_1:
>> + hwif->ultra_mask = 0x1f;
>> + break;
>
> Alas, I'm afraid this part is wrong!
> At least, the cable detection should work for 82801AA the same way as for
> the 82801Bx and newer chips, if Intel's datasheet is to be trusted... I think
> we should fall thru here.

yes (extra "break:" shouldn't be there), fixed

>> default:
>> if (!hwif->udma_four)
>> hwif->udma_four = piix_cable_detect(hwif);
>
> This one also certainly asks for explicit hwif->cds->ultra_mask
> initializers... Thus almost all of this switch statement could go away...

Alas doing it now would make the nice DECLARE_PIIX_DEV() macro go away
(=> a lot of duplicated code)... could be done in the future...

Thanks,
Bart

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-02-03 01:37    [W:0.110 / U:0.704 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site