Re: [PATCH] Palmchip BK3710 IDE driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello.

Alan Cox wrote:

This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.

New drivers should really be going at least parallel into drivers/ata and
legacy IDE.

Alas, it's unlikely that we can spend more time on developing libata equivalent which we don't need at this point.

+struct palm_bk3710_ideconfigregs {
+	unsigned short idetimp __attribute__((packed));
+	unsigned short idetims __attribute__((packed));
+	unsigned char sidetim;
+	unsigned short slewctl __attribute__((packed));
+	unsigned char idestatus;
+	unsigned short udmactl __attribute__((packed));
+	unsigned short udmatim __attribute__((packed));
+	unsigned char rsvd0[4];
+	unsigned int miscctl __attribute__((packed));
+	unsigned int regstb __attribute__((packed));

NAK - the size of an unsigned int in the kernel isn't defined. All the
packed stuff is also horribly platform dependant. True the driver is ARM
only currently but that doesn't make it a good idea.

Can't you use defined offsets ?

+struct palm_bk3710_ideregs {
+	struct palm_bk3710_dmaengineregs dmaengine;

So why are only some packed ?

+		/* udmatim Register */
+		palm_bk3710_base->config.udmatim &= 0xFFF0;
+		palm_bk3710_base->config.udmatim |= level;

Direct memory access to I/O space - should be using read/write functions

   Sigh, I was anticipationg that somebody would say that... :-)

+	if (mate && mate->present) {
+		u8 mode2 = ide_get_best_pio_mode(mate, 255, 4);

If the pair device is present but not in the best mode it can do this
will be wrong surely ?

   Why?  ATA/PI says in the note to the table "Register transfer to/from device":

5 Mode shall be selected no higher than the *highest* mode supported by the *slowest* device.

Some other drivers are already using the same code (I'm not even sure it's needed here -- never saw the chip documentation).

+	if (!request_region(mem->start, 8, palm_bk3710_hwif->name)) {
+		printk(KERN_ERR "Error, ports in use.\n");
+		return -EBUSY;
+	}
+
+	palm_bk3710_hwif->dmatable_cpu = dma_alloc_coherent(
+				NULL,
+				PRD_ENTRIES * PRD_BYTES,
+				&palm_bk3710_hwif->dmatable_dma,
+				GFP_ATOMIC);
+
+	if (!palm_bk3710_hwif->dmatable_cpu) {

Leaks the reserved region

   My bad -- I've managed to overlook this while reviewing.

+		printk(KERN_ERR "Error, unable to allocate DMA table.\n");
+		return -ENOMEM;
+	}

-static void ide_dma_exec_cmd(ide_drive_t *drive, u8 command)
+void ide_dma_exec_cmd(ide_drive_t *drive, u8 command)
{
	/* issue cmd to drive */
ide_execute_command(drive, command, &ide_dma_intr, 2*WAIT_CMD, dma_timer_expiry);
}
+EXPORT_SYMBOL(ide_dma_exec_cmd);

These are not needed the NULL default request will fill them in for you

It won't -- we can *not* call ide_setup_dma() which fills them out as this is not a PCI chip.

MBR, Sergei
-
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

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux