Skip to contentSkip to conversation

SH/Dreamcast - add support for GD-Rom device

PATCH In review Adrian McMenamin 6 participants 15 messages Dec 27, 2007 – Dec 31, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 SH/Dreamcast - add support for GD-Rom device

This patch adds support for the CD-Rom drive on the SEGA Dreamcast.

The SEGA Dreamcast has a built in CD-Rom drive, electrically similar to an ATA-3 drive, but implementing a proprietary packet interface - the so-called Sega Packet Interface (SPI)- and also supporting a proprietary format of disk - the Giga Disk Rom, with a 1GB capacity. The drive is know as the "GD-Rom drive".

This patch partially implements the SPI and also supports reading GD Rom disks. Unlike previous GD Rom drivers (which were never in the mainline), this driver implements DMA and not PIO for reads. It is a new driver, not a modified version of previous drivers.

This is the fifth iteration of this patch - which should work with both 2.6-24-rc5 and Paul Mundt's 2.6.25 queue.

Signed-off by: Adrian McMenamin <[email protected]>

----

drivers/block/Kconfig +11 -0
diff -rupN linux-2.6-orig/drivers/block/Kconfig linux-2.6/drivers/block/Kconfig
@@ -105,6 +105,17 @@ config PARIDE
 	  "MicroSolutions backpack protocol", "DataStor Commuter protocol"
 	  etc.).
 
+config GDROM
+	tristate "SEGA Dreamcast GD-ROM drive"
+	depends on SH_DREAMCAST
+	help
+	  A standard SEGA Dreamcast comes with a modified CD ROM drive called a
+	  "GD-ROM" by SEGA to signify it is capable of reading special disks
+	  with up to 1 GB of data. This drive will also read standard CD ROM
+	  disks. Select this option to access any disks in your GD ROM drive.
+	  Most users will want to say "Y" here.
+	  You can also build this as a module which will be called gdrom.ko
+
 source "drivers/block/paride/Kconfig"
 
 config BLK_CPQ_DA
drivers/cdrom/gdrom.c +776 -0

776 lines not on this page · raw message

drivers/cdrom/Makefile +1 -0
diff -rupN linux-2.6-orig/drivers/cdrom/Makefile linux-2.6/drivers/cdrom/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_PARIDE_PCD)	+=		cdrom.o
 obj-$(CONFIG_CDROM_PKTCDVD)	+=		cdrom.o
 
 obj-$(CONFIG_VIOCD)		+= viocd.o      cdrom.o
+obj-$(CONFIG_GDROM)		+= gdrom.o      cdrom.o
#1 lore raw
Quoted 13 lines · Adrian McMenamin

This patch adds support for the CD-Rom drive on the SEGA Dreamcast.

The SEGA Dreamcast has a built in CD-Rom drive, electrically similar to an ATA-3 drive, but implementing a proprietary packet interface - the so-called Sega Packet Interface (SPI)- and also supporting a proprietary format of disk - the Giga Disk Rom, with a 1GB capacity. The drive is know as the "GD-Rom drive".

This patch partially implements the SPI and also supports reading GD Rom disks. Unlike previous GD Rom drivers (which were never in the mainline), this driver implements DMA and not PIO for reads. It is a new driver, not a modified version of previous drivers.

Quoted 7 lines

This is the fifth iteration of this patch - which should work with both 2.6-24-rc5 and Paul Mundt's 2.6.25 queue.

Signed-off by: Adrian McMenamin <[email protected]>

----

Following up on more comments, here is a slightly reworked version of the patch with better request handling.

Signed-off by: Adrian McMenamin <[email protected]>

----

drivers/block/Kconfig +11 -0
diff -rupN linux-2.6-orig/drivers/block/Kconfig linux-2.6/drivers/block/Kconfig
@@ -105,6 +105,17 @@ config PARIDE
 	  "MicroSolutions backpack protocol", "DataStor Commuter protocol"
 	  etc.).
 
+config GDROM
+	tristate "SEGA Dreamcast GD-ROM drive"
+	depends on SH_DREAMCAST
+	help
+	  A standard SEGA Dreamcast comes with a modified CD ROM drive called a
+	  "GD-ROM" by SEGA to signify it is capable of reading special disks
+	  with up to 1 GB of data. This drive will also read standard CD ROM
+	  disks. Select this option to access any disks in your GD ROM drive.
+	  Most users will want to say "Y" here.
+	  You can also build this as a module which will be called gdrom.ko
+
 source "drivers/block/paride/Kconfig"
 
 config BLK_CPQ_DA
drivers/cdrom/gdrom.c +772 -0

772 lines not on this page · raw message

drivers/cdrom/Makefile +1 -0
diff -rupN linux-2.6-orig/drivers/cdrom/Makefile linux-2.6/drivers/cdrom/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_PARIDE_PCD)	+=		cdrom.o
 obj-$(CONFIG_CDROM_PKTCDVD)	+=		cdrom.o
 
 obj-$(CONFIG_VIOCD)		+= viocd.o      cdrom.o
+obj-$(CONFIG_GDROM)		+= gdrom.o      cdrom.o
#2 lore raw
In reply to Adrian McMenamin
Quoted 13 lines · Adrian McMenamin

This patch adds support for the CD-Rom drive on the SEGA Dreamcast.

The SEGA Dreamcast has a built in CD-Rom drive, electrically similar to an ATA-3 drive, but implementing a proprietary packet interface - the so-called Sega Packet Interface (SPI)- and also supporting a proprietary format of disk - the Giga Disk Rom, with a 1GB capacity. The drive is know as the "GD-Rom drive".

This patch partially implements the SPI and also supports reading GD Rom disks. Unlike previous GD Rom drivers (which were never in the mainline), this driver implements DMA and not PIO for reads. It is a new driver, not a modified version of previous drivers.

Quoted 5 lines

This is the fifth iteration of this patch - which should work with both 2.6-24-rc5 and Paul Mundt's 2.6.25 queue.

Signed-off by: Adrian McMenamin <[email protected]>

Looks fine to me. Thanks for cleaning it up, Adrian.

#3 lore raw
In reply to Adrian McMenamin
Quoted 1 line · Adrian McMenamin

This patch adds support for the CD-Rom drive on the SEGA Dreamcast.

Because it was already so close, might as well make it checkpatch clean.

I also added a function gdrom_is_busy() to make a couple of tests fit on a single line and perhaps easier to read.

drivers/cdrom/gdrom.c +55 -28
@@ -86,7 +86,8 @@
 	{MEDIUM_ERROR, "GDROM: Disk not ready"},
 	{HARDWARE_ERROR, "GDROM: Hardware error"},
 	{ILLEGAL_REQUEST, "GDROM: Command has failed"},
-	{UNIT_ATTENTION, "GDROM: Device needs attention - disk may have been changed"},
+	{UNIT_ATTENTION, "GDROM: Device needs attention - "
+			 "disk may have been changed"},
 	{DATA_PROTECT, "GDROM: Data protection error"},
 	{ABORTED_COMMAND, "GDROM: Command aborted"},
 };
@@ -130,14 +131,20 @@
 };
 
 static int gdrom_getsense(short *bufstring);
-static int gdrom_packetcommand(struct cdrom_device_info *cd_info, struct packet_command *command);
+static int gdrom_packetcommand(struct cdrom_device_info *cd_info,
+			       struct packet_command *command);
 static int gdrom_hardreset(struct cdrom_device_info *cd_info);
 
+static bool gdrom_is_busy(void)
+{
+	return (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) != 0;
+}
+
 static void gdrom_wait_clrbusy(void)
 {
 	/* long timeouts - typical for a CD Rom */
 	unsigned long timeout = jiffies + HZ * 60;
-	while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) && (time_before(jiffies, timeout)))
+	while (gdrom_is_busy() && (time_before(jiffies, timeout)))
 		cpu_relax();
 }
 
@@ -146,7 +153,7 @@
 	unsigned long timeout;
 	/* Wait to get busy first */
 	timeout = jiffies + HZ * 60;
-	while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) = 0) && (time_before(jiffies, timeout)))
+	while (!gdrom_is_busy() && (time_before(jiffies, timeout)))
 		cpu_relax();
 	/* Now wait for busy to clear */
 	gdrom_wait_clrbusy();
