On Tuesday 03 July 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
>
> > * Add ->host_flags to ide_hwif_t to store ide_pci_device_t.host_flags,
> > assign it in setup-pci.c:ide_pci_setup_ports().
>
> > * Add IDE_HFLAG_PIO_NO_{BLACKLIST,DOWNGRADE} to ide_pci_device_t.host_flags
> > and teach ide_get_best_pio_mode() about them. Also remove needless
> > !drive->id check while at it (drive->id is always present).
>
> > * Convert amd74xx, via82cxxx and ide-timing.h to use ide_get_best_pio_mode()
> > and then remove no longer needed ide_find_best_pio_mode().
>
> > There should be no functionality changes caused by this patch.
>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
>
> Acked-by: Shtylyov <sshtylyov@xxxxxxxxxxxxx>
added
> Still I have something to ask...
>
> > Index: b/drivers/ide/ide-lib.c
> > ===================================================================
> > --- a/drivers/ide/ide-lib.c
> > +++ b/drivers/ide/ide-lib.c
> > @@ -291,11 +291,11 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
> > struct hd_driveid* id = drive->id;
> > int overridden = 0;
> >
> > - if (mode_wanted != 255) {
> > - pio_mode = mode_wanted;
> > - } else if (!drive->id) {
> > - pio_mode = 0;
> > - } else if ((pio_mode = ide_scan_pio_blacklist(id->model)) != -1) {
> > + if (mode_wanted != 255)
> > + return min_t(u8, mode_wanted, max_mode);
> > +
> > + if ((drive->hwif->host_flags & IDE_HFLAG_PIO_NO_BLACKLIST) == 0 &&
>
> C also has ! operator. ;-)
IMO !() is less readable and more error-prone than () == 0.
Matter of style.
> > + (pio_mode = ide_scan_pio_blacklist(id->model)) != -1) {
> > printk(KERN_INFO "%s: is on PIO blacklist\n", drive->name);
> > } else {
> > pio_mode = id->tPIO;
> > @@ -324,7 +324,8 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
> > /*
> > * Conservative "downgrade" for all pre-ATA2 drives
> > */
> > - if (pio_mode && pio_mode < 4) {
> > + if ((drive->hwif->host_flags & IDE_HFLAG_PIO_NO_DOWNGRADE) == 0 &&
>
> It really does! :-)
This turns into ExtremeNitpicking. ;-)
> > + pio_mode && pio_mode < 4) {
> > pio_mode--;
> > printk(KERN_INFO "%s: applying conservative "
> > "PIO \"downgrade\"\n", drive->name);
> > Index: b/drivers/ide/pci/amd74xx.c
> > ===================================================================
> > --- a/drivers/ide/pci/amd74xx.c
> > +++ b/drivers/ide/pci/amd74xx.c
> > @@ -1,5 +1,5 @@
> > /*
> > - * Version 2.20
> > + * Version 2.21
> > *
> > * AMD 755/756/766/8111 and nVidia nForce/2/2s/3/3s/CK804/MCP04
> > * IDE driver for Linux.
> > @@ -275,10 +275,8 @@ static int amd_set_drive(ide_drive_t *dr
> >
> > static void amd74xx_tune_drive(ide_drive_t *drive, u8 pio)
> > {
> > - if (pio == 255) {
> > - amd_set_drive(drive, ide_find_best_pio_mode(drive));
> > - return;
> > - }
> > + if (pio == 255)
> > + pio = ide_get_best_pio_mode(drive, 255, 5);
> >
> > amd_set_drive(drive, XFER_PIO_0 + min_t(byte, pio, 5));
> > }
>
> Erm, I don't get it -- why not just use ide_get_best_pio_mode() for every
> 'pio'? There should be no functionality changes, no?
Indeed but ide_get_best_pio_mode() call goes away completely in patch #13/15.
Please don't miss "The Big Picture".
> > Index: b/drivers/ide/pci/via82cxxx.c
> > ===================================================================
> > --- a/drivers/ide/pci/via82cxxx.c
> > +++ b/drivers/ide/pci/via82cxxx.c
> > @@ -1,6 +1,6 @@
> > /*
> > *
> > - * Version 3.45
> > + * Version 3.46
> > *
> > * VIA IDE driver for Linux. Supported southbridges:
> > *
> > @@ -203,10 +203,8 @@ static int via_set_drive(ide_drive_t *dr
> >
> > static void via82cxxx_tune_drive(ide_drive_t *drive, u8 pio)
> > {
> > - if (pio == 255) {
> > - via_set_drive(drive, ide_find_best_pio_mode(drive));
> > - return;
> > - }
> > + if (pio == 255)
> > + pio = ide_get_best_pio_mode(drive, 255, 5);
> >
> > via_set_drive(drive, XFER_PIO_0 + min_t(u8, pio, 5));
>
> Same question here...
ditto
General remark: please note that the keeping the development _process_ steady
is much more important than getting _single_ change perfect (it just need to
be good enough) because otherwise we end up investing too much time on things
which are not that important in the long-term and time is a very limited
resource.
Well, I tended to be perfectionist myself until I took the grasp of above.
;-)
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
