Messages in this thread Patch in this message | | | Date | Sat, 16 Jun 2007 00:54:12 +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. :-)
> 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]
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()...
> /* On Dell PowerEdge servers with a CSB5/CSB6, the top two bits > @@ -341,7 +341,7 @@ static unsigned int __devinit ata66_svwk > * Bit 14 clear = primary IDE channel does not have 80-pin cable. > * Bit 14 set = primary IDE channel has 80-pin cable. > */ > -static unsigned int __devinit ata66_svwks_dell (ide_hwif_t *hwif) > +static u8 __devinit ata66_svwks_dell(ide_hwif_t *hwif) > { > struct pci_dev *dev = hwif->pci_dev; > if (dev->subsystem_vendor == PCI_VENDOR_ID_DELL &&
Isn't this check duplicating the check in ata66_svwks()?
> @@ -359,18 +359,18 @@ static unsigned int __devinit ata66_svwk > * > * WARNING: this only works on Alpine hardware! > */ > -static unsigned int __devinit ata66_svwks_cobalt (ide_hwif_t *hwif) > +static u8 __devinit ata66_svwks_cobalt(ide_hwif_t *hwif) > { > struct pci_dev *dev = hwif->pci_dev; > if (dev->subsystem_vendor == PCI_VENDOR_ID_SUN &&
Same question here...
> 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...
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/
| |