@@ -216,7 +223,8 @@
 	gd.pending = 1;
 	gdrom_packetcommand(gd.cd_info, spin_command);
 	/* 60 second timeout */
-	wait_event_interruptible_timeout(command_queue, gd.pending = 0, HZ * 60);
+	wait_event_interruptible_timeout(command_queue,
+					 gd.pending = 0, HZ * 60);
 	gd.pending = 0;
 	kfree(spin_command);
 	if (gd.status & 0x01) {
@@ -249,7 +257,8 @@
 	toc_command->buflen = tocsize;
 	gd.pending = 1;
 	gdrom_packetcommand(gd.cd_info, toc_command);
-	wait_event_interruptible_timeout(command_queue, gd.pending = 0, HZ * 60);
+	wait_event_interruptible_timeout(command_queue,
+					 gd.pending = 0, HZ * 60);
 	gd.pending = 0;
 	insw(PHYSADDR(GDROM_DATA_REG), toc, tocsize/2);
 	kfree(toc_command);
@@ -274,7 +283,8 @@
 	return (track & 0x0000ff00) >> 8;
 }
 
-static int gdrom_get_last_session(struct cdrom_device_info *cd_info, struct cdrom_multisession *ms_info)
+static int gdrom_get_last_session(struct cdrom_device_info *cd_info,
+				  struct cdrom_multisession *ms_info)
 {
 	int fentry, lentry, track, data, tocuse, err;
 	if (!gd.toc)
@@ -287,7 +297,8 @@
 		tocuse = 0;
 		err = gdrom_readtoc_cmd(gd.toc, 0);
 		if (err) {
-			printk(KERN_INFO "GDROM: Could not get CD table of contents\n");
+			printk(KERN_INFO "GDROM: Could not get CD "
+			       "table of contents\n");
 			return -ENXIO;
 		}
 	}
@@ -305,7 +316,8 @@
 
 	if ((track > 100) || (track < get_entry_track(gd.toc->first))) {
 		gdrom_getsense(NULL);
-		printk(KERN_INFO "GDROM: No data on the last session of the CD\n");
+		printk(KERN_INFO "GDROM: No data on the last session "
+		       "of the CD\n");
 		return -ENXIO;
 	}
 
@@ -355,8 +367,10 @@
 	return 0;
 }
 
-/* keep the function looking like the universal CD Rom specification - returning int*/
-static int gdrom_packetcommand(struct cdrom_device_info *cd_info, struct packet_command *command)
+/* keep the function looking like the universal CD Rom specification -
+ * returning int */
+static int gdrom_packetcommand(struct cdrom_device_info *cd_info,
+			       struct packet_command *command)
 {
 	gdrom_spicommand(&command->cmd, command->buflen);
 	return 0;
@@ -388,7 +402,8 @@
 	gd.pending = 1;
 	gdrom_packetcommand(gd.cd_info, sense_command);
 	/* 60 second timeout */
-	wait_event_interruptible_timeout(command_queue, gd.pending = 0, HZ * 60);
+	wait_event_interruptible_timeout(command_queue,
+					 gd.pending = 0, HZ * 60);
 	gd.pending = 0;
 	kfree(sense_command);
 	insw(PHYSADDR(GDROM_DATA_REG), &sense, 5);
@@ -400,7 +415,8 @@
 	printk(KERN_INFO "%s\n", sense_texts[sense_key].text);
 
 	if (bufstring)
-		memcpy(bufstring, &sense[4], 2); /* return additional sense data */
+		memcpy(bufstring, &sense[4], 2);
+					/* return additional sense data */
 
 	if (sense_key < 2)
 		return 0;
@@ -434,7 +450,8 @@
 	return cdrom_media_changed(gd.cd_info);
 }
 
-static int gdrom_bdops_ioctl(struct inode *inode, struct file *file, unsigned cmd, unsigned long arg)
+static int gdrom_bdops_ioctl(struct inode *inode, struct file *file,
+			     unsigned cmd, unsigned long arg)
 {
 	return cdrom_ioctl(file, gd.cd_info, inode, cmd, arg);
 }
@@ -471,11 +488,13 @@
 {
 	int err;
 	init_waitqueue_head(&command_queue);
-	err = request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt, IRQF_DISABLED, "gdrom_command", &gd);
+	err = request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt,
+			  IRQF_DISABLED, "gdrom_command", &gd);
 	if (err)
 		return err;
 	init_waitqueue_head(&request_queue);
-	err = request_irq(HW_EVENT_GDROM_DMA, gdrom_dma_interrupt, IRQF_DISABLED, "gdrom_dma", &gd);
+	err = request_irq(HW_EVENT_GDROM_DMA, gdrom_dma_interrupt,
+			  IRQF_DISABLED, "gdrom_dma", &gd);
 	if (err)
 		free_irq(HW_EVENT_GDROM_CMD, &gd);
 	return err;
@@ -531,27 +550,30 @@
 		ctrl_outb(0, GDROM_INTSEC_REG);
 		/* In multiple DMA transfers need to wait */
 		timeout = jiffies + HZ / 2;
-		while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) && (time_before(jiffies, timeout)))
+		while (gdrom_is_busy() && (time_before(jiffies, timeout)))
 			cpu_relax();
 		ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
 		timeout = jiffies + HZ / 2;
-		while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) && (time_before(jiffies, timeout)))
+		while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) &&
+		       (time_before(jiffies, timeout)))
 			cpu_relax(); /* wait for DRQ to be set to 1 */
 		gd.pending = 1;
 		gd.transfer = 1;
 		outsw(PHYSADDR(GDROM_DATA_REG), &read_command->cmd, 6);
 		timeout = jiffies + HZ / 2;
-		while ((ctrl_inb(GDROM_DMA_STATUS_REG)) && (time_before(jiffies, timeout)))
+		while ((ctrl_inb(GDROM_DMA_STATUS_REG)) &&
+		       (time_before(jiffies, timeout)))
 			cpu_relax();
 		ctrl_outb(1, GDROM_DMA_STATUS_REG);
 		/* 5 second error margin here seems more reasonable */
-		wait_event_interruptible_timeout(request_queue, gd.transfer = 0, HZ * 5);
+		wait_event_interruptible_timeout(request_queue,
+						 gd.transfer = 0, HZ * 5);
 		err = ctrl_inb(GDROM_DMA_STATUS_REG);
 		err = gd.transfer;
 		gd.transfer = 0;
 		gd.pending = 0;
 		/* now seek to take the request spinlock
- 		 * before handling ending the request */
+		 * before handling ending the request */
 		spin_lock(&gdrom_lock);
 		list_del_init(&req->queuelist);
 		blk_requeue_request(gd.gdrom_rq, req);
@@ -567,7 +589,7 @@
 static void gdrom_request_handler_dma(struct request *req)
 {
 	/* dequeue, add to list of deferred work
- 	 * and then schedule workqueue */
+	 * and then schedule workqueue */
 	blkdev_dequeue_request(req);
 	list_add_tail(&req->queuelist, &gdrom_deferred);
 	schedule_work(&work);
@@ -582,7 +604,8 @@
 			end_request(req, 0);
 		}
 		if (rq_data_dir(req) != READ) {
-			printk(KERN_NOTICE "GDROM: Read only device - write request ignored\n");
+			printk(KERN_NOTICE "GDROM: Read only device - "
+			       "write request ignored\n");
 			end_request(req, 0);
 		}
 		if (req->nr_sectors)
@@ -612,7 +635,8 @@
 	firmw_ver = kstrndup(id->firmver, 16, GFP_KERNEL);
 	if (!firmw_ver)
 		goto free_manuf_name;
-	printk(KERN_INFO "GDROM: %s from %s with firmware %s\n", model_name, manuf_name, firmw_ver);
+	printk(KERN_INFO "GDROM: %s from %s with firmware %s\n",
+	       model_name, manuf_name, firmw_ver);
 	err = 0;
 	kfree(firmw_ver);
 free_manuf_name:
@@ -641,7 +665,8 @@
 	gd.cd_info->ops = &gdrom_ops;
 	gd.cd_info->capacity = 1;
 	strcpy(gd.cd_info->name, GDROM_DEV_NAME);
