Re: [PATCH v2 8/9] DMA: shdma: initial of common code

[Arnd Bergmann <arnd@arndb.de>]

On Monday 14 April 2014 22:35:11 Ben Dooks wrote:
> Add support for building shdma internal data from the device tree to allow
> converting the driver to be device tree enabled.
> 
> It includes a helper for the of case to build the internal data used to
> select and filter out the DMA channels from the ID information in the
> device tree. Also updates the documentation for the DT case.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

I think you should try to explain better here why you need all the logic
that other drivers don't for the DT case. 

I've tried to understand what you are doing with shdma_of_client and
couldn't quite figure it out. The driver is already different from
all the others, because it still uses the 'slave_id' field in
dma_slave_config (only for the non-DT case) that all other drivers
don't use at all.

My guess is that if you manage to clean that up first, all of this
wouldn't be necessary.

>  static struct dma_chan *shdma_of_xlate(struct of_phandle_args *dma_spec,
>  				       struct of_dma *ofdma)
>  {
> +	struct platform_device *pdev = ofdma->of_dma_data;
>  	u32 id = dma_spec->args[0];
>  	dma_cap_mask_t mask;
>  	struct dma_chan *chan;
>  
> -	if (dma_spec->args_count != 1)
> +	if (dma_spec->args_count == 2) {
> +		struct shdma_of_state *state = platform_get_drvdata(pdev);
> +		struct shdma_of_client *client;
> +
> +		client = shdma_of_find_client(state, id);
> +		if (!client) {
> +			client = devm_kzalloc(&pdev->dev, sizeof(*client),
> +					      GFP_KERNEL);
> +			if (!client)
> +				return NULL;
> +
> +			client->index = atomic_inc_return(&of_slave_index);
> +			client->cfg.slave_id = id;
> +			client->cfg.mid_rid = id;
> +			client->cfg.chcr = dma_spec->args[1];

Can you explain the purpose of setting the chcr in DT? For all I can
tell, this should come from the device driver when that calls the
dma_slave_config function, unlike the mid_rid value.

	Arnd