> + * Called to enable the use of DMA based on kernel command line. > + */ > +void octeon_cf_enable_dma(void) > +{ > + use_cf_dma = 1; > +} Why not use the standard module parameter interface ? > + /* > + * PIO modes 0-4 all allow the device to deassert IORDY to slow down > + * the host. > + */ See ata_timing_compute() which also knows about master/slave timing, PIO5/6 rules and timing adjustments as well as doing bus clock quantisations and lengthenings for you. > + use_iordy = 1; This depends on the device as well and gets quite complicated. We have ata_pio_need_iordy() to do the work for you. > + t1 = (t1 * clocks_us) / 1000 / 2; > + if (t1) > + t1--; Even if you wanted to do it this way you could just use arrays and lookup tables as many other drivers do - ie pio = dev->pio_mode - XFER_PIO_0; t1 = data[pio]; > + mio_boot_reg_tim.s.wr_hld = t4; What guarantees the computation results always fit the bit fields. This is one reason the kerne strongly favours masks - it is a lot easier to check such things. > +static void octeon_cf_set_dmamode(struct ata_port *ap, struct ata_device *dev) > +{ > + case XFER_MW_DMA_0: > + dma_ackh = 20; /* Tj */ > + To = 480; > + Td = 215; > + Tkr = 50; > + break; Same comments apply as to PIO > + /* > + * Odd lengths are not supported. We should always be a > + * multiple of 512. > + */ > + BUG_ON(buflen & 1); If you get a request for an odd length you should write an extra word containing the last byte and one other. See how the standard methods handle this. > + if (ocd->is16bit) { Or you could have two methods and two transfer routines defined > + while (words--) { > + *(uint16_t *)buffer = ioread16(data_addr); > + buffer += sizeof(uint16_t); By definition tht is 2. Do you have an ioread16_rep ? > + > +/** > + * Get ready for a dma operation. We do nothing, as all DMA > + * operations are taken care of in octeon_cf_bmdma_start. > + */ > +void octeon_cf_qc_prep(struct ata_queued_cmd *qc) > +{ > +} ata_noop_qc_prep > +/** > + * Check if any queued commands have more DMAs, if so start the next > + * transfer, else do standard handling. > + */ > +irqreturn_t octeon_cf_interrupt(int irq, void *dev_instance) A lot of these functions could be static it seems > > +static void octeon_cf_delayed_irq(unsigned long data) > +{ What stops the following occuring ATA irq BUSY still set Queue tasklet Other irq on same line ATA busy clear Handle command Tasklet runs but command was sorted out (or a reset of the ata controller in the gap) > + base = cs0 + ocd->base_region_bias; > + if (!ocd->is16bit) { ata_std_ > + ap->ioaddr.cmd_addr = base + ATA_REG_CMD; ata_sff_std_ports ? (at least for the 8bit case) Alan -- "Alan, I'm getting a bit worried about you." -- Linus Torvalds -- 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