[PATCH 0/9] dmaengine: Implement generic slave capabilities retrieval

[PATCH 0/9] dmaengine: Implement generic slave capabilities retrieval

[Maxime Ripard]

Hi,

As we discussed a couple of weeks ago, this is a first attempt at
creating a generic behaviour for slave capabilities retrieval so that
generic layers using dmaengine can actually rely on that.

That has been done mostly through two steps: by moving out the
sub-commands of the device_control callback, so that the dmaengine
core can then infer from that wether a sub-command is implemented, and
then by moving the slave properties, such as the supported buswidth,
to the structure dma_device itself.

Comments are as usual appreciated!

Thanks,
Maxime

Maxime Ripard (9):
  dmaengine: Make the destination abbreviation coherent
  dmaengine: Make channel allocation callbacks optional
  dmaengine: Introduce a device_config callback
  dmaengine: split out pause/resume operations from device_control
  dmaengine: Add device_terminate_all callback
  dmaengine: Create a generic dma_slave_caps callback
  dmaengine: Move slave caps to dma_device
  dmaengine: Mark device_control as deprecated
  dmaengine: sun6i: Convert to generic slave_caps

 drivers/dma/bcm2835-dma.c |   2 +-
 drivers/dma/dmaengine.c   |   4 --
 drivers/dma/edma.c        |   2 +-
 drivers/dma/fsl-edma.c    |   2 +-
 drivers/dma/omap-dma.c    |   2 +-
 drivers/dma/pl330.c       |   2 +-
 drivers/dma/sirf-dma.c    |   2 +-
 drivers/dma/sun6i-dma.c   | 158 +++++++++++++++++++++++++---------------------
 include/linux/dmaengine.h |  81 +++++++++++++++++++-----
 9 files changed, 158 insertions(+), 97 deletions(-)

-- 
2.1.0

[PATCH 1/9] dmaengine: Make the destination abbreviation coherent

[Maxime Ripard]