-	gd.cd_info->mask = CDC_CLOSE_TRAY|CDC_OPEN_TRAY|CDC_LOCK|CDC_SELECT_DISC;
+	gd.cd_info->mask = CDC_CLOSE_TRAY | CDC_OPEN_TRAY |
+			   CDC_LOCK | CDC_SELECT_DISC;
 }
 
 static void __devinit probe_gdrom_setupdisk(void)
@@ -671,7 +696,7 @@
 {
 	int err;
 	if (gdrom_execute_diagnostic() != 1) {
-		printk(KERN_WARNING "GDROM: ATA Probe for GDROM failed.\n");
+		printk(KERN_WARNING "GDROM: ATA Probe for GDROM failed\n");
 		return -ENODEV;
 	}
 	if (gdrom_outputversion())
@@ -679,7 +704,8 @@
 	gdrom_major = register_blkdev(0, GDROM_DEV_NAME);
 	if (gdrom_major <= 0)
 		return gdrom_major;
-	printk(KERN_INFO "GDROM: Block device is registered with major number %d\n", gdrom_major);
+	printk(KERN_INFO "GDROM: Block device is registered "
+	       "with major number %d\n", gdrom_major);
 	gd.cd_info = kzalloc(sizeof(struct cdrom_device_info), GFP_KERNEL);
 	if (!gd.cd_info) {
 		err = -ENOMEM;
@@ -724,7 +750,8 @@
 	unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
 	gdrom_major = 0;
 probe_fail_no_mem:
-	printk(KERN_WARNING "GDROM: Could not probe for device - error is 0x%X\n", err);
+	printk(KERN_WARNING "GDROM: Could not probe for device - "
+	       "error is 0x%X\n", err);
 	return err;
 }
 
#4 lore raw

tor, 27 12 2007 kl. 14:58 -0800, skrev Joe Perches:

Quoted 32 lines · Adrian McMenamin
Quoted 1 line

This patch adds support for the CD-Rom drive on the SEGA Dreamcast.

Because it was already so close, might as well make it checkpatch clean.

I also added a function gdrom_is_busy() to make a couple of tests fit on a single line and perhaps easier to read.

drivers/cdrom/gdrom.c +5 -2
@@ -86,7 +86,8 @@
 	{MEDIUM_ERROR, "GDROM: Disk not ready"},
 	{HARDWARE_ERROR, "GDROM: Hardware error"},
 	{ILLEGAL_REQUEST, "GDROM: Command has failed"},
-	{UNIT_ATTENTION, "GDROM: Device needs attention - disk may have been changed"},
+	{UNIT_ATTENTION, "GDROM: Device needs attention - "
+			 "disk may have been changed"},
 	{DATA_PROTECT, "GDROM: Data protection error"},
 	{ABORTED_COMMAND, "GDROM: Command aborted"},
 };
@@ -130,14 +131,20 @@
 };
 
 static int gdrom_getsense(short *bufstring);
-static int gdrom_packetcommand(struct cdrom_device_info *cd_info, struct packet_command *command);
+static int gdrom_packetcommand(struct cdrom_device_info *cd_info,
+			       struct packet_command *command);
 static int gdrom_hardreset(struct cdrom_device_info *cd_info);
 
+static bool gdrom_is_busy(void)

drivers/cdrom/gdrom.c line 147

Quoted 10 lines
drivers/cdrom/gdrom.c +5 -1
+{
+	return (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) != 0;
+}
+
 static void gdrom_wait_clrbusy(void)
 {
 	/* long timeouts - typical for a CD Rom */
 	unsigned long timeout = jiffies + HZ * 60;
-	while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) && (time_before(jiffies, timeout)))
+	while (gdrom_is_busy() && (time_before(jiffies, timeout)))

^ ^
You don't need those parentheses.

drivers/cdrom/gdrom.c line 156

Quoted 9 lines
drivers/cdrom/gdrom.c +1 -1
 		cpu_relax();
 }
 
@@ -146,7 +153,7 @@
 	unsigned long timeout;
 	/* Wait to get busy first */
 	timeout = jiffies + HZ * 60;
-	while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) = 0) && (time_before(jiffies, timeout)))
+	while (!gdrom_is_busy() && (time_before(jiffies, timeout)))

Same here.

drivers/cdrom/gdrom.c line 230

Quoted 13 lines
drivers/cdrom/gdrom.c +2 -1
 		cpu_relax();
 	/* Now wait for busy to clear */
 	gdrom_wait_clrbusy();
@@ -216,7 +223,8 @@
 
 
 	gd.pending = 1;
 	gdrom_packetcommand(gd.cd_info, spin_command);
 	/* 60 second timeout */
-	wait_event_interruptible_timeout(command_queue, gd.pending = 0, HZ * 60);
+	wait_event_interruptible_timeout(command_queue,
+					 gd.pending = 0, HZ * 60);
 	gd.pending = 0;

All three users of gdrom_packetcommand do the same timeout, so consider moving this block into gdrom_packcommand.

Quoted 66 lines
drivers/cdrom/gdrom.c +14 -7
 	kfree(spin_command);
 	if (gd.status & 0x01) {
@@ -249,7 +257,8 @@
 	toc_command->buflen = tocsize;
 	gd.pending = 1;
 	gdrom_packetcommand(gd.cd_info, toc_command);
-	wait_event_interruptible_timeout(command_queue, gd.pending = 0, HZ * 60);
+	wait_event_interruptible_timeout(command_queue,
+					 gd.pending = 0, HZ * 60);
 	gd.pending = 0;
 	insw(PHYSADDR(GDROM_DATA_REG), toc, tocsize/2);
 	kfree(toc_command);
@@ -274,7 +283,8 @@
 	return (track & 0x0000ff00) >> 8;
 }
 
-static int gdrom_get_last_session(struct cdrom_device_info *cd_info, struct cdrom_multisession *ms_info)
+static int gdrom_get_last_session(struct cdrom_device_info *cd_info,
+				  struct cdrom_multisession *ms_info)
 {
 	int fentry, lentry, track, data, tocuse, err;
 	if (!gd.toc)
@@ -287,7 +297,8 @@
 		tocuse = 0;
 		err = gdrom_readtoc_cmd(gd.toc, 0);
 		if (err) {
-			printk(KERN_INFO "GDROM: Could not get CD table of contents\n");
+			printk(KERN_INFO "GDROM: Could not get CD "
+			       "table of contents\n");
 			return -ENXIO;
 		}
 	}
@@ -305,7 +316,8 @@
 
 	if ((track > 100) || (track < get_entry_track(gd.toc->first))) {
 		gdrom_getsense(NULL);
-		printk(KERN_INFO "GDROM: No data on the last session of the CD\n");
+		printk(KERN_INFO "GDROM: No data on the last session "
+		       "of the CD\n");
 		return -ENXIO;
 	}
 
@@ -355,8 +367,10 @@
 	return 0;
 }
 
-/* keep the function looking like the universal CD Rom specification - returning int*/
-static int gdrom_packetcommand(struct cdrom_device_info *cd_info, struct packet_command *command)
+/* keep the function looking like the universal CD Rom specification -
+ * returning int */
+static int gdrom_packetcommand(struct cdrom_device_info *cd_info,
+			       struct packet_command *command)
 {
 	gdrom_spicommand(&command->cmd, command->buflen);
 	return 0;
@@ -388,7 +402,8 @@
 	gd.pending = 1;
 	gdrom_packetcommand(gd.cd_info, sense_command);
 	/* 60 second timeout */
-	wait_event_interruptible_timeout(command_queue, gd.pending = 0, HZ * 60);
+	wait_event_interruptible_timeout(command_queue,
+					 gd.pending = 0, HZ * 60);
 	gd.pending = 0;
 	kfree(sense_command);
 	insw(PHYSADDR(GDROM_DATA_REG), &sense, 5);

drivers/cdrom/gdrom.c line 419

Quoted 6 lines
drivers/cdrom/gdrom.c +2 -1
@@ -400,7 +415,8 @@
 	printk(KERN_INFO "%s\n", sense_texts[sense_key].text);
 
 	if (bufstring)
-		memcpy(bufstring, &sense[4], 2); /* return additional sense data */
+		memcpy(bufstring, &sense[4], 2);
+					/* return additional sense data */

Put the comment on the if-line or use curly brackets and give the comment its own line above memcpy?

Quoted 30 lines
drivers/cdrom/gdrom.c +6 -3
 	if (sense_key < 2)
 		return 0;
@@ -434,7 +450,8 @@
 	return cdrom_media_changed(gd.cd_info);
 }
 
