lkml.org 
[lkml]   [2006]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] riport LADAR driver
From
Date

since this is for a tutorial... double nitpick mode ;-)
(since examples should be squeeky clean or people will turn the right
thing into the not quite right thing later in their own code)

> +#undef PDEBUG
> +#ifdef RIPORT_DEBUG
> +# define PDEBUG(fmt, args...) printk( KERN_DEBUG "riport: " fmt, ## args)
> +#else /* */
> +# define PDEBUG(fmt, args...)
> +#endif /* */

this is still there while it really shouldn't be; either use pr_debug()
or dev_printk().


> +struct devriport {
> + unsigned int io;
> + unsigned int io_ext;
> + int irq;
> + int dma;
> + int size; /* buffer size */
> + unsigned char *pbuf; /* pointer to the start of the memory that
> + stores scans from the riegl */
> + unsigned char *pbuf_top;/* pointer to the end of pbuf (see above) */
> + unsigned char *pin; /* pointer to the end of new data */
> + unsigned char *pout; /* pointer to the start of new data (end of
> + old/read data) */
> + wait_queue_head_t qwait;
> + struct inode *pinode;
> + struct file *pfile;
> + int usage;
> + int irqinuse;
> + int readstate;
> + short syncWord;
> + int numbytesthisstate;
> + int bytestoread;
> + char buf[4];
> + struct timeval timestamp;
> +
> + spinlock_t lock;
> +};

if this is for a tutorial.. might as well sort these fields in order of
decreasing size so that you get minimal alignment packing by the
compiler

> + if (!request_region(io + ECP_OFFSET, 3, "riport")) {
> + release_region(io,3);
> +
> + PDEBUG("request_region 0x%X of 3 bytes fails\n", io + ECP_OFFSET );
> + *presult = -EBUSY;
> + goto fail_io;
> + }

might as well make another goto target and have that do the
release_region...

> +
> +static int devriport_release(struct devriport *this)
> +{
> + this->irqinuse = 0;
> +
> + /* switch to compatibility mode */
> + outb(ECR_SPP_MODE | ECR_ERRINT_DISABLED | ECR_SERVICE_INTERRUPT,
> + this->io_ext + ECR_EXT);
> + outb(DCR_NOT_REVERSE_REQUEST | DCR_SELECT_IN, this->io + DCR_BASE);
> +
> + free_irq(this->irq, this);
> + kfree(this->pbuf);
> +
> + this->usage--;
> + WARN_ON(this->usage < 0);
> + PDEBUG("release\n");
> + return 0;
> +}
> +
> +
...

> +
> +
> +static int devriport_open(struct devriport *this)
> +{
> + int result;
> +
> + if (this->usage)
> + return -EBUSY;

this "usage count" thing is probably buggy and racy; what is it for?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-06-24 13:02    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site