The dmaengine header abbreviates destination as at least two different strings.
Make a coherent use of a single one.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/dma/bcm2835-dma.c |  2 +-
 drivers/dma/edma.c        |  2 +-
 drivers/dma/fsl-edma.c    |  2 +-
 drivers/dma/omap-dma.c    |  2 +-
 drivers/dma/pl330.c       |  2 +-
 drivers/dma/sirf-dma.c    |  2 +-
 include/linux/dmaengine.h | 12 ++++++------
 7 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 68007974961a..8a6a9c27fe83 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -571,7 +571,7 @@ static int bcm2835_dma_device_slave_caps(struct dma_chan *dchan,
 	struct dma_slave_caps *caps)
 {
 	caps->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
-	caps->dstn_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	caps->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
 	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
 	caps->cmd_pause = false;
 	caps->cmd_terminate = true;
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 7b65633f495e..b3d641881d9d 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -998,7 +998,7 @@ static int edma_dma_device_slave_caps(struct dma_chan *dchan,
 				      struct dma_slave_caps *caps)
 {
 	caps->src_addr_widths = EDMA_DMA_BUSWIDTHS;
-	caps->dstn_addr_widths = EDMA_DMA_BUSWIDTHS;
+	caps->dst_addr_widths = EDMA_DMA_BUSWIDTHS;
 	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
 	caps->cmd_pause = true;
 	caps->cmd_terminate = true;
diff --git a/drivers/dma/fsl-edma.c b/drivers/dma/fsl-edma.c
index 3c5711d5fe97..13df85a16438 100644
--- a/drivers/dma/fsl-edma.c
+++ b/drivers/dma/fsl-edma.c
@@ -781,7 +781,7 @@ static int fsl_dma_device_slave_caps(struct dma_chan *dchan,
 		struct dma_slave_caps *caps)
 {
 	caps->src_addr_widths = FSL_EDMA_BUSWIDTHS;
-	caps->dstn_addr_widths = FSL_EDMA_BUSWIDTHS;
+	caps->dst_addr_widths = FSL_EDMA_BUSWIDTHS;
 	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
 	caps->cmd_pause = true;
 	caps->cmd_terminate = true;
diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
index 4cf7d9a950d7..51dee6636a37 100644
--- a/drivers/dma/omap-dma.c
+++ b/drivers/dma/omap-dma.c
@@ -1095,7 +1095,7 @@ static int omap_dma_device_slave_caps(struct dma_chan *dchan,
 				      struct dma_slave_caps *caps)
 {
 	caps->src_addr_widths = OMAP_DMA_BUSWIDTHS;
-	caps->dstn_addr_widths = OMAP_DMA_BUSWIDTHS;
+	caps->dst_addr_widths = OMAP_DMA_BUSWIDTHS;
 	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
 	caps->cmd_pause = true;
 	caps->cmd_terminate = true;
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index d5149aacd2fe..8904891ed5d5 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2583,7 +2583,7 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
 	struct dma_slave_caps *caps)
 {
 	caps->src_addr_widths = PL330_DMA_BUSWIDTHS;
-	caps->dstn_addr_widths = PL330_DMA_BUSWIDTHS;
+	caps->dst_addr_widths = PL330_DMA_BUSWIDTHS;
 	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
 	caps->cmd_pause = false;
 	caps->cmd_terminate = true;
diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
index aac03ab10c54..149d7fddfa75 100644
--- a/drivers/dma/sirf-dma.c
+++ b/drivers/dma/sirf-dma.c
@@ -652,7 +652,7 @@ static int sirfsoc_dma_device_slave_caps(struct dma_chan *dchan,
 	struct dma_slave_caps *caps)
 {
 	caps->src_addr_widths = SIRFSOC_DMA_BUSWIDTHS;
-	caps->dstn_addr_widths = SIRFSOC_DMA_BUSWIDTHS;
+	caps->dst_addr_widths = SIRFSOC_DMA_BUSWIDTHS;
 	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
 	caps->cmd_pause = true;
 	caps->cmd_terminate = true;
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 1f9e642c66ad..f4b71ce1a0a6 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -388,7 +388,7 @@ enum dma_residue_granularity {
 /* struct dma_slave_caps - expose capabilities of a slave channel only
  *
  * @src_addr_widths: bit mask of src addr widths the channel supports
- * @dstn_addr_widths: bit mask of dstn addr widths the channel supports
+ * @dst_addr_widths: bit mask of dstn addr widths the channel supports
  * @directions: bit mask of slave direction the channel supported
  * 	since the enum dma_transfer_direction is not defined as bits for each
  * 	type of direction, the dma controller should fill (1 << <TYPE>) and same
@@ -399,7 +399,7 @@ enum dma_residue_granularity {
  */
 struct dma_slave_caps {
 	u32 src_addr_widths;
-	u32 dstn_addr_widths;
+	u32 dst_addr_widths;
 	u32 directions;
 	bool cmd_pause;
 	bool cmd_terminate;
@@ -639,10 +639,10 @@ struct dma_device {
 	void (*device_free_chan_resources)(struct dma_chan *chan);
 
 	struct dma_async_tx_descriptor *(*device_prep_dma_memcpy)(
-		struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
+		struct dma_chan *chan, dma_addr_t dst, dma_addr_t src,
 		size_t len, unsigned long flags);
 	struct dma_async_tx_descriptor *(*device_prep_dma_xor)(
-		struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
+		struct dma_chan *chan, dma_addr_t dst, dma_addr_t *src,
 		unsigned int src_cnt, size_t len, unsigned long flags);
 	struct dma_async_tx_descriptor *(*device_prep_dma_xor_val)(
 		struct dma_chan *chan, dma_addr_t *src,	unsigned int src_cnt,
@@ -935,11 +935,11 @@ async_dma_find_channel(enum dma_transaction_type type)
 #endif /* CONFIG_ASYNC_TX_DMA */
 
 dma_cookie_t dma_async_memcpy_buf_to_buf(struct dma_chan *chan,
-	void *dest, void *src, size_t len);
+	void *dst, void *src, size_t len);
 dma_cookie_t dma_async_memcpy_buf_to_pg(struct dma_chan *chan,
 	struct page *page, unsigned int offset, void *kdata, size_t len);
 dma_cookie_t dma_async_memcpy_pg_to_pg(struct dma_chan *chan,
-	struct page *dest_pg, unsigned int dest_off, struct page *src_pg,
+	struct page *dst_pg, unsigned int dst_off, struct page *src_pg,
 	unsigned int src_off, size_t len);
 void dma_async_tx_descriptor_init(struct dma_async_tx_descriptor *tx,
 	struct dma_chan *chan);
-- 
2.1.0

[PATCH 2/9] dmaengine: Make channel allocation callbacks optional

[Maxime Ripard]

Nowadays, some drivers don't have anything in there channel allocation
callbacks anymore.

Remove the BUG_ON if those callbacks aren't implemented, in order to allow
drivers to not implement them.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/dma/dmaengine.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index d5d30ed863ce..cfcb181b1184 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -817,8 +817,6 @@ int dma_async_device_register(struct dma_device *device)
 	BUG_ON(dma_has_cap(DMA_INTERLEAVE, device->cap_mask) &&
 		!device->device_prep_interleaved_dma);
 
-	BUG_ON(!device->device_alloc_chan_resources);
-	BUG_ON(!device->device_free_chan_resources);
 	BUG_ON(!device->device_tx_status);
 	BUG_ON(!device->device_issue_pending);
 	BUG_ON(!device->dev);
-- 
2.1.0

[PATCH 3/9] dmaengine: Introduce a device_config callback

[Maxime Ripard]

The fact that the channel configuration is done in device_control is rather
misleading, since it's not really advertised as such, plus, the fact that the
framework exposes a function of its own makes it not really intuitive, while
we're losing the type checking whenever we pass that unsigned long argument.

Add a device_config callback to dma_device, with a fallback on the old
behaviour for now for existing drivers to opt in.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 include/linux/dmaengine.h | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index f4b71ce1a0a6..7937f81e5e2e 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -608,6 +608,8 @@ struct dma_tx_state {
  *	The function takes a buffer of size buf_len. The callback function will
  *	be called after period_len bytes have been transferred.
  * @device_prep_interleaved_dma: Transfer expression in a generic way.
+ * @device_config: Pushes a new configuration to a channel, return 0 or an error
+ *	code
  * @device_control: manipulate all pending operations on a channel, returns
  *	zero or error code
  * @device_tx_status: poll for transaction completion, the optional
@@ -662,7 +664,6 @@ struct dma_device {
 		struct scatterlist *dst_sg, unsigned int dst_nents,
 		struct scatterlist *src_sg, unsigned int src_nents,
 		unsigned long flags);
-
 	struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
 		struct dma_chan *chan, struct scatterlist *sgl,
 		unsigned int sg_len, enum dma_transfer_direction direction,
@@ -674,9 +675,11 @@ struct dma_device {
 	struct dma_async_tx_descriptor *(*device_prep_interleaved_dma)(
 		struct dma_chan *chan, struct dma_interleaved_template *xt,
 		unsigned long flags);
-	int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
-		unsigned long arg);
 
+	int (*device_config)(struct dma_chan *chan,
+			     struct dma_slave_config *config);
+	int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
+			      unsigned long arg);
 	enum dma_status (*device_tx_status)(struct dma_chan *chan,
 					    dma_cookie_t cookie,
 					    struct dma_tx_state *txstate);
@@ -697,6 +700,9 @@ static inline int dmaengine_device_control(struct dma_chan *chan,
 static inline int dmaengine_slave_config(struct dma_chan *chan,
 					  struct dma_slave_config *config)
 {
+	if (chan->device->device_config)
+		return chan->device->device_config(chan, config);
+
 	return dmaengine_device_control(chan, DMA_SLAVE_CONFIG,
 			(unsigned long)config);
 }
-- 
2.1.0

[PATCH 4/9] dmaengine: split out pause/resume operations from device_control

[Maxime Ripard]

Split out the pause and resume operations to callbacks of their own. In order
to preserve some backwark compatibility, the dmaengine_pause/dmaengine_resume
are still falling back on dmaengine_device_control.

Hopefully, that will allow to get the device capabilities in a generic way,
removing the need to implement device_slave_caps.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 include/linux/dmaengine.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 7937f81e5e2e..b89c8004fcd8 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -612,6 +612,10 @@ struct dma_tx_state {
  *	code
  * @device_control: manipulate all pending operations on a channel, returns
  *	zero or error code
+ * @device_pause: Pauses any transfer happening on a channel. Returns
+ *	0 or an error code
+ * @device_resume: Resumes any transfer on a channel previously
+ *	paused. Returns 0 or an error code
  * @device_tx_status: poll for transaction completion, the optional
  *	txstate parameter can be supplied with a pointer to get a
  *	struct with auxiliary transfer status information, otherwise the call
@@ -680,6 +684,8 @@ struct dma_device {
 			     struct dma_slave_config *config);
 	int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 			      unsigned long arg);
+	int (*device_pause)(struct dma_chan *chan);
+	int (*device_resume)(struct dma_chan *chan);
 	enum dma_status (*device_tx_status)(struct dma_chan *chan,
 					    dma_cookie_t cookie,
 					    struct dma_tx_state *txstate);
@@ -783,11 +789,17 @@ static inline int dmaengine_terminate_all(struct dma_chan *chan)
 
 static inline int dmaengine_pause(struct dma_chan *chan)
 {
+	if (chan->device->device_pause)
+		return chan->device->device_pause(chan);
+
 	return dmaengine_device_control(chan, DMA_PAUSE, 0);
 }
 
 static inline int dmaengine_resume(struct dma_chan *chan)
 {
+	if (chan->device->device_resume)
+		return chan->device->device_resume(chan);
+
 	return dmaengine_device_control(chan, DMA_RESUME, 0);
 }
 
-- 
2.1.0

[PATCH 5/9] dmaengine: Add device_terminate_all callback

[Maxime Ripard]

Split out the terminate_all command from device_control to a dma_device
callback. In order to preserve backward capability, still rely on
device_control if no such callback has been implemented.

Eventually, this will allow to create a generic dma_slave_caps callback.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 include/linux/dmaengine.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index b89c8004fcd8..4d0294ec3567 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -616,6 +616,8 @@ struct dma_tx_state {
  *	0 or an error code
  * @device_resume: Resumes any transfer on a channel previously
  *	paused. Returns 0 or an error code
+ * @device_terminate_all: Aborts all transfers on a channel. Returns 0
+ *	or an error code
  * @device_tx_status: poll for transaction completion, the optional
  *	txstate parameter can be supplied with a pointer to get a
  *	struct with auxiliary transfer status information, otherwise the call
@@ -686,6 +688,7 @@ struct dma_device {
 			      unsigned long arg);
 	int (*device_pause)(struct dma_chan *chan);
 	int (*device_resume)(struct dma_chan *chan);
+	int (*device_terminate_all)(struct dma_chan *chan);
 	enum dma_status (*device_tx_status)(struct dma_chan *chan,
 					    dma_cookie_t cookie,
 					    struct dma_tx_state *txstate);
@@ -784,6 +787,9 @@ static inline int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_cap
 
 static inline int dmaengine_terminate_all(struct dma_chan *chan)
 {
+	if (chan->device->device_terminate_all)
+		return chan->device->device_terminate_all(chan);
+
 	return dmaengine_device_control(chan, DMA_TERMINATE_ALL, 0);
 }
 
-- 
2.1.0

[PATCH 6/9] dmaengine: Create a generic dma_slave_caps callback

[Maxime Ripard]

dma_slave_caps is very important to the generic layers that might interact with
dmaengine, such as ASoC. Unfortunately, it has been added as yet another
dma_device callback, and most of the existing drivers haven't implemented it,
reducing its reliability.

Introduce a generic behaviour, pre-filling the dma_slave_caps struct with what
it knows about the dma controller, before calling the dma_device callback, both
to maintain a backward compatibility and to make sure that drivers can override
that generic behaviour.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 include/linux/dmaengine.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 4d0294ec3567..cfcbee145898 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -772,17 +772,24 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_interleaved_dma(
 
 static inline int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
 {
+	struct dma_device *device;
+
 	if (!chan || !caps)
 		return -EINVAL;
 
+	device = chan->device;
+
 	/* check if the channel supports slave transactions */
-	if (!test_bit(DMA_SLAVE, chan->device->cap_mask.bits))
+	if (!test_bit(DMA_SLAVE, device->cap_mask.bits))
 		return -ENXIO;
 
-	if (chan->device->device_slave_caps)
-		return chan->device->device_slave_caps(chan, caps);
+	caps->cmd_pause = !!device->device_pause;
+	caps->cmd_terminate = !!device->device_terminate_all;
+
+	if (device->device_slave_caps)
+		return device->device_slave_caps(chan, caps);
 
-	return -ENXIO;
+	return 0;
 }
 
 static inline int dmaengine_terminate_all(struct dma_chan *chan)
-- 
2.1.0

[PATCH 7/9] dmaengine: Move slave caps to dma_device

[Maxime Ripard]

The previous code was relying on the fact that the slave_caps were to be
defined on a per channel basis.

However, this proved to be a bit overkill, since every driver filling these so
far were hardcoding it, disregarding which channel was actually given.

Add these capabilities to the dma_device structure, so that drivers can just
provide them at probe time, and be done with it.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 include/linux/dmaengine.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index cfcbee145898..e25a365b808c 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -594,6 +594,14 @@ struct dma_tx_state {
  * @fill_align: alignment shift for memset operations
  * @dev_id: unique device ID
  * @dev: struct device reference for dma mapping api
+ * @src_addr_widths: bit mask of src addr widths the device supports
+ * @dst_addr_widths: bit mask of dst addr widths the device supports
+ * @directions: bit mask of slave direction the device supports since
+ * 	the enum dma_transfer_direction is not defined as bits for
+ * 	each type of direction, the dma controller should fill (1 <<
+ * 	<TYPE>) and same should be checked by controller as well
+ * @residue_granularity: granularity of the transfer residue reported
+ *	by tx_status
  * @device_alloc_chan_resources: allocate resources and return the
  *	number of allocated descriptors
  * @device_free_chan_resources: release DMA channel's resources
@@ -643,6 +651,11 @@ struct dma_device {
 	int dev_id;
 	struct device *dev;
 
+	u32 src_addr_widths;
+	u32 dst_addr_widths;
+	u32 directions;
+	enum dma_residue_granularity residue_granularity;
+
 	int (*device_alloc_chan_resources)(struct dma_chan *chan);
 	void (*device_free_chan_resources)(struct dma_chan *chan);
 
@@ -783,6 +796,11 @@ static inline int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_cap
 	if (!test_bit(DMA_SLAVE, device->cap_mask.bits))
 		return -ENXIO;
 
+	caps->src_addr_widths = device->src_addr_widths;
+	caps->dst_addr_widths = device->dst_addr_widths;
+	caps->directions = device->directions;
+	caps->residue_granularity = device->residue_granularity;
+
 	caps->cmd_pause = !!device->device_pause;
 	caps->cmd_terminate = !!device->device_terminate_all;
 
-- 
2.1.0

[PATCH 8/9] dmaengine: Mark device_control as deprecated

[Maxime Ripard]

Now that device_control has been split into several functions, we can consider
it deprecated.

That also means that we don't want to make it mandatory any more. Since any
device was free to implement any command of device_control, the only least
common denominator is to not expect any callback anymore. Hopefully,
device_slave_caps can provide such informations to the client.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/dma/dmaengine.c   |  2 --
 include/linux/dmaengine.h | 10 ++++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index cfcb181b1184..87348e6a3791 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -812,8 +812,6 @@ int dma_async_device_register(struct dma_device *device)
 		!device->device_prep_dma_sg);
 	BUG_ON(dma_has_cap(DMA_CYCLIC, device->cap_mask) &&
 		!device->device_prep_dma_cyclic);
-	BUG_ON(dma_has_cap(DMA_SLAVE, device->cap_mask) &&
-		!device->device_control);
 	BUG_ON(dma_has_cap(DMA_INTERLEAVE, device->cap_mask) &&
 		!device->device_prep_interleaved_dma);
 
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index e25a365b808c..9c0d16273ef9 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -619,7 +619,7 @@ struct dma_tx_state {
  * @device_config: Pushes a new configuration to a channel, return 0 or an error
  *	code
  * @device_control: manipulate all pending operations on a channel, returns
- *	zero or error code
+ *	zero or error code (Deprecated)
  * @device_pause: Pauses any transfer happening on a channel. Returns
  *	0 or an error code
  * @device_resume: Resumes any transfer on a channel previously
@@ -631,7 +631,7 @@ struct dma_tx_state {
  *	struct with auxiliary transfer status information, otherwise the call
  *	will just return a simple status code
  * @device_issue_pending: push pending transactions to hardware
- * @device_slave_caps: return the slave channel capabilities
+ * @device_slave_caps: return the slave channel capabilities (Deprecated)
  */
 struct dma_device {
 
@@ -697,8 +697,6 @@ struct dma_device {
 
 	int (*device_config)(struct dma_chan *chan,
 			     struct dma_slave_config *config);
-	int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
-			      unsigned long arg);
 	int (*device_pause)(struct dma_chan *chan);
 	int (*device_resume)(struct dma_chan *chan);
 	int (*device_terminate_all)(struct dma_chan *chan);
@@ -706,6 +704,10 @@ struct dma_device {
 					    dma_cookie_t cookie,
 					    struct dma_tx_state *txstate);
 	void (*device_issue_pending)(struct dma_chan *chan);
+
+	/* Deprecated */
+	int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
+			      unsigned long arg);
 	int (*device_slave_caps)(struct dma_chan *chan, struct dma_slave_caps *caps);
 };
 
-- 
2.1.0

[PATCH 9/9] dmaengine: sun6i: Convert to generic slave_caps

[Maxime Ripard]

Convert the Allwinner A31 driver to the callbacks that have been introduced
before, in order to support calls to retrieve slave capabilities.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/dma/sun6i-dma.c | 158 ++++++++++++++++++++++++++----------------------
 1 file changed, 86 insertions(+), 72 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 1f92a56fd2b6..447de4d42251 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -358,38 +358,6 @@ static void sun6i_dma_free_desc(struct virt_dma_desc *vd)
 	kfree(txd);
 }
 
-static int sun6i_dma_terminate_all(struct sun6i_vchan *vchan)
-{
-	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(vchan->vc.chan.device);
-	struct sun6i_pchan *pchan = vchan->phy;
-	unsigned long flags;
-	LIST_HEAD(head);
-
-	spin_lock(&sdev->lock);
-	list_del_init(&vchan->node);
-	spin_unlock(&sdev->lock);
-
-	spin_lock_irqsave(&vchan->vc.lock, flags);
-
-	vchan_get_all_descriptors(&vchan->vc, &head);
-
-	if (pchan) {
-		writel(DMA_CHAN_ENABLE_STOP, pchan->base + DMA_CHAN_ENABLE);
-		writel(DMA_CHAN_PAUSE_RESUME, pchan->base + DMA_CHAN_PAUSE);
-
-		vchan->phy = NULL;
-		pchan->vchan = NULL;
-		pchan->desc = NULL;
-		pchan->done = NULL;
-	}
-
-	spin_unlock_irqrestore(&vchan->vc.lock, flags);
-
-	vchan_dma_desc_free_list(&vchan->vc, &head);
-
-	return 0;
-}
-
 static int sun6i_dma_start_desc(struct sun6i_vchan *vchan)
 {
 	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(vchan->vc.chan.device);
@@ -673,57 +641,92 @@ err_lli_free:
 	return NULL;
 }
 
-static int sun6i_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
-		       unsigned long arg)
+static int sun6i_dma_config(struct dma_chan *chan,
+			    struct dma_slave_config *config)
+{
+	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
+
+	memcpy(&vchan->cfg, config, sizeof(*config));
+
+	return 0;
+}
+
+static int sun6i_dma_pause(struct dma_chan *chan)
+{
+	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
+	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
+	struct sun6i_pchan *pchan = vchan->phy;
+
+	dev_dbg(chan2dev(chan), "vchan %p: pause\n", &vchan->vc);
+
+	if (pchan) {
+		writel(DMA_CHAN_PAUSE_PAUSE,
+		       pchan->base + DMA_CHAN_PAUSE);
+	} else {
+		spin_lock(&sdev->lock);
+		list_del_init(&vchan->node);
+		spin_unlock(&sdev->lock);
+	}
+
+	return 0;
+}
+
+static int sun6i_dma_resume(struct dma_chan *chan)
 {
 	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
 	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
 	struct sun6i_pchan *pchan = vchan->phy;
 	unsigned long flags;
-	int ret = 0;
 
-	switch (cmd) {
-	case DMA_RESUME:
-		dev_dbg(chan2dev(chan), "vchan %p: resume\n", &vchan->vc);
+	dev_dbg(chan2dev(chan), "vchan %p: resume\n", &vchan->vc);
 
-		spin_lock_irqsave(&vchan->vc.lock, flags);
+	spin_lock_irqsave(&vchan->vc.lock, flags);
 
-		if (pchan) {
-			writel(DMA_CHAN_PAUSE_RESUME,
-			       pchan->base + DMA_CHAN_PAUSE);
-		} else if (!list_empty(&vchan->vc.desc_issued)) {
-			spin_lock(&sdev->lock);
-			list_add_tail(&vchan->node, &sdev->pending);
-			spin_unlock(&sdev->lock);
-		}
+	if (pchan) {
+		writel(DMA_CHAN_PAUSE_RESUME,
+		       pchan->base + DMA_CHAN_PAUSE);
+	} else if (!list_empty(&vchan->vc.desc_issued)) {
+		spin_lock(&sdev->lock);
+		list_add_tail(&vchan->node, &sdev->pending);
+		spin_unlock(&sdev->lock);
+	}
 
-		spin_unlock_irqrestore(&vchan->vc.lock, flags);
-		break;
+	spin_unlock_irqrestore(&vchan->vc.lock, flags);
 
-	case DMA_PAUSE:
-		dev_dbg(chan2dev(chan), "vchan %p: pause\n", &vchan->vc);
+	return 0;
+}
 
-		if (pchan) {
-			writel(DMA_CHAN_PAUSE_PAUSE,
-			       pchan->base + DMA_CHAN_PAUSE);
-		} else {
-			spin_lock(&sdev->lock);
-			list_del_init(&vchan->node);
-			spin_unlock(&sdev->lock);
-		}
-		break;
+static int sun6i_dma_terminate_all(struct dma_chan *chan)
+{
+	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
+	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
+	struct sun6i_pchan *pchan = vchan->phy;
+	unsigned long flags;
+	LIST_HEAD(head);
 
-	case DMA_TERMINATE_ALL:
-		ret = sun6i_dma_terminate_all(vchan);
-		break;
-	case DMA_SLAVE_CONFIG:
-		memcpy(&vchan->cfg, (void *)arg, sizeof(struct dma_slave_config));
-		break;
-	default:
-		ret = -ENXIO;
-		break;
+	spin_lock(&sdev->lock);
+	list_del_init(&vchan->node);
+	spin_unlock(&sdev->lock);
+
+	spin_lock_irqsave(&vchan->vc.lock, flags);
+
+	vchan_get_all_descriptors(&vchan->vc, &head);
+
+	if (pchan) {
+		writel(DMA_CHAN_ENABLE_STOP, pchan->base + DMA_CHAN_ENABLE);
+		writel(DMA_CHAN_PAUSE_RESUME, pchan->base + DMA_CHAN_PAUSE);
+
+		vchan->phy = NULL;
+		pchan->vchan = NULL;
+		pchan->desc = NULL;
+		pchan->done = NULL;
 	}
-	return ret;
+
+	spin_unlock_irqrestore(&vchan->vc.lock, flags);
+
+	vchan_dma_desc_free_list(&vchan->vc, &head);
+
+	return 0;
 }
 
 static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
@@ -936,9 +939,20 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 	sdc->slave.device_issue_pending		= sun6i_dma_issue_pending;
 	sdc->slave.device_prep_slave_sg		= sun6i_dma_prep_slave_sg;
 	sdc->slave.device_prep_dma_memcpy	= sun6i_dma_prep_dma_memcpy;
-	sdc->slave.device_control		= sun6i_dma_control;
+	sdc->slave.device_config		= sun6i_dma_config;
+	sdc->slave.device_pause			= sun6i_dma_pause;
+	sdc->slave.device_resume		= sun6i_dma_resume;
+	sdc->slave.device_terminate_all		= sun6i_dma_terminate_all;
 	sdc->slave.chancnt			= NR_MAX_VCHANS;
-
+	sdc->slave.src_addr_widths		= BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+						  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+						  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	sdc->slave.dst_addr_widths		= BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+						  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+						  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	sdc->slave.directions			= BIT(DMA_DEV_TO_MEM) |
+						  BIT(DMA_MEM_TO_DEV);
+	sdc->slave.residue_granularity		= DMA_RESIDUE_GRANULARITY_BURST;
 	sdc->slave.dev = &pdev->dev;
 
 	sdc->pchans = devm_kcalloc(&pdev->dev, NR_MAX_CHANNELS,
-- 
2.1.0

[PATCH 6/9] dmaengine: Create a generic dma_slave_caps callback

[Russell King - ARM Linux]

On Sat, Sep 27, 2014 at 10:54:42AM +0200, Maxime Ripard wrote:
> dma_slave_caps is very important to the generic layers that might interact with
> dmaengine, such as ASoC. Unfortunately, it has been added as yet another
> dma_device callback, and most of the existing drivers haven't implemented it,
> reducing its reliability.

Many haven't implemented it probably because either (a) they don't get used
with ASoC, or (b) they aren't aware of the new interface, or (c) can't be
bothered with the churn.

However, trying to return something introduces a bug:

>  static inline int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
>  {
> +	struct dma_device *device;
> +
>  	if (!chan || !caps)
>  		return -EINVAL;
>  
> +	device = chan->device;
> +
>  	/* check if the channel supports slave transactions */
> -	if (!test_bit(DMA_SLAVE, chan->device->cap_mask.bits))
> +	if (!test_bit(DMA_SLAVE, device->cap_mask.bits))
>  		return -ENXIO;
>  
> -	if (chan->device->device_slave_caps)
> -		return chan->device->device_slave_caps(chan, caps);
> +	caps->cmd_pause = !!device->device_pause;
> +	caps->cmd_terminate = !!device->device_terminate_all;
> +
> +	if (device->device_slave_caps)
> +		return device->device_slave_caps(chan, caps);
>  
> -	return -ENXIO;
> +	return 0;

So this now returns success if the driver doesn't implement device_slave_caps(),
but with most of the structure zero.

Now, consider what effect this has with:


        ret = dma_get_slave_caps(chan, &dma_caps);
        if (ret == 0) {
                if (dma_caps.cmd_pause)
                        hw.info |= SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME;
                if (dma_caps.residue_granularity <= DMA_RESIDUE_GRANULARITY_SEGMENT)
                        hw.info |= SNDRV_PCM_INFO_BATCH;

                if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
                        addr_widths = dma_caps.dstn_addr_widths;
                else
                        addr_widths = dma_caps.src_addr_widths;
        }

addr_widths becomes zero, and we also get SNDRV_PCM_INFO_BATCH turned
on for _all_ DMA engine drivers.  The first renders ASoC useless with
DMA engine.

It may be a good way to get people to implement it, but this will cause
regressions.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 7/9] dmaengine: Move slave caps to dma_device

[Russell King - ARM Linux]

On Sat, Sep 27, 2014 at 10:54:43AM +0200, Maxime Ripard wrote:
> The previous code was relying on the fact that the slave_caps were to be
> defined on a per channel basis.
> 
> However, this proved to be a bit overkill, since every driver filling these so
> far were hardcoding it, disregarding which channel was actually given.
> 
> Add these capabilities to the dma_device structure, so that drivers can just
> provide them at probe time, and be done with it.

This is also buggy for the same reason as patch 6.

The only way to do this is to either have a flag day, fixing all drivers
at once (which isn't going to happen) or leave the caps code as-is, and
provide a library function which drivers can hook into the caps callback
which retrieves the information from dma_device.

That way, DMA engine drivers which are using the new method can just
install the new function, and those which haven't been updated with
capabilities can carry on as they are, and are detectable to drivers.

What would be acceptable is to have the DMA engine registration function
spot the lack of DMA caps function and print a warning at boot to
encourage people to add it.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/9] dmaengine: Make channel allocation callbacks optional

[Vinod Koul]

On Sat, Sep 27, 2014 at 10:54:38AM +0200, Maxime Ripard wrote:
> Nowadays, some drivers don't have anything in there channel allocation
> callbacks anymore.
> 
> Remove the BUG_ON if those callbacks aren't implemented, in order to allow
> drivers to not implement them.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/dma/dmaengine.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index d5d30ed863ce..cfcb181b1184 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -817,8 +817,6 @@ int dma_async_device_register(struct dma_device *device)
>  	BUG_ON(dma_has_cap(DMA_INTERLEAVE, device->cap_mask) &&
>  		!device->device_prep_interleaved_dma);
>  
> -	BUG_ON(!device->device_alloc_chan_resources);
> -	BUG_ON(!device->device_free_chan_resources);
Dont think we have drivers without free. IIRC cppi one might be only
instance

-- 
~Vinod
>  	BUG_ON(!device->device_tx_status);
>  	BUG_ON(!device->device_issue_pending);
>  	BUG_ON(!device->dev);
> -- 
> 2.1.0
> 

-- 

[PATCH 3/9] dmaengine: Introduce a device_config callback

[Vinod Koul]

On Sat, Sep 27, 2014 at 10:54:39AM +0200, Maxime Ripard wrote:
> The fact that the channel configuration is done in device_control is rather
> misleading, since it's not really advertised as such, plus, the fact that the
> framework exposes a function of its own makes it not really intuitive, while
> we're losing the type checking whenever we pass that unsigned long argument.
> 
> Add a device_config callback to dma_device, with a fallback on the old
> behaviour for now for existing drivers to opt in.
This will break bisect, we have users of device_control in tree, they need
to be converted first to use wrappers and then we should introduce these

-- 
~Vinod
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  include/linux/dmaengine.h | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index f4b71ce1a0a6..7937f81e5e2e 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -608,6 +608,8 @@ struct dma_tx_state {
>   *	The function takes a buffer of size buf_len. The callback function will
>   *	be called after period_len bytes have been transferred.
>   * @device_prep_interleaved_dma: Transfer expression in a generic way.
> + * @device_config: Pushes a new configuration to a channel, return 0 or an error
> + *	code
>   * @device_control: manipulate all pending operations on a channel, returns
>   *	zero or error code
>   * @device_tx_status: poll for transaction completion, the optional
> @@ -662,7 +664,6 @@ struct dma_device {
>  		struct scatterlist *dst_sg, unsigned int dst_nents,
>  		struct scatterlist *src_sg, unsigned int src_nents,
>  		unsigned long flags);
> -
>  	struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
>  		struct dma_chan *chan, struct scatterlist *sgl,
>  		unsigned int sg_len, enum dma_transfer_direction direction,
> @@ -674,9 +675,11 @@ struct dma_device {
>  	struct dma_async_tx_descriptor *(*device_prep_interleaved_dma)(
>  		struct dma_chan *chan, struct dma_interleaved_template *xt,
>  		unsigned long flags);
> -	int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> -		unsigned long arg);
>  
> +	int (*device_config)(struct dma_chan *chan,
> +			     struct dma_slave_config *config);
> +	int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> +			      unsigned long arg);
>  	enum dma_status (*device_tx_status)(struct dma_chan *chan,
>  					    dma_cookie_t cookie,
>  					    struct dma_tx_state *txstate);
> @@ -697,6 +700,9 @@ static inline int dmaengine_device_control(struct dma_chan *chan,
>  static inline int dmaengine_slave_config(struct dma_chan *chan,
>  					  struct dma_slave_config *config)
>  {
> +	if (chan->device->device_config)
> +		return chan->device->device_config(chan, config);
> +
>  	return dmaengine_device_control(chan, DMA_SLAVE_CONFIG,
>  			(unsigned long)config);
>  }
> -- 
> 2.1.0
> 

-- 

[PATCH 3/9] dmaengine: Introduce a device_config callback

[Vinod Koul]

On Sun, Sep 28, 2014 at 09:43:33PM +0530, Vinod Koul wrote:
> On Sat, Sep 27, 2014 at 10:54:39AM +0200, Maxime Ripard wrote:
> > The fact that the channel configuration is done in device_control is rather
> > misleading, since it's not really advertised as such, plus, the fact that the
> > framework exposes a function of its own makes it not really intuitive, while
> > we're losing the type checking whenever we pass that unsigned long argument.
> > 
> > Add a device_config callback to dma_device, with a fallback on the old
> > behaviour for now for existing drivers to opt in.
> This will break bisect, we have users of device_control in tree, they need
> to be converted first to use wrappers and then we should introduce these
Okay sorry missed the later addition part. This should be okay

-- 
~Vinod
> 
> -- 
> ~Vinod
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  include/linux/dmaengine.h | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index f4b71ce1a0a6..7937f81e5e2e 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -608,6 +608,8 @@ struct dma_tx_state {
> >   *	The function takes a buffer of size buf_len. The callback function will
> >   *	be called after period_len bytes have been transferred.
> >   * @device_prep_interleaved_dma: Transfer expression in a generic way.
> > + * @device_config: Pushes a new configuration to a channel, return 0 or an error
> > + *	code
> >   * @device_control: manipulate all pending operations on a channel, returns
> >   *	zero or error code
> >   * @device_tx_status: poll for transaction completion, the optional
> > @@ -662,7 +664,6 @@ struct dma_device {
> >  		struct scatterlist *dst_sg, unsigned int dst_nents,
> >  		struct scatterlist *src_sg, unsigned int src_nents,
> >  		unsigned long flags);
> > -
> >  	struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
> >  		struct dma_chan *chan, struct scatterlist *sgl,
> >  		unsigned int sg_len, enum dma_transfer_direction direction,
> > @@ -674,9 +675,11 @@ struct dma_device {
> >  	struct dma_async_tx_descriptor *(*device_prep_interleaved_dma)(
> >  		struct dma_chan *chan, struct dma_interleaved_template *xt,
> >  		unsigned long flags);
> > -	int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> > -		unsigned long arg);
> >  
> > +	int (*device_config)(struct dma_chan *chan,
> > +			     struct dma_slave_config *config);
> > +	int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> > +			      unsigned long arg);
> >  	enum dma_status (*device_tx_status)(struct dma_chan *chan,
> >  					    dma_cookie_t cookie,
> >  					    struct dma_tx_state *txstate);
> > @@ -697,6 +700,9 @@ static inline int dmaengine_device_control(struct dma_chan *chan,
> >  static inline int dmaengine_slave_config(struct dma_chan *chan,
> >  					  struct dma_slave_config *config)
> >  {
> > +	if (chan->device->device_config)
> > +		return chan->device->device_config(chan, config);
> > +
> >  	return dmaengine_device_control(chan, DMA_SLAVE_CONFIG,
> >  			(unsigned long)config);
> >  }
> > -- 
> > 2.1.0
> > 
> 
> -- 

-- 

[PATCH 6/9] dmaengine: Create a generic dma_slave_caps callback

[Maxime Ripard]

Hi Russell,

On Sat, Sep 27, 2014 at 10:25:36AM +0100, Russell King - ARM Linux wrote:
> On Sat, Sep 27, 2014 at 10:54:42AM +0200, Maxime Ripard wrote:
> > dma_slave_caps is very important to the generic layers that might interact with
> > dmaengine, such as ASoC. Unfortunately, it has been added as yet another
> > dma_device callback, and most of the existing drivers haven't implemented it,
> > reducing its reliability.
> 
> Many haven't implemented it probably because either (a) they don't get used
> with ASoC, or (b) they aren't aware of the new interface, or (c) can't be
> bothered with the churn.

For a), I really see this as a chicken-egg issue. ASoC is the only
user of it because it's the only framework that has a generic layer on
top, and it's the only framework that has a generic layer because most
drivers don't implement it.

Now, there seems to be a trend to actually use a generic DMA layer in
other frameworks. SPI gained one recently, I think I saw something
about some discussions for IIO and I2C too. And in order for this to
work, we have to make it reliable, and as such, implemented on most
drivers.

> However, trying to return something introduces a bug:
> 
> >  static inline int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
> >  {
> > +	struct dma_device *device;
> > +
> >  	if (!chan || !caps)
> >  		return -EINVAL;
> >  
> > +	device = chan->device;
> > +
> >  	/* check if the channel supports slave transactions */
> > -	if (!test_bit(DMA_SLAVE, chan->device->cap_mask.bits))
> > +	if (!test_bit(DMA_SLAVE, device->cap_mask.bits))
> >  		return -ENXIO;
> >  
> > -	if (chan->device->device_slave_caps)
> > -		return chan->device->device_slave_caps(chan, caps);
> > +	caps->cmd_pause = !!device->device_pause;
> > +	caps->cmd_terminate = !!device->device_terminate_all;
> > +
> > +	if (device->device_slave_caps)
> > +		return device->device_slave_caps(chan, caps);
> >  
> > -	return -ENXIO;
> > +	return 0;
> 
> So this now returns success if the driver doesn't implement device_slave_caps(),
> but with most of the structure zero.
> 
> Now, consider what effect this has with:
> 
> 
>         ret = dma_get_slave_caps(chan, &dma_caps);
>         if (ret == 0) {
>                 if (dma_caps.cmd_pause)
>                         hw.info |= SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME;
>                 if (dma_caps.residue_granularity <= DMA_RESIDUE_GRANULARITY_SEGMENT)
>                         hw.info |= SNDRV_PCM_INFO_BATCH;
> 
>                 if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>                         addr_widths = dma_caps.dstn_addr_widths;
>                 else
>                         addr_widths = dma_caps.src_addr_widths;
>         }
> 
> addr_widths becomes zero, and we also get SNDRV_PCM_INFO_BATCH turned
> on for _all_ DMA engine drivers.  The first renders ASoC useless with
> DMA engine.
> 
> It may be a good way to get people to implement it, but this will cause
> regressions.

Hmmm, nasty indeed. Maybe we could add a test to see if any of the
field we're going to use are filled, and if not, return an error?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141001/262b71c3/attachment.sig>

[PATCH 7/9] dmaengine: Move slave caps to dma_device

[Maxime Ripard]

On Sat, Sep 27, 2014 at 10:28:47AM +0100, Russell King - ARM Linux wrote:
> On Sat, Sep 27, 2014 at 10:54:43AM +0200, Maxime Ripard wrote:
> > The previous code was relying on the fact that the slave_caps were to be
> > defined on a per channel basis.
> > 
> > However, this proved to be a bit overkill, since every driver filling these so
> > far were hardcoding it, disregarding which channel was actually given.
> > 
> > Add these capabilities to the dma_device structure, so that drivers can just
> > provide them at probe time, and be done with it.
> 
> This is also buggy for the same reason as patch 6.

Indeed

> The only way to do this is to either have a flag day, fixing all drivers
> at once (which isn't going to happen) or leave the caps code as-is, and
> provide a library function which drivers can hook into the caps callback
> which retrieves the information from dma_device.
> 
> That way, DMA engine drivers which are using the new method can just
> install the new function, and those which haven't been updated with
> capabilities can carry on as they are, and are detectable to drivers.

Which is pretty much the current behaviour, isn't it?

> What would be acceptable is to have the DMA engine registration function
> spot the lack of DMA caps function and print a warning at boot to
> encourage people to add it.

That would be an option too.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141001/d72be5ed/attachment-0001.sig>

[PATCH 2/9] dmaengine: Make channel allocation callbacks optional

[Maxime Ripard]

Hi Vinod,

On Sun, Sep 28, 2014 at 09:37:57PM +0530, Vinod Koul wrote:
> On Sat, Sep 27, 2014 at 10:54:38AM +0200, Maxime Ripard wrote:
> > Nowadays, some drivers don't have anything in there channel allocation
> > callbacks anymore.
> > 
> > Remove the BUG_ON if those callbacks aren't implemented, in order to allow
> > drivers to not implement them.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/dma/dmaengine.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > index d5d30ed863ce..cfcb181b1184 100644
> > --- a/drivers/dma/dmaengine.c
> > +++ b/drivers/dma/dmaengine.c
> > @@ -817,8 +817,6 @@ int dma_async_device_register(struct dma_device *device)
> >  	BUG_ON(dma_has_cap(DMA_INTERLEAVE, device->cap_mask) &&
> >  		!device->device_prep_interleaved_dma);
> >  
> > -	BUG_ON(!device->device_alloc_chan_resources);
> > -	BUG_ON(!device->device_free_chan_resources);
> Dont think we have drivers without free. IIRC cppi one might be only
> instance

It was more a matter of simmetry to be honest, I'm fine dropping it if
you want.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141001/74b512bf/attachment.sig>

[PATCH 7/9] dmaengine: Move slave caps to dma_device

[Russell King - ARM Linux]

On Wed, Oct 01, 2014 at 10:27:05AM +0200, Maxime Ripard wrote:
> On Sat, Sep 27, 2014 at 10:28:47AM +0100, Russell King - ARM Linux wrote:
> > The only way to do this is to either have a flag day, fixing all drivers
> > at once (which isn't going to happen) or leave the caps code as-is, and
> > provide a library function which drivers can hook into the caps callback
> > which retrieves the information from dma_device.
> > 
> > That way, DMA engine drivers which are using the new method can just
> > install the new function, and those which haven't been updated with
> > capabilities can carry on as they are, and are detectable to drivers.
> 
> Which is pretty much the current behaviour, isn't it?

Yes, because the ASoC code has obviously already thought about this and
solved the lack-of-caps-function problem in a way that permits existing
solutions to continue working.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.