Re: [PATCH v4 1/2] dmaengine: at_hdmac: platform data move to use.id_table

[Grant Likely]

On Mon, Oct 17, 2011 at 02:56:40PM +0200, Nicolas Ferre wrote:
> We remove the use of platform data from DMA controller driver.
> We now use of .id_table to distinguish between compatible
> types. The two implementations allow to determine the
> number of channels and the capabilities of the controller.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxx>
> Acked-by: Grant Likely <grant.likely@xxxxxxxxxxxx>
> ---
>  drivers/dma/at_hdmac.c      |   48 ++++++++++++++++++++++++++++++++++---------
>  drivers/dma/at_hdmac_regs.h |    8 +++++++
>  2 files changed, 46 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index fcfa0a8..d1869c5 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -1175,6 +1175,18 @@ static void atc_free_chan_resources(struct dma_chan *chan)
>  
>  /*--  Module Management  -----------------------------------------------*/
>  
> +static struct platform_device_id atdma_devtypes[] = {
> +	{
> +		.name = "at91sam9rl_dma",
> +		.driver_data = ATDMA_DEVTYPE_SAM9RL,
> +	}, {
> +		.name = "at91sam9g45_dma",
> +		.driver_data = ATDMA_DEVTYPE_SAM9G45,
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +
>  /**
>   * at_dma_off - disable DMA controller
>   * @atdma: the Atmel HDAMC device
> @@ -1193,18 +1205,32 @@ static void at_dma_off(struct at_dma *atdma)
>  
>  static int __init at_dma_probe(struct platform_device *pdev)
>  {
> -	struct at_dma_platform_data *pdata;
>  	struct resource		*io;
>  	struct at_dma		*atdma;
>  	size_t			size;
>  	int			irq;
>  	int			err;
>  	int			i;
> +	u32                     nr_channels;
> +	dma_cap_mask_t          cap_mask = {};
> +	enum atdma_devtype	atdmatype;
> +
> +	dma_cap_set(DMA_MEMCPY, cap_mask);
> +
> +	/* get DMA parameters from controller type */
> +	atdmatype = platform_get_device_id(pdev)->driver_data;
>  
> -	/* get DMA Controller parameters from platform */
> -	pdata = pdev->dev.platform_data;
> -	if (!pdata || pdata->nr_channels > AT_DMA_MAX_NR_CHANNELS)
> +	switch (atdmatype) {
> +	case ATDMA_DEVTYPE_SAM9RL:
> +		nr_channels = 2;
> +		break;
> +	case ATDMA_DEVTYPE_SAM9G45:
> +		nr_channels = 8;
> +		dma_cap_set(DMA_SLAVE, cap_mask);
> +		break;
> +	default:
>  		return -EINVAL;

Instead of this song and dance, why not make a configuration structure
and embed that into the platform_device_id table?  Like this:

struct at_dma_platform_data at91sam9rl_config {
	.nr_channels = 2;
	.cap_mask = 0;
};
struct at_dma_platform_data at91samg45_config {
	.nr_channels = 8;
	.cap_mask = DMA_SLAVE;
};
static struct platform_device_id atdma_devtypes[] = {
	{
		.name = "at91sam9rl_dma",
		.driver_data = (unsigned long) at91sam9rl_config,
		/*
		 * Yes, I know, ugly cast; but one case will be needed
		 * regardless when the of_device_id table is added.
		 * It's due to the platform_device_id not using a
		 * void*
		 */
	}, {
		.name = "at91sam9g45_dma",
		.driver_data = (unsigned long) at91sam9g45_config,
	}, {
		/* sentinel */
	}
};

And then the enum can be eliminated entirely.

> +	}
>  
>  	io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!io)
> @@ -1215,14 +1241,15 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  		return irq;
>  
>  	size = sizeof(struct at_dma);
> -	size += pdata->nr_channels * sizeof(struct at_dma_chan);
> +	size += nr_channels * sizeof(struct at_dma_chan);
>  	atdma = kzalloc(size, GFP_KERNEL);
>  	if (!atdma)
>  		return -ENOMEM;
>  
> -	/* discover transaction capabilites from the platform data */
> -	atdma->dma_common.cap_mask = pdata->cap_mask;
> -	atdma->all_chan_mask = (1 << pdata->nr_channels) - 1;
> +	/* discover transaction capabilities */
> +	atdma->dma_common.cap_mask = cap_mask;
> +	atdma->all_chan_mask = (1 << nr_channels) - 1;
> +	atdma->devtype = atdmatype;
>  
>  	size = resource_size(io);
>  	if (!request_mem_region(io->start, size, pdev->dev.driver->name)) {
> @@ -1268,7 +1295,7 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  
>  	/* initialize channels related values */
>  	INIT_LIST_HEAD(&atdma->dma_common.channels);
> -	for (i = 0; i < pdata->nr_channels; i++) {
> +	for (i = 0; i < nr_channels; i++) {
>  		struct at_dma_chan	*atchan = &atdma->chan[i];
>  
>  		atchan->chan_common.device = &atdma->dma_common;
> @@ -1313,7 +1340,7 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  	dev_info(&pdev->dev, "Atmel AHB DMA Controller ( %s%s), %d channels\n",
>  	  dma_has_cap(DMA_MEMCPY, atdma->dma_common.cap_mask) ? "cpy " : "",
>  	  dma_has_cap(DMA_SLAVE, atdma->dma_common.cap_mask)  ? "slave " : "",
> -	  pdata->nr_channels);
> +	  nr_channels);
>  
>  	dma_async_device_register(&atdma->dma_common);
>  
> @@ -1495,6 +1522,7 @@ static const struct dev_pm_ops at_dma_dev_pm_ops = {
>  static struct platform_driver at_dma_driver = {
>  	.remove		= __exit_p(at_dma_remove),
>  	.shutdown	= at_dma_shutdown,
> +	.id_table	= atdma_devtypes,
>  	.driver = {
>  		.name	= "at_hdmac",
>  		.pm	= &at_dma_dev_pm_ops,
> diff --git a/drivers/dma/at_hdmac_regs.h b/drivers/dma/at_hdmac_regs.h
> index aa4c9ae..d7d6737 100644
> --- a/drivers/dma/at_hdmac_regs.h
> +++ b/drivers/dma/at_hdmac_regs.h
> @@ -248,9 +248,16 @@ static inline struct at_dma_chan *to_at_dma_chan(struct dma_chan *dchan)
>  
>  /*--  Controller  ------------------------------------------------------*/
>  
> +enum atdma_devtype {
> +	ATDMA_DEVTYPE_UNDEFINED = 0,
> +	ATDMA_DEVTYPE_SAM9RL,	/* compatible with SAM9RL DMA controller */
> +	ATDMA_DEVTYPE_SAM9G45,	/* compatible with SAM9G45 DMA controller */
> +};
> +
>  /**
>   * struct at_dma - internal representation of an Atmel HDMA Controller
>   * @chan_common: common dmaengine dma_device object members
> + * @atdma_devtype: identifier of DMA controller compatibility
>   * @ch_regs: memory mapped register base
>   * @clk: dma controller clock
>   * @save_imr: interrupt mask register that is saved on suspend/resume cycle
> @@ -260,6 +267,7 @@ static inline struct at_dma_chan *to_at_dma_chan(struct dma_chan *dchan)
>   */
>  struct at_dma {
>  	struct dma_device	dma_common;
> +	enum atdma_devtype	devtype;
>  	void __iomem		*regs;
>  	struct clk		*clk;
>  	u32			save_imr;
> -- 
> 1.7.5.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/