Patchwork [2/5] dma: mxs-dma: make platform_device_id more generic

login
register
mail settings
Submitter Dong Aisheng
Date April 18, 2012, 12:46 p.m.
Message ID <1334753197-12032-3-git-send-email-b29396@freescale.com>
Download mbox | patch
Permalink /patch/153489/
State New
Headers show

Comments

Dong Aisheng - April 18, 2012, 12:46 p.m.
From: Dong Aisheng <dong.aisheng@linaro.org>

Rewrite mxs_dma_is_apbh and mxs_dma_is_apbx in order to support
other SoCs like imx6q and reform the platform_device_id for the
better further dt support.

Cc: Vinod Koul <vinod.koul@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Huang Shijie <b32955@freescale.com>
Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 arch/arm/mach-mxs/clock-mx23.c           |    4 +-
 arch/arm/mach-mxs/clock-mx28.c           |    4 +-
 arch/arm/mach-mxs/devices/platform-dma.c |   14 ++--
 drivers/dma/mxs-dma.c                    |  111 +++++++++++++++++++++++-------
 include/linux/fsl/mxs-dma.h              |   12 +---
 5 files changed, 101 insertions(+), 44 deletions(-)
Marek Vasut - April 18, 2012, 6:01 p.m.
Dear Dong Aisheng,

> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Rewrite mxs_dma_is_apbh and mxs_dma_is_apbx in order to support
> other SoCs like imx6q and reform the platform_device_id for the
> better further dt support.
> 
> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Huang Shijie <b32955@freescale.com>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

Reviewed-by: Marek Vasut <marex@denx.de>

