From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Holm =?ISO-8859-1?Q?Th=F8gersen?= Date: Fri, 28 Dec 2007 00:18:57 +0000 Subject: Re: [PATCH] SH/Dreamcast - add support for GD-Rom device Message-Id: <1198801142.5354.67.camel@odie> List-Id: References: <1198774339.6170.11.camel@localhost.localdomain> <1198796281.4833.31.camel@localhost> In-Reply-To: <1198796281.4833.31.camel@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Joe Perches Cc: Adrian McMenamin , Paul Mundt , Jens Axboe , LKML , linux-sh tor, 27 12 2007 kl. 14:58 -0800, skrev Joe Perches: > On Thu, 2007-12-27 at 16:52 +0000, Adrian McMenamin wrote: > > This patch adds support for the CD-Rom drive on the SEGA Dreamcast. >=20 > Because it was already so close, might as well make it checkpatch clean. >=20 > I also added a function gdrom_is_busy() to make a couple of tests > fit on a single line and perhaps easier to read. >=20 > Signed-off-by: Joe Perches >=20 > --- a/drivers/cdrom/gdrom.c 2007-12-27 14:49:27.000000000 -0800 > +++ b/drivers/cdrom/gdrom.c 2007-12-27 14:46:20.000000000 -0800 > @@ -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 ch= anged"}, > + {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 @@ > }; > =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); > =20 > +static bool gdrom_is_busy(void) > +{ > + return (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) !=3D 0; > +} > + > static void gdrom_wait_clrbusy(void) > { > /* long timeouts - typical for a CD Rom */ > unsigned long timeout =3D 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. > cpu_relax(); > } > =20 > @@ -146,7 +153,7 @@ > unsigned long timeout; > /* Wait to get busy first */ > timeout =3D jiffies + HZ * 60; > - while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) =3D 0) && (time_before(j= iffies, timeout))) > + while (!gdrom_is_busy() && (time_before(jiffies, timeout))) Same here. > cpu_relax(); > /* Now wait for busy to clear */ > gdrom_wait_clrbusy(); > @@ -216,7 +223,8 @@ > gd.pending =3D 1; > gdrom_packetcommand(gd.cd_info, spin_command); > /* 60 second timeout */ > - wait_event_interruptible_timeout(command_queue, gd.pending =3D 0, HZ * = 60); > + wait_event_interruptible_timeout(command_queue, > + gd.pending =3D 0, HZ * 60); > gd.pending =3D 0; All three users of gdrom_packetcommand do the same timeout, so consider moving this block into gdrom_packcommand. > kfree(spin_command); > if (gd.status & 0x01) { > @@ -249,7 +257,8 @@ > toc_command->buflen =3D tocsize; > gd.pending =3D 1; > gdrom_packetcommand(gd.cd_info, toc_command); > - wait_event_interruptible_timeout(command_queue, gd.pending =3D 0, HZ * = 60); > + wait_event_interruptible_timeout(command_queue, > + gd.pending =3D 0, HZ * 60); > gd.pending =3D 0; > insw(PHYSADDR(GDROM_DATA_REG), toc, tocsize/2); > kfree(toc_command); > @@ -274,7 +283,8 @@ > return (track & 0x0000ff00) >> 8; > } > =20 > -static int gdrom_get_last_session(struct cdrom_device_info *cd_info, str= uct 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 =3D 0; > err =3D 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 @@ > =20 > 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; > } > =20 > @@ -355,8 +367,10 @@ > return 0; > } > =20 > -/* keep the function looking like the universal CD Rom specification - r= eturning 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 =3D 1; > gdrom_packetcommand(gd.cd_info, sense_command); > /* 60 second timeout */ > - wait_event_interruptible_timeout(command_queue, gd.pending =3D 0, HZ * = 60); > + wait_event_interruptible_timeout(command_queue, > + gd.pending =3D 0, HZ * 60); > gd.pending =3D 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); > =20 > 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? > =20 > if (sense_key < 2) > return 0; > @@ -434,7 +450,8 @@ > return cdrom_media_changed(gd.cd_info); > } > =20 > -static int gdrom_bdops_ioctl(struct inode *inode, struct file *file, uns= igned 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 =3D request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt, IRQF_D= ISABLED, "gdrom_command", &gd); > + err =3D request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt, > + IRQF_DISABLED, "gdrom_command", &gd); > if (err) > return err; > init_waitqueue_head(&request_queue); > - err =3D request_irq(HW_EVENT_GDROM_DMA, gdrom_dma_interrupt, IRQF_DISAB= LED, "gdrom_dma", &gd); > + err =3D 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 =3D 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. > ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG); > timeout =3D jiffies + HZ / 2; > - while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) !=3D 8) && (time_before= (jiffies, timeout))) > + while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) !=3D 8) && > + (time_before(jiffies, timeout))) What about a nice telling function like gdrom_is_busy for this? > cpu_relax(); /* wait for DRQ to be set to 1 */ > gd.pending =3D 1; > gd.transfer =3D 1; > outsw(PHYSADDR(GDROM_DATA_REG), &read_command->cmd, 6); > timeout =3D jiffies + HZ / 2; > - while ((ctrl_inb(GDROM_DMA_STATUS_REG)) && (time_before(jiffies, timeo= ut))) > + 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. > 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 =3D 0, HZ = * 5); > + wait_event_interruptible_timeout(request_queue, > + gd.transfer =3D 0, HZ * 5); > err =3D ctrl_inb(GDROM_DMA_STATUS_REG); > err =3D gd.transfer; > gd.transfer =3D 0; > gd.pending =3D 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) !=3D 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 =3D 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, ma= nuf_name, firmw_ver); > + printk(KERN_INFO "GDROM: %s from %s with firmware %s\n", > + model_name, manuf_name, firmw_ver); > err =3D 0; > kfree(firmw_ver); > free_manuf_name: > @@ -641,7 +665,8 @@ > gd.cd_info->ops =3D &gdrom_ops; > gd.cd_info->capacity =3D 1; > strcpy(gd.cd_info->name, GDROM_DEV_NAME); > - gd.cd_info->mask =3D CDC_CLOSE_TRAY|CDC_OPEN_TRAY|CDC_LOCK|CDC_SELECT_D= ISC; > + gd.cd_info->mask =3D CDC_CLOSE_TRAY | CDC_OPEN_TRAY | > + CDC_LOCK | CDC_SELECT_DISC; > } > =20 > static void __devinit probe_gdrom_setupdisk(void) > @@ -671,7 +696,7 @@ > { > int err; > if (gdrom_execute_diagnostic() !=3D 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 =3D register_blkdev(0, GDROM_DEV_NAME); > if (gdrom_major <=3D 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 =3D kzalloc(sizeof(struct cdrom_device_info), GFP_KERNEL); > if (!gd.cd_info) { > err =3D -ENOMEM; > @@ -724,7 +750,8 @@ > unregister_blkdev(gdrom_major, GDROM_DEV_NAME); > gdrom_major =3D 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=C3=B8gersen