-static int gdrom_bdops_ioctl(struct inode *inode, struct file *file, unsigned cmd, unsigned long arg)
+static int gdrom_bdops_ioctl(struct inode *inode, struct file *file,
+			     unsigned cmd, unsigned long arg)
 {
 	return cdrom_ioctl(file, gd.cd_info, inode, cmd, arg);
 }
@@ -471,11 +488,13 @@
 {
 	int err;
 	init_waitqueue_head(&command_queue);
-	err = request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt, IRQF_DISABLED, "gdrom_command", &gd);
+	err = request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt,
+			  IRQF_DISABLED, "gdrom_command", &gd);
 	if (err)
 		return err;
 	init_waitqueue_head(&request_queue);
-	err = request_irq(HW_EVENT_GDROM_DMA, gdrom_dma_interrupt, IRQF_DISABLED, "gdrom_dma", &gd);
+	err = request_irq(HW_EVENT_GDROM_DMA, gdrom_dma_interrupt,
+			  IRQF_DISABLED, "gdrom_dma", &gd);
 	if (err)
 		free_irq(HW_EVENT_GDROM_CMD, &gd);
 	return err;

drivers/cdrom/gdrom.c line 554

Quoted 6 lines
drivers/cdrom/gdrom.c +1 -1
@@ -531,27 +550,30 @@
 		ctrl_outb(0, GDROM_INTSEC_REG);
 		/* In multiple DMA transfers need to wait */
 		timeout = jiffies + HZ / 2;
-		while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) && (time_before(jiffies, timeout)))
+		while (gdrom_is_busy() && (time_before(jiffies, timeout)))
 			cpu_relax();

Another one here.

drivers/cdrom/gdrom.c line 558

Quoted 5 lines
drivers/cdrom/gdrom.c +2 -1
 		ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
 		timeout = jiffies + HZ / 2;
-		while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) && (time_before(jiffies, timeout)))
+		while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) &&
+		       (time_before(jiffies, timeout)))

What about a nice telling function like gdrom_is_busy for this?

drivers/cdrom/gdrom.c line 565

Quoted 8 lines
drivers/cdrom/gdrom.c +2 -1
 			cpu_relax(); /* wait for DRQ to be set to 1 */
 		gd.pending = 1;
 		gd.transfer = 1;
 		outsw(PHYSADDR(GDROM_DATA_REG), &read_command->cmd, 6);
 		timeout = jiffies + HZ / 2;
-		while ((ctrl_inb(GDROM_DMA_STATUS_REG)) && (time_before(jiffies, timeout)))
+		while ((ctrl_inb(GDROM_DMA_STATUS_REG)) &&
+		       (time_before(jiffies, timeout)))

Extra parentheses, also around ctrl_inb. And there should be curly brackets around the following line, since the while condition is on two lines.

Quoted 75 lines
drivers/cdrom/gdrom.c +13 -8
 			cpu_relax();
 		ctrl_outb(1, GDROM_DMA_STATUS_REG);
 		/* 5 second error margin here seems more reasonable */
-		wait_event_interruptible_timeout(request_queue, gd.transfer = 0, HZ * 5);
+		wait_event_interruptible_timeout(request_queue,
+						 gd.transfer = 0, HZ * 5);
 		err = ctrl_inb(GDROM_DMA_STATUS_REG);
 		err = gd.transfer;
 		gd.transfer = 0;
 		gd.pending = 0;
 		/* now seek to take the request spinlock
- 		 * before handling ending the request */
+		 * before handling ending the request */
 		spin_lock(&gdrom_lock);
 		list_del_init(&req->queuelist);
 		blk_requeue_request(gd.gdrom_rq, req);
@@ -567,7 +589,7 @@
 static void gdrom_request_handler_dma(struct request *req)
 {
 	/* dequeue, add to list of deferred work
- 	 * and then schedule workqueue */
+	 * and then schedule workqueue */
 	blkdev_dequeue_request(req);
 	list_add_tail(&req->queuelist, &gdrom_deferred);
 	schedule_work(&work);
@@ -582,7 +604,8 @@
 			end_request(req, 0);
 		}
 		if (rq_data_dir(req) != READ) {
-			printk(KERN_NOTICE "GDROM: Read only device - write request ignored\n");
+			printk(KERN_NOTICE "GDROM: Read only device - "
+			       "write request ignored\n");
 			end_request(req, 0);
 		}
 		if (req->nr_sectors)
@@ -612,7 +635,8 @@
 	firmw_ver = kstrndup(id->firmver, 16, GFP_KERNEL);
 	if (!firmw_ver)
 		goto free_manuf_name;
-	printk(KERN_INFO "GDROM: %s from %s with firmware %s\n", model_name, manuf_name, firmw_ver);
+	printk(KERN_INFO "GDROM: %s from %s with firmware %s\n",
+	       model_name, manuf_name, firmw_ver);
 	err = 0;
 	kfree(firmw_ver);
 free_manuf_name:
@@ -641,7 +665,8 @@
 	gd.cd_info->ops = &gdrom_ops;
 	gd.cd_info->capacity = 1;
 	strcpy(gd.cd_info->name, GDROM_DEV_NAME);
-	gd.cd_info->mask = CDC_CLOSE_TRAY|CDC_OPEN_TRAY|CDC_LOCK|CDC_SELECT_DISC;
+	gd.cd_info->mask = CDC_CLOSE_TRAY | CDC_OPEN_TRAY |
+			   CDC_LOCK | CDC_SELECT_DISC;
 }
 
 static void __devinit probe_gdrom_setupdisk(void)
@@ -671,7 +696,7 @@
 {
 	int err;
 	if (gdrom_execute_diagnostic() != 1) {
-		printk(KERN_WARNING "GDROM: ATA Probe for GDROM failed.\n");
+		printk(KERN_WARNING "GDROM: ATA Probe for GDROM failed\n");
 		return -ENODEV;
 	}
 	if (gdrom_outputversion())
@@ -679,7 +704,8 @@
 	gdrom_major = register_blkdev(0, GDROM_DEV_NAME);
 	if (gdrom_major <= 0)
 		return gdrom_major;
-	printk(KERN_INFO "GDROM: Block device is registered with major number %d\n", gdrom_major);
+	printk(KERN_INFO "GDROM: Block device is registered "
+	       "with major number %d\n", gdrom_major);
 	gd.cd_info = kzalloc(sizeof(struct cdrom_device_info), GFP_KERNEL);
 	if (!gd.cd_info) {
 		err = -ENOMEM;

drivers/cdrom/gdrom.c line 756

Quoted 8 lines
drivers/cdrom/gdrom.c +2 -1
@@ -724,7 +750,8 @@
 	unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
 	gdrom_major = 0;
 probe_fail_no_mem:
-	printk(KERN_WARNING "GDROM: Could not probe for device - error is 0x%X\n", err);
+	printk(KERN_WARNING "GDROM: Could not probe for device - "
+	       "error is 0x%X\n", err);
 	return err;
 }

Simon Holm Thøgersen

#5 lore raw
Quoted 6 lines · Simon Holm Thøgersen

Review

Quoted 2 lines
(quoted diff) +1 -1
-	while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) && (time_before(jiffies, timeout)))
+	while (gdrom_is_busy() && (time_before(jiffies, timeout)))

You don't need those parentheses.

Review

Quoted 2 lines
(quoted diff) +2 -0
+		while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) &&
+		       (time_before(jiffies, timeout)))

What about a nice telling function like gdrom_is_busy for this?

Perhaps (uncompiled/untested):

Remove unnecessary parenthesis
Remove GDROM: prefix from sense_texts
Add function gdrom_data_request
Check sense_key against sense_text array size

---
 drivers/cdrom/gdrom.c |   53 ++++++++++++++++++++++++++----------------------
 1 files changed, 29 insertions(+), 24 deletions(-)
