DMA: add help function to check whether dma controller registered

PATCH Superseded Richard Zhao 2 participants 7 messages Aug 2, 2013 – Aug 23, 2013

Subsystems (2)

dmaengine Device tree

lore

Share & tools

lore

Richard Zhao

author

2013-08-02 06:04 UTC #0 lore raw

PATCH 1/1 DMA: add help function to check whether dma controller registered

DMA client device driver usually needs to know at probe time whether dma controller has been registered to deffer probe. So add a help function of_dma_check_controller.

DMA request channel functions can also used to check it, but they are usually called at open() time.

Signed-off-by Richard Zhao <[email protected]>

---
 drivers/dma/of-dma.c   | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/of_dma.h |  6 ++++++
 2 files changed, 44 insertions(+)

drivers/dma/of-dma.c +38 -0

index e1c4d3b..b6828c1 100644
@@ -188,6 +188,44 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 }
 
 /**
+ * of_dma_check_controller - Check whether dma controller registered
+ * @dev:	pointer to client device structure
+ * @name:	slave channel name
+ */
+int of_dma_check_controller(struct device *dev, const char *name)
+{
+	struct device_node *np = dev->of_node;
+	struct of_phandle_args dma_spec;
+	struct of_dma *ofdma = NULL;
+	int count, i;
+
+	if (!np || !name)
+		return -EINVAL;
+
+	count = of_property_count_strings(np, "dma-names");
+	if (count < 0) {
+		dev_err(dev, "dma-names property missing or empty\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < count; i++) {
+		if (of_dma_match_channel(np, name, i, &dma_spec))
+			continue;
+
+		mutex_lock(&of_dma_lock);
+		ofdma = of_dma_find_controller(&dma_spec);
+		mutex_unlock(&of_dma_lock);
+		of_node_put(dma_spec.np);
+	}
+
+	if (ofdma)
+		return 0;
+	else
+		return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(of_dma_check_controller);
+
+/**
  * of_dma_simple_xlate - Simple DMA engine translation function
  * @dma_spec:	pointer to DMA specifier as found in the device tree
  * @of_dma:	pointer to DMA controller data

include/linux/of_dma.h +6 -0

index ae36298..bc7195f 100644
@@ -39,6 +39,7 @@ extern int of_dma_controller_register(struct device_node *np,
 extern void of_dma_controller_free(struct device_node *np);
 extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 						     const char *name);
+extern int of_dma_check_controller(struct device *dev, const char *name);
 extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 		struct of_dma *ofdma);
 #else

@@ -60,6 +61,11 @@ static inline struct dma_chan *of_dma_request_slave_channel(struct device_node *
 	return NULL;
 }
 
+static inline int of_dma_check_controller(struct device *dev, const char *name)
+{
+	return 0;
+}
+
 static inline struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 		struct of_dma *ofdma)
 {

Signature · 1 line

1.8.1.5

Stephen Warren

2013-08-02 19:59 UTC #1 lore raw

Quoted 11 lines · Richard Zhao

DMA client device driver usually needs to know at probe time whether dma controller has been registered to deffer probe. So add a help function of_dma_check_controller.
DMA request channel functions can also used to check it, but they are usually called at open() time.
drivers/dma/of-dma.c +1 -0
index e1c4d3b..b6828c1 100644
 
+int of_dma_check_controller(struct device *dev, const char *name)

drivers/dma/of-dma.c struct dma_chan *of_dma_request_slave_channel(struct device_node *np, · line 218

Quoted 10 lines

drivers/dma/of-dma.c +8 -0

 
 
+	for (i = 0; i < count; i++) {
+		if (of_dma_match_channel(np, name, i, &dma_spec))
+			continue;
+
+		mutex_lock(&of_dma_lock);
+		ofdma = of_dma_find_controller(&dma_spec);
+		mutex_unlock(&of_dma_lock);
+		of_node_put(dma_spec.np);

Do we need to add the following here:

if (ofdma)
break

To ensure that as soon as a successful match is found, the loop exits? Otherwise, if there are multiple providers for that name, and the first N are registered but the last isn't, this function will still return failure.

drivers/dma/of-dma.c struct dma_chan *of_dma_request_slave_channel(struct device_node *np, · line 224

Quoted 4 lines

drivers/dma/of-dma.c +4 -0

+	if (ofdma)
+		return 0;
+	else
+		return -ENODEV;

That probably should be -EPROBE_DEFER?

Although, what about differentiating between "entry not found by of_dma_match_channel" and "controller not yet probed"?

Richard Zhao

author

2013-08-05 05:56 UTC #2 lore raw

Quoted 25 lines · Stephen Warren

Quoted 11 lines · Richard Zhao
DMA client device driver usually needs to know at probe time whether dma controller has been registered to deffer probe. So add a help function of_dma_check_controller.
DMA request channel functions can also used to check it, but they are usually called at open() time.
drivers/dma/of-dma.c +1 -0
index e1c4d3b..b6828c1 100644
 
+int of_dma_check_controller(struct device *dev, const char *name)
drivers/dma/of-dma.c struct dma_chan *of_dma_request_slave_channel(struct device_node *np, · line 218
Quoted 10 lines
drivers/dma/of-dma.c +8 -0
 
 
+	for (i = 0; i < count; i++) {
+		if (of_dma_match_channel(np, name, i, &dma_spec))
+			continue;
+
+		mutex_lock(&of_dma_lock);
+		ofdma = of_dma_find_controller(&dma_spec);
+		mutex_unlock(&of_dma_lock);
+		of_node_put(dma_spec.np);
Do we need to add the following here:

Quoted 7 lines

if (ofdma)
break
To ensure that as soon as a successful match is found, the loop exits? Otherwise, if there are multiple providers for that name, and the first N are registered but the last isn't, this function will still return failure.

Right. Thanks.

Quoted 10 lines

drivers/dma/of-dma.c struct dma_chan *of_dma_request_slave_channel(struct device_node *np, · line 224
Quoted 5 lines
drivers/dma/of-dma.c +4 -0
+	if (ofdma)
+		return 0;
+	else
+		return -ENODEV;
That probably should be -EPROBE_DEFER?
Although, what about differentiating between "entry not found by of_dma_match_channel" and "controller not yet probed"?

I thought it might be called in non-probe functions. If no people against it, I'll change it to EPROBE_DEFER.

Thanks
Richard

Richard Zhao

author

2013-08-22 06:43 UTC #3 lore raw

PATCH v2 1/1 DMA: add help function to check whether dma controller registered

In reply to Richard Zhao

DMA client device driver usually needs to know at probe time whether dma controller has been registered to deffer probe. So add a help function of_dma_check_controller.

DMA request channel functions can also used to check it, but they are usually called at open() time.

Signed-off-by Richard Zhao <[email protected]>

---
 drivers/dma/of-dma.c   | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/of_dma.h |  6 ++++++
 2 files changed, 45 insertions(+)

drivers/dma/of-dma.c +39 -0

index 8cd5f3f..0063ddf 100644
@@ -189,6 +189,45 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 }
 
 /**
+ * of_dma_check_controller - Check whether dma controller registered
+ * @dev:	pointer to client device structure
+ * @name:	slave channel name
+ */
+int of_dma_check_controller(struct device *dev, const char *name)
+{
+	struct device_node *np = dev->of_node;
+	struct of_phandle_args dma_spec;
+	struct of_dma *ofdma = NULL;
+	int count, i, ret = 0;
+
+	if (!np || !name)
+		return -EINVAL;
+
+	count = of_property_count_strings(np, "dma-names");
+	if (count < 0) {
+		dev_err(dev, "dma-names property missing or empty\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < count; i++) {
+		ret = of_dma_match_channel(np, name, i, &dma_spec);
+		if (ret)
+			continue;
+
+		mutex_lock(&of_dma_lock);
+		ofdma = of_dma_find_controller(&dma_spec);
+		mutex_unlock(&of_dma_lock);
+		of_node_put(dma_spec.np);
+		if (!ofdma)
+			ret = -EPROBE_DEFER;
+		break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_dma_check_controller);
+
+/**
  * of_dma_simple_xlate - Simple DMA engine translation function
  * @dma_spec:	pointer to DMA specifier as found in the device tree
  * @of_dma:	pointer to DMA controller data

include/linux/of_dma.h +6 -0

index ae36298..bc7195f 100644
@@ -39,6 +39,7 @@ extern int of_dma_controller_register(struct device_node *np,
 extern void of_dma_controller_free(struct device_node *np);
 extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 						     const char *name);
+extern int of_dma_check_controller(struct device *dev, const char *name);
 extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 		struct of_dma *ofdma);
 #else

@@ -60,6 +61,11 @@ static inline struct dma_chan *of_dma_request_slave_channel(struct device_node *
 	return NULL;
 }
 
+static inline int of_dma_check_controller(struct device *dev, const char *name)
+{
+	return 0;
+}
+
 static inline struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 		struct of_dma *ofdma)
 {

Signature · 1 line

1.8.1.5

Stephen Warren

2013-08-22 20:36 UTC #4 lore raw

Quoted 6 lines · Richard Zhao

DMA client device driver usually needs to know at probe time whether dma controller has been registered to deffer probe. So add a help function of_dma_check_controller.
DMA request channel functions can also used to check it, but they are usually called at open() time.

This new function is almost identical to the existing of_dma_request_slave_channel(). Surely the code should be shared?

But that said, I don't see any need for a new function; why can't drivers simply call of_dma_request_slave_channel() at probe time; from what I can see, that function doesn't actually request the channel, but rather simply looks it up, just like this one. The only difference is that of_dma_xlate() is also called, but that's just doing some data transformation, not actually recording channel ownership.

Richard Zhao

author

2013-08-23 01:17 UTC #5 lore raw

Quoted 10 lines · Stephen Warren

Quoted 6 lines · Richard Zhao
DMA client device driver usually needs to know at probe time whether dma controller has been registered to deffer probe. So add a help function of_dma_check_controller.
DMA request channel functions can also used to check it, but they are usually called at open() time.
This new function is almost identical to the existing of_dma_request_slave_channel(). Surely the code should be shared?

ofdma->of_dma_xlate(&dma_spec, ofdma);
The above is called holding of_dma_lock. If I want to abstract the common lines, there' two options.

Option 1:
static struct of_dma* of_dma_check_controller_locked(np, name) {
parameter check
get dma-names count and check return value for loop to get of_dma
return PTR_ERR(err) or of_dma
}

struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
const char *name)

{
	chan = null;
	mutex_lock(&of_dma_lock);
	of_dma = of_dma_check_controller_locked(np, name)
	if(!IS_ERR(of_dma))
		chan = ofdma->of_dma_xlate(&dma_spec, ofdma);
	mutex_unlock(&of_dma_lock);
	return chan;
}

int of_dma_check_controller(struct device *dev, const char *name)

{
	mutex_lock(&of_dma_lock);
	ofdma = of_dma_check_controller_locked(dev->of_node, name);
	mutex_unlock(&of_dma_lock);
	if (IS_ERR(ofdma))
		return ERR_PTR(ofdma);
	else
		return 0;
}

Option 2:
static struct of_dma* of_dma_check_controller_getlock(np, name) {
parameter check
get dma-names count and check return value for loop to get of_dma, get lock at old place if failed, unlock.
return PTR_ERR(err) or of_dma
}

struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
const char *name)
{
} of_dma = of_dma_check_controller_getlock(np, name)

	if(!IS_ERR(of_dma)) {
		chan = ofdma->of_dma_xlate(&dma_spec, ofdma);
		unlock;
	}
	return chan;
}

int of_dma_check_controller(struct device *dev, const char *name)

	ofdma = of_dma_check_controller_locked(dev->of_node, name);

	if (IS_ERR(ofdma)) {
		return ERR_PTR(ofdma);
	} else {
		unlock;
		return 0;
	}
}

Quoted 2 lines

But that said, I don't see any need for a new function; why can't drivers simply call of_dma_request_slave_channel() at probe time;

It'll mislead user. channel supposed to be request at open time.

Quoted 5 lines

from
what I can see, that function doesn't actually request the channel, but rather simply looks it up, just like this one. The only difference is that of_dma_xlate() is also called, but that's just doing some data transformation, not actually recording channel ownership.

xlate function request the channel if things go well.

Thanks
Richard

Stephen Warren

2013-08-23 15:56 UTC #6 lore raw

Quoted 9 lines · Richard Zhao

Quoted 8 lines · Stephen Warren
Quoted 6 lines · Richard Zhao
DMA client device driver usually needs to know at probe time whether dma controller has been registered to deffer probe. So add a help function of_dma_check_controller.
DMA request channel functions can also used to check it, but they are usually called at open() time.

Quoted 6 lines

Quoted 2 lines
This new function is almost identical to the existing of_dma_request_slave_channel(). Surely the code should be shared?
ofdma->of_dma_xlate(&dma_spec, ofdma);
The above is called holding of_dma_lock. If I want to abstract the common lines, there' two options.

What is the problem with acquiring the lock? If request_slave_channel() needs to take the lock, then there must be a reason which presumably applies to the other path too.

...

Quoted 4 lines

Quoted 2 lines
But that said, I don't see any need for a new function; why can't drivers simply call of_dma_request_slave_channel() at probe time;
It'll mislead user. channel supposed to be request at open time.

I don't agree.

Quoted 7 lines

Quoted 5 lines
from
what I can see, that function doesn't actually request the channel, but rather simply looks it up, just like this one. The only difference is that of_dma_xlate() is also called, but that's just doing some data transformation, not actually recording channel ownership.
xlate function request the channel if things go well.

Oh. xlate should not do that; that's a design flaw. xlate should do nothing more than translate the DT content to an internal channel ID.