lkml.org 
[lkml]   [2007]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 10/15] ide: add ide_set_dma() helper
    Hello again. :-)

    Bartlomiej Zolnierkiewicz wrote:
    > [PATCH] ide: add ide_set_dma() helper

    > * add ide_set_dma() helper and make ide_hwif_t.ide_dma_check return
    > -1 when DMA needs to be disabled (== need to call ->ide_dma_off_quietly)
    > 0 when DMA needs to be enabled (== need to call ->ide_dma_on)
    > 1 when DMA setting shouldn't be changed
    > * fix IDE code to use ide_set_dma() instead if using ->ide_dma_check directly

    Here are a few comments related to the code being patched:

    > Index: b/drivers/ide/pci/alim15x3.c
    > ===================================================================
    > --- a/drivers/ide/pci/alim15x3.c
    > +++ b/drivers/ide/pci/alim15x3.c
    > @@ -507,17 +507,15 @@ static int config_chipset_for_dma (ide_d
    > *
    > * Configure a drive for DMA operation. If DMA is not possible we
    > * drop the drive into PIO mode instead.
    > - *
    > - * FIXME: exactly what are we trying to return here
    > */
    > -
    > +
    > static int ali15x3_config_drive_for_dma(ide_drive_t *drive)
    > {
    > ide_hwif_t *hwif = HWIF(drive);
    > struct hd_driveid *id = drive->id;
    >
    > if ((m5229_revision<=0x20) && (drive->media!=ide_disk))
    > - return hwif->ide_dma_off_quietly(drive);
    > + goto no_dma_set;

    Isn't it better to just return -1?

    > @@ -552,9 +550,10 @@ try_dma_modes:
    > ata_pio:
    > hwif->tuneproc(drive, 255);
    > no_dma_set:
    > - return hwif->ide_dma_off_quietly(drive);
    > + return -1;
    > }
    > - return hwif->ide_dma_on(drive);
    > +
    > + return 0;
    > }

    Ugh, this code looks like it's asking to be converted into calling
    ide_use_dma(). instead all of that...

    > Index: b/drivers/ide/pci/cs5520.c
    > ===================================================================
    > --- a/drivers/ide/pci/cs5520.c
    > +++ b/drivers/ide/pci/cs5520.c
    > @@ -132,12 +132,11 @@ static void cs5520_tune_drive(ide_drive_
    >
    > static int cs5520_config_drive_xfer_rate(ide_drive_t *drive)
    > {
    > - ide_hwif_t *hwif = HWIF(drive);
    > -
    > /* Tune the drive for PIO modes up to PIO 4 */
    > cs5520_tune_drive(drive, 4);

    Ugh. Why not ask drive? :-/

    > /* Then tell the core to use DMA operations */
    > - return hwif->ide_dma_on(drive);
    > + return 0;

    That must be the famous VDMA thing... :-)
    I wonder if it *actually* works on HPT36x/37x which seem to have support
    for it...

    > Index: b/drivers/ide/pci/jmicron.c
    > ===================================================================
    > --- a/drivers/ide/pci/jmicron.c
    > +++ b/drivers/ide/pci/jmicron.c
    > @@ -164,14 +164,12 @@ static int config_chipset_for_dma (ide_d
    >
    > static int jmicron_config_drive_for_dma (ide_drive_t *drive)
    > {
    > - ide_hwif_t *hwif = drive->hwif;
    > + if (ide_use_dma(drive) && config_chipset_for_dma(drive))
    > + return 0;
    >
    > - if (ide_use_dma(drive)) {
    > - if (config_chipset_for_dma(drive))
    > - return hwif->ide_dma_on(drive);
    > - }
    > config_jmicron_chipset_for_pio(drive, 1);

    The 2nd argument of that funtion is useless -- it basically does nothing
    if 0 is passed. Another case of mindless copy-paste. :-)

    > Index: b/drivers/ide/pci/pdc202xx_old.c
    > ===================================================================
    > --- a/drivers/ide/pci/pdc202xx_old.c
    > +++ b/drivers/ide/pci/pdc202xx_old.c
    > @@ -332,17 +332,15 @@ chipset_is_set:
    >
    > static int pdc202xx_config_drive_xfer_rate (ide_drive_t *drive)
    > {
    > - ide_hwif_t *hwif = HWIF(drive);
    > -
    > drive->init_speed = 0;
    >
    > if (ide_use_dma(drive) && config_chipset_for_dma(drive))
    > - return hwif->ide_dma_on(drive);
    > + return 0;
    >
    > if (ide_use_fast_pio(drive))
    > - hwif->tuneproc(drive, 5);
    > + config_chipset_for_pio(drive, 5);

    That part is obsoleted by my recent fix...

    > Index: b/drivers/ide/pci/piix.c
    > ===================================================================
    > --- a/drivers/ide/pci/piix.c
    > +++ b/drivers/ide/pci/piix.c
    > @@ -386,20 +386,18 @@ static int piix_config_drive_for_dma (id
    >
    > static int piix_config_drive_xfer_rate (ide_drive_t *drive)
    > {
    > - ide_hwif_t *hwif = HWIF(drive);
    > -
    > drive->init_speed = 0;
    >
    > if (ide_use_dma(drive) && piix_config_drive_for_dma(drive))
    > - return hwif->ide_dma_on(drive);
    > + return 0;
    >
    > if (ide_use_fast_pio(drive)) {
    > /* Find best PIO mode. */
    > - (void) hwif->speedproc(drive, XFER_PIO_0 +
    > - ide_get_best_pio_mode(drive, 255, 4, NULL));
    > + u8 pio = ide_get_best_pio_mode(drive, 255, 4, NULL);
    > + (void)piix_tune_chipset(drive, XFER_PIO_0 + pio);
    > }

    Will try to fix the tuneproc() nuisance RSN. :-)

    > Index: b/drivers/ide/pci/serverworks.c
    > ===================================================================
    > --- a/drivers/ide/pci/serverworks.c
    > +++ b/drivers/ide/pci/serverworks.c
    > @@ -315,17 +315,15 @@ static int config_chipset_for_dma (ide_d
    >
    > static int svwks_config_drive_xfer_rate (ide_drive_t *drive)
    > {
    > - ide_hwif_t *hwif = HWIF(drive);
    > -
    > drive->init_speed = 0;
    >
    > if (ide_use_dma(drive) && config_chipset_for_dma(drive))
    > - return hwif->ide_dma_on(drive);
    > + return 0;
    >
    > if (ide_use_fast_pio(drive))
    > config_chipset_for_pio(drive);

    I have no idea why that function is so huge in this driver, i.e. why all
    this code is not in svwks_tune_chipset()...

    > - return hwif->ide_dma_off_quietly(drive);
    > + return -1;
    > }
    >
    > static unsigned int __devinit init_chipset_svwks (struct pci_dev *dev, const char *name)
    > Index: b/drivers/ide/pci/sl82c105.c
    > ===================================================================
    > --- a/drivers/ide/pci/sl82c105.c
    > +++ b/drivers/ide/pci/sl82c105.c
    > @@ -161,14 +161,14 @@ static int sl82c105_check_drive (ide_dri
    > if (id->field_valid & 2) {
    > if ((id->dma_mword & hwif->mwdma_mask) ||
    > (id->dma_1word & hwif->swdma_mask))

    Ugh. This driver claims the full MW/SW DMA support while actually only
    supports MWDMA2.

    > - return hwif->ide_dma_on(drive);
    > + return 0;
    > }
    >
    > if (__ide_dma_good_drive(drive))
    > - return hwif->ide_dma_on(drive);
    > + return 0;
    > } while (0);

    This also asks to be converted into ide_use_dma() call. The patch is
    cooking...

    > - return hwif->ide_dma_off_quietly(drive);
    > + return -1;
    > }
    >
    > /*
    > Index: b/drivers/ide/pci/slc90e66.c
    > ===================================================================
    > --- a/drivers/ide/pci/slc90e66.c
    > +++ b/drivers/ide/pci/slc90e66.c
    > @@ -179,19 +179,17 @@ static int slc90e66_config_drive_for_dma
    >
    > static int slc90e66_config_drive_xfer_rate (ide_drive_t *drive)
    > {
    > - ide_hwif_t *hwif = HWIF(drive);
    > -
    > drive->init_speed = 0;
    >
    > if (ide_use_dma(drive) && slc90e66_config_drive_for_dma(drive))
    > - return hwif->ide_dma_on(drive);
    > + return 0;
    >
    > if (ide_use_fast_pio(drive)) {
    > - (void) hwif->speedproc(drive, XFER_PIO_0 +
    > - ide_get_best_pio_mode(drive, 255, 4, NULL));
    > + u8 pio = ide_get_best_pio_mode(drive, 255, 4, NULL);
    > + (void)slc90e66_tune_chipset(drive, XFER_PIO_0 + pio);
    > }

    The same promise about tuneproc() in this driver... :-)

    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/

    \
     
     \ /
      Last update: 2007-01-20 22:29    [W:0.035 / U:29.776 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site