drivers/cdrom/gdrom.c +29 -24
index 59d26e0..ab95438 100644
@@ -80,16 +80,15 @@ static const struct {
 	int sense_key;
 	const char * const text;
 } sense_texts[] = {
-	{NO_SENSE, "GDROM: OK"},
-	{RECOVERED_ERROR, "GDROM: Recovered from error"},
-	{NOT_READY, "GDROM: Device not ready"},
-	{MEDIUM_ERROR, "GDROM: Disk not ready"},
-	{HARDWARE_ERROR, "GDROM: Hardware error"},
-	{ILLEGAL_REQUEST, "GDROM: Command has failed"},
-	{UNIT_ATTENTION, "GDROM: Device needs attention - "
-			 "disk may have been changed"},
-	{DATA_PROTECT, "GDROM: Data protection error"},
-	{ABORTED_COMMAND, "GDROM: Command aborted"},
+	{NO_SENSE, "OK"},
+	{RECOVERED_ERROR, "Recovered from error"},
+	{NOT_READY, "Device not ready"},
+	{MEDIUM_ERROR, "Disk not ready"},
+	{HARDWARE_ERROR, "Hardware error"},
+	{ILLEGAL_REQUEST, "Command has failed"},
+	{UNIT_ATTENTION, "Device needs attention - disk may have been changed"},
+	{DATA_PROTECT, "Data protection error"},
+	{ABORTED_COMMAND, "Command aborted"},
 };
 
 static struct platform_device *pd;
@@ -140,11 +139,16 @@ static bool gdrom_is_busy(void)
 	return (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) != 0;
 }
 
+static bool gdrom_data_request(void)
+{
+	return (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) = 8;
+}
+
 static void gdrom_wait_clrbusy(void)
 {
 	/* long timeouts - typical for a CD Rom */
 	unsigned long timeout = jiffies + HZ * 60;
-	while (gdrom_is_busy() && (time_before(jiffies, timeout)))
+	while (gdrom_is_busy() && time_before(jiffies, timeout))
 		cpu_relax();
 }
 
@@ -153,7 +157,7 @@ static void gdrom_wait_busy_sleeps(void)
 	unsigned long timeout;
 	/* Wait to get busy first */
 	timeout = jiffies + HZ * 60;
-	while (!gdrom_is_busy() && (time_before(jiffies, timeout)))
+	while (!gdrom_is_busy() && time_before(jiffies, timeout))
 		cpu_relax();
 	/* Now wait for busy to clear */
 	gdrom_wait_clrbusy();
@@ -186,8 +190,8 @@ static void gdrom_spicommand(void *spi_string, int buflen)
 	/* Wait until we can go */
 	gdrom_wait_clrbusy();
 	ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
-	while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8)
-		cpu_relax(); /* wait for DRQ to be set to 1 */
+	while (!gdrom_data_request())
+		cpu_relax();
 	outsw(PHYSADDR(GDROM_DATA_REG), cmd, 6);
 }
 
@@ -354,7 +358,7 @@ static int gdrom_drivestatus(struct cdrom_device_info *cd_info, int ignore)
 static int gdrom_mediachanged(struct cdrom_device_info *cd_info, int ignore)
 {
 	/* check the sense key */
-	return ((ctrl_inb(GDROM_ERROR_REG) & 0xF0) = 0x60);
+	return (ctrl_inb(GDROM_ERROR_REG) & 0xF0) = 0x60;
 }
 
 /* reset the G1 bus */
@@ -412,11 +416,13 @@ static int gdrom_getsense(short *bufstring)
 		return -EIO;
 	}
 	sense_key = sense[1] & 0x0F;
-	printk(KERN_INFO "%s\n", sense_texts[sense_key].text);
+	if (sense_key < ARRAY_SIZE(sense_texts))
+		printk(KERN_INFO "GDROM: %s\n", sense_texts[sense_key].text);
+	else
+		printk(KERN_ERR "GDROM: Unknown sense key: %d\n", sense_key);
 
-	if (bufstring)
+	if (bufstring)			/* return additional sense data */
 		memcpy(bufstring, &sense[4], 2);
-					/* return additional sense data */
 
 	if (sense_key < 2)
 		return 0;
@@ -550,19 +556,18 @@ static void gdrom_readdisk_dma(struct work_struct *work)
 		ctrl_outb(0, GDROM_INTSEC_REG);
 		/* In multiple DMA transfers need to wait */
 		timeout = jiffies + HZ / 2;
-		while (gdrom_is_busy() && (time_before(jiffies, timeout)))
+		while (gdrom_is_busy() && time_before(jiffies, timeout))
 			cpu_relax();
 		ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
 		timeout = jiffies + HZ / 2;
-		while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) &&
-		       (time_before(jiffies, timeout)))
-			cpu_relax(); /* wait for DRQ to be set to 1 */
+		while (!gdrom_data_request() && time_before(jiffies, timeout))
+			cpu_relax();
 		gd.pending = 1;
 		gd.transfer = 1;
 		outsw(PHYSADDR(GDROM_DATA_REG), &read_command->cmd, 6);
 		timeout = jiffies + HZ / 2;
-		while ((ctrl_inb(GDROM_DMA_STATUS_REG)) &&
-		       (time_before(jiffies, timeout)))
+		while (ctrl_inb(GDROM_DMA_STATUS_REG) &&
+		       time_before(jiffies, timeout))
 			cpu_relax();
 		ctrl_outb(1, GDROM_DMA_STATUS_REG);
 		/* 5 second error margin here seems more reasonable */
#6 lore raw
Quoted 79 lines · Joe Perches

Perhaps (uncompiled/untested):

Remove unnecessary parenthesis
Remove GDROM: prefix from sense_texts
Add function gdrom_data_request
Check sense_key against sense_text array size

---
 drivers/cdrom/gdrom.c |   53 ++++++++++++++++++++++++++----------------------
 1 files changed, 29 insertions(+), 24 deletions(-)
drivers/cdrom/gdrom.c +9 -10
index 59d26e0..ab95438 100644
@@ -80,16 +80,15 @@ static const struct {
 	int sense_key;
 	const char * const text;
 } sense_texts[] = {
-	{NO_SENSE, "GDROM: OK"},
-	{RECOVERED_ERROR, "GDROM: Recovered from error"},
-	{NOT_READY, "GDROM: Device not ready"},
-	{MEDIUM_ERROR, "GDROM: Disk not ready"},
-	{HARDWARE_ERROR, "GDROM: Hardware error"},
-	{ILLEGAL_REQUEST, "GDROM: Command has failed"},
-	{UNIT_ATTENTION, "GDROM: Device needs attention - "
-			 "disk may have been changed"},
-	{DATA_PROTECT, "GDROM: Data protection error"},
-	{ABORTED_COMMAND, "GDROM: Command aborted"},
+	{NO_SENSE, "OK"},
+	{RECOVERED_ERROR, "Recovered from error"},
+	{NOT_READY, "Device not ready"},
+	{MEDIUM_ERROR, "Disk not ready"},
+	{HARDWARE_ERROR, "Hardware error"},
+	{ILLEGAL_REQUEST, "Command has failed"},
+	{UNIT_ATTENTION, "Device needs attention - disk may have been changed"},
+	{DATA_PROTECT, "Data protection error"},
+	{ABORTED_COMMAND, "Command aborted"},
 };
 
 

/* reset the G1 bus */

drivers/cdrom/gdrom.c +8 -7
@@ -412,11 +416,13 @@ static int gdrom_getsense(short *bufstring)
 		return -EIO;
 	}
 	sense_key = sense[1] & 0x0F;
-	printk(KERN_INFO "%s\n", sense_texts[sense_key].text);
+	if (sense_key < ARRAY_SIZE(sense_texts))
+		printk(KERN_INFO "GDROM: %s\n", sense_texts[sense_key].text);
+	else
+		printk(KERN_ERR "GDROM: Unknown sense key: %d\n", sense_key);
 
-	if (bufstring)
+	if (bufstring)			/* return additional sense data */
 		memcpy(bufstring, &sense[4], 2);
-					/* return additional sense data */
 
 	if (sense_key < 2)
 		return 0;
