RE: [PATCH] riport LADAR driver

[Gross, Mark]

>-----Original Message-----
>From: Arjan van de Ven [mailto:arjan@xxxxxxxxxxxxx]
>Sent: Saturday, June 24, 2006 4:00 AM
>To: mgross@xxxxxxxxxxxxxxx
>Cc: Randy.Dunlap; linux-kernel@xxxxxxxxxxxxxxx; Gross, Mark
>Subject: Re: [PATCH] riport LADAR driver
>
>
>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)

Way cool. Thanks! I'll update the patch and respond to your issues later
today.


>
>> +#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().
>

OK.  I had a dumb reason in my head for not making this change.  (I had
it in my head the DEBUG define was more global than it was...)

>
>> +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

Ok

>
>> +	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...

OK,

>
>> +
>> +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?

I'll look at this more closely.  I bet we can loose it.

Thanks again,

The patch will come later this weekend.

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