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    [W:0.027 / U:29.512 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site