Messages in this thread | | | Subject | Re: [PATCH] riport LADAR driver | From | Arjan van de Ven <> | Date | Sat, 24 Jun 2006 13:00:20 +0200 |
| |
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/
| |