Re: [PATCH] riport LADAR driver

[Arjan van de Ven <arjan@infradead.org>]

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/