Re: [PATCH 2.6.11-rc3 1/1] altix: Device driver support for the CX port of SGI's TIO chip

Christoph Hellwig
2005-03-02T04:47:25Z

(care to split this into a two-patch series for the next submission,
one for tiocx, one for mbcs?)

> +obj-$(CONFIG_SGI_TIOCX) += tiocx.o

this one should really be in arch/ia64/sn/

> +#endif
> +int mbcs_major = 0;
> +
> +extern struct bus_type tiocx_bus_type;

externs in .c files are a big no-go in Linux. But you
shouldn't need this variable anyway.

> +static void getDma_init(struct getDma_soft_s *pGetDma)

please avoid oddly-Caps names and _s prefix for structures,
should more look like

static void mbcs_getdma_init(struct getdma *gd)

> +{
> + /* setup engine parameters */
> + pGetDma->hostAddr = 0;
> + pGetDma->localAddr = 0;
> + pGetDma->bytes = 0;
> + pGetDma->DoneAmoEnable = 0;
> + pGetDma->DoneIntEnable = 1;
> + pGetDma->peerIO = 0;
> + pGetDma->amoHostDest = 0;
> + pGetDma->amoModType = 0;
> + pGetDma->intrHostDest = 0;
> + pGetDma->intrVector = 0;

why not memset the whole structure?

> +static ssize_t
> +do_mbcs_sram_dmaread(struct mbcs_soft_s *soft, uint64_t hostAddr,
> + size_t len, loff_t * off)
> +{
> + int rv = 0;
> +
> + soft->getDma.hostAddr = hostAddr;
> + soft->getDma.localAddr = *off;
> + soft->getDma.bytes = len;
> +
> + if (getDma_start(soft) < 0) {
> + DBG(KERN_ALERT "mbcs_strategy: getDma_start failed\n");
> + return -EAGAIN;
> + }
> +
> + interruptible_sleep_on(&soft->dmaread_queue);

never use sleep_on or its variants. Use wait_event instead with a proper
condition to wait for. (Also in various other places in the driver)

> +{
> + struct mbcs_callback_arg cba;
> +
> + if (ip = NULL || fp = NULL)
> + return -EINVAL;

can't ever happen.

>
> +
> + cba.minor = iminor(ip);
> +
> + cba.cx_dev = NULL;
> + bus_for_each_dev(&tiocx_bus_type, NULL, &cba, mbcs_find_device);

urgg, no. Please keep a local doubly-linked list of devices you
got ->porbe called for and search that one.

> +static int mbcs_probe(struct cx_dev *dev, const struct cx_device_id *id)
> +{
> + struct mbcs_soft_s *soft;
> +
> + dev->soft = NULL;
> +
> + soft = (struct mbcs_soft_s *)kcalloc(1,
> + sizeof(struct mbcs_soft_s),
> + GFP_KERNEL);

no need to cast the kcalloc return value

the whole mbcs driver looks rather odd, what is it trying to do?