Messages in this thread | | | Date | Sun, 17 Jun 2007 21:05:01 +0400 | From | Sergei Shtylyov <> | Subject | Re: [PATCH 1/5] ide: add short cables support |
| |
Hello.
Bartlomiej Zolnierkiewicz wrote:
>>>This patch allows users to override both host and device side cable detection >>>with "ideX=ata66" kernel parameter. Thanks to this it should be now possible >>>to use UDMA > 2 modes on systems (laptops mainly) which use short 40-pin cable >>>instead of 80-pin one.
>>>Next patches add automatic detection of some systems using short cables.
>>>Changes:
>>>* Rename hwif->udma_four to hwif->cbl and make it u8.
>> Not sure if already short word "cable" was worth further shortening. :-)
> The name/usage is the same as in libata.
> When we add ->cable_detect method it will make grepping easier. 8)
Hm... :-)
>>>Index: b/drivers/ide/pci/alim15x3.c >>>=================================================================== >>>--- a/drivers/ide/pci/alim15x3.c >>>+++ b/drivers/ide/pci/alim15x3.c >>>@@ -594,7 +594,7 @@ out: >>> * FIXME: frobs bits that are not defined on newer ALi devicea >>> */ >>> >>>-static unsigned int __devinit ata66_ali15x3 (ide_hwif_t *hwif) >>>+static u8 __devinit ata66_ali15x3(ide_hwif_t *hwif) >>> { >>> struct pci_dev *dev = hwif->pci_dev; >>> unsigned int ata66 = 0; >>>@@ -657,7 +657,7 @@ static unsigned int __devinit ata66_ali1 >>> >>> local_irq_restore(flags); >>> >>>- return(ata66); >>>+ return ata66 ? ATA_CBL_PATA80 : ATA_CBL_PATA40;
>> Ahem... I'd think it was about the right time to fix the abomination which >>those ata66 and cable_80_pin[2] are, something like this:
>>static unsigned int __devinit ata66_ali15x3 (ide_hwif_t *hwif) >>{ >> struct pci_dev *dev = hwif->pci_dev; >> unsigned int cbl = ATA_CBL_PATA40; >> unsigned long flags; >> u8 tmpbyte, mask = hwif->channel ? 0x02 : 0x01; >> >> local_irq_save(flags); /* Not sure if it's necessary... */ >> >> if (m5229_revision >= 0xC2) { >> /* >> * Ultra66 cable detection (from Host View) >> * m5229, 0x4a, bit0: primary, bit1: secondary >> * 0: 80 pin, 1: 40 pin >> */ >> pci_read_config_byte(dev, 0x4a, &tmpbyte); >> /* >> * Allow ata66 if cable of current channel has 80 pins >> */ >> cbl = (tmpbyte & mask) ? ATA_CBL_PATA40 : ATA_CBL_PATA80; >> } else {
>>[Following code frankly speaking has no business being in this function]
> patch #3/5 already takes care of cable_80_pin[2] part,
I really should have looked thru all the series but lacked the time. :-<
> ata66 part was left as an exercise for the reader ;-)
Hm, now that I've looked at that patch I saw both these things killed. What, you don't know your own code? ;-)
>> local_irq_restore(flags); /* Not sure if it's necessary... */
>> return cbl;
>>> }
>>> /** >>>Index: b/drivers/ide/pci/serverworks.c >>>=================================================================== >>>--- a/drivers/ide/pci/serverworks.c >>>+++ b/drivers/ide/pci/serverworks.c >>>@@ -329,9 +329,9 @@ static unsigned int __devinit init_chips >>> return dev->irq; >>> } >>> >>>-static unsigned int __devinit ata66_svwks_svwks (ide_hwif_t *hwif) >>>+static u8 __devinit ata66_svwks_svwks(ide_hwif_t *hwif) >>> { >>>- return 1; >>>+ return ATA_CBL_PATA80; >>> }
>> Hm, worth folding into ata66_svwks()...
> Yes and no...
> Alan did a very nice rewrite of cable detection code for pata_serverworks.c > driver. We may be better off just back-porting it (patches are welcomed).
Thanks, no. I still have much to do. :-)
>>>Index: b/drivers/ide/pci/sis5513.c >>>=================================================================== >>>--- a/drivers/ide/pci/sis5513.c >>>+++ b/drivers/ide/pci/sis5513.c >>>@@ -796,7 +796,7 @@ static unsigned int __devinit init_chips >>> return 0; >>> } >>> >>>-static unsigned int __devinit ata66_sis5513 (ide_hwif_t *hwif) >>>+static u8 __devinit ata66_sis5513(ide_hwif_t *hwif) >>> { >>> u8 ata66 = 0; >>> >>>@@ -811,7 +811,8 @@ static unsigned int __devinit ata66_sis5 >>> pci_read_config_byte(hwif->pci_dev, 0x48, ®48h); >>> ata66 = (reg48h & mask) ? 0 : 1; >>> } >>>- return ata66; >>>+ >>>+ return ata66 ? ATA_CBL_PATA80 : ATA_CBL_PATA40; >>> }
>> That doesn't look good as well. I think we should part with 'ata66' >>variable here completely...
> Could be fixed in the follow-up patch as we should also split ata66_sis5513() > into two separate functions (one for chipset_family >= ATA_133 and one for > chipset_family >= ATA_66).
That's only worth doing if you actually intend to introduce cable_detect() method...
> Thanks, > Bart
MBR, Sergei
- 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/
| |