> ---
>  arch/arm/mach-mxs/clock-mx23.c           |    4 +-
>  arch/arm/mach-mxs/clock-mx28.c           |    4 +-
>  arch/arm/mach-mxs/devices/platform-dma.c |   14 ++--
>  drivers/dma/mxs-dma.c                    |  111
> +++++++++++++++++++++++------- include/linux/fsl/mxs-dma.h              | 
>  12 +---
>  5 files changed, 101 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/clock-mx23.c
> b/arch/arm/mach-mxs/clock-mx23.c index e3ac52c..1e67b85 100644
> --- a/arch/arm/mach-mxs/clock-mx23.c
> +++ b/arch/arm/mach-mxs/clock-mx23.c
> @@ -427,8 +427,8 @@ static struct clk_lookup lookups[] = {
>  	_REGISTER_CLOCK("duart", NULL, uart_clk)
>  	_REGISTER_CLOCK("mxs-auart.0", NULL, uart_clk)
>  	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
> -	_REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk)
> -	_REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk)
> +	_REGISTER_CLOCK("imx23-dma-apbh", NULL, hbus_clk)
> +	_REGISTER_CLOCK("imx23-dma-apbx", NULL, xbus_clk)
>  	_REGISTER_CLOCK("mxs-mmc.0", NULL, ssp_clk)
>  	_REGISTER_CLOCK("mxs-mmc.1", NULL, ssp_clk)
>  	_REGISTER_CLOCK(NULL, "usb", usb_clk)
> diff --git a/arch/arm/mach-mxs/clock-mx28.c
> b/arch/arm/mach-mxs/clock-mx28.c index 1867a17..cca2cf7 100644
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -625,8 +625,8 @@ static struct clk_lookup lookups[] = {
>  	_REGISTER_CLOCK("mxs-auart.4", NULL, uart_clk)
>  	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
>  	_REGISTER_CLOCK("pll2", NULL, pll2_clk)
> -	_REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk)
> -	_REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk)
> +	_REGISTER_CLOCK("imx28-dma-apbh", NULL, hbus_clk)
> +	_REGISTER_CLOCK("imx28-dma-apbx", NULL, xbus_clk)
>  	_REGISTER_CLOCK("mxs-mmc.0", NULL, ssp0_clk)
>  	_REGISTER_CLOCK("mxs-mmc.1", NULL, ssp1_clk)
>  	_REGISTER_CLOCK("mxs-mmc.2", NULL, ssp2_clk)
> diff --git a/arch/arm/mach-mxs/devices/platform-dma.c
> b/arch/arm/mach-mxs/devices/platform-dma.c index 6a0202b..aff4813 100644
> --- a/arch/arm/mach-mxs/devices/platform-dma.c
> +++ b/arch/arm/mach-mxs/devices/platform-dma.c
> @@ -32,17 +32,19 @@ static struct platform_device *__init mxs_add_dma(const
> char *devid,
> 
>  static int __init mxs_add_mxs_dma(void)
>  {
> -	char *apbh = "mxs-dma-apbh";
> -	char *apbx = "mxs-dma-apbx";
> +	char *mx23_apbh = "imx23-dma-apbh";
> +	char *mx23_apbx = "imx23-dma-apbx";
> +	char *mx28_apbh = "imx28-dma-apbh";
> +	char *mx28_apbx = "imx28-dma-apbx";
> 
>  	if (cpu_is_mx23()) {
> -		mxs_add_dma(apbh, MX23_APBH_DMA_BASE_ADDR);
> -		mxs_add_dma(apbx, MX23_APBX_DMA_BASE_ADDR);
> +		mxs_add_dma(mx23_apbh, MX23_APBH_DMA_BASE_ADDR);
> +		mxs_add_dma(mx23_apbx, MX23_APBX_DMA_BASE_ADDR);
>  	}
> 
>  	if (cpu_is_mx28()) {
> -		mxs_add_dma(apbh, MX28_APBH_DMA_BASE_ADDR);
> -		mxs_add_dma(apbx, MX28_APBX_DMA_BASE_ADDR);
> +		mxs_add_dma(mx28_apbh, MX28_APBH_DMA_BASE_ADDR);
> +		mxs_add_dma(mx28_apbx, MX28_APBX_DMA_BASE_ADDR);
>  	}
> 
>  	return 0;
> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index b0051a8..51a29f9 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
> @@ -50,7 +50,8 @@
>  #define HW_APBHX_CTRL2				0x020
>  #define HW_APBHX_CHANNEL_CTRL			0x030
>  #define BP_APBHX_CHANNEL_CTRL_RESET_CHANNEL	16
> -#define HW_APBH_VERSION				(cpu_is_mx23() ? 0x3f0 : 
0x800)
> +#define HW_APBH_VERSION_IMX23			0x3f0
> +#define HW_APBH_VERSION_IMX28			0x800
>  #define HW_APBX_VERSION				0x800
>  #define BP_APBHX_VERSION_MAJOR			24
>  #define HW_APBHX_CHn_NXTCMDAR(n) \
> @@ -121,7 +122,8 @@ struct mxs_dma_chan {
>  #define MXS_DMA_CHANNELS_MASK		0xffff
> 
>  struct mxs_dma_engine {
> -	int				dev_id;
> +	unsigned int			type;
> +	unsigned int			dev_id;
>  	unsigned int			version;
>  	void __iomem			*base;
>  	struct clk			*clk;
> @@ -130,6 +132,74 @@ struct mxs_dma_engine {
>  	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
>  };
> 
> +enum mxs_dma_devtype {
> +	IMX23_DMA,
> +	IMX28_DMA,
> +};
> +
> +struct mxs_dma_type {
> +	unsigned int type;
> +	unsigned int id;
> +};
> +
> +static struct mxs_dma_type mxs_dma_types[] = {
> +	{
> +		.type = IMX23_DMA,
> +		.id = MXS_DMA_APBH,
> +	}, {
> +		.type = IMX23_DMA,
> +		.id = MXS_DMA_APBX,
> +	}, {
> +		.type = IMX28_DMA,
> +		.id = MXS_DMA_APBH,
> +	}, {
> +		.type = IMX28_DMA,
> +		.id = MXS_DMA_APBX,
> +	}, {
> +		/* end of list */
> +	}
> +};
> +
> +static struct platform_device_id mxs_dma_idt[] = {
> +	{
> +		.name = "imx23-dma-apbh",
> +		.driver_data = (kernel_ulong_t) &mxs_dma_types[0],
> +	}, {
> +		.name = "imx23-dma-apbx",
> +		.driver_data = (kernel_ulong_t) &mxs_dma_types[1],
> +	}, {
> +		.name = "imx28-dma-apbh",
> +		.driver_data = (kernel_ulong_t) &mxs_dma_types[2],
> +	}, {
> +		.name = "imx28-dma-apbx",
> +		.driver_data = (kernel_ulong_t) &mxs_dma_types[3],
> +	}, {
> +		/* end of list */
> +	}
> +};
> +
> +static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan)
> +{
> +	return container_of(chan, struct mxs_dma_chan, chan);
> +}
> +
> +int mxs_dma_is_apbh(struct dma_chan *chan)
> +{
> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> +
> +	return dma_is_apbh();
> +}
> +
> +int mxs_dma_is_apbx(struct dma_chan *chan)
> +{
> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> +
> +	return !dma_is_apbh();
> +}
> +
> +
>  static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
>  {
>  	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> @@ -193,11 +263,6 @@ static void mxs_dma_resume_chan(struct mxs_dma_chan
> *mxs_chan) mxs_chan->status = DMA_IN_PROGRESS;
>  }
> 
> -static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan)
> -{
> -	return container_of(chan, struct mxs_dma_chan, chan);
> -}
> -
>  static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx)
>  {
>  	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan);
> @@ -574,11 +639,18 @@ static int __init mxs_dma_init(struct mxs_dma_engine
> *mxs_dma) if (ret)
>  		goto err_out;
> 
> -	/* only major version matters */
> -	mxs_dma->version = readl(mxs_dma->base +
> +	if (mxs_dma->type == IMX23_DMA) {
> +		/* only major version matters */
> +		mxs_dma->version = readl(mxs_dma->base +
>  				((mxs_dma->dev_id == MXS_DMA_APBX) ?
> -				HW_APBX_VERSION : HW_APBH_VERSION)) >>
> +				HW_APBX_VERSION : HW_APBH_VERSION_IMX23)) >>
>  				BP_APBHX_VERSION_MAJOR;
> +	} else if (mxs_dma->type == IMX28_DMA) {
> +		mxs_dma->version = readl(mxs_dma->base +
> +				((mxs_dma->dev_id == MXS_DMA_APBX) ?
> +				HW_APBX_VERSION : HW_APBH_VERSION_IMX28)) >>
> +				BP_APBHX_VERSION_MAJOR;
> +	}
> 
>  	/* enable apbh burst */
>  	if (dma_is_apbh()) {
> @@ -601,6 +673,8 @@ static int __init mxs_dma_probe(struct platform_device
> *pdev) {
>  	const struct platform_device_id *id_entry =
>  				platform_get_device_id(pdev);
> +	const struct mxs_dma_type *mtype =
> +			(struct mxs_dma_type *)id_entry->driver_data;
>  	struct mxs_dma_engine *mxs_dma;
>  	struct resource *iores;
>  	int ret, i;
> @@ -609,7 +683,8 @@ static int __init mxs_dma_probe(struct platform_device
> *pdev) if (!mxs_dma)
>  		return -ENOMEM;
> 
> -	mxs_dma->dev_id = id_entry->driver_data;
> +	mxs_dma->type = mtype->type;
> +	mxs_dma->dev_id = mtype->id;
> 
>  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 
> @@ -692,23 +767,11 @@ err_request_region:
>  	return ret;
>  }
> 
> -static struct platform_device_id mxs_dma_type[] = {
> -	{
> -		.name = "mxs-dma-apbh",
> -		.driver_data = MXS_DMA_APBH,
> -	}, {
> -		.name = "mxs-dma-apbx",
> -		.driver_data = MXS_DMA_APBX,
> -	}, {
> -		/* end of list */
> -	}
> -};
> -
>  static struct platform_driver mxs_dma_driver = {
>  	.driver		= {
>  		.name	= "mxs-dma",
>  	},
> -	.id_table	= mxs_dma_type,
> +	.id_table	= mxs_dma_idt,
>  };
> 
>  static int __init mxs_dma_module_init(void)
> diff --git a/include/linux/fsl/mxs-dma.h b/include/linux/fsl/mxs-dma.h
> index 203d7c4..55d8702 100644
> --- a/include/linux/fsl/mxs-dma.h
> +++ b/include/linux/fsl/mxs-dma.h
> @@ -15,14 +15,6 @@ struct mxs_dma_data {
>  	int chan_irq;
>  };
> 
> -static inline int mxs_dma_is_apbh(struct dma_chan *chan)
> -{
> -	return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbh");
> -}
> -
> -static inline int mxs_dma_is_apbx(struct dma_chan *chan)
> -{
> -	return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbx");
> -}
> -
> +extern int mxs_dma_is_apbh(struct dma_chan *chan);
> +extern int mxs_dma_is_apbx(struct dma_chan *chan);
>  #endif /* __MACH_MXS_DMA_H__ */
Shawn Guo - April 23, 2012, 3:01 a.m.
On Wed, Apr 18, 2012 at 08:46:34PM +0800, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Rewrite mxs_dma_is_apbh and mxs_dma_is_apbx in order to support
> other SoCs like imx6q and reform the platform_device_id for the
> better further dt support.
> 
Nice cleanup.  But I would like to ask more.

> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Huang Shijie <b32955@freescale.com>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
>  arch/arm/mach-mxs/clock-mx23.c           |    4 +-
>  arch/arm/mach-mxs/clock-mx28.c           |    4 +-
>  arch/arm/mach-mxs/devices/platform-dma.c |   14 ++--
>  drivers/dma/mxs-dma.c                    |  111 +++++++++++++++++++++++-------
>  include/linux/fsl/mxs-dma.h              |   12 +---
>  5 files changed, 101 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/clock-mx23.c b/arch/arm/mach-mxs/clock-mx23.c
> index e3ac52c..1e67b85 100644
> --- a/arch/arm/mach-mxs/clock-mx23.c
> +++ b/arch/arm/mach-mxs/clock-mx23.c
> @@ -427,8 +427,8 @@ static struct clk_lookup lookups[] = {
>  	_REGISTER_CLOCK("duart", NULL, uart_clk)
>  	_REGISTER_CLOCK("mxs-auart.0", NULL, uart_clk)
>  	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
> -	_REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk)
> -	_REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk)
> +	_REGISTER_CLOCK("imx23-dma-apbh", NULL, hbus_clk)
> +	_REGISTER_CLOCK("imx23-dma-apbx", NULL, xbus_clk)
>  	_REGISTER_CLOCK("mxs-mmc.0", NULL, ssp_clk)
>  	_REGISTER_CLOCK("mxs-mmc.1", NULL, ssp_clk)
>  	_REGISTER_CLOCK(NULL, "usb", usb_clk)
> diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> index 1867a17..cca2cf7 100644
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -625,8 +625,8 @@ static struct clk_lookup lookups[] = {
>  	_REGISTER_CLOCK("mxs-auart.4", NULL, uart_clk)
>  	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
>  	_REGISTER_CLOCK("pll2", NULL, pll2_clk)
> -	_REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk)
> -	_REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk)
> +	_REGISTER_CLOCK("imx28-dma-apbh", NULL, hbus_clk)
> +	_REGISTER_CLOCK("imx28-dma-apbx", NULL, xbus_clk)
>  	_REGISTER_CLOCK("mxs-mmc.0", NULL, ssp0_clk)
>  	_REGISTER_CLOCK("mxs-mmc.1", NULL, ssp1_clk)
>  	_REGISTER_CLOCK("mxs-mmc.2", NULL, ssp2_clk)
> diff --git a/arch/arm/mach-mxs/devices/platform-dma.c b/arch/arm/mach-mxs/devices/platform-dma.c
> index 6a0202b..aff4813 100644
> --- a/arch/arm/mach-mxs/devices/platform-dma.c
> +++ b/arch/arm/mach-mxs/devices/platform-dma.c
> @@ -32,17 +32,19 @@ static struct platform_device *__init mxs_add_dma(const char *devid,
>  
>  static int __init mxs_add_mxs_dma(void)
>  {
> -	char *apbh = "mxs-dma-apbh";
> -	char *apbx = "mxs-dma-apbx";
> +	char *mx23_apbh = "imx23-dma-apbh";
> +	char *mx23_apbx = "imx23-dma-apbx";
> +	char *mx28_apbh = "imx28-dma-apbh";
> +	char *mx28_apbx = "imx28-dma-apbx";
>  
>  	if (cpu_is_mx23()) {
> -		mxs_add_dma(apbh, MX23_APBH_DMA_BASE_ADDR);
> -		mxs_add_dma(apbx, MX23_APBX_DMA_BASE_ADDR);
> +		mxs_add_dma(mx23_apbh, MX23_APBH_DMA_BASE_ADDR);
> +		mxs_add_dma(mx23_apbx, MX23_APBX_DMA_BASE_ADDR);
>  	}
>  
>  	if (cpu_is_mx28()) {
> -		mxs_add_dma(apbh, MX28_APBH_DMA_BASE_ADDR);
> -		mxs_add_dma(apbx, MX28_APBX_DMA_BASE_ADDR);
> +		mxs_add_dma(mx28_apbh, MX28_APBH_DMA_BASE_ADDR);
> +		mxs_add_dma(mx28_apbx, MX28_APBX_DMA_BASE_ADDR);
>  	}
>  
>  	return 0;

With soc specific hook introduced, we can remove this mxs_add_mxs_dma
call completely.

> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index b0051a8..51a29f9 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
> @@ -50,7 +50,8 @@
>  #define HW_APBHX_CTRL2				0x020
>  #define HW_APBHX_CHANNEL_CTRL			0x030
>  #define BP_APBHX_CHANNEL_CTRL_RESET_CHANNEL	16
> -#define HW_APBH_VERSION				(cpu_is_mx23() ? 0x3f0 : 0x800)
> +#define HW_APBH_VERSION_IMX23			0x3f0
> +#define HW_APBH_VERSION_IMX28			0x800

Since the VERSION is broken for identifying the dma type and we are
introduce dma type field, I would suggest to remove the use of VERSION
completely and use mxs_dma_type everywhere.

>  #define HW_APBX_VERSION				0x800
>  #define BP_APBHX_VERSION_MAJOR			24
>  #define HW_APBHX_CHn_NXTCMDAR(n) \
> @@ -121,7 +122,8 @@ struct mxs_dma_chan {
>  #define MXS_DMA_CHANNELS_MASK		0xffff
>  
>  struct mxs_dma_engine {
> -	int				dev_id;
> +	unsigned int			type;
> +	unsigned int			dev_id;
>  	unsigned int			version;
>  	void __iomem			*base;
>  	struct clk			*clk;
> @@ -130,6 +132,74 @@ struct mxs_dma_engine {
>  	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
>  };
>  
> +enum mxs_dma_devtype {
> +	IMX23_DMA,
> +	IMX28_DMA,
> +};
> +
> +struct mxs_dma_type {
> +	unsigned int type;
> +	unsigned int id;
> +};
> +
> +static struct mxs_dma_type mxs_dma_types[] = {
> +	{
> +		.type = IMX23_DMA,
> +		.id = MXS_DMA_APBH,

To me, IMX23_DMA is more like a id, while MXS_DMA_APBH is more like
a type.

> +	}, {
> +		.type = IMX23_DMA,
> +		.id = MXS_DMA_APBX,
> +	}, {
> +		.type = IMX28_DMA,
> +		.id = MXS_DMA_APBH,
> +	}, {
> +		.type = IMX28_DMA,
> +		.id = MXS_DMA_APBX,
> +	}, {
> +		/* end of list */
> +	}

This end sign is not needed, I think.

> +};
> +
> +static struct platform_device_id mxs_dma_idt[] = {

s/mxs_dma_idt/mxs_dma_ids?

> +	{
> +		.name = "imx23-dma-apbh",
> +		.driver_data = (kernel_ulong_t) &mxs_dma_types[0],
> +	}, {
> +		.name = "imx23-dma-apbx",
> +		.driver_data = (kernel_ulong_t) &mxs_dma_types[1],
> +	}, {
> +		.name = "imx28-dma-apbh",
> +		.driver_data = (kernel_ulong_t) &mxs_dma_types[2],
> +	}, {
> +		.name = "imx28-dma-apbx",
> +		.driver_data = (kernel_ulong_t) &mxs_dma_types[3],
> +	}, {
> +		/* end of list */
> +	}
> +};
> +
> +static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan)
> +{
> +	return container_of(chan, struct mxs_dma_chan, chan);
> +}
> +
> +int mxs_dma_is_apbh(struct dma_chan *chan)
> +{
> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> +
> +	return dma_is_apbh();

I would expect it simply be

	return (mxs_dma->dev_id == MXS_DMA_APBH);

And dma_is_apbh() can be removed completely now.

> +}
> +
> +int mxs_dma_is_apbx(struct dma_chan *chan)
> +{
> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> +
> +	return !dma_is_apbh();

	return (mxs_dma->dev_id == MXS_DMA_APBX);

> +}
> +
> +
>  static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
>  {
>  	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> @@ -193,11 +263,6 @@ static void mxs_dma_resume_chan(struct mxs_dma_chan *mxs_chan)
>  	mxs_chan->status = DMA_IN_PROGRESS;
>  }
>  
> -static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan)
> -{
> -	return container_of(chan, struct mxs_dma_chan, chan);
> -}
> -
>  static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx)
>  {
>  	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan);
> @@ -574,11 +639,18 @@ static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
>  	if (ret)
>  		goto err_out;
>  
> -	/* only major version matters */
> -	mxs_dma->version = readl(mxs_dma->base +
> +	if (mxs_dma->type == IMX23_DMA) {
> +		/* only major version matters */
> +		mxs_dma->version = readl(mxs_dma->base +
>  				((mxs_dma->dev_id == MXS_DMA_APBX) ?
> -				HW_APBX_VERSION : HW_APBH_VERSION)) >>
> +				HW_APBX_VERSION : HW_APBH_VERSION_IMX23)) >>
>  				BP_APBHX_VERSION_MAJOR;
> +	} else if (mxs_dma->type == IMX28_DMA) {
> +		mxs_dma->version = readl(mxs_dma->base +
> +				((mxs_dma->dev_id == MXS_DMA_APBX) ?
> +				HW_APBX_VERSION : HW_APBH_VERSION_IMX28)) >>
> +				BP_APBHX_VERSION_MAJOR;
> +	}

