[V2] dma: add common of_dma_slave_xlate()

Commit Message

Stephen Warren Nov. 26, 2013, 1:25 a.m. UTC

From: Stephen Warren <[e-mail]>

mmp_pdma.c implements a custom of_xlate() function that is 95% identical
to what Tegra will need. Create a function to implement the common part,
so everyone doesn't just cut/paste the implementation.

Cc: Dan Williams <[e-mail]>
Cc: Vinod Koul <[e-mail]>
Cc: [e-mail]
Signed-off-by: Stephen Warren <[e-mail]>
---
v2: Squashed the conversion of mmp_pdma.c into the patch that added the
common implementation, so it's easier to see the whole conversion in one
go.

This patch is a dependency for a series that reworks many of the Tegra
drivers.

As such, it needs to go into a topic branch on its own, based directly
on 3.13-rc1. If the DMA maintainers ack the patches I'm happy to create
this topic branch myself and send a pull request to the DMA tree. Or the
patches can be applied to a topic branch by the DMA maintainers and I
will merge their topic branch into the Tegra rework branch that I
mentioned.

Note that this patch is independant from the "dma: add channel request
API that supports deferred probe" which I just sent, so it could
(should?) be a different topic branch.
---
 drivers/dma/mmp_pdma.c | 39 ++++++++++-----------------------------
 drivers/dma/of-dma.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_dma.h | 14 ++++++++++++++
 3 files changed, 67 insertions(+), 29 deletions(-)

Comments

Lars-Peter Clausen Nov. 26, 2013, 7:40 a.m. UTC | #1

