Re: [PATCH - SH/Dreamcast] CD-Rom (GD-Rom) support for the SEGA Dreamcast

[Paul Mundt <lethal@linux-sh.org>]

On Sun, Dec 23, 2007 at 07:43:24PM +0000, Adrian McMenamin wrote:
> This patch adds support for the CD-Rom (the so-called "GD-Rom") on the
> SEGA Dreamcast.
> 
Looks quite a bit better. Just a few minor things.

> +static void gdrom_spicommand(void *spi_string, int buflen)
> +{
> +	short *cmd = spi_string;
> +	/* ensure IRQ_WAIT is set */
> +	ctrl_outb(0x08, GDROM_ALTSTATUS_REG);
> +	/* specify how many bytes we expect back */
> +	ctrl_outb(buflen & 0xFF, GDROM_BCL_REG);
> +	ctrl_outb((buflen >> 8) & 0xFF, GDROM_BCH_REG);
> +	/* other parameters */
> +	ctrl_outb(0, GDROM_INTSEC_REG);
> +	ctrl_outb(0, GDROM_SECNUM_REG);
> +	ctrl_outb(0, GDROM_ERROR_REG);
> +	/* Wait until we can go */
> +	gdrom_wait_clrbusy();
> +	ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
> +	while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8)
> +		; /* wait for DRQ to be set to 1 */

cpu_relax()

> +static int gdrom_get_last_session(struct cdrom_device_info *cd_info,
> struct cdrom_multisession *ms_info)
> +{
> +	struct gdromtoc *tocA, *tocB;
> +	int fentry, lentry, track, data, tocuse;
> +	int err = -ENOMEM;
> +	tocA = kzalloc(sizeof(struct gdromtoc), GFP_KERNEL);
> +	if (!tocA)
> +		goto exit;	
> +	tocB = kzalloc(sizeof(struct gdromtoc), GFP_KERNEL);
> +	if (!tocB)
> +		goto clean_tocA;
> +	tocuse = 1;
> +	err = gdrom_readtoc_cmd(tocB, 1);
> +	if (err) {
> +		tocuse = 0;
> +		err = gdrom_readtoc_cmd(tocA, 0);
> +		if (err) {
> +			err = -ENXIO;
> +			printk(KERN_INFO "Could not get CD table of contents\n");
> +			goto clean_tocB;
> +		}	
> +	} else
> +		printk(KERN_DEBUG "Disk is GDROM\n");
> +
> +	kfree(gd.toc);
> +
> +	if (tocuse) {
> +		gd.toc = tocB;
> +		kfree(tocA);
> +	} else {
> +		gd.toc = tocA;
> +		kfree(tocB);
> +	}

tocA and tocB seem to be useless, as you're not using them for anything
but gd.toc assignment. If you're catching errors from gdrom_readtoc_cmd()
directly, then just pass in gd.toc in both cases, then you can skip all
of this temporary state stuff entirely.

> +	sense_key = sense[1] & 0x0F;
> +	switch (sense_key){
> +	case 0xB:
> +		printk(KERN_INFO "GDROM: Command aborted\n");
> +		break;
> +	case 0x7:
> +		printk(KERN_INFO "GDROM: Data protection error\n");
> +		break;
> +	case 0x6:
> +		printk(KERN_NOTICE "GDROM: Unit needs attention - possible disk switch\n");
> +		break;
> +	case 0x5:
> +		printk(KERN_INFO "GDROM: Illegal request - command has failed\n");
> +		break;
> +	case 0x4:
> +		printk(KERN_CRIT "GDROM: Hardware error\n");
> +		break;
> +	case 0x3:
> +		printk(KERN_WARNING "GDROM: Disk read error\n");
> +		break;
> +	case 0x2:
> +		printk(KERN_INFO "GDROM: Not ready\n");
> +		break;
> +	case 0x1:
> +		printk(KERN_DEBUG "GDROM: Recovered from error\n");
> +		break;
> +	case 0x0:
> +		/*success - stay silent*/
> +		break;
> +		
> +	default:
> +		printk(KERN_INFO "GDROM: Unknown error\n");
> +	}

These are all pretty close together, maybe it would be cleaner to throw
these strings in to an array and just use the sense_key as the array
index. Some other drivers do this already.