Again, let's just get rid of the use of VERSION completely.

>  
>  	/* enable apbh burst */
>  	if (dma_is_apbh()) {
> @@ -601,6 +673,8 @@ static int __init mxs_dma_probe(struct platform_device *pdev)
>  {
>  	const struct platform_device_id *id_entry =
>  				platform_get_device_id(pdev);
> +	const struct mxs_dma_type *mtype =
> +			(struct mxs_dma_type *)id_entry->driver_data;
>  	struct mxs_dma_engine *mxs_dma;
>  	struct resource *iores;
>  	int ret, i;
> @@ -609,7 +683,8 @@ static int __init mxs_dma_probe(struct platform_device *pdev)
>  	if (!mxs_dma)
>  		return -ENOMEM;
>  
> -	mxs_dma->dev_id = id_entry->driver_data;
> +	mxs_dma->type = mtype->type;
> +	mxs_dma->dev_id = mtype->id;
>  
>  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  
> @@ -692,23 +767,11 @@ err_request_region:
>  	return ret;
>  }
>  
> -static struct platform_device_id mxs_dma_type[] = {
> -	{
> -		.name = "mxs-dma-apbh",
> -		.driver_data = MXS_DMA_APBH,
> -	}, {
> -		.name = "mxs-dma-apbx",
> -		.driver_data = MXS_DMA_APBX,
> -	}, {
> -		/* end of list */
> -	}
> -};
> -
>  static struct platform_driver mxs_dma_driver = {
>  	.driver		= {
>  		.name	= "mxs-dma",
>  	},
> -	.id_table	= mxs_dma_type,
> +	.id_table	= mxs_dma_idt,
>  };
>  
>  static int __init mxs_dma_module_init(void)
> diff --git a/include/linux/fsl/mxs-dma.h b/include/linux/fsl/mxs-dma.h
> index 203d7c4..55d8702 100644
> --- a/include/linux/fsl/mxs-dma.h
> +++ b/include/linux/fsl/mxs-dma.h
> @@ -15,14 +15,6 @@ struct mxs_dma_data {
>  	int chan_irq;
>  };
>  
> -static inline int mxs_dma_is_apbh(struct dma_chan *chan)
> -{
> -	return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbh");
> -}
> -
> -static inline int mxs_dma_is_apbx(struct dma_chan *chan)
> -{
> -	return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbx");
> -}
> -
> +extern int mxs_dma_is_apbh(struct dma_chan *chan);
> +extern int mxs_dma_is_apbx(struct dma_chan *chan);

I would expect these two calls still be inline here.

>  #endif /* __MACH_MXS_DMA_H__ */
> -- 
> 1.7.0.4
> 
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
Dong Aisheng - April 23, 2012, 3:58 a.m.
On Mon, Apr 23, 2012 at 11:01:18AM +0800, Shawn Guo wrote:
> On Wed, Apr 18, 2012 at 08:46:34PM +0800, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > Rewrite mxs_dma_is_apbh and mxs_dma_is_apbx in order to support
> > other SoCs like imx6q and reform the platform_device_id for the
> > better further dt support.
> > 
> Nice cleanup.  But I would like to ask more.
> 
> > Cc: Vinod Koul <vinod.koul@intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Huang Shijie <b32955@freescale.com>
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> >  arch/arm/mach-mxs/clock-mx23.c           |    4 +-
> >  arch/arm/mach-mxs/clock-mx28.c           |    4 +-
> >  arch/arm/mach-mxs/devices/platform-dma.c |   14 ++--
> >  drivers/dma/mxs-dma.c                    |  111 +++++++++++++++++++++++-------
> >  include/linux/fsl/mxs-dma.h              |   12 +---
> >  5 files changed, 101 insertions(+), 44 deletions(-)
> > 
> > diff --git a/arch/arm/mach-mxs/clock-mx23.c b/arch/arm/mach-mxs/clock-mx23.c
> > index e3ac52c..1e67b85 100644
> > --- a/arch/arm/mach-mxs/clock-mx23.c
> > +++ b/arch/arm/mach-mxs/clock-mx23.c
> > @@ -427,8 +427,8 @@ static struct clk_lookup lookups[] = {
> >  	_REGISTER_CLOCK("duart", NULL, uart_clk)
> >  	_REGISTER_CLOCK("mxs-auart.0", NULL, uart_clk)
> >  	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
> > -	_REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk)
> > -	_REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk)
> > +	_REGISTER_CLOCK("imx23-dma-apbh", NULL, hbus_clk)
> > +	_REGISTER_CLOCK("imx23-dma-apbx", NULL, xbus_clk)
> >  	_REGISTER_CLOCK("mxs-mmc.0", NULL, ssp_clk)
> >  	_REGISTER_CLOCK("mxs-mmc.1", NULL, ssp_clk)
> >  	_REGISTER_CLOCK(NULL, "usb", usb_clk)
> > diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> > index 1867a17..cca2cf7 100644
> > --- a/arch/arm/mach-mxs/clock-mx28.c
> > +++ b/arch/arm/mach-mxs/clock-mx28.c
> > @@ -625,8 +625,8 @@ static struct clk_lookup lookups[] = {
> >  	_REGISTER_CLOCK("mxs-auart.4", NULL, uart_clk)
> >  	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
> >  	_REGISTER_CLOCK("pll2", NULL, pll2_clk)
> > -	_REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk)
> > -	_REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk)
> > +	_REGISTER_CLOCK("imx28-dma-apbh", NULL, hbus_clk)
> > +	_REGISTER_CLOCK("imx28-dma-apbx", NULL, xbus_clk)
> >  	_REGISTER_CLOCK("mxs-mmc.0", NULL, ssp0_clk)
> >  	_REGISTER_CLOCK("mxs-mmc.1", NULL, ssp1_clk)
> >  	_REGISTER_CLOCK("mxs-mmc.2", NULL, ssp2_clk)
> > diff --git a/arch/arm/mach-mxs/devices/platform-dma.c b/arch/arm/mach-mxs/devices/platform-dma.c
> > index 6a0202b..aff4813 100644
> > --- a/arch/arm/mach-mxs/devices/platform-dma.c
> > +++ b/arch/arm/mach-mxs/devices/platform-dma.c
> > @@ -32,17 +32,19 @@ static struct platform_device *__init mxs_add_dma(const char *devid,
> >  
> >  static int __init mxs_add_mxs_dma(void)
> >  {
> > -	char *apbh = "mxs-dma-apbh";
> > -	char *apbx = "mxs-dma-apbx";
> > +	char *mx23_apbh = "imx23-dma-apbh";
> > +	char *mx23_apbx = "imx23-dma-apbx";
> > +	char *mx28_apbh = "imx28-dma-apbh";
> > +	char *mx28_apbx = "imx28-dma-apbx";
> >  
> >  	if (cpu_is_mx23()) {
> > -		mxs_add_dma(apbh, MX23_APBH_DMA_BASE_ADDR);
> > -		mxs_add_dma(apbx, MX23_APBX_DMA_BASE_ADDR);
> > +		mxs_add_dma(mx23_apbh, MX23_APBH_DMA_BASE_ADDR);
> > +		mxs_add_dma(mx23_apbx, MX23_APBX_DMA_BASE_ADDR);
> >  	}
> >  
> >  	if (cpu_is_mx28()) {
> > -		mxs_add_dma(apbh, MX28_APBH_DMA_BASE_ADDR);
> > -		mxs_add_dma(apbx, MX28_APBX_DMA_BASE_ADDR);
> > +		mxs_add_dma(mx28_apbh, MX28_APBH_DMA_BASE_ADDR);
> > +		mxs_add_dma(mx28_apbx, MX28_APBX_DMA_BASE_ADDR);
> >  	}
> >  
> >  	return 0;
> 
> With soc specific hook introduced, we can remove this mxs_add_mxs_dma
> call completely.
> 
Yes, it's true.
A mx23/mx28_soc_init can do it well.

> > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> > index b0051a8..51a29f9 100644
> > --- a/drivers/dma/mxs-dma.c
> > +++ b/drivers/dma/mxs-dma.c
> > @@ -50,7 +50,8 @@
> >  #define HW_APBHX_CTRL2				0x020
> >  #define HW_APBHX_CHANNEL_CTRL			0x030
> >  #define BP_APBHX_CHANNEL_CTRL_RESET_CHANNEL	16
> > -#define HW_APBH_VERSION				(cpu_is_mx23() ? 0x3f0 : 0x800)
> > +#define HW_APBH_VERSION_IMX23			0x3f0
> > +#define HW_APBH_VERSION_IMX28			0x800
> 
> Since the VERSION is broken for identifying the dma type and we are
> introduce dma type field, I would suggest to remove the use of VERSION
> completely and use mxs_dma_type pbh_is_old()           (mxs_dma->everywhere.
> 
Remove HW_APBH_VERSION_*?
But i see the some other places in original code still need to use it.
Like:
#define apbh_is_old()	(mxs_dma->version < APBH_VERSION_LATEST)
..
#define HW_APBHX_CHn_NXTCMDAR(n) \
        (((dma_is_apbh() && apbh_is_old()) ? 0x050 : 0x110) + (n) * 0x70)
#define HW_APBHX_CHn_SEMA(n) \
        (((dma_is_apbh() && apbh_is_old()) ? 0x080 : 0x140) + (n) * 0x70)

Do you mean apbh_is_old represents the same as SoC type and can be replaced
by mxs_dma_type?

 >  #define HW_APBX_VERSION				0x800
> >  #define BP_APBHX_VERSION_MAJOR			24
> >  #define HW_APBHX_CHn_NXTCMDAR(n) \
> > @@ -121,7 +122,8 @@ struct mxs_dma_chan {
> >  #define MXS_DMA_CHANNELS_MASK		0xffff
> >  
> >  struct mxs_dma_engine {
> > -	int				dev_id;
> > +	unsigned int			type;
> > +	unsigned int			dev_id;
> >  	unsigned int			version;
> >  	void __iomem			*base;
> >  	struct clk			*clk;
> > @@ -130,6 +132,74 @@ struct mxs_dma_engine {
> >  	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
> >  };
> >  
> > +enum mxs_dma_devtype {
> > +	IMX23_DMA,
> > +	IMX28_DMA,
> > +};
> > +
> > +struct mxs_dma_type {
> > +	unsigned int type;
> > +	unsigned int id;
> > +};
> > +
> > +static struct mxs_dma_type mxs_dma_types[] = {
> > +	{
> > +		.type = IMX23_DMA,
> > +		.id = MXS_DMA_APBH,
> 
> To me, IMX23_DMA is more like a id, while MXS_DMA_APBH is more like
> a type.
> 
Well, i'm more like IMX23_DMA since they're different devices.
Id to me is more like a same device but different id.
And you see i already have enum mxs_dma_devtype.

> > +	}, {
> > +		.type = IMX23_DMA,
> > +		.id = MXS_DMA_APBX,
> > +	}, {
> > +		.type = IMX28_DMA,
> > +		.id = MXS_DMA_APBH,
> > +	}, {
> > +		.type = IMX28_DMA,
> > +		.id = MXS_DMA_APBX,
> > +	}, {
> > +		/* end of list */
> > +	}
> 
> This end sign is not needed, I think.
> 
Ok to me.
btw i always had this question before,
Can you tell what's purpose of adding a '/* end of list */'?

> > +};
> > +
> > +static struct platform_device_id mxs_dma_idt[] = {
> 
> s/mxs_dma_idt/mxs_dma_ids?
> 
Ok to me.

> > +	{
> > +		.name = "imx23-dma-apbh",
> > +		.driver_data = (kernel_ulong_t) &mxs_dma_types[0],
> > +	}, {
> > +		.name = "imx23-dma-apbx",
> > +		.driver_data = (kernel_ulong_t) &mxs_dma_types[1],
> > +	}, {
> > +		.name = "imx28-dma-apbh",
> > +		.driver_data = (kernel_ulong_t) &mxs_dma_types[2],
> > +	}, {
> > +		.name = "imx28-dma-apbx",
> > +		.driver_data = (kernel_ulong_t) &mxs_dma_types[3],
> > +	}, {
> > +		/* end of list */
> > +	}
> > +};
> > +
> > +static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan)
> > +{
> > +	return container_of(chan, struct mxs_dma_chan, chan);
> > +}
> > +
> > +int mxs_dma_is_apbh(struct dma_chan *chan)
> > +{
> > +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > +
> > +	return dma_is_apbh();
> 
> I would expect it simply be
> 
> 	return (mxs_dma->dev_id == MXS_DMA_APBH);
> 
> And dma_is_apbh() can be removed completely now.
> 
Yes, it's ture.
Basically what i did in this patch is only convert the driver to
a more generic platform_device_id, seems i need to do more...

> > +}
> > +
> > +int mxs_dma_is_apbx(struct dma_chan *chan)
> > +{
> > +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > +
> > +	return !dma_is_apbh();
> 
> 	return (mxs_dma->dev_id == MXS_DMA_APBX);
> 
> > +}
> > +
> > +
> >  static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
> >  {
> >  	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > @@ -193,11 +263,6 @@ static void mxs_dma_resume_chan(struct mxs_dma_chan *mxs_chan)
> >  	mxs_chan->status = DMA_IN_PROGRESS;
> >  }
> >  
> > -static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan)
> > -{
> > -	return container_of(chan, struct mxs_dma_chan, chan);
> > -}
> > -
> >  static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx)
> >  {
> >  	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan);
> > @@ -574,11 +639,18 @@ static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
> >  	if (ret)
> >  		goto err_out;
> >  
> > -	/* only major version matters */
> > -	mxs_dma->version = readl(mxs_dma->base +
> > +	if (mxs_dma->type == IMX23_DMA) {
> > +		/* only major version matters */
> > +		mxs_dma->version = readl(mxs_dma->base +
> >  				((mxs_dma->dev_id == MXS_DMA_APBX) ?
> > -				HW_APBX_VERSION : HW_APBH_VERSION)) >>
> > +				HW_APBX_VERSION : HW_APBH_VERSION_IMX23)) >>
> >  				BP_APBHX_VERSION_MAJOR;
> > +	} else if (mxs_dma->type == IMX28_DMA) {
> > +		mxs_dma->version = readl(mxs_dma->base +
> > +				((mxs_dma->dev_id == MXS_DMA_APBX) ?
> > +				HW_APBX_VERSION : HW_APBH_VERSION_IMX28)) >>
> > +				BP_APBHX_VERSION_MAJOR;
> > +	}
> 
> Again, let's just get rid of the use of VERSION completely.
> 
> >  
> >  	/* enable apbh burst */
> >  	if (dma_is_apbh()) {
> > @@ -601,6 +673,8 @@ static int __init mxs_dma_probe(struct platform_device *pdev)
> >  {
> >  	const struct platform_device_id *id_entry =
> >  				platform_get_device_id(pdev);
> > +	const struct mxs_dma_type *mtype =
> > +			(struct mxs_dma_type *)id_entry->driver_data;
> >  	struct mxs_dma_engine *mxs_dma;
> >  	struct resource *iores;
> >  	int ret, i;
> > @@ -609,7 +683,8 @@ static int __init mxs_dma_probe(struct platform_device *pdev)
> >  	if (!mxs_dma)
> >  		return -ENOMEM;
> >  
> > -	mxs_dma->dev_id = id_entry->driver_data;
> > +	mxs_dma->type = mtype->type;
> > +	mxs_dma->dev_id = mtype->id;
> >  
> >  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  
> > @@ -692,23 +767,11 @@ err_request_region:
> >  	return ret;
> >  }
> >  
> > -static struct platform_device_id mxs_dma_type[] = {
> > -	{
> > -		.name = "mxs-dma-apbh",
> > -		.driver_data = MXS_DMA_APBH,
> > -	}, {
> > -		.name = "mxs-dma-apbx",
> > -		.driver_data = MXS_DMA_APBX,
> > -	}, {
> > -		/* end of list */
> > -	}
> > -};
> > -
> >  static struct platform_driver mxs_dma_driver = {
> >  	.driver		= {
> >  		.name	= "mxs-dma",
> >  	},
> > -	.id_table	= mxs_dma_type,
> > +	.id_table	= mxs_dma_idt,
> >  };
> >  
> >  static int __init mxs_dma_module_init(void)
> > diff --git a/include/linux/fsl/mxs-dma.h b/include/linux/fsl/mxs-dma.h
> > index 203d7c4..55d8702 100644
> > --- a/include/linux/fsl/mxs-dma.h
> > +++ b/include/linux/fsl/mxs-dma.h
> > @@ -15,14 +15,6 @@ struct mxs_dma_data {
> >  	int chan_irq;
> >  };
> >  
> > -static inline int mxs_dma_is_apbh(struct dma_chan *chan)
> > -{
> > -	return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbh");
> > -}
> > -
> > -static inline int mxs_dma_is_apbx(struct dma_chan *chan)
> > -{
> > -	return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbx");
> > -}
> > -
> > +extern int mxs_dma_is_apbh(struct dma_chan *chan);
> > +extern int mxs_dma_is_apbx(struct dma_chan *chan);
> 
> I would expect these two calls still be inline here.
> 
Ok to me.

Regards
Dong Aisheng
Shawn Guo - April 23, 2012, 4:58 a.m.
On Mon, Apr 23, 2012 at 11:58:48AM +0800, Dong Aisheng wrote:
...
> Remove HW_APBH_VERSION_*?
> But i see the some other places in original code still need to use it.
> Like:
> #define apbh_is_old()	(mxs_dma->version < APBH_VERSION_LATEST)
> ..
> #define HW_APBHX_CHn_NXTCMDAR(n) \
>         (((dma_is_apbh() && apbh_is_old()) ? 0x050 : 0x110) + (n) * 0x70)
> #define HW_APBHX_CHn_SEMA(n) \
>         (((dma_is_apbh() && apbh_is_old()) ? 0x080 : 0x140) + (n) * 0x70)
> 
> Do you mean apbh_is_old represents the same as SoC type and can be replaced
> by mxs_dma_type?
> 
Yes.

>  >  #define HW_APBX_VERSION				0x800
> > >  #define BP_APBHX_VERSION_MAJOR			24
> > >  #define HW_APBHX_CHn_NXTCMDAR(n) \
> > > @@ -121,7 +122,8 @@ struct mxs_dma_chan {
> > >  #define MXS_DMA_CHANNELS_MASK		0xffff
> > >  
> > >  struct mxs_dma_engine {
> > > -	int				dev_id;
> > > +	unsigned int			type;
> > > +	unsigned int			dev_id;
> > >  	unsigned int			version;
> > >  	void __iomem			*base;
> > >  	struct clk			*clk;
> > > @@ -130,6 +132,74 @@ struct mxs_dma_engine {
> > >  	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
> > >  };
> > >  
> > > +enum mxs_dma_devtype {
> > > +	IMX23_DMA,
> > > +	IMX28_DMA,
> > > +};
> > > +
> > > +struct mxs_dma_type {
> > > +	unsigned int type;
> > > +	unsigned int id;
> > > +};
> > > +
> > > +static struct mxs_dma_type mxs_dma_types[] = {
> > > +	{
> > > +		.type = IMX23_DMA,
> > > +		.id = MXS_DMA_APBH,
> > 
> > To me, IMX23_DMA is more like a id, while MXS_DMA_APBH is more like
> > a type.
> > 
> Well, i'm more like IMX23_DMA since they're different devices.
> Id to me is more like a same device but different id.
> And you see i already have enum mxs_dma_devtype.
> 
Ok, let me put more comment on this.  You have mxs_dma_devtype defined,
and then you should use it rather than "unsigned int" to define "type"
in struct mxs_dma_type.  And for better alignment, the MXS_DMA_APBH
and MXS_DMA_APBX should be changed to some enum type from the exsiting
macro.  Then, struct mxs_dma_type should probably look like the
following.

struct mxs_dma_type {
	enum mxs_dma_devtype type;
	enum mxs_dma_id id;
};

As you agreed below, we will have mxs_dma_ids for the platform_device_id
table, and mxs_dma_id here for another enum.

I think we really need a better semantics for "type" and "id" in this
driver.

> > > +	}, {
> > > +		.type = IMX23_DMA,
> > > +		.id = MXS_DMA_APBX,
> > > +	}, {
> > > +		.type = IMX28_DMA,
> > > +		.id = MXS_DMA_APBH,
> > > +	}, {
> > > +		.type = IMX28_DMA,
> > > +		.id = MXS_DMA_APBX,
> > > +	}, {
> > > +		/* end of list */
> > > +	}
> > 
> > This end sign is not needed, I think.
> > 
> Ok to me.
> btw i always had this question before,
> Can you tell what's purpose of adding a '/* end of list */'?
> 

The end sign I meant is "{ }".  It's needed for platform_device_id
and of_device_id table by driver core.  But I do not see why we need
it for mxs_dma_type table here.

> > > +};
> > > +
> > > +static struct platform_device_id mxs_dma_idt[] = {
> > 
> > s/mxs_dma_idt/mxs_dma_ids?
> > 
> Ok to me.
> 
> > > +	{
> > > +		.name = "imx23-dma-apbh",
> > > +		.driver_data = (kernel_ulong_t) &mxs_dma_types[0],
> > > +	}, {
> > > +		.name = "imx23-dma-apbx",
> > > +		.driver_data = (kernel_ulong_t) &mxs_dma_types[1],
> > > +	}, {
> > > +		.name = "imx28-dma-apbh",
> > > +		.driver_data = (kernel_ulong_t) &mxs_dma_types[2],
> > > +	}, {
> > > +		.name = "imx28-dma-apbx",
> > > +		.driver_data = (kernel_ulong_t) &mxs_dma_types[3],
> > > +	}, {
> > > +		/* end of list */
> > > +	}
> > > +};
> > > +

Patch

diff --git a/arch/arm/mach-mxs/clock-mx23.c b/arch/arm/mach-mxs/clock-mx23.c
index e3ac52c..1e67b85 100644
--- a/arch/arm/mach-mxs/clock-mx23.c
+++ b/arch/arm/mach-mxs/clock-mx23.c
@@ -427,8 +427,8 @@  static struct clk_lookup lookups[] = {
 	_REGISTER_CLOCK("duart", NULL, uart_clk)
 	_REGISTER_CLOCK("mxs-auart.0", NULL, uart_clk)
 	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
-	_REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk)
-	_REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk)
+	_REGISTER_CLOCK("imx23-dma-apbh", NULL, hbus_clk)
+	_REGISTER_CLOCK("imx23-dma-apbx", NULL, xbus_clk)
 	_REGISTER_CLOCK("mxs-mmc.0", NULL, ssp_clk)
 	_REGISTER_CLOCK("mxs-mmc.1", NULL, ssp_clk)
 	_REGISTER_CLOCK(NULL, "usb", usb_clk)
diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
index 1867a17..cca2cf7 100644
--- a/arch/arm/mach-mxs/clock-mx28.c
+++ b/arch/arm/mach-mxs/clock-mx28.c
@@ -625,8 +625,8 @@  static struct clk_lookup lookups[] = {
 	_REGISTER_CLOCK("mxs-auart.4", NULL, uart_clk)
 	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
 	_REGISTER_CLOCK("pll2", NULL, pll2_clk)
-	_REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk)
-	_REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk)
+	_REGISTER_CLOCK("imx28-dma-apbh", NULL, hbus_clk)
+	_REGISTER_CLOCK("imx28-dma-apbx", NULL, xbus_clk)
 	_REGISTER_CLOCK("mxs-mmc.0", NULL, ssp0_clk)
 	_REGISTER_CLOCK("mxs-mmc.1", NULL, ssp1_clk)
 	_REGISTER_CLOCK("mxs-mmc.2", NULL, ssp2_clk)
diff --git a/arch/arm/mach-mxs/devices/platform-dma.c b/arch/arm/mach-mxs/devices/platform-dma.c
index 6a0202b..aff4813 100644
--- a/arch/arm/mach-mxs/devices/platform-dma.c
+++ b/arch/arm/mach-mxs/devices/platform-dma.c
@@ -32,17 +32,19 @@  static struct platform_device *__init mxs_add_dma(const char *devid,
 
 static int __init mxs_add_mxs_dma(void)
 {
-	char *apbh = "mxs-dma-apbh";
-	char *apbx = "mxs-dma-apbx";
+	char *mx23_apbh = "imx23-dma-apbh";
+	char *mx23_apbx = "imx23-dma-apbx";
+	char *mx28_apbh = "imx28-dma-apbh";
+	char *mx28_apbx = "imx28-dma-apbx";
 
 	if (cpu_is_mx23()) {
-		mxs_add_dma(apbh, MX23_APBH_DMA_BASE_ADDR);
-		mxs_add_dma(apbx, MX23_APBX_DMA_BASE_ADDR);
+		mxs_add_dma(mx23_apbh, MX23_APBH_DMA_BASE_ADDR);
+		mxs_add_dma(mx23_apbx, MX23_APBX_DMA_BASE_ADDR);
 	}
 
 	if (cpu_is_mx28()) {
-		mxs_add_dma(apbh, MX28_APBH_DMA_BASE_ADDR);
-		mxs_add_dma(apbx, MX28_APBX_DMA_BASE_ADDR);
+		mxs_add_dma(mx28_apbh, MX28_APBH_DMA_BASE_ADDR);
+		mxs_add_dma(mx28_apbx, MX28_APBX_DMA_BASE_ADDR);
 	}
 
 	return 0;
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index b0051a8..51a29f9 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -50,7 +50,8 @@ 
 #define HW_APBHX_CTRL2				0x020
 #define HW_APBHX_CHANNEL_CTRL			0x030
 #define BP_APBHX_CHANNEL_CTRL_RESET_CHANNEL	16
-#define HW_APBH_VERSION				(cpu_is_mx23() ? 0x3f0 : 0x800)
+#define HW_APBH_VERSION_IMX23			0x3f0
+#define HW_APBH_VERSION_IMX28			0x800
 #define HW_APBX_VERSION				0x800
 #define BP_APBHX_VERSION_MAJOR			24
 #define HW_APBHX_CHn_NXTCMDAR(n) \
@@ -121,7 +122,8 @@  struct mxs_dma_chan {
 #define MXS_DMA_CHANNELS_MASK		0xffff
 
 struct mxs_dma_engine {
-	int				dev_id;
+	unsigned int			type;
+	unsigned int			dev_id;
 	unsigned int			version;
 	void __iomem			*base;
 	struct clk			*clk;
@@ -130,6 +132,74 @@  struct mxs_dma_engine {
 	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
 };
 
+enum mxs_dma_devtype {
+	IMX23_DMA,
+	IMX28_DMA,
+};
+
+struct mxs_dma_type {
+	unsigned int type;
+	unsigned int id;
+};
+
+static struct mxs_dma_type mxs_dma_types[] = {
+	{
+		.type = IMX23_DMA,
+		.id = MXS_DMA_APBH,
+	}, {
+		.type = IMX23_DMA,
+		.id = MXS_DMA_APBX,
+	}, {
+		.type = IMX28_DMA,
+		.id = MXS_DMA_APBH,
+	}, {
+		.type = IMX28_DMA,
+		.id = MXS_DMA_APBX,
+	}, {
+		/* end of list */
+	}
+};
+
+static struct platform_device_id mxs_dma_idt[] = {
+	{
+		.name = "imx23-dma-apbh",
+		.driver_data = (kernel_ulong_t) &mxs_dma_types[0],
+	}, {
+		.name = "imx23-dma-apbx",
+		.driver_data = (kernel_ulong_t) &mxs_dma_types[1],
+	}, {
+		.name = "imx28-dma-apbh",
+		.driver_data = (kernel_ulong_t) &mxs_dma_types[2],
+	}, {
+		.name = "imx28-dma-apbx",
+		.driver_data = (kernel_ulong_t) &mxs_dma_types[3],
+	}, {
+		/* end of list */
+	}
+};
+
+static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan)
+{
+	return container_of(chan, struct mxs_dma_chan, chan);
+}
+
+int mxs_dma_is_apbh(struct dma_chan *chan)
+{
+	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+
+	return dma_is_apbh();
+}
+
+int mxs_dma_is_apbx(struct dma_chan *chan)
+{
+	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+
+	return !dma_is_apbh();
+}
+
+
 static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
 {
 	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
@@ -193,11 +263,6 @@  static void mxs_dma_resume_chan(struct mxs_dma_chan *mxs_chan)
 	mxs_chan->status = DMA_IN_PROGRESS;
 }
 
-static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan)
-{
-	return container_of(chan, struct mxs_dma_chan, chan);
-}
-
 static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx)
 {
 	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan);
@@ -574,11 +639,18 @@  static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
 	if (ret)
 		goto err_out;
 
-	/* only major version matters */
-	mxs_dma->version = readl(mxs_dma->base +
+	if (mxs_dma->type == IMX23_DMA) {
+		/* only major version matters */
+		mxs_dma->version = readl(mxs_dma->base +
 				((mxs_dma->dev_id == MXS_DMA_APBX) ?
-				HW_APBX_VERSION : HW_APBH_VERSION)) >>
+				HW_APBX_VERSION : HW_APBH_VERSION_IMX23)) >>
 				BP_APBHX_VERSION_MAJOR;
+	} else if (mxs_dma->type == IMX28_DMA) {
+		mxs_dma->version = readl(mxs_dma->base +
+				((mxs_dma->dev_id == MXS_DMA_APBX) ?
+				HW_APBX_VERSION : HW_APBH_VERSION_IMX28)) >>
+				BP_APBHX_VERSION_MAJOR;
+	}
 
 	/* enable apbh burst */
 	if (dma_is_apbh()) {
@@ -601,6 +673,8 @@  static int __init mxs_dma_probe(struct platform_device *pdev)
 {
 	const struct platform_device_id *id_entry =
 				platform_get_device_id(pdev);
+	const struct mxs_dma_type *mtype =
+			(struct mxs_dma_type *)id_entry->driver_data;
 	struct mxs_dma_engine *mxs_dma;
 	struct resource *iores;
 	int ret, i;
@@ -609,7 +683,8 @@  static int __init mxs_dma_probe(struct platform_device *pdev)
 	if (!mxs_dma)
 		return -ENOMEM;
 
-	mxs_dma->dev_id = id_entry->driver_data;
+	mxs_dma->type = mtype->type;
+	mxs_dma->dev_id = mtype->id;
 
 	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
@@ -692,23 +767,11 @@  err_request_region:
 	return ret;
 }
 
-static struct platform_device_id mxs_dma_type[] = {
-	{
-		.name = "mxs-dma-apbh",
-		.driver_data = MXS_DMA_APBH,
-	}, {
-		.name = "mxs-dma-apbx",
-		.driver_data = MXS_DMA_APBX,
-	}, {
-		/* end of list */
-	}
-};
-
 static struct platform_driver mxs_dma_driver = {
 	.driver		= {
 		.name	= "mxs-dma",
 	},
-	.id_table	= mxs_dma_type,
+	.id_table	= mxs_dma_idt,
 };
 
 static int __init mxs_dma_module_init(void)
diff --git a/include/linux/fsl/mxs-dma.h b/include/linux/fsl/mxs-dma.h
index 203d7c4..55d8702 100644
--- a/include/linux/fsl/mxs-dma.h
+++ b/include/linux/fsl/mxs-dma.h
@@ -15,14 +15,6 @@  struct mxs_dma_data {
 	int chan_irq;
 };
 
-static inline int mxs_dma_is_apbh(struct dma_chan *chan)
-{
-	return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbh");
-}
-
-static inline int mxs_dma_is_apbx(struct dma_chan *chan)
-{
-	return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbx");
-}
-
+extern int mxs_dma_is_apbh(struct dma_chan *chan);
+extern int mxs_dma_is_apbx(struct dma_chan *chan);
 #endif /* __MACH_MXS_DMA_H__ */