Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 3512 bytes --]

On Saturday 15 December 2007, Adrian McMenamin wrote:
> diff -ruN ./linux-2.6-orig/drivers/sh/gdrom/gdrom.c
> ./linux-2.6/drivers/sh/gdrom/gdrom.c

i think your e-mail client word wrapped a little ...

> +	if (gd.toc)
> +		kfree(gd.toc);

i dont know how the kernel functions, but in userspace, free(NULL) is 
acceptable ...

> +		memcpy(gd.toc, tocB, sizeof (struct gdromtoc));
> +	else
> +		memcpy(gd.toc, tocA, sizeof (struct gdromtoc));

since gd.toc and toc[BA] are of the same type, cant you just:
*gd.toc = *tocA

also, since tocB/tocA only exist in this function (you kzalloc() at the top 
and kfree() at the bottom), and you dont do something like "gd.toc = tocA", 
why use the memory allocator at all ?  i dont think they are too large for 
the stack (~400bytes a piece) ... maybe they are ...

> +static int gdrom_open(struct cdrom_device_info *cd_info, int purpose)
> +{
> +	int err;
> +	/* spin up the disk */
> +	err = gdrom_preparedisk_cmd();
> +	if (err)
> +		return -EIO;
> +
> +	return 0;
> +}

is it normal to normalize all errors like this ?  it'd be a much simpler 
function like:
{ return gdrom_preparedisk_cmd(); }

> +static irqreturn_t gdrom_command_interrupt(int irq, void *dev_id)
> +{
> +	if (dev_id != &gd)
> +		return IRQ_NONE;
> +	gd.status = ctrl_inb(GDROM_STATUSCOMMAND_REG);
> +	if (gd.pending != 1)
> +		return IRQ_HANDLED;
> ....
> +static irqreturn_t gdrom_dma_interrupt(int irq, void *dev_id)
> +{
> +	if (dev_id != &gd)
> +		return IRQ_NONE;
> +	gd.status = ctrl_inb(GDROM_STATUSCOMMAND_REG);
> +	if (gd.transfer != 1)
> +		return IRQ_HANDLED;

if you dont have a pending interrupt, shouldnt it return IRQ_NONE here ?  Paul 
already mentioned the weird dev_id check.

> +static int gdrom_readdisk_dma(int block, int block_cnt, char *buffer)
> +{
> +	int err;
> +	struct packet_command *read_command;
> +	/* release the spin lock but check later
> + 	 * we're not in the middle of some dma */
> +	spin_unlock(&gdrom_lock);
> +	ctrl_outl(0x8843407F, GDROM_DMA_ACCESS_CTRL_REG); /* memory setting */

it'd be nice if these magic #'s had more explanation behind them, but you may 
simply not have that information :/

> +static void gdrom_request(struct request_queue *rq)
> +{
> +	struct request *req;
> +	unsigned long pages;
> +	pages = rq->backing_dev_info.ra_pages;
> +	while ((req = elv_next_request(rq)) != NULL) {
> +		if (! blk_fs_request(req)) {
> +			printk(KERN_DEBUG "GDROM: Non-fs request ignored\n");
> +			end_request(req, 0);
> +		}
> +		if (rq_data_dir(req)) {
> +			printk(KERN_NOTICE "GDROM: Read only device - write request
> ignored\n"); +			end_request(req, 0);
> +		}
> +		if (req->nr_sectors) {
> +			gdrom_request_handler_dma(req);
> +		}
> +	}
> +}

no need for all the {} in the last two if()'s

> +/* Print string identifying GD ROM device */
> +static void gdrom_outputversion(void)
> +{
> +	struct gdrom_id *id;
> +	char *model_name, *manuf_name, *firmw_ver;
> +	/* query device ID */
> +	id = kzalloc(sizeof(struct gdrom_id), GFP_KERNEL);

i dont know how other people feel, but i think the style:
sizeof(*id)
is cleaner and leads to less bitrot ... also, you dont have an "if (!id)" 
check there ... Paul pointed out plenty of other stuff for this func ;)

also, wrt to sizes ("16" and "17"), arent there some defines you can key off 
of or something ?

> +MODULE_DESCRIPTION("GD-ROM Driver");

SEGA Dreamcast GD-ROM Driver ?
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]