Hi!
> Since you have not the input or insight to why it was expanded maybe you
> should just leave it alone. Since I am working on how to fix the
> MUNGE/KLUDGE that was added to make the PIO work, just maybe you may want
> rething the rathole you just created by attempting to compress the code
> before it is finished. What you doing is BORKING the driver to the point
> where adding in the TCQ will not work. Next you have screwed the future
> interface for Serial ATA.
I'm pretty sure I did not BORK anything. If I did BORK anything, show
me. If you need modified version, you can always just inline it;
programming with cut-copy-paste is bad, *always*.
Perhaps you should clean the drivers up before continuing
development. What is in drivers/ide is a mess.
I need driverfs support in drivers/ide, but code is such a pain to
look at that it is neccessary to clean it up first.
Let's take a look:
> > static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, unsigned long block)
> > {
> > - if (rq->flags & REQ_CMD)
> > - goto good_command;
> > -
> > - blk_dump_rq_flags(rq, "do_rw_disk, bad command");
> > - ide_end_request(0, HWGROUP(drive));
> > - return ide_stopped;
> > -
> > -good_command:
Goto without reason.
> > -#ifdef CONFIG_BLK_DEV_PDC4030
> > if (IS_PDC4030_DRIVE) {
> > extern ide_startstop_t promise_rw_disk(ide_drive_t *, struct request *, unsigned long);
> > return promise_rw_disk(drive, rq, block);
> > }
> > -#endif /* CONFIG_BLK_DEV_PDC4030 */
Completely unneccessary ifdef.
> > +static int get_sectors (ide_drive_t *drive, struct request *rq, task_ioreg_t command)
> > +{
> > + int sectors = rq->nr_sectors;
> > +
> > + if (sectors == 256)
> > + sectors = 0;
> > + if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
> > + sectors = drive->mult_count;
> > + if (sectors > rq->current_nr_sectors)
> > + sectors = rq->current_nr_sectors;
> > + }
> > + return sectors;
> > +}
Get_sectors was included 3 times below. That makes you wonder that
maybe there are small changes there? No, they were not.
> > @@ -186,17 +205,10 @@
> > unsigned int head = (track % drive->head);
> > unsigned int cyl = (track / drive->head);
> >
> > - memset(&taskfile, 0, sizeof(task_struct_t));
taskfile was defined as struct hd_drive_task_hdr, above. Very nice way
how to obfuscate code.
> > - rq->special = NULL;
> > - rq->special = (ide_task_t *)&args;
Two assignments to same variable [yep, these lines are just below each
other], and unneccessary cast as a bonus. [Unneccessary casts are
dangerous: they hide real errors.]
> > - void *hwif; /* actually (ide_hwif_t *) */
...
> > - void *driver; /* (ide_driver_t *) */
Two unneccessary pointers. Makes me wonder if original author knew C?
[how old is this code, btw?]
Pavel
-- Casualities in World Trade Center: ~3k dead inside the building, cryptography in U.S.A. and free speech in Czech Republic. - 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/
This archive was generated by hypermail 2b29 : Thu Feb 07 2002 - 21:01:01 EST