On 11/26/2013 02:25 AM, Stephen Warren wrote:
[...]
> +struct dma_chan *of_dma_slave_xlate(struct of_phandle_args *dma_spec,
> +				    struct of_dma *ofdma)
> +{
> +	struct of_dma_slave_xlate_info *info = ofdma->of_dma_data;
> +	struct dma_chan *candidate, *chan;
> +	int ret;
> +
> +retry:
> +	candidate = NULL;
> +
> +	/*
> +	 * walk the list of channels registered with the current instance and
> +	 * find one that is currently unused
> +	 */
> +	list_for_each_entry(chan, &info->device->channels, device_node)
> +		if (chan->client_count == 0) {
> +			candidate = chan;
> +			break;
> +		}
> +
> +	if (!candidate)
> +		return NULL;
> +
> +	/*
> +	 * dma_get_slave_channel will return NULL if we lost a race between
> +	 * the lookup and the reservation
> +	 */
> +	chan = dma_get_slave_channel(candidate);
> +	if (!chan)
> +		goto retry;

I think it will be better to implement this functionality in the core. This
means we can hold the dma_list_mutex and don't have to do this retry loop
and do not have to peek at the client_count field in non-core code.
Something like dma_get_free_slave_channel(), which would call
private_candidate() followed by dma_chan_get().

> +
> +	if (info->post_alloc) {
> +		ret = info->post_alloc(chan, dma_spec);

If you need to do something at the end of the function I think it is nicer
to just wrap this function with your own function instead of adding a callback.

- Lars


--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dan Williams Nov. 26, 2013, 4:57 p.m. UTC | #2

On Mon, Nov 25, 2013 at 11:40 PM, Lars-Peter Clausen <[e-mail]> wrote:
> On 11/26/2013 02:25 AM, Stephen Warren wrote:
> [...]
>> +struct dma_chan *of_dma_slave_xlate(struct of_phandle_args *dma_spec,
>> +                                 struct of_dma *ofdma)
>> +{
>> +     struct of_dma_slave_xlate_info *info = ofdma->of_dma_data;
>> +     struct dma_chan *candidate, *chan;
>> +     int ret;
>> +
>> +retry:
>> +     candidate = NULL;
>> +
>> +     /*
>> +      * walk the list of channels registered with the current instance and
>> +      * find one that is currently unused
>> +      */
>> +     list_for_each_entry(chan, &info->device->channels, device_node)
>> +             if (chan->client_count == 0) {
>> +                     candidate = chan;
>> +                     break;
>> +             }
>> +
>> +     if (!candidate)
>> +             return NULL;
>> +
>> +     /*
>> +      * dma_get_slave_channel will return NULL if we lost a race between
>> +      * the lookup and the reservation
>> +      */
>> +     chan = dma_get_slave_channel(candidate);
>> +     if (!chan)
>> +             goto retry;
>
> I think it will be better to implement this functionality in the core. This
> means we can hold the dma_list_mutex and don't have to do this retry loop
> and do not have to peek at the client_count field in non-core code.
> Something like dma_get_free_slave_channel(), which would call
> private_candidate() followed by dma_chan_get().
>
>> +
>> +     if (info->post_alloc) {
>> +             ret = info->post_alloc(chan, dma_spec);
>
> If you need to do something at the end of the function I think it is nicer
> to just wrap this function with your own function instead of adding a callback.
>

Agree with both points above.  And if the common pattern is to limit
the domain to children of a given parent device that can be
implemented generically with a device_for_each_child_loop or similar.
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index dcb1e05149a7..9705cedf623d 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -128,6 +128,7 @@  struct mmp_pdma_device {
 	void __iomem			*base;
 	struct device			*dev;
 	struct dma_device		device;
+	struct of_dma_slave_xlate_info	xlate_info;
 	struct mmp_pdma_phy		*phy;
 	spinlock_t phy_lock; /* protect alloc/free phy channels */
 };
@@ -889,37 +890,14 @@  static struct of_device_id mmp_pdma_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, mmp_pdma_dt_ids);
 
-static struct dma_chan *mmp_pdma_dma_xlate(struct of_phandle_args *dma_spec,
-					   struct of_dma *ofdma)
+static int mmp_pdma_of_xlate_post_alloc(struct dma_chan *chan,
+					struct of_phandle_args *dma_spec)
 {
-	struct mmp_pdma_device *d = ofdma->of_dma_data;
-	struct dma_chan *chan, *candidate;
-
-retry:
-	candidate = NULL;
-
-	/* walk the list of channels registered with the current instance and
-	 * find one that is currently unused */
-	list_for_each_entry(chan, &d->device.channels, device_node)
-		if (chan->client_count == 0) {
-			candidate = chan;
-			break;
-		}
-
-	if (!candidate)
-		return NULL;
+	struct mmp_pdma_chan *c = to_mmp_pdma_chan(chan);
 
-	/* dma_get_slave_channel will return NULL if we lost a race between
-	 * the lookup and the reservation */
-	chan = dma_get_slave_channel(candidate);
+	c->drcmr = dma_spec->args[0];
 
-	if (chan) {
-		struct mmp_pdma_chan *c = to_mmp_pdma_chan(chan);
-		c->drcmr = dma_spec->args[0];
-		return chan;
-	}
-
-	goto retry;
+	return 0;
 }
 
 static int mmp_pdma_probe(struct platform_device *op)
@@ -1009,8 +987,11 @@  static int mmp_pdma_probe(struct platform_device *op)
 
 	if (op->dev.of_node) {
 		/* Device-tree DMA controller registration */
+		pdev->xlate_info.device = &pdev->device;
+		pdev->xlate_info.post_alloc = mmp_pdma_of_xlate_post_alloc;
 		ret = of_dma_controller_register(op->dev.of_node,
-						 mmp_pdma_dma_xlate, pdev);
+						 of_dma_slave_xlate,
+						 &pdev->xlate_info);
 		if (ret < 0) {
 			dev_err(&op->dev, "of_dma_controller_register failed\n");
 			return ret;
diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index f68e25c9af3a..d9786a9c8797 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -218,3 +218,46 @@  struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 			&dma_spec->args[0]);
 }
 EXPORT_SYMBOL_GPL(of_dma_simple_xlate);
+
+struct dma_chan *of_dma_slave_xlate(struct of_phandle_args *dma_spec,
+				    struct of_dma *ofdma)
+{
+	struct of_dma_slave_xlate_info *info = ofdma->of_dma_data;
+	struct dma_chan *candidate, *chan;
+	int ret;
+
+retry:
+	candidate = NULL;
+
+	/*
+	 * walk the list of channels registered with the current instance and
+	 * find one that is currently unused
+	 */
+	list_for_each_entry(chan, &info->device->channels, device_node)
+		if (chan->client_count == 0) {
+			candidate = chan;
+			break;
+		}
+
+	if (!candidate)
+		return NULL;
+
+	/*
+	 * dma_get_slave_channel will return NULL if we lost a race between
+	 * the lookup and the reservation
+	 */
+	chan = dma_get_slave_channel(candidate);
+	if (!chan)
+		goto retry;
+
+	if (info->post_alloc) {
+		ret = info->post_alloc(chan, dma_spec);
+		if (ret) {
+			dma_release_channel(chan);
+			return NULL;
+		}
+	}
+
+	return chan;
+}
+EXPORT_SYMBOL_GPL(of_dma_slave_xlate);
diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
index ae36298ba076..fb4187adc68b 100644
--- a/include/linux/of_dma.h
+++ b/include/linux/of_dma.h
@@ -31,6 +31,12 @@  struct of_dma_filter_info {
 	dma_filter_fn	filter_fn;
 };
 
+struct of_dma_slave_xlate_info {
+	struct dma_device *device;
+	int (*post_alloc)(struct dma_chan *chan,
+			  struct of_phandle_args *dma_spec);
+};
+
 #ifdef CONFIG_OF
 extern int of_dma_controller_register(struct device_node *np,
 		struct dma_chan *(*of_dma_xlate)
@@ -41,6 +47,8 @@  extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 						     const char *name);
 extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 		struct of_dma *ofdma);
+extern struct dma_chan *of_dma_slave_xlate(struct of_phandle_args *dma_spec,
+		struct of_dma *ofdma);
 #else
 static inline int of_dma_controller_register(struct device_node *np,
 		struct dma_chan *(*of_dma_xlate)
@@ -66,6 +74,12 @@  static inline struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_s
 	return NULL;
 }
 
+static inline struct dma_chan *of_dma_slave_xlate(struct of_phandle_args *dma_spec,
+		struct of_dma *ofdma)
+{
+	return NULL;
+}
+
 #endif
 
 #endif /* __LINUX_OF_DMA_H */