@@ -550,19 +556,18 @@ static void gdrom_readdisk_dma(struct work_struct *work)
 		ctrl_outb(0, GDROM_INTSEC_REG);
 		/* In multiple DMA transfers need to wait */
 		timeout = jiffies + HZ / 2;
-		while (gdrom_is_busy() && (time_before(jiffies, timeout)))
+		while (gdrom_is_busy() && time_before(jiffies, timeout))
 			cpu_relax();
 		ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
 		timeout = jiffies + HZ / 2;
-		while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) &&
-		       (time_before(jiffies, timeout)))
-			cpu_relax(); /* wait for DRQ to be set to 1 */
+		while (!gdrom_data_request() && time_before(jiffies, timeout))
+			cpu_relax();
 		gd.pending = 1;
 		gd.transfer = 1;
 		outsw(PHYSADDR(GDROM_DATA_REG), &read_command->cmd, 6);
 		timeout = jiffies + HZ / 2;

drivers/cdrom/gdrom.c static void gdrom_readdisk_dma(struct work_struct *work)

Quoted 10 lines
drivers/cdrom/gdrom.c +2 -3
-		while ((ctrl_inb(GDROM_DMA_STATUS_REG)) &&
-		       (time_before(jiffies, timeout)))
+		while (ctrl_inb(GDROM_DMA_STATUS_REG) &&
+		       time_before(jiffies, timeout))
 			cpu_relax();
 		ctrl_outb(1, GDROM_DMA_STATUS_REG);
 		/* 5 second error margin here seems more reasonable */
 
 
-

This won't work see include/scsi/scsi.h

/*
* SENSE KEYS
*/

#define NO_SENSE            0x00
#define RECOVERED_ERROR     0x01
#define NOT_READY           0x02
#define MEDIUM_ERROR        0x03
#define HARDWARE_ERROR      0x04
#define ILLEGAL_REQUEST     0x05
#define UNIT_ATTENTION      0x06
#define DATA_PROTECT        0x07
#define BLANK_CHECK         0x08
#define COPY_ABORTED        0x0a
#define ABORTED_COMMAND     0x0b
#define VOLUME_OVERFLOW     0x0d
#define MISCOMPARE          0x0e

(The GD device specs says it supports 0, 1, 2, 3,4, 5, 6, 7 and 0xB)

ie we could get a sense key of 0x0B which would be greater than the array size. I think you'd have to hard code the limit.

#7 lore raw
Quoted 114 lines · Adrian McMenamin
Quoted 79 lines · Joe Perches

Perhaps (uncompiled/untested):

Remove unnecessary parenthesis
Remove GDROM: prefix from sense_texts
Add function gdrom_data_request
Check sense_key against sense_text array size

---
 drivers/cdrom/gdrom.c |   53 ++++++++++++++++++++++++++----------------------
 1 files changed, 29 insertions(+), 24 deletions(-)
drivers/cdrom/gdrom.c +9 -10
index 59d26e0..ab95438 100644
@@ -80,16 +80,15 @@ static const struct {
 	int sense_key;
 	const char * const text;
 } sense_texts[] = {
-	{NO_SENSE, "GDROM: OK"},
-	{RECOVERED_ERROR, "GDROM: Recovered from error"},
-	{NOT_READY, "GDROM: Device not ready"},
-	{MEDIUM_ERROR, "GDROM: Disk not ready"},
-	{HARDWARE_ERROR, "GDROM: Hardware error"},
-	{ILLEGAL_REQUEST, "GDROM: Command has failed"},
-	{UNIT_ATTENTION, "GDROM: Device needs attention - "
-			 "disk may have been changed"},
-	{DATA_PROTECT, "GDROM: Data protection error"},
-	{ABORTED_COMMAND, "GDROM: Command aborted"},
+	{NO_SENSE, "OK"},
+	{RECOVERED_ERROR, "Recovered from error"},
+	{NOT_READY, "Device not ready"},
+	{MEDIUM_ERROR, "Disk not ready"},
+	{HARDWARE_ERROR, "Hardware error"},
+	{ILLEGAL_REQUEST, "Command has failed"},
+	{UNIT_ATTENTION, "Device needs attention - disk may have been changed"},
+	{DATA_PROTECT, "Data protection error"},
+	{ABORTED_COMMAND, "Command aborted"},
 };
 
 

/* reset the G1 bus */

drivers/cdrom/gdrom.c +8 -7
@@ -412,11 +416,13 @@ static int gdrom_getsense(short *bufstring)
 		return -EIO;
 	}
 	sense_key = sense[1] & 0x0F;
-	printk(KERN_INFO "%s\n", sense_texts[sense_key].text);
+	if (sense_key < ARRAY_SIZE(sense_texts))
+		printk(KERN_INFO "GDROM: %s\n", sense_texts[sense_key].text);
+	else
+		printk(KERN_ERR "GDROM: Unknown sense key: %d\n", sense_key);
 
-	if (bufstring)
+	if (bufstring)			/* return additional sense data */
 		memcpy(bufstring, &sense[4], 2);
-					/* return additional sense data */
 
 	if (sense_key < 2)
 		return 0;
@@ -550,19 +556,18 @@ static void gdrom_readdisk_dma(struct work_struct *work)
 		ctrl_outb(0, GDROM_INTSEC_REG);
 		/* In multiple DMA transfers need to wait */
 		timeout = jiffies + HZ / 2;
-		while (gdrom_is_busy() && (time_before(jiffies, timeout)))
+		while (gdrom_is_busy() && time_before(jiffies, timeout))
 			cpu_relax();
 		ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
 		timeout = jiffies + HZ / 2;
-		while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) &&
-		       (time_before(jiffies, timeout)))
-			cpu_relax(); /* wait for DRQ to be set to 1 */
+		while (!gdrom_data_request() && time_before(jiffies, timeout))
+			cpu_relax();
 		gd.pending = 1;
 		gd.transfer = 1;
 		outsw(PHYSADDR(GDROM_DATA_REG), &read_command->cmd, 6);
 		timeout = jiffies + HZ / 2;

drivers/cdrom/gdrom.c static void gdrom_readdisk_dma(struct work_struct *work)

Quoted 10 lines
drivers/cdrom/gdrom.c +2 -3
-		while ((ctrl_inb(GDROM_DMA_STATUS_REG)) &&
-		       (time_before(jiffies, timeout)))
+		while (ctrl_inb(GDROM_DMA_STATUS_REG) &&
+		       time_before(jiffies, timeout))
 			cpu_relax();
 		ctrl_outb(1, GDROM_DMA_STATUS_REG);
 		/* 5 second error margin here seems more reasonable */
 
 
-

This won't work see include/scsi/scsi.h

/*
* SENSE KEYS
*/

#define NO_SENSE            0x00
#define RECOVERED_ERROR     0x01
#define NOT_READY           0x02
#define MEDIUM_ERROR        0x03
#define HARDWARE_ERROR      0x04
#define ILLEGAL_REQUEST     0x05
#define UNIT_ATTENTION      0x06
#define DATA_PROTECT        0x07
#define BLANK_CHECK         0x08
#define COPY_ABORTED        0x0a
#define ABORTED_COMMAND     0x0b
#define VOLUME_OVERFLOW     0x0d
#define MISCOMPARE          0x0e
Quoted 6 lines

(The GD device specs says it supports 0, 1, 2, 3,4, 5, 6, 7 and 0xB)

ie we could get a sense key of 0x0B which would be greater than the array size. I think you'd have to hard code the limit.

Ignore that. Talking rubbish. As usual.

#8 lore raw
In reply to Adrian McMenamin
Quoted 19 lines · Adrian McMenamin

This won't work see include/scsi/scsi.h
/*
* SENSE KEYS
*/

#define NO_SENSE            0x00
#define RECOVERED_ERROR     0x01
#define NOT_READY           0x02
#define MEDIUM_ERROR        0x03
#define HARDWARE_ERROR      0x04
#define ILLEGAL_REQUEST     0x05
#define UNIT_ATTENTION      0x06
#define DATA_PROTECT        0x07
#define BLANK_CHECK         0x08
#define COPY_ABORTED        0x0a
#define ABORTED_COMMAND     0x0b
#define VOLUME_OVERFLOW     0x0d
#define MISCOMPARE          0x0e
Quoted 4 lines

