[PATCH V2] ARM: NUC900: add GDMA driver support for nuc900 platform

[Russell King - ARM Linux]

On Wed, Dec 09, 2009 at 11:37:50PM +0800, Hu Ruihuan wrote:
> +static void match_dmachan_fromname(const char *dev_name,
> +					struct nuc900_dma  **dma)
> +{
> +	BUG_ON(!dev_name);
> +
> +	if (dmachan1->device) {
> +		if (strcmp(dmachan1->device, dev_name) == 0)
> +			*dma = dmachan1;
> +	} else if (dmachan0->device) {
> +		if (strcmp(dmachan0->device, dev_name) == 0)
> +			*dma = dmachan0;
> +	}
> +}

Never write functions like this without explaination of why you're doing
it this way: having a void function returning something through a
double pointer is just silly, and leads to bugs (when someone forgets
to NULL-initialize before-hand.)

static struct nuc900_dma *match_dmachan_fromname(const char *dev_name)
{
	BUG_ON(!dev_name);

	if (dmachan1->device) {
		if (strcmp(dmachan1->device, dev_name) == 0)
			return dmachan1;
	} else if (dmachan0->device) {
		if (strcmp(dmachan0->device, dev_name) == 0)
			return dmachan0;
	}
	return NULL;
}

This way, whenever you do:

	dmachan = match_dmachan_fromname(foo);

you know that dmachan will always be initialized, whereas:

	match_dmachan_fromname(foo, &dmachan);

there's little to tell the compiler that dmachan will be initialized -
in fact, the compiler _can't_ tell you whether you're possibly using it
without it having been initialized.

> +int nuc900_request_dma(const char *dev_name, dma_callback_t callback,
> +								void *data)
> +{
> +	struct nuc900_dma  *dma = NULL;
> +	int err, irq_request;
> +	err = 0;
> +	irq_request = 0;
> +	spin_lock(&dma_chan_lock);
> +	dma = register_dmachan_fromname(dev_name, &irq_request);

It seems that the irq_request handling is now unnecessary, please remove.

> +	if (IS_ERR(dma)) {
> +		err = PTR_ERR(dma);
> +		spin_unlock(&dma_chan_lock);
> +		return err;
> +	}
> +	dma->device = dev_name;
> +	atomic_inc(&shared_irq_using);

shared_irq_using is not required anymore.

> +	dma->callback = callback;
> +	dma->data = data;
> +	spin_unlock(&dma_chan_lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL(nuc900_request_dma);
> +
> +int nuc900_free_dma(const char *dev_name)
> +{
> +	struct nuc900_dma  *dma = NULL;
> +
> +	BUG_ON(!dev_name);
> +	WARN_ON(shared_irq_using.counter == 0);

This is buggy, never go inside an 'atomic' object - use the accessor
functions.  In any case, shared_irq_using is not required so this can
be deleted.

> +	spin_lock(&dma_chan_lock);
> +	match_dmachan_fromname(dev_name, &dma);
> +
> +	if (!dma)
> +		return -ENXIO;
> +
> +	dma->device = NULL;
> +	spin_unlock(&dma_chan_lock);
> +
> +	atomic_dec(&shared_irq_using);

Can be removed.

> +
> +	kzfree(dma->descp);
> +	return 0;
> +}
> +EXPORT_SYMBOL(nuc900_free_dma);
> +
> +static unsigned int set_start_cmd(unsigned int start)
> +{
> +	unsigned int dscp_cmd;
> +
> +	dscp_cmd = 0;
> +
> +	if (start) {
> +		dscp_cmd = (ORDEN|RUN);
> +		dscp_cmd &= ~(NON_DSCRIP|RESET);
> +	} else {
> +		dscp_cmd = (NON_DSCRIP);
> +	}
> +		dscp_cmd &= DSCRIPMASK;

Weird formatting - is this correctly placed?  It looks like it should
be within the else { } clause.

> +
> +	return dscp_cmd;
> +}