[PATCH 1/3] dmaengine: dw_dmac: Update documentation style comments for dw_dma_platform_data

[PATCH 1/3] dmaengine: dw_dmac: Update documentation style comments for dw_dma_platform_data

[Viresh Kumar]

Documentation style comments were missing for few fields in struct
dw_dma_platform_data. Add these.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/dw_dmac.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
index e1c8c9e..62a6190 100644
--- a/include/linux/dw_dmac.h
+++ b/include/linux/dw_dmac.h
@@ -19,6 +19,8 @@
  * @nr_channels: Number of channels supported by hardware (max 8)
  * @is_private: The device channels should be marked as private and not for
  *	by the general purpose DMA channel allocator.
+ * @chan_allocation_order: Allocate channels starting from 0 or 7
+ * @chan_priority: Set channel priority increasing from 0 to 7 or 7 to 0.
  * @block_size: Maximum block size supported by the controller
  * @nr_masters: Number of AHB masters supported by the controller
  * @data_width: Maximum data width supported by hardware per AHB master
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Viresh Kumar]

dw_dmac driver already supports device tree but it used to have its platform
data passed the non-DT way.

This patch does following changes:
- pass platform data via DT, non-DT way still takes precedence if both are used.
- create generic filter routine
- Earlier slave information was made available by slave specific filter routines
  in chan->private field. Now, this information would be passed from within dmac
  DT node. Slave drivers would now be required to pass bus_id (a string) as
  parameter to this generic filter(), which would be compared against the slave
  data passed from DT, by the generic filter routine.
- Update binding document

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/devicetree/bindings/dma/snps-dma.txt |  44 ++++++
 drivers/dma/dw_dmac.c                              | 147 +++++++++++++++++++++
 drivers/dma/dw_dmac_regs.h                         |   4 +
 include/linux/dw_dmac.h                            |  43 +++---
 4 files changed, 221 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
index c0d85db..5bb3dfb 100644
--- a/Documentation/devicetree/bindings/dma/snps-dma.txt
+++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
@@ -6,6 +6,26 @@ Required properties:
 - interrupt-parent: Should be the phandle for the interrupt controller
   that services interrupts for this device
 - interrupt: Should contain the DMAC interrupt number
+- nr_channels: Number of channels supported by hardware
+- is_private: The device channels should be marked as private and not for by the
+  general purpose DMA channel allocator. False if not passed.
+- chan_allocation_order: order of allocation of channel, 0 (default): ascending,
+  1: descending
+- chan_priority: priority of channels. 0 (default): increase from chan 0->n, 1:
+  increase from chan n->0
+- block_size: Maximum block size supported by the controller
+- nr_masters: Number of AHB masters supported by the controller
+- data_width: Maximum data width supported by hardware per AHB master
+  (0 - 8bits, 1 - 16bits, ..., 5 - 256bits)
+- slave_info:
+	- bus_id: name of this device channel, not just a device name since
+	  devices may have more than one channel e.g. "foo_tx". For using the
+	  dw_generic_filter(), slave drivers must pass exactly this string as
+	  param to filter function.
+	- cfg_hi: Platform-specific initializer for the CFG_HI register
+	- cfg_lo: Platform-specific initializer for the CFG_LO register
+	- src_master: src master for transfers on allocated channel.
+	- dst_master: dest master for transfers on allocated channel.
 
 Example:
 
@@ -14,4 +34,28 @@ Example:
 		reg = <0xfc000000 0x1000>;
 		interrupt-parent = <&vic1>;
 		interrupts = <12>;
+
+		nr_channels = <8>;
+		chan_allocation_order = <1>;
+		chan_priority = <1>;
+		block_size = <0xfff>;
+		nr_masters = <2>;
+		data_width = <3 3 0 0>;
+
+		slave_info {
+			uart0-tx {
+				bus_id = "uart0-tx";
+				cfg_hi = <0x4000>;	/* 0x8 << 11 */
+				cfg_lo = <0>;
+				src_master = <0>;
+				dst_master = <1>;
+			};
+			spi0-tx {
+				bus_id = "spi0-tx";
+				cfg_hi = <0x2000>;	/* 0x4 << 11 */
+				cfg_lo = <0>;
+				src_master = <0>;
+				dst_master = <0>;
+			};
+		};
 	};
diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index c4b0eb3..9a7d084 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1179,6 +1179,58 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
 }
 