(The GD device specs says it supports 0, 1, 2, 3,4, 5, 6, 7 and 0xB)

ie we could get a sense key of 0x0B which would be greater than the array size. I think you'd have to hard code the limit.

Then shouldn't this test be:

	for (i = 0; i < ARRAY_SIZE(sense_texts); i++) {
		if (sense_key = sense_texts[i].sense_key)
			printk(KERN_INFO "GDROM: %s\n", sense_texts[i].text);
	}
	if (i >= ARRAY_SIZE(sense_texts))
		printk(KERN_ERR "GDROM: Unknown sense key: %d\n", sense_key);

cheers, Joe

#9 lore raw
In reply to Joe Perches
Quoted 4 lines · Joe Perches
Quoted 1 line · Adrian McMenamin

This patch adds support for the CD-Rom drive on the SEGA Dreamcast.

Because it was already so close, might as well make it checkpatch clean.

for the 80 col limit, that's up to the author. if Adrian is happy with it as is, that's fine.

Quoted 2 lines

I also added a function gdrom_is_busy() to make a couple of tests fit on a single line and perhaps easier to read.

i'd tend to agree it does make it easier to read, however you didnt make it inline which means gcc may not inline it which means there's pointless overhead of doing a function call.

otherwise, arbitrary breaking on 80 cols actually makes the code harder to read for no gain.
-mike

#10 lore raw
Quoted 8 lines · Mike Frysinger
Quoted 2 lines · Joe Perches

I also added a function gdrom_is_busy() to make a couple of tests fit on a single line and perhaps easier to read.

i'd tend to agree it does make it easier to read, however you didnt make it inline which means gcc may not inline it which means there's pointless overhead of doing a function call.

These days GCC is usually smart enough to automatically inline things that are that small, it doesn't need to be explicitly labelled. If your toolchain doesn't inline that gdrom_is_busy() implementation, get a better toolchain.

Quoted 2 lines

otherwise, arbitrary breaking on 80 cols actually makes the code harder to read for no gain.

Hey now, some of us have no intention of giving up our 80x25 consoles, not everyone buys in to this whole newfangled X thing. ;-)

More than 80 columns is generally fine so long as it's done so tastefully rather than arbitrarily based on a larger console width. However, if it can be split up cleanly for 80 columns, then it should be. ie, use common sense, rather than blindly adhering to the whims of an anal-retentive script.

#11 lore raw
In reply to Joe Perches

Op donderdag 27-12-2007 om 14:58 uur [tijdzone -0800], schreef Joe Perches:

Quoted 271 lines · Adrian McMenamin
Quoted 1 line

This patch adds support for the CD-Rom drive on the SEGA Dreamcast.

Because it was already so close, might as well make it checkpatch clean.

I also added a function gdrom_is_busy() to make a couple of tests fit on a single line and perhaps easier to read.

drivers/cdrom/gdrom.c +55 -29
@@ -86,7 +86,8 @@
 	{MEDIUM_ERROR, "GDROM: Disk not ready"},
 	{HARDWARE_ERROR, "GDROM: Hardware error"},
 	{ILLEGAL_REQUEST, "GDROM: Command has failed"},
-	{UNIT_ATTENTION, "GDROM: Device needs attention - disk may have been changed"},
+	{UNIT_ATTENTION, "GDROM: Device needs attention - "
+			 "disk may have been changed"},
 	{DATA_PROTECT, "GDROM: Data protection error"},
 	{ABORTED_COMMAND, "GDROM: Command aborted"},
 };
@@ -130,14 +131,20 @@
 };
 
 static int gdrom_getsense(short *bufstring);
-static int gdrom_packetcommand(struct cdrom_device_info *cd_info, struct packet_command *command);
+static int gdrom_packetcommand(struct cdrom_device_info *cd_info,
+			       struct packet_command *command);
 static int gdrom_hardreset(struct cdrom_device_info *cd_info);
 
+static bool gdrom_is_busy(void)
+{
+	return (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) != 0;
+}
+
 static void gdrom_wait_clrbusy(void)
 {
 	/* long timeouts - typical for a CD Rom */
 	unsigned long timeout = jiffies + HZ * 60;
-	while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) && (time_before(jiffies, timeout)))
+	while (gdrom_is_busy() && (time_before(jiffies, timeout)))
 		cpu_relax();
 }
 
@@ -146,7 +153,7 @@
 	unsigned long timeout;
 	/* Wait to get busy first */
 	timeout = jiffies + HZ * 60;
-	while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) = 0) && (time_before(jiffies, timeout)))
+	while (!gdrom_is_busy() && (time_before(jiffies, timeout)))
 		cpu_relax();
 	/* Now wait for busy to clear */
 	gdrom_wait_clrbusy();
@@ -216,7 +223,8 @@
 	gd.pending = 1;
 	gdrom_packetcommand(gd.cd_info, spin_command);
 	/* 60 second timeout */
-	wait_event_interruptible_timeout(command_queue, gd.pending = 0, HZ * 60);
+	wait_event_interruptible_timeout(command_queue,
+					 gd.pending = 0, HZ * 60);
 	gd.pending = 0;
 	kfree(spin_command);
 	if (gd.status & 0x01) {
@@ -249,7 +257,8 @@
 	toc_command->buflen = tocsize;
 	gd.pending = 1;
 	gdrom_packetcommand(gd.cd_info, toc_command);
-	wait_event_interruptible_timeout(command_queue, gd.pending = 0, HZ * 60);
+	wait_event_interruptible_timeout(command_queue,
+					 gd.pending = 0, HZ * 60);
 	gd.pending = 0;
 	insw(PHYSADDR(GDROM_DATA_REG), toc, tocsize/2);
 	kfree(toc_command);
@@ -274,7 +283,8 @@
 	return (track & 0x0000ff00) >> 8;
 }
 
-static int gdrom_get_last_session(struct cdrom_device_info *cd_info, struct cdrom_multisession *ms_info)
+static int gdrom_get_last_session(struct cdrom_device_info *cd_info,
+				  struct cdrom_multisession *ms_info)
 {
 	int fentry, lentry, track, data, tocuse, err;
 	if (!gd.toc)
@@ -287,7 +297,8 @@
 		tocuse = 0;
 		err = gdrom_readtoc_cmd(gd.toc, 0);
 		if (err) {
-			printk(KERN_INFO "GDROM: Could not get CD table of contents\n");
+			printk(KERN_INFO "GDROM: Could not get CD "
+			       "table of contents\n");
 			return -ENXIO;
 		}
 	}
@@ -305,7 +316,8 @@
 
 	if ((track > 100) || (track < get_entry_track(gd.toc->first))) {
 		gdrom_getsense(NULL);
-		printk(KERN_INFO "GDROM: No data on the last session of the CD\n");
+		printk(KERN_INFO "GDROM: No data on the last session "
+		       "of the CD\n");
 		return -ENXIO;
 	}
 
@@ -355,8 +367,10 @@
 	return 0;
 }
 
-/* keep the function looking like the universal CD Rom specification - returning int*/
-static int gdrom_packetcommand(struct cdrom_device_info *cd_info, struct packet_command *command)
+/* keep the function looking like the universal CD Rom specification -
+ * returning int */
+static int gdrom_packetcommand(struct cdrom_device_info *cd_info,
+			       struct packet_command *command)
 {
 	gdrom_spicommand(&command->cmd, command->buflen);
 	return 0;
@@ -388,7 +402,8 @@
 	gd.pending = 1;
 	gdrom_packetcommand(gd.cd_info, sense_command);
 	/* 60 second timeout */
-	wait_event_interruptible_timeout(command_queue, gd.pending = 0, HZ * 60);
+	wait_event_interruptible_timeout(command_queue,
+					 gd.pending = 0, HZ * 60);
 	gd.pending = 0;
 	kfree(sense_command);
 	insw(PHYSADDR(GDROM_DATA_REG), &sense, 5);
@@ -400,7 +415,8 @@
 	printk(KERN_INFO "%s\n", sense_texts[sense_key].text);
 
 	if (bufstring)
-		memcpy(bufstring, &sense[4], 2); /* return additional sense data */
+		memcpy(bufstring, &sense[4], 2);
+					/* return additional sense data */
 
 	if (sense_key < 2)
 		return 0;
@@ -434,7 +450,8 @@
 	return cdrom_media_changed(gd.cd_info);
 }
 
