Hi,
On Sunday 22 July 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
>
> > Use ->set_pio_mode method to program PIO modes in ide_set_xfer_rate()
> > (the only place which used ->speedproc to program PIO modes) and remove
> > handling of PIO modes from all ->speedproc implementations.
>
> > There should be no functionality changes caused by this patch.
>
> Does a missing printk(KERN_ERR, ...) in the cs5520.c count as a
> functionality change? ;-)
Psst. ;-)
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
> > ---
> > drivers/ide/cris/ide-cris.c | 5 -----
> > drivers/ide/pci/alim15x3.c | 5 -----
> > drivers/ide/pci/atiixp.c | 5 -----
> > drivers/ide/pci/cmd64x.c | 8 --------
> > drivers/ide/pci/cs5530.c | 7 -------
> > drivers/ide/pci/it8213.c | 5 -----
> > drivers/ide/pci/it821x.c | 9 ---------
> > drivers/ide/pci/piix.c | 5 -----
> > drivers/ide/pci/sc1200.c | 10 ----------
> > drivers/ide/pci/scc_pata.c | 7 -------
> > drivers/ide/pci/serverworks.c | 5 -----
> > drivers/ide/pci/siimage.c | 7 -------
> > drivers/ide/pci/sis5513.c | 12 ++----------
> > drivers/ide/pci/sl82c105.c | 8 --------
> > drivers/ide/pci/slc90e66.c | 5 -----
>
> These don't need their *_tune_pio() as a separate item anymore, however it
> should be OK as gcc will inline them...
Removal of redundant *_tune_pio() is WIP.
> > Index: b/drivers/ide/ide-lib.c
> > ===================================================================
> > --- a/drivers/ide/ide-lib.c
> > +++ b/drivers/ide/ide-lib.c
> > @@ -398,6 +398,18 @@ int ide_set_xfer_rate(ide_drive_t *drive
> >
> > rate = ide_rate_filter(drive, rate);
> >
> > + if (rate >= XFER_PIO_0 && rate <= XFER_PIO_5) {
> > + if (hwif->set_pio_mode)
> > + hwif->set_pio_mode(drive, rate - XFER_PIO_0);
> > +
> > + /*
> > + * FIXME: this incorrect to return zero here but
>
> Missing "is" before "this"...
Fixed, thanks.
> > + * since all users of ide_set_xfer_rate() ignore
> > + * the return value it is not a problem currently
> > + */
> > + return 0;
> > + }
> > +
> > return hwif->speedproc(drive, rate);
> > }
>
> Erm, what if there's no DMA mode support an therefore no speedproc()
> method, only set_pio_mode()? I guess that moving the (speedproc() == NULL)
> check in a prior patch was somewhat rash...
Quite the contrary, it was intended... :)
Few old host drivers have only ->set_pio_mode and they don't set ->autotune
so not checking for ->speedproc would be a major change in behavior for them
while this patch was meant to be as non-intrusive and simple as possible.
> > Index: b/drivers/ide/pci/cs5520.c
> > ===================================================================
> > --- a/drivers/ide/pci/cs5520.c
> > +++ b/drivers/ide/pci/cs5520.c
> > @@ -66,30 +66,13 @@ static struct pio_clocks cs5520_pio_cloc
> > {1, 2, 1}
> > };
> >
> > -static int cs5520_tune_chipset(ide_drive_t *drive, const u8 speed)
> > +static void cs5520_set_pio_mode(ide_drive_t *drive, const u8 pio)
> > {
> > ide_hwif_t *hwif = HWIF(drive);
> > struct pci_dev *pdev = hwif->pci_dev;
> > - int pio = speed;
> > - u8 reg;
> > int controller = drive->dn > 1 ? 1 : 0;
>
> Wouldn't hwif->channel be enough?
Agreed.
> > + u8 reg;
> >
> > - switch(speed)
> > - {
> > - case XFER_PIO_4:
> > - case XFER_PIO_3:
> > - case XFER_PIO_2:
> > - case XFER_PIO_1:
> > - case XFER_PIO_0:
> > - pio -= XFER_PIO_0;
> > - break;
> > - default:
> > - pio = 0;
> > - printk(KERN_ERR "cs55x0: bad ide timing.\n");
> > - }
> > -
> > - printk("PIO clocking = %d\n", pio);
> > -
> > /* FIXME: if DMA = 1 do we need to set the DMA bit here ? */
>
> Indeed? Guess not. :-)
>
> > /* 8bit CAT/CRT - 8bit command timing for channel */
> > @@ -114,12 +97,21 @@ static int cs5520_tune_chipset(ide_drive
> > reg |= 1<<((drive->dn&1)+5);
> > outb(reg, hwif->dma_base + 0x02 + 8*controller);
>
> That should go...
-ENOPATCH :)
> > - return ide_config_drive_speed(drive, speed);
> > + (void)ide_config_drive_speed(drive, XFER_PIO_0 + pio);
> > }
> >
> > -static void cs5520_set_pio_mode(ide_drive_t *drive, const u8 pio)
> > +static int cs5520_tune_chipset(ide_drive_t *drive, const u8 speed)
> > {
> > - cs5520_tune_chipset(drive, XFER_PIO_0 + pio);
> > + printk(KERN_ERR "cs55x0: bad ide timing.\n");
> > +
> > + cs5520_set_pio_mode(drive, 0);
>
> Huh?
Yes, this needs fixing but not in this patch.
"There should be no functionality changes caused by this patch."
> > +
> > + /*
> > + * FIXME: this incorrect to return zero here but
>
> Again, no "is" before "this"...
Fixed.
> > + * since all users of ide_set_xfer_rate() ignore
> > + * the return value it is not a problem currently
> > + */
> > + return 0;
> > }
>
> The question is why we have to leave the speedproc() handler to be in this
> driver that doesn't support DMA modes per se?
"There should be no functionality changes caused by this patch."
However prior or incremental patches are of course welcomed.
Thanks,
Bart
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