+bool dw_generic_filter(struct dma_chan *chan, void *param)
+{
+	struct dw_dma *dw = to_dw_dma(chan->device);
+	static struct dw_dma *last_dw;
+	static char *last_bus_id;
+	int found = 0, i = -1;
+
+	/*
+	 * dmaengine framework calls this routine for all channels of all dma
+	 * controller, until true is returned. If 'param' bus_id is not
+	 * registered with a dma controller (dw), then there is no need of
+	 * running below function for all channels of dw.
+	 *
+	 * This block of code does this by saving the parameters of last
+	 * failure. If dw and param are same, i.e. trying on same dw with
+	 * different channel, return false.
+	 */
+	if (last_dw) {
+		if ((last_bus_id == param) && (last_dw == dw))
+			return false;
+	}
+
+	/*
+	 * Return true:
+	 * - If dw_dma's platform data is not filled with slave info, then all
+	 *   dma controllers are fine for transfer.
+	 * - Or if param is NULL
+	 */
+	if (!dw->sd || !param)
+		return true;
+
+	while (++i < dw->sd_count) {
+		if (!strcmp(dw->sd[i].bus_id, param)) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (!found) {
+		last_dw = dw;
+		last_bus_id = param;
+		return false;
+	}
+
+	chan->private = &dw->sd[i];
+	last_dw = NULL;
+	last_bus_id = NULL;
+
+	return true;
+}
+EXPORT_SYMBOL(dw_generic_filter);
+
 /* --------------------- Cyclic DMA API extensions -------------------- */
 
 /**
@@ -1462,6 +1514,96 @@ static void dw_dma_off(struct dw_dma *dw)
 		dw->chan[i].initialized = false;
 }
 
+#ifdef CONFIG_OF
+static struct dw_dma_platform_data *
+__devinit dw_dma_parse_dt(struct platform_device *pdev)
+{
+	struct device_node *sn, *cn, *np = pdev->dev.of_node;
+	struct dw_dma_platform_data *pdata;
+	struct dw_dma_slave *sd;
+	u32 count, val, arr[4];
+
+	if (!np) {
+		dev_err(&pdev->dev, "Missing DT data\n");
+		return NULL;
+	}
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	if (of_property_read_u32(np, "nr_channels", &pdata->nr_channels))
+		return NULL;
+
+	if (of_property_read_bool(np, "is_private"))
+		pdata->is_private = true;
+
+	if (!of_property_read_u32(np, "chan_allocation_order",
+				&val))
+		pdata->chan_allocation_order = (unsigned char)val;
+
+	if (!of_property_read_u32(np, "chan_priority", &val))
+		pdata->chan_priority = (unsigned char)val;
+
+	if (!of_property_read_u32(np, "block_size", &val))
+		pdata->block_size = (unsigned short)val;
+
+	if (!of_property_read_u32(np, "nr_masters", &val)) {
+		if (val > 4)
+			return NULL;
+
+		pdata->nr_masters = (unsigned char)val;
+	}
+
+	if (!of_property_read_u32_array(np, "data_width", arr,
+				pdata->nr_masters))
+		for (count = 0; count < pdata->nr_masters; count++)
+			pdata->data_width[count] = arr[count];
+
+	/* parse slave data */
+	sn = of_find_node_by_name(np, "slave_info");
+	if (!sn)
+		return pdata;
+
+	count = 0;
+	/* calculate number of slaves */
+	for_each_child_of_node(sn, cn)
+		count++;
+
+	if (!count)
+		return NULL;
+
+	sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * count, GFP_KERNEL);
+	if (!sd)
+		return NULL;
+
+	count = 0;
+	for_each_child_of_node(sn, cn) {
+		of_property_read_string(cn, "bus_id", &sd[count].bus_id);
+		of_property_read_u32(cn, "cfg_hi", &sd[count].cfg_hi);
+		of_property_read_u32(cn, "cfg_lo", &sd[count].cfg_lo);
+		if (!of_property_read_u32(cn, "src_master", &val))
+			sd[count].src_master = (u8)val;
+
+		if (!of_property_read_u32(cn, "dst_master", &val))
+			sd[count].dst_master = (u8)val;
+
+		sd[count].dma_dev = &pdev->dev;
+		count++;
+	}
+
+	pdata->sd = sd;
+	pdata->sd_count = count;
+
+	return pdata;
+}
+#else
+static inline int dw_dma_parse_dt(struct platform_device *pdev)
+{
+	return -ENOSYS;
+}
+#endif
+
 static int __devinit dw_probe(struct platform_device *pdev)
 {
 	struct dw_dma_platform_data *pdata;
@@ -1478,6 +1620,9 @@ static int __devinit dw_probe(struct platform_device *pdev)
 	int			i;
 
 	pdata = dev_get_platdata(&pdev->dev);
+	if (!pdata)
+		pdata = dw_dma_parse_dt(pdev);
+
 	if (!pdata || pdata->nr_channels > DW_DMA_MAX_NR_CHANNELS)
 		return -EINVAL;
 
@@ -1512,6 +1657,8 @@ static int __devinit dw_probe(struct platform_device *pdev)
 	clk_prepare_enable(dw->clk);
 
 	dw->regs = regs;
+	dw->sd = pdata->sd;
+	dw->sd_count = pdata->sd_count;
 
 	/* get hardware configuration parameters */
 	if (autocfg) {
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index ff39fa6..5cc61ba 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -231,6 +231,10 @@ struct dw_dma {
 	struct tasklet_struct	tasklet;
 	struct clk		*clk;
 
+	/* slave information */
+	struct dw_dma_slave	*sd;
+	unsigned int		sd_count;
+
 	u8			all_chan_mask;
 
 	/* hardware configuration */
diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
index 62a6190..4d1c8c3 100644
--- a/include/linux/dw_dmac.h
+++ b/include/linux/dw_dmac.h
@@ -15,6 +15,26 @@
 #include <linux/dmaengine.h>
 
 /**
+ * struct dw_dma_slave - Controller-specific information about a slave
+ *
+ * @dma_dev: required DMA master device. Depricated.
+ * @bus_id: name of this device channel, not just a device name since
+ *          devices may have more than one channel e.g. "foo_tx"
+ * @cfg_hi: Platform-specific initializer for the CFG_HI register
+ * @cfg_lo: Platform-specific initializer for the CFG_LO register
+ * @src_master: src master for transfers on allocated channel.
+ * @dst_master: dest master for transfers on allocated channel.
+ */
+struct dw_dma_slave {
+	struct device		*dma_dev;
+	const char		*bus_id;
+	u32			cfg_hi;
+	u32			cfg_lo;
+	u8			src_master;
+	u8			dst_master;
+};
+
+/**
  * struct dw_dma_platform_data - Controller configuration parameters
  * @nr_channels: Number of channels supported by hardware (max 8)
  * @is_private: The device channels should be marked as private and not for
@@ -25,6 +45,8 @@
  * @nr_masters: Number of AHB masters supported by the controller
  * @data_width: Maximum data width supported by hardware per AHB master
  *		(0 - 8bits, 1 - 16bits, ..., 5 - 256bits)
+ * @sd: slave specific data. Used for configuring channels
+ * @sd_count: count of slave data structures passed.
  */
 struct dw_dma_platform_data {
 	unsigned int	nr_channels;
@@ -38,6 +60,9 @@ struct dw_dma_platform_data {
 	unsigned short	block_size;
 	unsigned char	nr_masters;
 	unsigned char	data_width[4];
+
+	struct dw_dma_slave *sd;
+	unsigned int sd_count;
 };
 
 /* bursts size */
@@ -52,23 +77,6 @@ enum dw_dma_msize {
 	DW_DMA_MSIZE_256,
 };
 
-/**
- * struct dw_dma_slave - Controller-specific information about a slave
- *
- * @dma_dev: required DMA master device
- * @cfg_hi: Platform-specific initializer for the CFG_HI register
- * @cfg_lo: Platform-specific initializer for the CFG_LO register
- * @src_master: src master for transfers on allocated channel.
- * @dst_master: dest master for transfers on allocated channel.
- */
-struct dw_dma_slave {
-	struct device		*dma_dev;
-	u32			cfg_hi;
-	u32			cfg_lo;
-	u8			src_master;
-	u8			dst_master;
-};
-
 /* Platform-configurable bits in CFG_HI */
 #define DWC_CFGH_FCMODE		(1 << 0)
 #define DWC_CFGH_FIFO_MODE	(1 << 1)
@@ -106,5 +114,6 @@ void dw_dma_cyclic_stop(struct dma_chan *chan);
 dma_addr_t dw_dma_get_src_addr(struct dma_chan *chan);
 
 dma_addr_t dw_dma_get_dst_addr(struct dma_chan *chan);
+bool dw_generic_filter(struct dma_chan *chan, void *param);
 
 #endif /* DW_DMAC_H */
-- 
1.7.12.rc2.18.g61b472e

[PATCH 3/3] ARM: SPEAr13xx: Pass DW DMAC platform data from DT

[Viresh Kumar]

This patch adds dw_dmac's platform data to DT node. It also creates slave info
node for SPEAr13xx, for the devices which were using dw_dmac.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm/boot/dts/spear1340.dtsi             | 19 ++++++++++
 arch/arm/boot/dts/spear13xx.dtsi             | 38 ++++++++++++++++++++
 arch/arm/mach-spear13xx/include/mach/spear.h |  2 --
 arch/arm/mach-spear13xx/spear1310.c          |  4 +--
 arch/arm/mach-spear13xx/spear1340.c          | 27 +++-----------
 arch/arm/mach-spear13xx/spear13xx.c          | 54 ++--------------------------
 6 files changed, 65 insertions(+), 79 deletions(-)

diff --git a/arch/arm/boot/dts/spear1340.dtsi b/arch/arm/boot/dts/spear1340.dtsi
index d71fe2a..8ea3f66 100644
--- a/arch/arm/boot/dts/spear1340.dtsi
+++ b/arch/arm/boot/dts/spear1340.dtsi
@@ -24,6 +24,25 @@
 			status = "disabled";
 		};
 
+		dma at ea800000 {
+			slave_info {
+				uart1_tx {
+					bus_id = "uart1_tx";
+					cfg_hi = <0x6000>;	/* 0xC << 11 */
+					cfg_lo = <0>;
+					src_master = <0>;
+					dst_master = <1>;
+				};
+				uart1_tx {
+					bus_id = "uart1_tx";
+					cfg_hi = <0x680>;	/* 0xD << 7 */
+					cfg_lo = <0>;
+					src_master = <1>;
+					dst_master = <0>;
+				};
+			};
+		};
+
 		spi1: spi at 5d400000 {
 			compatible = "arm,pl022", "arm,primecell";
 			reg = <0x5d400000 0x1000>;
diff --git a/arch/arm/boot/dts/spear13xx.dtsi b/arch/arm/boot/dts/spear13xx.dtsi
index f7b84ac..f06bb50 100644
--- a/arch/arm/boot/dts/spear13xx.dtsi
+++ b/arch/arm/boot/dts/spear13xx.dtsi
@@ -91,6 +91,37 @@
 			reg = <0xea800000 0x1000>;
 			interrupts = <0 19 0x4>;
 			status = "disabled";
+
+			nr_channels = <8>;
+			chan_allocation_order = <1>;
+			chan_priority = <1>;
+			block_size = <0xfff>;
+			nr_masters = <2>;
+			data_width = <3 3 0 0>;
+
+			slave_info {
+				ssp0_tx {
+					bus_id = "ssp0_tx";
+					cfg_hi = <0x2000>;	/* 0x4 << 11 */
+					cfg_lo = <0>;
+					src_master = <0>;
+					dst_master = <0>;
+				};
+				ssp0_rx {
+					bus_id = "ssp0_rx";
+					cfg_hi = <0x280>;	/* 0x5 << 7 */
+					cfg_lo = <0>;
+					src_master = <0>;
+					dst_master = <0>;
+				};
+				cf {
+					bus_id = "cf";
+					cfg_hi = <0>;
+					cfg_lo = <0>;
+					src_master = <0>;
+					dst_master = <0>;
+				};
+			};
 		};
 
 		dma at eb000000 {
@@ -98,6 +129,13 @@
 			reg = <0xeb000000 0x1000>;
 			interrupts = <0 59 0x4>;
 			status = "disabled";
+
+			nr_channels = <8>;
+			chan_allocation_order = <1>;
+			chan_priority = <1>;
+			block_size = <0xfff>;
+			nr_masters = <2>;
+			data_width = <3 3 0 0>;
 		};
 
 		fsmc: flash at b0000000 {
diff --git a/arch/arm/mach-spear13xx/include/mach/spear.h b/arch/arm/mach-spear13xx/include/mach/spear.h
index 07d90ac..71bf5b6 100644
--- a/arch/arm/mach-spear13xx/include/mach/spear.h
+++ b/arch/arm/mach-spear13xx/include/mach/spear.h
@@ -43,8 +43,6 @@
 #define VA_L2CC_BASE				IOMEM(UL(0xFB000000))
 
 /* others */
-#define DMAC0_BASE				UL(0xEA800000)
-#define DMAC1_BASE				UL(0xEB000000)
 #define MCIF_CF_BASE				UL(0xB2800000)
 
 /* Devices present in SPEAr1310 */
diff --git a/arch/arm/mach-spear13xx/spear1310.c b/arch/arm/mach-spear13xx/spear1310.c
index 9fbbfc5..0e60195 100644
--- a/arch/arm/mach-spear13xx/spear1310.c
+++ b/arch/arm/mach-spear13xx/spear1310.c
@@ -36,9 +36,7 @@ static struct pl022_ssp_controller ssp1_plat_data = {
 
 /* Add SPEAr1310 auxdata to pass platform data */
 static struct of_dev_auxdata spear1310_auxdata_lookup[] __initdata = {
-	OF_DEV_AUXDATA("arasan,cf-spear1340", MCIF_CF_BASE, NULL, &cf_dma_priv),
-	OF_DEV_AUXDATA("snps,dma-spear1340", DMAC0_BASE, NULL, &dmac_plat_data),
-	OF_DEV_AUXDATA("snps,dma-spear1340", DMAC1_BASE, NULL, &dmac_plat_data),
+	OF_DEV_AUXDATA("arasan,cf-spear1340", MCIF_CF_BASE, NULL, "cf"),
 	OF_DEV_AUXDATA("arm,pl022", SSP_BASE, NULL, &pl022_plat_data),
 
 	OF_DEV_AUXDATA("arm,pl022", SPEAR1310_SSP1_BASE, NULL, &ssp1_plat_data),
diff --git a/arch/arm/mach-spear13xx/spear1340.c b/arch/arm/mach-spear13xx/spear1340.c
index 081014f..4ea4546 100644
--- a/arch/arm/mach-spear13xx/spear1340.c
+++ b/arch/arm/mach-spear13xx/spear1340.c
@@ -20,7 +20,6 @@
 #include <linux/of_platform.h>
 #include <asm/hardware/gic.h>
 #include <asm/mach/arch.h>
-#include <mach/dma.h>
 #include <mach/generic.h>
 #include <mach/spear.h>
 
@@ -78,26 +77,10 @@
 			(SPEAR1340_MIPHY_OSC_BYPASS_EXT | \
 			SPEAR1340_MIPHY_PLL_RATIO_TOP(25))
 
-static struct dw_dma_slave uart1_dma_param[] = {
-	{
-		/* Tx */
-		.cfg_hi = DWC_CFGH_DST_PER(SPEAR1340_DMA_REQ_UART1_TX),
-		.cfg_lo = 0,
-		.src_master = DMA_MASTER_MEMORY,
-		.dst_master = SPEAR1340_DMA_MASTER_UART1,
-	}, {
-		/* Rx */
-		.cfg_hi = DWC_CFGH_SRC_PER(SPEAR1340_DMA_REQ_UART1_RX),
-		.cfg_lo = 0,
-		.src_master = SPEAR1340_DMA_MASTER_UART1,
-		.dst_master = DMA_MASTER_MEMORY,
-	}
-};
-
 static struct amba_pl011_data uart1_data = {
-	.dma_filter = dw_dma_filter,
-	.dma_tx_param = &uart1_dma_param[0],
-	.dma_rx_param = &uart1_dma_param[1],
+	.dma_filter = dw_generic_filter,
+	.dma_tx_param = "uart1_tx",
+	.dma_rx_param = "uart1_rx",
 };
 
 /* SATA device registration */
@@ -158,9 +141,7 @@ static struct ahci_platform_data sata_pdata = {
 
 /* Add SPEAr1340 auxdata to pass platform data */
 static struct of_dev_auxdata spear1340_auxdata_lookup[] __initdata = {
-	OF_DEV_AUXDATA("arasan,cf-spear1340", MCIF_CF_BASE, NULL, &cf_dma_priv),
-	OF_DEV_AUXDATA("snps,dma-spear1340", DMAC0_BASE, NULL, &dmac_plat_data),
-	OF_DEV_AUXDATA("snps,dma-spear1340", DMAC1_BASE, NULL, &dmac_plat_data),
+	OF_DEV_AUXDATA("arasan,cf-spear1340", MCIF_CF_BASE, NULL, "cf"),
 	OF_DEV_AUXDATA("arm,pl022", SSP_BASE, NULL, &pl022_plat_data),
 
 	OF_DEV_AUXDATA("snps,spear-ahci", SPEAR1340_SATA_BASE, NULL,
diff --git a/arch/arm/mach-spear13xx/spear13xx.c b/arch/arm/mach-spear13xx/spear13xx.c
index 5633d69..90ec4ad 100644
--- a/arch/arm/mach-spear13xx/spear13xx.c
+++ b/arch/arm/mach-spear13xx/spear13xx.c
@@ -22,67 +22,19 @@
 #include <asm/hardware/gic.h>
 #include <asm/mach/map.h>
 #include <asm/smp_twd.h>
-#include <mach/dma.h>
 #include <mach/generic.h>
 #include <mach/spear.h>
 
-/* common dw_dma filter routine to be used by peripherals */
-bool dw_dma_filter(struct dma_chan *chan, void *slave)
-{
-	struct dw_dma_slave *dws = (struct dw_dma_slave *)slave;
-
-	if (chan->device->dev == dws->dma_dev) {
-		chan->private = slave;
-		return true;
-	} else {
-		return false;
-	}
-}
-
 /* ssp device registration */
-static struct dw_dma_slave ssp_dma_param[] = {
-	{
-		/* Tx */
-		.cfg_hi = DWC_CFGH_DST_PER(DMA_REQ_SSP0_TX),
-		.cfg_lo = 0,
-		.src_master = DMA_MASTER_MEMORY,
-		.dst_master = DMA_MASTER_SSP0,
-	}, {
-		/* Rx */
-		.cfg_hi = DWC_CFGH_SRC_PER(DMA_REQ_SSP0_RX),
-		.cfg_lo = 0,
-		.src_master = DMA_MASTER_SSP0,
-		.dst_master = DMA_MASTER_MEMORY,
-	}
-};
-
 struct pl022_ssp_controller pl022_plat_data = {
 	.bus_id = 0,
 	.enable_dma = 1,
-	.dma_filter = dw_dma_filter,
-	.dma_rx_param = &ssp_dma_param[1],
-	.dma_tx_param = &ssp_dma_param[0],
+	.dma_filter = dw_generic_filter,
+	.dma_rx_param = "ssp0_rx",
+	.dma_tx_param = "ssp0_tx",
 	.num_chipselect = 3,
 };
 
-/* CF device registration */
-struct dw_dma_slave cf_dma_priv = {
-	.cfg_hi = 0,
-	.cfg_lo = 0,
-	.src_master = 0,
-	.dst_master = 0,
-};
-
-/* dmac device registeration */
-struct dw_dma_platform_data dmac_plat_data = {
-	.nr_channels = 8,
-	.chan_allocation_order = CHAN_ALLOCATION_DESCENDING,
-	.chan_priority = CHAN_PRIORITY_DESCENDING,
-	.block_size = 4095U,
-	.nr_masters = 2,
-	.data_width = { 3, 3, 0, 0 },
-};
-
 void __init spear13xx_l2x0_init(void)
 {
 	/*
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[viresh kumar]

On Fri, Oct 12, 2012 at 11:14 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> dw_dmac driver already supports device tree but it used to have its platform
> data passed the non-DT way.
>
> This patch does following changes:
> - pass platform data via DT, non-DT way still takes precedence if both are used.
> - create generic filter routine
> - Earlier slave information was made available by slave specific filter routines
>   in chan->private field. Now, this information would be passed from within dmac
>   DT node. Slave drivers would now be required to pass bus_id (a string) as
>   parameter to this generic filter(), which would be compared against the slave
>   data passed from DT, by the generic filter routine.
> - Update binding document

DT parsing of this patch can be tested with following non-official patch :)


    dmaengine: dw_dmac: Add dt params debug routine

    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/dma/dw_dmac.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 05c1dff..569914d 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1504,6 +1504,43 @@ static inline int dw_dma_parse_dt(struct
platform_device *pdev)
 }
 #endif

+static void dw_dma_parse_dt_debug(struct dw_dma_platform_data *pdata)
+{
+       int i = -1;
+
+       if (!pdata) {
+               printk(KERN_ERR "dw_dma: unable to read info from DT\n");
+               return;
+       }
+
+       printk(KERN_ERR "\nPrinting dw_dma DT info\n");
+
+       printk(KERN_ERR "nr_channels: %x\n", pdata->nr_channels);
+       printk(KERN_ERR "is_private: %x\n", pdata->is_private);
+       printk(KERN_ERR "chan_allocation_order: %x\n",
+               pdata->chan_allocation_order);
+
+       printk(KERN_ERR "chan_priority: %x\n", pdata->chan_priority);
+       printk(KERN_ERR "block_size: %x\n", pdata->block_size);
+
+       printk(KERN_ERR "nr_masters: %x\n", pdata->nr_masters);
+       printk(KERN_ERR "data_width: %d %d %d %d\n", pdata->data_width[0],
+               pdata->data_width[1], pdata->data_width[2],
+               pdata->data_width[3]);
+
+       /* parse slave data */
+       printk(KERN_ERR "slave_info\n");
+
+       while (++i < pdata->sd_count) {
+               printk(KERN_INFO "bus_id: %s\n", pdata->sd[i].bus_id);
+               printk(KERN_INFO "cfg_hi: %x\n", pdata->sd[i].cfg_hi);
+               printk(KERN_INFO "cfg_lo: %x\n", pdata->sd[i].cfg_lo);
+               printk(KERN_INFO "src_master: %x\n",
+                               pdata->sd[i].src_master);
+               printk(KERN_INFO "dst_master: %x\n",
+                               pdata->sd[i].dst_master);
+       }
+}
 static int __devinit dw_probe(struct platform_device *pdev)
 {
        struct dw_dma_platform_data *pdata;
@@ -1515,9 +1552,12 @@ static int __devinit dw_probe(struct
platform_device *pdev)
        int                     i;

        pdata = dev_get_platdata(&pdev->dev);
-       if (!pdata)
+       if (!pdata) {
                pdata = dw_dma_parse_dt(pdev);
+               dw_dma_parse_dt_debug(pdata);
+       }

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Andy Shevchenko]

On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote: 
> dw_dmac driver already supports device tree but it used to have its platform
> data passed the non-DT way.

My few comments are below.

> 
> This patch does following changes:
> - pass platform data via DT, non-DT way still takes precedence if both are used.
> - create generic filter routine
> - Earlier slave information was made available by slave specific filter routines
>   in chan->private field. Now, this information would be passed from within dmac
>   DT node. Slave drivers would now be required to pass bus_id (a string) as
>   parameter to this generic filter(), which would be compared against the slave
>   data passed from DT, by the generic filter routine.
> - Update binding document
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/devicetree/bindings/dma/snps-dma.txt |  44 ++++++
>  drivers/dma/dw_dmac.c                              | 147 +++++++++++++++++++++
>  drivers/dma/dw_dmac_regs.h                         |   4 +
>  include/linux/dw_dmac.h                            |  43 +++---
>  4 files changed, 221 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
> index c0d85db..5bb3dfb 100644
> --- a/Documentation/devicetree/bindings/dma/snps-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
> @@ -6,6 +6,26 @@ Required properties:
>  - interrupt-parent: Should be the phandle for the interrupt controller
>    that services interrupts for this device
>  - interrupt: Should contain the DMAC interrupt number
> +- nr_channels: Number of channels supported by hardware
> +- is_private: The device channels should be marked as private and not for by the
> +  general purpose DMA channel allocator. False if not passed.
> +- chan_allocation_order: order of allocation of channel, 0 (default): ascending,
> +  1: descending
> +- chan_priority: priority of channels. 0 (default): increase from chan 0->n, 1:
> +  increase from chan n->0
> +- block_size: Maximum block size supported by the controller
> +- nr_masters: Number of AHB masters supported by the controller
> +- data_width: Maximum data width supported by hardware per AHB master
> +  (0 - 8bits, 1 - 16bits, ..., 5 - 256bits)
> +- slave_info:
> +	- bus_id: name of this device channel, not just a device name since
> +	  devices may have more than one channel e.g. "foo_tx". For using the
> +	  dw_generic_filter(), slave drivers must pass exactly this string as
> +	  param to filter function.
> +	- cfg_hi: Platform-specific initializer for the CFG_HI register
> +	- cfg_lo: Platform-specific initializer for the CFG_LO register
> +	- src_master: src master for transfers on allocated channel.
> +	- dst_master: dest master for transfers on allocated channel.
>  
>  Example:
>  
> @@ -14,4 +34,28 @@ Example:
>  		reg = <0xfc000000 0x1000>;
>  		interrupt-parent = <&vic1>;
>  		interrupts = <12>;
> +
> +		nr_channels = <8>;
> +		chan_allocation_order = <1>;
> +		chan_priority = <1>;
> +		block_size = <0xfff>;
> +		nr_masters = <2>;
> +		data_width = <3 3 0 0>;
> +
> +		slave_info {
> +			uart0-tx {
> +				bus_id = "uart0-tx";
> +				cfg_hi = <0x4000>;	/* 0x8 << 11 */
> +				cfg_lo = <0>;
> +				src_master = <0>;
> +				dst_master = <1>;
> +			};
> +			spi0-tx {
> +				bus_id = "spi0-tx";
> +				cfg_hi = <0x2000>;	/* 0x4 << 11 */
> +				cfg_lo = <0>;
> +				src_master = <0>;
> +				dst_master = <0>;
> +			};
> +		};
Why do you locate slave information under DMA controller node? From my
point of view the slave info belongs to corresponding device. For
example, above sections belong to UART0 and SPI0.

> 	};
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index c4b0eb3..9a7d084 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -1179,6 +1179,58 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
>  	dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
>  }
>  
> +bool dw_generic_filter(struct dma_chan *chan, void *param)
> +{
> +	struct dw_dma *dw = to_dw_dma(chan->device);
> +	static struct dw_dma *last_dw;
> +	static char *last_bus_id;
> +	int found = 0, i = -1;
> +
> +	/*
> +	 * dmaengine framework calls this routine for all channels of all dma
> +	 * controller, until true is returned. If 'param' bus_id is not
> +	 * registered with a dma controller (dw), then there is no need of
> +	 * running below function for all channels of dw.
> +	 *
> +	 * This block of code does this by saving the parameters of last
> +	 * failure. If dw and param are same, i.e. trying on same dw with
> +	 * different channel, return false.
> +	 */
> +	if (last_dw) {
> +		if ((last_bus_id == param) && (last_dw == dw))
> +			return false;
> +	}
> +
> +	/*
> +	 * Return true:
> +	 * - If dw_dma's platform data is not filled with slave info, then all
> +	 *   dma controllers are fine for transfer.
> +	 * - Or if param is NULL
> +	 */
> +	if (!dw->sd || !param)
> +		return true;
> +
> +	while (++i < dw->sd_count) {
> +		if (!strcmp(dw->sd[i].bus_id, param)) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if (!found) {
> +		last_dw = dw;
> +		last_bus_id = param;
> +		return false;
Because of return here you could eliminate 'found' flag at all. 
> +	}
> +
> +	chan->private = &dw->sd[i];
> +	last_dw = NULL;
> +	last_bus_id = NULL;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(dw_generic_filter);
> +
>  /* --------------------- Cyclic DMA API extensions -------------------- */
>  
>  /**
> @@ -1462,6 +1514,96 @@ static void dw_dma_off(struct dw_dma *dw)
>  		dw->chan[i].initialized = false;
>  }
>  
> +#ifdef CONFIG_OF
> +static struct dw_dma_platform_data *
> +__devinit dw_dma_parse_dt(struct platform_device *pdev)
> +{
> +	struct device_node *sn, *cn, *np = pdev->dev.of_node;
> +	struct dw_dma_platform_data *pdata;
> +	struct dw_dma_slave *sd;
> +	u32 count, val, arr[4];
> +
> +	if (!np) {
> +		dev_err(&pdev->dev, "Missing DT data\n");
> +		return NULL;
> +	}
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return NULL;
> +
> +	if (of_property_read_u32(np, "nr_channels", &pdata->nr_channels))
> +		return NULL;
> +
> +	if (of_property_read_bool(np, "is_private"))
> +		pdata->is_private = true;
> +
> +	if (!of_property_read_u32(np, "chan_allocation_order",
> +				&val))
> +		pdata->chan_allocation_order = (unsigned char)val;
> +
> +	if (!of_property_read_u32(np, "chan_priority", &val))
> +		pdata->chan_priority = (unsigned char)val;
> +
> +	if (!of_property_read_u32(np, "block_size", &val))
> +		pdata->block_size = (unsigned short)val;
> +
> +	if (!of_property_read_u32(np, "nr_masters", &val)) {
> +		if (val > 4)
> +			return NULL;
> +
> +		pdata->nr_masters = (unsigned char)val;
> +	}
> +
> +	if (!of_property_read_u32_array(np, "data_width", arr,
> +				pdata->nr_masters))
> +		for (count = 0; count < pdata->nr_masters; count++)
> +			pdata->data_width[count] = arr[count];
> +
> +	/* parse slave data */
> +	sn = of_find_node_by_name(np, "slave_info");
> +	if (!sn)
> +		return pdata;
> +
> +	count = 0;
> +	/* calculate number of slaves */
> +	for_each_child_of_node(sn, cn)
> +		count++;
Is there any other way to get amount of children?

> +
> +	if (!count)
> +		return NULL;
> +
> +	sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * count, GFP_KERNEL);
> +	if (!sd)
> +		return NULL;
> +
> +	count = 0;
> +	for_each_child_of_node(sn, cn) {
> +		of_property_read_string(cn, "bus_id", &sd[count].bus_id);
> +		of_property_read_u32(cn, "cfg_hi", &sd[count].cfg_hi);
> +		of_property_read_u32(cn, "cfg_lo", &sd[count].cfg_lo);
> +		if (!of_property_read_u32(cn, "src_master", &val))
> +			sd[count].src_master = (u8)val;
> +
> +		if (!of_property_read_u32(cn, "dst_master", &val))
> +			sd[count].dst_master = (u8)val;
> +
> +		sd[count].dma_dev = &pdev->dev;
> +		count++;
> +	}
> +
> +	pdata->sd = sd;
> +	pdata->sd_count = count;
> +
> +	return pdata;
Here you return NULL or valid pointer 
> +}
> +#else
> +static inline int dw_dma_parse_dt(struct platform_device *pdev)
> +{
> +	return -ENOSYS;
...here you return an error. 
> +}
> +#endif
> +
>  static int __devinit dw_probe(struct platform_device *pdev)
>  {
>  	struct dw_dma_platform_data *pdata;
> @@ -1478,6 +1620,9 @@ static int __devinit dw_probe(struct platform_device *pdev)
>  	int			i;
>  
>  	pdata = dev_get_platdata(&pdev->dev);
> +	if (!pdata)
> +		pdata = dw_dma_parse_dt(pdev);
> +
>  	if (!pdata || pdata->nr_channels > DW_DMA_MAX_NR_CHANNELS)
...and here you didn't check for an error. 
> 		return -EINVAL;
>  
> @@ -1512,6 +1657,8 @@ static int __devinit dw_probe(struct platform_device *pdev)
>  	clk_prepare_enable(dw->clk);
>  
>  	dw->regs = regs;
> +	dw->sd = pdata->sd;
> +	dw->sd_count = pdata->sd_count;
>  
>  	/* get hardware configuration parameters */
>  	if (autocfg) {
> diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
> index ff39fa6..5cc61ba 100644
> --- a/drivers/dma/dw_dmac_regs.h
> +++ b/drivers/dma/dw_dmac_regs.h
> @@ -231,6 +231,10 @@ struct dw_dma {
>  	struct tasklet_struct	tasklet;
>  	struct clk		*clk;
>  
> +	/* slave information */
> +	struct dw_dma_slave	*sd;
> +	unsigned int		sd_count;
> +
>  	u8			all_chan_mask;
>  
>  	/* hardware configuration */
> diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
> index 62a6190..4d1c8c3 100644
> --- a/include/linux/dw_dmac.h
> +++ b/include/linux/dw_dmac.h
> @@ -15,6 +15,26 @@
>  #include <linux/dmaengine.h>
>  
>  /**
> + * struct dw_dma_slave - Controller-specific information about a slave
> + *
> + * @dma_dev: required DMA master device. Depricated.
> + * @bus_id: name of this device channel, not just a device name since
> + *          devices may have more than one channel e.g. "foo_tx"
> + * @cfg_hi: Platform-specific initializer for the CFG_HI register
> + * @cfg_lo: Platform-specific initializer for the CFG_LO register
> + * @src_master: src master for transfers on allocated channel.
> + * @dst_master: dest master for transfers on allocated channel.
> + */
> +struct dw_dma_slave {
> +	struct device		*dma_dev;
> +	const char		*bus_id;
> +	u32			cfg_hi;
> +	u32			cfg_lo;
> +	u8			src_master;
> +	u8			dst_master;
> +};
> +
> +/**
>   * struct dw_dma_platform_data - Controller configuration parameters
>   * @nr_channels: Number of channels supported by hardware (max 8)
>   * @is_private: The device channels should be marked as private and not for
> @@ -25,6 +45,8 @@
>   * @nr_masters: Number of AHB masters supported by the controller
>   * @data_width: Maximum data width supported by hardware per AHB master
>   *		(0 - 8bits, 1 - 16bits, ..., 5 - 256bits)
> + * @sd: slave specific data. Used for configuring channels
> + * @sd_count: count of slave data structures passed.
>   */
>  struct dw_dma_platform_data {
>  	unsigned int	nr_channels;
> @@ -38,6 +60,9 @@ struct dw_dma_platform_data {
>  	unsigned short	block_size;
>  	unsigned char	nr_masters;
>  	unsigned char	data_width[4];
> +
> +	struct dw_dma_slave *sd;
> +	unsigned int sd_count;
>  };
>  
>  /* bursts size */
> @@ -52,23 +77,6 @@ enum dw_dma_msize {
>  	DW_DMA_MSIZE_256,
>  };
>  
> -/**
> - * struct dw_dma_slave - Controller-specific information about a slave
> - *
> - * @dma_dev: required DMA master device
> - * @cfg_hi: Platform-specific initializer for the CFG_HI register
> - * @cfg_lo: Platform-specific initializer for the CFG_LO register
> - * @src_master: src master for transfers on allocated channel.
> - * @dst_master: dest master for transfers on allocated channel.
> - */
> -struct dw_dma_slave {
> -	struct device		*dma_dev;
> -	u32			cfg_hi;
> -	u32			cfg_lo;
> -	u8			src_master;
> -	u8			dst_master;
> -};
> -
>  /* Platform-configurable bits in CFG_HI */
>  #define DWC_CFGH_FCMODE		(1 << 0)
>  #define DWC_CFGH_FIFO_MODE	(1 << 1)
> @@ -106,5 +114,6 @@ void dw_dma_cyclic_stop(struct dma_chan *chan);
>  dma_addr_t dw_dma_get_src_addr(struct dma_chan *chan);
>  
>  dma_addr_t dw_dma_get_dst_addr(struct dma_chan *chan);
> +bool dw_generic_filter(struct dma_chan *chan, void *param);
>  
>  #endif /* DW_DMAC_H */

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Viresh Kumar]

On 12 October 2012 13:53, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote:

> My few comments are below.

Most welcome :)

>> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
>> @@ -14,4 +34,28 @@ Example:
>>               reg = <0xfc000000 0x1000>;
>>               interrupt-parent = <&vic1>;
>>               interrupts = <12>;
>> +
>> +             nr_channels = <8>;
>> +             chan_allocation_order = <1>;
>> +             chan_priority = <1>;
>> +             block_size = <0xfff>;
>> +             nr_masters = <2>;
>> +             data_width = <3 3 0 0>;
>> +
>> +             slave_info {
>> +                     uart0-tx {
>> +                             bus_id = "uart0-tx";
>> +                             cfg_hi = <0x4000>;      /* 0x8 << 11 */
>> +                             cfg_lo = <0>;
>> +                             src_master = <0>;
>> +                             dst_master = <1>;
>> +                     };
>> +                     spi0-tx {
>> +                             bus_id = "spi0-tx";
>> +                             cfg_hi = <0x2000>;      /* 0x4 << 11 */
>> +                             cfg_lo = <0>;
>> +                             src_master = <0>;
>> +                             dst_master = <0>;
>> +                     };
>> +             };
> Why do you locate slave information under DMA controller node? From my
> point of view the slave info belongs to corresponding device. For
> example, above sections belong to UART0 and SPI0.

Consider one spi driver is used on 5 different platforms with
different DMA controllers.
So, 5 DMA drivers and so 5 DMA platform_data. Wherever we add this node, this
data can't be processed by spi-driver, as we can't add DT processing routines
for all DMA drivers in spi.

The best place to process DT nodes is DW_DMAC driver, because it is
dw_dmac's data.
That's why i added them under DMA.

>> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

>> +     while (++i < dw->sd_count) {
>> +             if (!strcmp(dw->sd[i].bus_id, param)) {
>> +                     found = 1;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (!found) {
>> +             last_dw = dw;
>> +             last_bus_id = param;
>> +             return false;
> Because of return here you could eliminate 'found' flag at all.

Yeah.

>> +static struct dw_dma_platform_data *
>> +__devinit dw_dma_parse_dt(struct platform_device *pdev)
>> +{

>> +     for_each_child_of_node(sn, cn)
>> +             count++;
> Is there any other way to get amount of children?

I tried my best to find one, referred to lots of drivers. And found
this way in most of the places. ex: drivers/pinctrl/***

>> +     return pdata;
> Here you return NULL or valid pointer
>> +}
>> +#else
>> +static inline int dw_dma_parse_dt(struct platform_device *pdev)
>> +{
>> +     return -ENOSYS;
> ...here you return an error.

Look at prototype of both versions of these routines. Its a bug. Thanks
for pointing out.

>>       if (!pdata || pdata->nr_channels > DW_DMA_MAX_NR_CHANNELS)
> ...and here you didn't check for an error.

With above bug fixed, this will be automatically resolved as NULL is treated as
failure.

--
viresh

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Andy Shevchenko]

On Fri, Oct 12, 2012 at 11:34 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 12 October 2012 13:53, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote:

>>> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
>>> @@ -14,4 +34,28 @@ Example:
>>>               reg = <0xfc000000 0x1000>;
>>>               interrupt-parent = <&vic1>;
>>>               interrupts = <12>;
>>> +
>>> +             nr_channels = <8>;
>>> +             chan_allocation_order = <1>;
>>> +             chan_priority = <1>;
>>> +             block_size = <0xfff>;
>>> +             nr_masters = <2>;
>>> +             data_width = <3 3 0 0>;
>>> +
>>> +             slave_info {
>>> +                     uart0-tx {
>>> +                             bus_id = "uart0-tx";
>>> +                             cfg_hi = <0x4000>;      /* 0x8 << 11 */
>>> +                             cfg_lo = <0>;
>>> +                             src_master = <0>;
>>> +                             dst_master = <1>;
>>> +                     };
>>> +                     spi0-tx {
>>> +                             bus_id = "spi0-tx";
>>> +                             cfg_hi = <0x2000>;      /* 0x4 << 11 */
>>> +                             cfg_lo = <0>;
>>> +                             src_master = <0>;
>>> +                             dst_master = <0>;
>>> +                     };
>>> +             };
>> Why do you locate slave information under DMA controller node? From my
>> point of view the slave info belongs to corresponding device. For
>> example, above sections belong to UART0 and SPI0.
>
> Consider one spi driver is used on 5 different platforms with
> different DMA controllers.
> So, 5 DMA drivers and so 5 DMA platform_data. Wherever we add this node, this
> data can't be processed by spi-driver, as we can't add DT processing routines
> for all DMA drivers in spi.
>
> The best place to process DT nodes is DW_DMAC driver, because it is
> dw_dmac's data.
> That's why i added them under DMA.
Fair enough.

>>> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

>>> +static struct dw_dma_platform_data *
>>> +__devinit dw_dma_parse_dt(struct platform_device *pdev)
>>> +{
>
>>> +     for_each_child_of_node(sn, cn)
>>> +             count++;
>> Is there any other way to get amount of children?
>
> I tried my best to find one, referred to lots of drivers. And found
> this way in most of the places. ex: drivers/pinctrl/***
I understand your way of allocating memory, but what about using
linked list instead of array?

And one more thing. May be we could introduce backlink to the platform
data in the dw_dma structure instead of copying certain parameters?

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Viresh Kumar]

On 12 October 2012 14:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> I understand your way of allocating memory, but what about using
> linked list instead of array?

I gave it a thought. Overhead was 4 bytes more per slave structure +
more cycles in filter routine (arrays are more faster :) ).

Because the DT capture is a one time activity, its better to save time
in filter fns.

> And one more thing. May be we could introduce backlink to the platform
> data in the dw_dma structure instead of copying certain parameters?

Will do that in a separate patch, outside the scope of this patchset. :)

--
viresh

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[viresh kumar]

On Fri, Oct 12, 2012 at 1:53 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote:

>> +     while (++i < dw->sd_count) {
>> +             if (!strcmp(dw->sd[i].bus_id, param)) {
>> +                     found = 1;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (!found) {
>> +             last_dw = dw;
>> +             last_bus_id = param;
>> +             return false;
> Because of return here you could eliminate 'found' flag at all.

how?

Fixed the other issue as:

Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Fri Oct 12 16:06:32 2012 +0530

    fixup! dmaengine: dw_dmac: Enhance device tree support
---
 drivers/dma/dw_dmac.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 9a7d084..a4ff04c 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1598,9 +1598,10 @@ __devinit dw_dma_parse_dt(struct platform_device *pdev)
        return pdata;
 }
 #else
-static inline int dw_dma_parse_dt(struct platform_device *pdev)
+static inline struct dw_dma_platform_data *
+dw_dma_parse_dt(struct platform_device *pdev)
 {
-       return -ENOSYS;
+       return NULL;
 }
 #endif

--
viresh

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Andy Shevchenko]

On Fri, Oct 12, 2012 at 1:36 PM, viresh kumar <viresh.kumar@linaro.org> wrote:
> On Fri, Oct 12, 2012 at 1:53 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote:
>
>>> +     while (++i < dw->sd_count) {
>>> +             if (!strcmp(dw->sd[i].bus_id, param)) {
>>> +                     found = 1;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     if (!found) {
>>> +             last_dw = dw;
>>> +             last_bus_id = param;
>>> +             return false;
>> Because of return here you could eliminate 'found' flag at all.
> how?

while (++i < dw->sd_count) {
               if (!strcmp(dw->sd[i].bus_id, param)) {
       chan->private = &dw->sd[i];
       last_dw = NULL;
       last_bus_id = NULL;

       return true;
               }
       }

               last_dw = dw;
               last_bus_id = param;
               return false;

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Viresh Kumar]

On 12 October 2012 16:10, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Fri, Oct 12, 2012 at 1:36 PM, viresh kumar <viresh.kumar@linaro.org> wrote:
>> On Fri, Oct 12, 2012 at 1:53 PM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>> On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote:
>>
>>>> +     while (++i < dw->sd_count) {
>>>> +             if (!strcmp(dw->sd[i].bus_id, param)) {
>>>> +                     found = 1;
>>>> +                     break;

I was just not looking at this place.

>>>> +             }
>>>> +     }
>>>> +
>>>> +     if (!found) {
>>>> +             last_dw = dw;
>>>> +             last_bus_id = param;
>>>> +             return false;
>>> Because of return here you could eliminate 'found' flag at all.
>> how?

And was looking here only.

> while (++i < dw->sd_count) {
>                if (!strcmp(dw->sd[i].bus_id, param)) {
>        chan->private = &dw->sd[i];
>        last_dw = NULL;
>        last_bus_id = NULL;
>
>        return true;
>                }
>        }
>
>                last_dw = dw;
>                last_bus_id = param;
>                return false;

will be done.

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Viresh Kumar]

On 12 October 2012 16:10, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>> +     if (!found) {
>>>> +             last_dw = dw;
>>>> +             last_bus_id = param;
>>>> +             return false;
>>> Because of return here you could eliminate 'found' flag at all.

Here is the stuff copied from you ;)


diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index a4ff04c..c24859e 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1184,7 +1184,7 @@ bool dw_generic_filter(struct dma_chan *chan, void *param)
        struct dw_dma *dw = to_dw_dma(chan->device);
        static struct dw_dma *last_dw;
        static char *last_bus_id;
-       int found = 0, i = -1;
+       int i = -1;

        /*
         * dmaengine framework calls this routine for all channels of all dma
@@ -1212,22 +1212,17 @@ bool dw_generic_filter(struct dma_chan *chan,
void *param)

        while (++i < dw->sd_count) {
                if (!strcmp(dw->sd[i].bus_id, param)) {
-                       found = 1;
-                       break;
-               }
-       }
+                       chan->private = &dw->sd[i];
+                       last_dw = NULL;
+                       last_bus_id = NULL;

-       if (!found) {
-               last_dw = dw;
-               last_bus_id = param;
-               return false;
+                       return true;
+               }
        }

-       chan->private = &dw->sd[i];
-       last_dw = NULL;
-       last_bus_id = NULL;
-
-       return true;
+       last_dw = dw;
+       last_bus_id = param;
+       return false;
 }
 EXPORT_SYMBOL(dw_generic_filter);

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Andy Shevchenko]

On Fri, Oct 12, 2012 at 2:01 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 12 October 2012 16:10, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>>> +     if (!found) {
>>>>> +             last_dw = dw;
>>>>> +             last_bus_id = param;
>>>>> +             return false;
>>>> Because of return here you could eliminate 'found' flag at all.
>
> Here is the stuff copied from you ;)
>
>
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index a4ff04c..c24859e 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -1184,7 +1184,7 @@ bool dw_generic_filter(struct dma_chan *chan, void *param)
>         struct dw_dma *dw = to_dw_dma(chan->device);
>         static struct dw_dma *last_dw;
>         static char *last_bus_id;
> -       int found = 0, i = -1;
> +       int i = -1;
>
>         /*
>          * dmaengine framework calls this routine for all channels of all dma
> @@ -1212,22 +1212,17 @@ bool dw_generic_filter(struct dma_chan *chan,
> void *param)
>
>         while (++i < dw->sd_count) {
>                 if (!strcmp(dw->sd[i].bus_id, param)) {
> -                       found = 1;
> -                       break;
> -               }
> -       }
> +                       chan->private = &dw->sd[i];
> +                       last_dw = NULL;
> +                       last_bus_id = NULL;
>
> -       if (!found) {
> -               last_dw = dw;
> -               last_bus_id = param;
> -               return false;
> +                       return true;
> +               }
>         }
>
> -       chan->private = &dw->sd[i];
> -       last_dw = NULL;
> -       last_bus_id = NULL;
> -
> -       return true;
> +       last_dw = dw;
> +       last_bus_id = param;
> +       return false;
>  }
Yes, that what I refer to.

>  EXPORT_SYMBOL(dw_generic_filter);
Could we change a name to be more precise, like dw_dma_generic_filter ?


-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Viresh Kumar]

On 12 October 2012 16:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>  EXPORT_SYMBOL(dw_generic_filter);
> Could we change a name to be more precise, like dw_dma_generic_filter ?

Sure.

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Andy Shevchenko]

On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote: 
> dw_dmac driver already supports device tree but it used to have its platform
> data passed the non-DT way.
Another portion of comments.

> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

> +static struct dw_dma_platform_data *
> +__devinit dw_dma_parse_dt(struct platform_device *pdev)
> +{
> +	struct device_node *sn, *cn, *np = pdev->dev.of_node;
> +	struct dw_dma_platform_data *pdata;
> +	struct dw_dma_slave *sd;
> +	u32 count, val, arr[4];
> +
> +	if (!np) {
> +		dev_err(&pdev->dev, "Missing DT data\n");
> +		return NULL;
> +	}
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return NULL;
> +
> +	if (of_property_read_u32(np, "nr_channels", &pdata->nr_channels))
> +		return NULL;
> +
> +	if (of_property_read_bool(np, "is_private"))
> +		pdata->is_private = true;
> +
> +	if (!of_property_read_u32(np, "chan_allocation_order",
> +				&val))
Fits one line?

> +		pdata->chan_allocation_order = (unsigned char)val;
do we really need explicit casting here?

> +
> +	if (!of_property_read_u32(np, "chan_priority", &val))
> +		pdata->chan_priority = (unsigned char)val;
ditto

> +
> +	if (!of_property_read_u32(np, "block_size", &val))
> +		pdata->block_size = (unsigned short)val;
ditto 
> +
> +	if (!of_property_read_u32(np, "nr_masters", &val)) {
> +		if (val > 4)
I thought once that we might introduce constant like #define
DW_MAX_AHB_MASTERS 4. It seems a bit useless because hw is designed for
that number anyway, but maybe you have another opinion. 

> +			return NULL;
> +
> +		pdata->nr_masters = (unsigned char)val;
> +	}
> +
> +	if (!of_property_read_u32_array(np, "data_width", arr,
> +				pdata->nr_masters))
> +		for (count = 0; count < pdata->nr_masters; count++)
> +			pdata->data_width[count] = arr[count];
Ah, it would be nice to have of_property_read_u8_array and so on...

> +
> +	/* parse slave data */
> +	sn = of_find_node_by_name(np, "slave_info");
> +	if (!sn)
> +		return pdata;
> +
> +	count = 0;
> +	/* calculate number of slaves */
> +	for_each_child_of_node(sn, cn)
> +		count++;

of.h: static inline int of_get_child_count(const struct device_node *np)


> +
> +	if (!count)
> +		return NULL;
> +
> +	sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * count, GFP_KERNEL);
> +	if (!sd)
> +		return NULL;
> +
> +	count = 0;
> +	for_each_child_of_node(sn, cn) {
> +		of_property_read_string(cn, "bus_id", &sd[count].bus_id);
> +		of_property_read_u32(cn, "cfg_hi", &sd[count].cfg_hi);
> +		of_property_read_u32(cn, "cfg_lo", &sd[count].cfg_lo);
> +		if (!of_property_read_u32(cn, "src_master", &val))
> +			sd[count].src_master = (u8)val;
Explicit casting?

> +
> +		if (!of_property_read_u32(cn, "dst_master", &val))
> +			sd[count].dst_master = (u8)val;
ditto 
> +
> +		sd[count].dma_dev = &pdev->dev;
> +		count++;
> +	}

what about to define sd as sd[0]; in the platform_data structure and
then use smth like following

struct ... *sd = pdata->sd;
for_each... {
sd->... = ;
sd++;
}

it will probably require to split this part in a separate function and
calculate count of slave_info children before pdata memory allocation.


> +
> +	pdata->sd = sd;
> +	pdata->sd_count = count;
> +
> +	return pdata;
> +}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Shevchenko, Andriy]

On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote: 
> dw_dmac driver already supports device tree but it used to have its platform
> data passed the non-DT way.
Another portion of comments.

> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

> +static struct dw_dma_platform_data *
> +__devinit dw_dma_parse_dt(struct platform_device *pdev)
> +{
> +	struct device_node *sn, *cn, *np = pdev->dev.of_node;
> +	struct dw_dma_platform_data *pdata;
> +	struct dw_dma_slave *sd;
> +	u32 count, val, arr[4];
> +
> +	if (!np) {
> +		dev_err(&pdev->dev, "Missing DT data\n");
> +		return NULL;
> +	}
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return NULL;
> +
> +	if (of_property_read_u32(np, "nr_channels", &pdata->nr_channels))
> +		return NULL;
> +
> +	if (of_property_read_bool(np, "is_private"))
> +		pdata->is_private = true;
> +
> +	if (!of_property_read_u32(np, "chan_allocation_order",
> +				&val))
Fits one line?

> +		pdata->chan_allocation_order = (unsigned char)val;
do we really need explicit casting here?

> +
> +	if (!of_property_read_u32(np, "chan_priority", &val))
> +		pdata->chan_priority = (unsigned char)val;
ditto

> +
> +	if (!of_property_read_u32(np, "block_size", &val))
> +		pdata->block_size = (unsigned short)val;
ditto 
> +
> +	if (!of_property_read_u32(np, "nr_masters", &val)) {
> +		if (val > 4)
I thought once that we might introduce constant like #define
DW_MAX_AHB_MASTERS 4. It seems a bit useless because hw is designed for
that number anyway, but maybe you have another opinion. 

> +			return NULL;
> +
> +		pdata->nr_masters = (unsigned char)val;
> +	}
> +
> +	if (!of_property_read_u32_array(np, "data_width", arr,
> +				pdata->nr_masters))
> +		for (count = 0; count < pdata->nr_masters; count++)
> +			pdata->data_width[count] = arr[count];
Ah, it would be nice to have of_property_read_u8_array and so on...

> +
> +	/* parse slave data */
> +	sn = of_find_node_by_name(np, "slave_info");
> +	if (!sn)
> +		return pdata;
> +
> +	count = 0;
> +	/* calculate number of slaves */
> +	for_each_child_of_node(sn, cn)
> +		count++;

of.h: static inline int of_get_child_count(const struct device_node *np)


> +
> +	if (!count)
> +		return NULL;
> +
> +	sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * count, GFP_KERNEL);
> +	if (!sd)
> +		return NULL;
> +
> +	count = 0;
> +	for_each_child_of_node(sn, cn) {
> +		of_property_read_string(cn, "bus_id", &sd[count].bus_id);
> +		of_property_read_u32(cn, "cfg_hi", &sd[count].cfg_hi);
> +		of_property_read_u32(cn, "cfg_lo", &sd[count].cfg_lo);
> +		if (!of_property_read_u32(cn, "src_master", &val))
> +			sd[count].src_master = (u8)val;
Explicit casting?

> +
> +		if (!of_property_read_u32(cn, "dst_master", &val))
> +			sd[count].dst_master = (u8)val;
ditto 
> +
> +		sd[count].dma_dev = &pdev->dev;
> +		count++;
> +	}

what about to define sd as sd[0]; in the platform_data structure and
then use smth like following

struct ... *sd = pdata->sd;
for_each... {
sd->... = ;
sd++;
}

it will probably require to split this part in a separate function and
calculate count of slave_info children before pdata memory allocation.


> +
> +	pdata->sd = sd;
> +	pdata->sd_count = count;
> +
> +	return pdata;
> +}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Viresh Kumar]

On 12 October 2012 18:58, Shevchenko, Andriy
<andriy.shevchenko@intel.com> wrote:
> On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote:

>> +     if (!of_property_read_u32(np, "chan_allocation_order",
>> +                             &val))
> Fits one line?

yes.

>> +             pdata->chan_allocation_order = (unsigned char)val;
> do we really need explicit casting here?

Obviously not, bigger to smaller is automatically casted. My coding standard is
going down :(

Same for all casting comments.

>> +     if (!of_property_read_u32(np, "nr_masters", &val)) {
>> +             if (val > 4)
> I thought once that we might introduce constant like #define
> DW_MAX_AHB_MASTERS 4. It seems a bit useless because hw is designed for
> that number anyway, but maybe you have another opinion.

Not everyone have four masters in there SoC configurations. So its better
to export correct values.

>> +     if (!of_property_read_u32_array(np, "data_width", arr,
>> +                             pdata->nr_masters))
>> +             for (count = 0; count < pdata->nr_masters; count++)
>> +                     pdata->data_width[count] = arr[count];
> Ah, it would be nice to have of_property_read_u8_array and so on...

:)
Maybe yes. I don't want to do it with this patchset, as that will take more
time to go through maintainers.

>> +     /* calculate number of slaves */
>> +     for_each_child_of_node(sn, cn)
>> +             count++;
>
> of.h: static inline int of_get_child_count(const struct device_node *np)

Hehe.. Will use that.
This proves there is no efficient way of finding child nodes, than this.

>> +     for_each_child_of_node(sn, cn) {
>> +             of_property_read_string(cn, "bus_id", &sd[count].bus_id);
>> +             of_property_read_u32(cn, "cfg_hi", &sd[count].cfg_hi);
>> +             of_property_read_u32(cn, "cfg_lo", &sd[count].cfg_lo);
>> +             if (!of_property_read_u32(cn, "src_master", &val))
>> +                     sd[count].src_master = (u8)val;
>> +
>> +             if (!of_property_read_u32(cn, "dst_master", &val))
>> +                     sd[count].dst_master = (u8)val;
>> +
>> +             sd[count].dma_dev = &pdev->dev;
>> +             count++;
>> +     }
>
> what about to define sd as sd[0]; in the platform_data structure and
> then use smth like following
>
> struct ... *sd = pdata->sd;
> for_each... {
> sd->... = ;
> sd++;
> }
>
> it will probably require to split this part in a separate function and
> calculate count of slave_info children before pdata memory allocation.

Liked the idea partially. Check my new implementation in fixup for all
these comments:

---------------------------8<---------------------------

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Fri, 12 Oct 2012 19:35:44 +0530
Subject: [PATCH] fixup! dmaengine: dw_dmac: Enhance device tree support

---
 drivers/dma/dw_dmac.c   | 50 ++++++++++++++++++++++---------------------------
 include/linux/dw_dmac.h |  2 +-
 2 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index c24859e..d72c26f 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1179,7 +1179,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
 }

-bool dw_generic_filter(struct dma_chan *chan, void *param)
+bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
 {
 	struct dw_dma *dw = to_dw_dma(chan->device);
 	static struct dw_dma *last_dw;
@@ -1224,7 +1224,7 @@ bool dw_generic_filter(struct dma_chan *chan, void *param)
 	last_bus_id = param;
 	return false;
 }
-EXPORT_SYMBOL(dw_generic_filter);
+EXPORT_SYMBOL(dw_dma_generic_filter);

 /* --------------------- Cyclic DMA API extensions -------------------- */

@@ -1516,7 +1516,7 @@ __devinit dw_dma_parse_dt(struct platform_device *pdev)
 	struct device_node *sn, *cn, *np = pdev->dev.of_node;
 	struct dw_dma_platform_data *pdata;
 	struct dw_dma_slave *sd;
-	u32 count, val, arr[4];
+	u32 val, arr[4];

 	if (!np) {
 		dev_err(&pdev->dev, "Missing DT data\n");
@@ -1533,63 +1533,57 @@ __devinit dw_dma_parse_dt(struct platform_device *pdev)
 	if (of_property_read_bool(np, "is_private"))
 		pdata->is_private = true;

-	if (!of_property_read_u32(np, "chan_allocation_order",
-				&val))
+	if (!of_property_read_u32(np, "chan_allocation_order", &val))
 		pdata->chan_allocation_order = (unsigned char)val;

 	if (!of_property_read_u32(np, "chan_priority", &val))
-		pdata->chan_priority = (unsigned char)val;
+		pdata->chan_priority = val;

 	if (!of_property_read_u32(np, "block_size", &val))
-		pdata->block_size = (unsigned short)val;
+		pdata->block_size = val;

 	if (!of_property_read_u32(np, "nr_masters", &val)) {
 		if (val > 4)
 			return NULL;

-		pdata->nr_masters = (unsigned char)val;
+		pdata->nr_masters = val;
 	}

 	if (!of_property_read_u32_array(np, "data_width", arr,
 				pdata->nr_masters))
-		for (count = 0; count < pdata->nr_masters; count++)
-			pdata->data_width[count] = arr[count];
+		for (val = 0; val < pdata->nr_masters; val++)
+			pdata->data_width[val] = arr[val];

 	/* parse slave data */
 	sn = of_find_node_by_name(np, "slave_info");
 	if (!sn)
 		return pdata;

-	count = 0;
 	/* calculate number of slaves */
-	for_each_child_of_node(sn, cn)
-		count++;
-
-	if (!count)
+	val = of_get_child_count(sn);
+	if (!val)
 		return NULL;

-	sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * count, GFP_KERNEL);
+	sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * val, GFP_KERNEL);
 	if (!sd)
 		return NULL;

-	count = 0;
+	pdata->sd = sd;
+	pdata->sd_count = val;
+
 	for_each_child_of_node(sn, cn) {
-		of_property_read_string(cn, "bus_id", &sd[count].bus_id);
-		of_property_read_u32(cn, "cfg_hi", &sd[count].cfg_hi);
-		of_property_read_u32(cn, "cfg_lo", &sd[count].cfg_lo);
+		sd->dma_dev = &pdev->dev;
+		of_property_read_string(cn, "bus_id", &sd->bus_id);
+		of_property_read_u32(cn, "cfg_hi", &sd->cfg_hi);
+		of_property_read_u32(cn, "cfg_lo", &sd->cfg_lo);
 		if (!of_property_read_u32(cn, "src_master", &val))
-			sd[count].src_master = (u8)val;
+			sd->src_master = val;

 		if (!of_property_read_u32(cn, "dst_master", &val))
-			sd[count].dst_master = (u8)val;
-
-		sd[count].dma_dev = &pdev->dev;
-		count++;
+			sd->dst_master = val;
+		sd++;
 	}

-	pdata->sd = sd;
-	pdata->sd_count = count;
-
 	return pdata;
 }
 #else
diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
index 4d1c8c3..41766de 100644
--- a/include/linux/dw_dmac.h
+++ b/include/linux/dw_dmac.h
@@ -114,6 +114,6 @@ void dw_dma_cyclic_stop(struct dma_chan *chan);
 dma_addr_t dw_dma_get_src_addr(struct dma_chan *chan);

 dma_addr_t dw_dma_get_dst_addr(struct dma_chan *chan);
-bool dw_generic_filter(struct dma_chan *chan, void *param);
+bool dw_dma_generic_filter(struct dma_chan *chan, void *param);

 #endif /* DW_DMAC_H */

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Shevchenko, Andriy]

On Fri, 2012-10-12 at 19:36 +0530, Viresh Kumar wrote: 
> On 12 October 2012 18:58, Shevchenko, Andriy
> <andriy.shevchenko@intel.com> wrote:
> > On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote:
> 
> >> +     if (!of_property_read_u32(np, "nr_masters", &val)) {
> >> +             if (val > 4)
> > I thought once that we might introduce constant like #define
> > DW_MAX_AHB_MASTERS 4. It seems a bit useless because hw is designed for
> > that number anyway, but maybe you have another opinion.
> 
> Not everyone have four masters in there SoC configurations. So its better
> to export correct values.
I meant is to use that constant instead of hard coding 4 everywhere.
It's a maximum value, not the SoC specific.

> >> +     if (!of_property_read_u32_array(np, "data_width", arr,
> >> +                             pdata->nr_masters))
> >> +             for (count = 0; count < pdata->nr_masters; count++)
> >> +                     pdata->data_width[count] = arr[count];
> > Ah, it would be nice to have of_property_read_u8_array and so on...
> 
> :)
> Maybe yes. I don't want to do it with this patchset, as that will take more
> time to go through maintainers.
Agreed, the comment just for future. I don't know if there any other
users of *_u8/u16_array().

> > it will probably require to split this part in a separate function and
> > calculate count of slave_info children before pdata memory allocation.
> 
> Liked the idea partially. Check my new implementation in fixup for all
> these comments:
Let me see.

> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

> @@ -1516,7 +1516,7 @@ __devinit dw_dma_parse_dt(struct platform_device *pdev)
>  	struct device_node *sn, *cn, *np = pdev->dev.of_node;
>  	struct dw_dma_platform_data *pdata;
>  	struct dw_dma_slave *sd;
> -	u32 count, val, arr[4];
> +	u32 val, arr[4];
what about to use tmp name instead of val? It's really minor, but I
think val name is little bit unrelated to a loop counter.

> @@ -1533,63 +1533,57 @@ __devinit dw_dma_parse_dt(struct platform_device *pdev)

> -	count = 0;
> +	pdata->sd = sd;
> +	pdata->sd_count = val;
> +
>  	for_each_child_of_node(sn, cn) {
> -		of_property_read_string(cn, "bus_id", &sd[count].bus_id);
> -		of_property_read_u32(cn, "cfg_hi", &sd[count].cfg_hi);
> -		of_property_read_u32(cn, "cfg_lo", &sd[count].cfg_lo);
> +		sd->dma_dev = &pdev->dev;
> +		of_property_read_string(cn, "bus_id", &sd->bus_id);
> +		of_property_read_u32(cn, "cfg_hi", &sd->cfg_hi);
> +		of_property_read_u32(cn, "cfg_lo", &sd->cfg_lo);
>  		if (!of_property_read_u32(cn, "src_master", &val))
> -			sd[count].src_master = (u8)val;
> +			sd->src_master = val;
> 
>  		if (!of_property_read_u32(cn, "dst_master", &val))
> -			sd[count].dst_master = (u8)val;
> -
> -		sd[count].dma_dev = &pdev->dev;
> -		count++;
> +			sd->dst_master = val;
> +		sd++;
>  	}
This looks good.

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Viresh Kumar]

On 12 October 2012 20:11, Shevchenko, Andriy
<andriy.shevchenko@intel.com> wrote:
> On Fri, 2012-10-12 at 19:36 +0530, Viresh Kumar wrote:
> I meant is to use that constant instead of hard coding 4 everywhere.
> It's a maximum value, not the SoC specific.

Can be done.

>> +     u32 val, arr[4];
> what about to use tmp name instead of val? It's really minor, but I
> think val name is little bit unrelated to a loop counter.

Its not minor at all, its major. Even when i was coding it, i thought
about renaming
it several times during this last patch. But couldn't find a better name.

I don't like tmp. Give me something better.

viresh