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:
signature.asc
Description: This is a digitally signed message part.
- References:
- [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
- From: "Adrian McMenamin" <[email protected]>
- [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
- Prev by Date: [PATCH] net/ipv4/netfilter/ip_tables.c: remove some inlines
- Next by Date: Re: [PATCH] [RFC] be more verbose when probing EDD
- Previous by thread: Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
- Next by thread: Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
- Index(es):
 |