On Thu, Aug 10, 2006 at 11:18:04PM +0200, Thomas Koeller wrote:
> This is a driver used for image capturing by the Basler eXcite smart camera
> platform. It utilizes the integrated GPI DMA engine of the MIPS RM9122
> processor. Since this driver does not fit into one of the existing categories
> I created a new toplevel directory for it (which may not be appropriate?).
Hi Thomas.
As others have pointed out, drivers/media/video is probably a better home.
Some speedy mostly-nitpicking comments below. I didn't give it an indepth review,
but this is stuff that jumped out at me from a quick skim.
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#include <linux/config.h>
Unnecessary include (kbuild does this for you now)
> +static unsigned long devnum_bitmap = 0;
Unneeded initialisation. (static uninitialised vars go in .bss)
> +/* Function prototypes */
> +static void xicap_device_release(struct class_device *);
> +static long xicap_ioctl(struct file *, unsigned int, unsigned long);
> +static unsigned int xicap_poll(struct file *, poll_table *);
> +static ssize_t xicap_read(struct file *, char __user *, size_t, loff_t *);
> +static int xicap_open(struct inode *, struct file *);
> +static int xicap_release(struct inode *, struct file *);
> +static int xicap_queue_buffer(xicap_device_context_t *,
> + const xicap_arg_qbuf_t *);
You could lose all these forward declarations if you move
the xicap_fops after the function declarations.
> +/* A class for xicap devices */
> +static struct class xicap_class = {
> + .name = (char *) xicap_name,
Is that cast necessary ?
> +/* Device registration */
> +xicap_device_context_t *
> +xicap_device_register(struct device *dev, const xicap_hw_driver_t *hwdrv)
The typedef had me dancing around trying to find out what it was a few
times. Can we just replace it with uses of struct xicap_devctx ?
Ditto for xicap_frame_context_t
> + /* Set up a device context */
> + xicap_device_context_t * const dc =
> + (xicap_device_context_t *) kmalloc(sizeof *dc, GFP_KERNEL);
> + if (!dc) {
> + res = -ENOMEM;
> + goto ex;
> + }
> +
> + memset(dc, 0, sizeof *dc);
You could lose the memset, and use kzalloc instead.
> +MODULE_VERSION("0.0");
Heh, early days ? :-)
> +++ b/drivers/xicap/xicap_gpi.c
> ...
> +
> +#include <linux/config.h>
Same as above. Unneeded.
> +#define VMAP_WORKAROUND 1
This needs a comment to explain what its doing.
> +/*
> + * I/O register access macros
> + * Do not use __raw_writeq() and __raw_readq(), these do not seem to work!
> + */
> +#define io_writeq(__v__, __a__) \
> + *(volatile unsigned long long *) (__a__) = (__v__)
> +#define io_readq(__a__) (*(volatile unsigned long long *) (__a__))
> +#define io_readl(__a__) __raw_readl((__a__))
> +#define io_writel(__v__, __a__) __raw_writel((__v__), (__a__))
> +#define io_readb(__a__) __raw_readb((__a__))
> +#define io_writeb(__v__, __a__) __raw_writeb((__v__), (__a__))
If they don't work, it'd be nice to get them fixed instead of reinventing new ones.
> + /* Create and set up the device context */
> + dc = (xicap_gpi_device_context_t *)
> + kmalloc(sizeof (xicap_gpi_device_context_t), GFP_KERNEL);
> + if (!dc) {
> + res = -ENOMEM;
> + goto errex;
> + }
> + memset(dc, 0, sizeof *dc);
kzalloc.
> + rsrc = xicap_gpi_get_resource(pdv, 0, rsrcname_gpi_slice);
> + if (unlikely(!rsrc)) goto errex;
if (unlikely(!rsrc))
goto errex;
> + if (unlikely(!rsrc)) goto errex;
if (unlikely(!rsrc))
goto errex;
> + if (unlikely(!rsrc)) goto errex;
if (unlikely(!rsrc))
goto errex;
> + if (unlikely(!rsrc)) goto errex;
etc.
> + if (res) {
> + if (dc->regaddr_fifo_rx) iounmap(dc->regaddr_fifo_rx);
> + if (dc->regaddr_fifo_tx) iounmap(dc->regaddr_fifo_tx);
> + if (dc->regaddr_xdma) iounmap(dc->regaddr_xdma);
> + if (dc->regaddr_pktproc) iounmap(dc->regaddr_pktproc);
> + if (dc->regaddr_fpga) iounmap(dc->regaddr_fpga);
> + if (dc->dmadesc) iounmap(dc->dmadesc);
> + if (dc) kfree(dc);
etc
> + /* Set up the XDMA descriptor ring & enable the XDMA */
> + dc->curdesc = dc->dmadesc;
> + atomic_set(&dc->desc_cnt, XDMA_DESC_RING_SIZE);
> + io_writel(dc->dmadesc_p, dc->regaddr_xdma + 0x0018);
> + wmb();
Uncommented wmb's are a sin :)
This one may actually need to be a io_readl if its just to flush
the previous io_writel ?
> + /*
> + * Enable the rx fifo we are going to use. Disable the
> + * unused ones as well as the tx fifo.
> + */
> + io_writel(0x00100000 | ((dc->fifomem_size) << 10)
> + | dc->fifomem_start,
> + dc->regaddr_fifo_rx + 0x0000);
> + wmb();
same again.
> + titan_writel(0xf << (dc->slice * 4), 0x482c);
> + wmb();
and again for a whole bunch more writel's, which really make me wonder...
Asides from all these points, the only thing that really makes me nervous
is the amount of access_ok & __copy_*_user()/memcpy() uses we have rather than
just doing a copy_*_user. It's one of those "are we sure we've checked everything"
paranoia's I have..
Dave
--
http://www.codemonkey.org.uk
