Re: [PATCH V4 1/2] of: Add generic device tree DMA helpers

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 13 September 2012, Jon Hunter wrote:
> This is based upon the work by Benoit Cousson [1] and Nicolas Ferre [2]
> to add some basic helpers to retrieve a DMA controller device_node and the
> DMA request/channel information.

I think we're getting very close now, I only have a few small comments left
that should all be uncontroversial.

> +
> +Client drivers should specify the DMA property using a phandle to the controller
> +followed by DMA controller specific data.
> +
> +Required property:
> +- dmas:			List of one or more DMA specifiers, each consisting of
> +			- A phandle pointing to DMA controller node
> +			- A single integer cell containing DMA controller
> +			  specific information. This typically contains a dma
> +			  request line number or a channel number, but can
> +			  contain any data that is used required for configuring
> +			  a channel.
> +- dma-names: 		Contains one identifier string for each dma specifier in
> +			the dmas property. The specific strings that can be used
> +			are defined in the binding of the DMA client device.

I think here we need to clarify that listing the same name multiple times implies
having multiple alternatives for the same channel.

> +Example:
> +
> +- One DMA write channel, one DMA read/write channel:
> +
> +	i2c1: i2c@1 {
> +		...
> +		dmas = <&sdma 2		/* read channel */
> +			&sdma 3>;	/* write channel */
> +		dma-names = "rx", "tx"
> +		...
> +	};

And please add an example documenting this case.

> +/**
> + * of_dma_find_channel - Find a DMA channel by name
> + * @np:		device node to look for DMA channels
> + * @name:	name of desired channel
> + * @dma_spec:	pointer to DMA specifier as found in the device tree
> + *
> + * Find a DMA channel by the name. Returns 0 on success or appropriate
> + * errno value on error.
> + */
> +static int of_dma_find_channel(struct device_node *np, char *name,
> +			       struct of_phandle_args *dma_spec)
> +{
> +	int count, i;
> +	const char *s;
> +
> +	count = of_property_count_strings(np, "dma-names");
> +	if (count < 0)
> +		return count;
> +
> +	for (i = 0; i < count; i++) {
> +		of_property_read_string_index(np, "dma-names", i, &s);
> +
> +		if (strcmp(name, s))
> +			continue;
> +
> +		return of_parse_phandle_with_args(np, "dmas", "#dma-cells",
> +							i, dma_spec);
> +	}
> +
> +	return -ENODEV;
> +}

I think we should at least keep trying the other channels with the same name when 
of_parse_phandle_with_args fails. We might want to do something smarter in
the long run, e.g. to spread channel allocations across the avaialable controllers.

> +/**
> + * of_dma_request_slave_channel - Get the DMA slave channel
> + * @np:		device node to get DMA request from
> + * @name:	name of desired channel
> + *
> + * Returns pointer to appropriate dma channel on success or NULL on error.
> + */
> +struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
> +					      char *name)
> +{
> ...
> +}
> +EXPORT_SYMBOL_GPL(of_dma_request_slave_channel);

I think this no longer needs to be exported, with patch 2 on top.

> +/**
> + * of_dma_simple_xlate - Simple DMA engine translation function
> + * @dma_spec:	pointer to DMA specifier as found in the device tree
> + * @of_dma:	pointer to DMA controller data
> + *
> + * A simple translation function for devices that use a 32-bit value for the
> + * filter_param when calling the DMA engine dma_request_channel() function.
> + * Note that this translation function requires that #dma-cells is equal to 1
> + * and the argument of the dma specifier is the 32-bit filter_param. Returns
> + * pointer to appropriate dma channel on success or NULL on error.
> + */
> +struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
> +						struct of_dma *ofdma)
> +{
> +	int count = dma_spec->args_count;
> +	struct of_dma_filter_info *info = ofdma->of_dma_data;
> +
> +	if (!info || !info->filter_fn)
> +		return NULL;
> +
> +	if (count != 1)
> +		return NULL;
> +
> +	return dma_request_channel(info->dma_cap, info->filter_fn,
> +			&dma_spec->args[0]);
> +}

But this one does.

	Arnd