-static int gdrom_bdops_ioctl(struct inode *inode, struct file *file, unsigned cmd, unsigned long arg)
+static int gdrom_bdops_ioctl(struct inode *inode, struct file *file,
+			     unsigned cmd, unsigned long arg)
 {
 	return cdrom_ioctl(file, gd.cd_info, inode, cmd, arg);
 }
@@ -471,11 +488,13 @@
 {
 	int err;
 	init_waitqueue_head(&command_queue);
-	err = request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt, IRQF_DISABLED, "gdrom_command", &gd);
+	err = request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt,
+			  IRQF_DISABLED, "gdrom_command", &gd);
 	if (err)
 		return err;
 	init_waitqueue_head(&request_queue);
-	err = request_irq(HW_EVENT_GDROM_DMA, gdrom_dma_interrupt, IRQF_DISABLED, "gdrom_dma", &gd);
+	err = request_irq(HW_EVENT_GDROM_DMA, gdrom_dma_interrupt,
+			  IRQF_DISABLED, "gdrom_dma", &gd);
 	if (err)
 		free_irq(HW_EVENT_GDROM_CMD, &gd);
 	return err;
@@ -531,27 +550,30 @@
 		ctrl_outb(0, GDROM_INTSEC_REG);
 		/* In multiple DMA transfers need to wait */
 		timeout = jiffies + HZ / 2;
-		while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) && (time_before(jiffies, timeout)))
+		while (gdrom_is_busy() && (time_before(jiffies, timeout)))
 			cpu_relax();
 		ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
 		timeout = jiffies + HZ / 2;
-		while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) && (time_before(jiffies, timeout)))
+		while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) &&
+		       (time_before(jiffies, timeout)))
 			cpu_relax(); /* wait for DRQ to be set to 1 */
 		gd.pending = 1;
 		gd.transfer = 1;
 		outsw(PHYSADDR(GDROM_DATA_REG), &read_command->cmd, 6);
 		timeout = jiffies + HZ / 2;
-		while ((ctrl_inb(GDROM_DMA_STATUS_REG)) && (time_before(jiffies, timeout)))
+		while ((ctrl_inb(GDROM_DMA_STATUS_REG)) &&
+		       (time_before(jiffies, timeout)))
 			cpu_relax();
 		ctrl_outb(1, GDROM_DMA_STATUS_REG);
 		/* 5 second error margin here seems more reasonable */
-		wait_event_interruptible_timeout(request_queue, gd.transfer = 0, HZ * 5);
+		wait_event_interruptible_timeout(request_queue,
+						 gd.transfer = 0, HZ * 5);
 		err = ctrl_inb(GDROM_DMA_STATUS_REG);
 		err = gd.transfer;
 		gd.transfer = 0;
 		gd.pending = 0;
 		/* now seek to take the request spinlock
- 		 * before handling ending the request */
+		 * before handling ending the request */
 		spin_lock(&gdrom_lock);
 		list_del_init(&req->queuelist);
 		blk_requeue_request(gd.gdrom_rq, req);
@@ -567,7 +589,7 @@
 static void gdrom_request_handler_dma(struct request *req)
 {
 	/* dequeue, add to list of deferred work
- 	 * and then schedule workqueue */
+	 * and then schedule workqueue */
 	blkdev_dequeue_request(req);
 	list_add_tail(&req->queuelist, &gdrom_deferred);
 	schedule_work(&work);
@@ -582,7 +604,8 @@
 			end_request(req, 0);
 		}
 		if (rq_data_dir(req) != READ) {
-			printk(KERN_NOTICE "GDROM: Read only device - write request ignored\n");
+			printk(KERN_NOTICE "GDROM: Read only device - "
+			       "write request ignored\n");
 			end_request(req, 0);
 		}
 		if (req->nr_sectors)
@@ -612,7 +635,8 @@
 	firmw_ver = kstrndup(id->firmver, 16, GFP_KERNEL);
 	if (!firmw_ver)
 		goto free_manuf_name;
-	printk(KERN_INFO "GDROM: %s from %s with firmware %s\n", model_name, manuf_name, firmw_ver);
+	printk(KERN_INFO "GDROM: %s from %s with firmware %s\n",
+	       model_name, manuf_name, firmw_ver);
 	err = 0;
 	kfree(firmw_ver);
 free_manuf_name:
@@ -641,7 +665,8 @@
 	gd.cd_info->ops = &gdrom_ops;
 	gd.cd_info->capacity = 1;
 	strcpy(gd.cd_info->name, GDROM_DEV_NAME);
-	gd.cd_info->mask = CDC_CLOSE_TRAY|CDC_OPEN_TRAY|CDC_LOCK|CDC_SELECT_DISC;
+	gd.cd_info->mask = CDC_CLOSE_TRAY | CDC_OPEN_TRAY |
+			   CDC_LOCK | CDC_SELECT_DISC;
 }
 
 static void __devinit probe_gdrom_setupdisk(void)
@@ -671,7 +696,7 @@
 {
 	int err;
 	if (gdrom_execute_diagnostic() != 1) {
-		printk(KERN_WARNING "GDROM: ATA Probe for GDROM failed.\n");
+		printk(KERN_WARNING "GDROM: ATA Probe for GDROM failed\n");
 		return -ENODEV;
 	}
 	if (gdrom_outputversion())
@@ -679,7 +704,8 @@
 	gdrom_major = register_blkdev(0, GDROM_DEV_NAME);
 	if (gdrom_major <= 0)
 		return gdrom_major;
-	printk(KERN_INFO "GDROM: Block device is registered with major number %d\n", gdrom_major);
+	printk(KERN_INFO "GDROM: Block device is registered "
+	       "with major number %d\n", gdrom_major);
 	gd.cd_info = kzalloc(sizeof(struct cdrom_device_info), GFP_KERNEL);
 	if (!gd.cd_info) {
 		err = -ENOMEM;
@@ -724,7 +750,8 @@
 	unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
 	gdrom_major = 0;
 probe_fail_no_mem:
-	printk(KERN_WARNING "GDROM: Could not probe for device - error is 0x%X\n", err);
+	printk(KERN_WARNING "GDROM: Could not probe for device - "
+	       "error is 0x%X\n", err);
 	return err;
 }
 
 
 
 
-
Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Applied this over the last patch and I can confirm it works like a charm :)
Tested on the "sh-2.6" git kernel using two different GD-Rom drives. Both commercial gd-rom's and cd-r's can be mounted.

#12 lore raw
Quoted 2 lines · Gino Badouri

Applied this over the last patch and I can confirm it works like a charm :)

It's Adrian's patch that works well. Thanks Adrian.

#13 lore raw
In reply to Joe Perches
Quoted 10 lines · Joe Perches
Quoted 1 line · Adrian McMenamin

This patch adds support for the CD-Rom drive on the SEGA Dreamcast.

Because it was already so close, might as well make it checkpatch clean.

I also added a function gdrom_is_busy() to make a couple of tests fit on a single line and perhaps easier to read.

Not really for me to decide if this gets applied - I'll leave that to Paul as the maintainer. But I have no objection to them :)

#14 lore raw
Quoted 6 lines · Adrian McMenamin
Quoted 5 lines · Joe Perches
Quoted 1 line · Adrian McMenamin

This patch adds support for the CD-Rom drive on the SEGA Dreamcast.

Because it was already so close, might as well make it checkpatch clean.

Quoted 7 lines
Quoted 5 lines

I also added a function gdrom_is_busy() to make a couple of tests fit on a single line and perhaps easier to read.

Not really for me to decide if this gets applied - I'll leave that to Paul as the maintainer. But I have no objection to them :)

Both patches are fine with me, but it is ultimately Jens (as the drivers/cdrom authority) that will have to decide whether to sign off on the GDROM patches or not, and through which tree the patches will end up being merged. I can certainly merge them through my tree with his Acked-by if that's deemed the most convenient.