There is no sensible reason why #dma-cells shouldn't be allowed to be 0. It is
completely up to the DMA controller how many additional parameters, besides the
phandle, it needs to identify a channel. E.g. for DMA controller with only one
channel or for DMA controllers which don't have a restriction on which channel
can be used for which peripheral it completely legitimate to not require any
additional parameters.
Also fixes the following warning:
drivers/dma/of-dma.c: In function 'of_dma_controller_register':
drivers/dma/of-dma.c:67:7: warning: 'nbcells' may be used uninitialized in this function
Signed-off-by: Lars-Peter Clausen <[email protected]>
---
drivers/dma/of-dma.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 7aa0864..268cc8a 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -64,7 +64,6 @@ int of_dma_controller_register(struct device_node *np,
void *data)
{
struct of_dma *ofdma;
- int nbcells;
const __be32 *prop;
if (!np || !of_dma_xlate) {
@@ -77,18 +76,16 @@ int of_dma_controller_register(struct device_node *np,
return -ENOMEM;
prop = of_get_property(np, "#dma-cells", NULL);
- if (prop)
- nbcells = be32_to_cpup(prop);
-
- if (!prop || !nbcells) {
- pr_err("%s: #dma-cells property is missing or invalid\n",
+ if (!prop) {
+ pr_err("%s: #dma-cells property is missing\n",
__func__);
kfree(ofdma);
return -EINVAL;
}
+
ofdma->of_node = np;
- ofdma->of_dma_nbcells = nbcells;
+ ofdma->of_dma_nbcells = be32_to_cpup(prop);
ofdma->of_dma_xlate = of_dma_xlate;
ofdma->of_dma_data = data;
--
1.8.0
Both of_dma_nbcells field of the of_dma_controller and the args_count field of
the dma_spec are initialized by parsing the #dma-cells attribute of their device
tree node. So if the device tree nodes of a DMA controller and the dma_spec
match this means that of_dma_nbcells and args_count will also match. So the
second test in the of_dma_find_controller loop is redundant because given the
first test yields true the second test will also yield true. So we can safely
remove the test whether of_dma_nbcells matches args_count. Since this was the
last user of the of_dma_nbcells field we can remove it altogether.
Signed-off-by: Lars-Peter Clausen <[email protected]>
---
drivers/dma/of-dma.c | 14 +-------------
include/linux/of_dma.h | 1 -
2 files changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 268cc8a..75334bd 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -35,8 +35,7 @@ static struct of_dma *of_dma_find_controller(struct of_phandle_args *dma_spec)
struct of_dma *ofdma;
list_for_each_entry(ofdma, &of_dma_list, of_dma_controllers)
- if ((ofdma->of_node == dma_spec->np) &&
- (ofdma->of_dma_nbcells == dma_spec->args_count))
+ if (ofdma->of_node == dma_spec->np)
return ofdma;
pr_debug("%s: can't find DMA controller %s\n", __func__,
@@ -64,7 +63,6 @@ int of_dma_controller_register(struct device_node *np,
void *data)
{
struct of_dma *ofdma;
- const __be32 *prop;
if (!np || !of_dma_xlate) {
pr_err("%s: not enough information provided\n", __func__);
@@ -75,17 +73,7 @@ int of_dma_controller_register(struct device_node *np,
if (!ofdma)
return -ENOMEM;
- prop = of_get_property(np, "#dma-cells", NULL);
- if (!prop) {
- pr_err("%s: #dma-cells property is missing\n",
- __func__);
- kfree(ofdma);
- return -EINVAL;
- }
-
-
ofdma->of_node = np;
- ofdma->of_dma_nbcells = be32_to_cpup(prop);
ofdma->of_dma_xlate = of_dma_xlate;
ofdma->of_dma_data = data;
diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
index 364dda7..ae36298 100644
--- a/include/linux/of_dma.h
+++ b/include/linux/of_dma.h
@@ -21,7 +21,6 @@ struct device_node;
struct of_dma {
struct list_head of_dma_controllers;
struct device_node *of_node;
- int of_dma_nbcells;
struct dma_chan *(*of_dma_xlate)
(struct of_phandle_args *, struct of_dma *);
void *of_dma_data;
--
1.8.0
On Monday 22 April 2013, Lars-Peter Clausen wrote:
>
> There is no sensible reason why #dma-cells shouldn't be allowed to be 0. It is
> completely up to the DMA controller how many additional parameters, besides the
> phandle, it needs to identify a channel. E.g. for DMA controller with only one
> channel or for DMA controllers which don't have a restriction on which channel
> can be used for which peripheral it completely legitimate to not require any
> additional parameters.
>
> Also fixes the following warning:
> drivers/dma/of-dma.c: In function 'of_dma_controller_register':
> drivers/dma/of-dma.c:67:7: warning: 'nbcells' may be used uninitialized in this function
>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
Do you have an example for this? If a dma engine has only one request line,
why would you even use the dmaengine subsystem for it, rather than including
the code to program it in the slave driver?
Arnd
On Monday 22 April 2013, Lars-Peter Clausen wrote:
> Both of_dma_nbcells field of the of_dma_controller and the args_count field of
> the dma_spec are initialized by parsing the #dma-cells attribute of their device
> tree node. So if the device tree nodes of a DMA controller and the dma_spec
> match this means that of_dma_nbcells and args_count will also match. So the
> second test in the of_dma_find_controller loop is redundant because given the
> first test yields true the second test will also yield true. So we can safely
> remove the test whether of_dma_nbcells matches args_count. Since this was the
> last user of the of_dma_nbcells field we can remove it altogether.
>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
> ---
Acked-by: Arnd Bergmann <[email protected]>
On 04/22/2013 02:38 PM, Arnd Bergmann wrote:
> On Monday 22 April 2013, Lars-Peter Clausen wrote:
>>
>> There is no sensible reason why #dma-cells shouldn't be allowed to be 0. It is
>> completely up to the DMA controller how many additional parameters, besides the
>> phandle, it needs to identify a channel. E.g. for DMA controller with only one
>> channel or for DMA controllers which don't have a restriction on which channel
>> can be used for which peripheral it completely legitimate to not require any
>> additional parameters.
>>
>> Also fixes the following warning:
>> drivers/dma/of-dma.c: In function 'of_dma_controller_register':
>> drivers/dma/of-dma.c:67:7: warning: 'nbcells' may be used uninitialized in this function
>>
>> Signed-off-by: Lars-Peter Clausen <[email protected]>
>
> Do you have an example for this? If a dma engine has only one request line,
> why would you even use the dmaengine subsystem for it, rather than including
> the code to program it in the slave driver?
Why wouldn't I use the dmaengine subsystem for a DMA controller? In my
particular case different instances of the same DMA core will be used with
different DMA slaves. And the DMA slaves can also have different DMA master
cores, depending on the system.
- Lars
On Monday 22 April 2013, Lars-Peter Clausen wrote:
>
> On 04/22/2013 02:38 PM, Arnd Bergmann wrote:
> > On Monday 22 April 2013, Lars-Peter Clausen wrote:
> >>
> >> There is no sensible reason why #dma-cells shouldn't be allowed to be 0. It is
> >> completely up to the DMA controller how many additional parameters, besides the
> >> phandle, it needs to identify a channel. E.g. for DMA controller with only one
> >> channel or for DMA controllers which don't have a restriction on which channel
> >> can be used for which peripheral it completely legitimate to not require any
> >> additional parameters.
> >>
> >> Also fixes the following warning:
> >> drivers/dma/of-dma.c: In function 'of_dma_controller_register':
> >> drivers/dma/of-dma.c:67:7: warning: 'nbcells' may be used uninitialized in this function
> >>
> >> Signed-off-by: Lars-Peter Clausen <[email protected]>
> >
> > Do you have an example for this? If a dma engine has only one request line,
> > why would you even use the dmaengine subsystem for it, rather than including
> > the code to program it in the slave driver?
>
> Why wouldn't I use the dmaengine subsystem for a DMA controller? In my
> particular case different instances of the same DMA core will be used with
> different DMA slaves. And the DMA slaves can also have different DMA master
> cores, depending on the system.
Right, that makes sense.
Acked-by: Arnd Bergmann <[email protected]>
On 04/22/2013 03:33 AM, Lars-Peter Clausen wrote:
> Both of_dma_nbcells field of the of_dma_controller and the args_count field of
> the dma_spec are initialized by parsing the #dma-cells attribute of their device
> tree node. So if the device tree nodes of a DMA controller and the dma_spec
> match this means that of_dma_nbcells and args_count will also match. So the
> second test in the of_dma_find_controller loop is redundant because given the
> first test yields true the second test will also yield true. So we can safely
> remove the test whether of_dma_nbcells matches args_count. Since this was the
> last user of the of_dma_nbcells field we can remove it altogether.
This assumes that someone has correctly added the dma information to the
dma slave binding. I could see systems where different dma controllers
have different of_dma_nbcells and so someone could put the enter wrong
number of cells for a dma slave binding. Its really to catch user error.
> Signed-off-by: Lars-Peter Clausen <[email protected]>
> ---
> drivers/dma/of-dma.c | 14 +-------------
> include/linux/of_dma.h | 1 -
> 2 files changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
> index 268cc8a..75334bd 100644
> --- a/drivers/dma/of-dma.c
> +++ b/drivers/dma/of-dma.c
> @@ -35,8 +35,7 @@ static struct of_dma *of_dma_find_controller(struct of_phandle_args *dma_spec)
> struct of_dma *ofdma;
>
> list_for_each_entry(ofdma, &of_dma_list, of_dma_controllers)
> - if ((ofdma->of_node == dma_spec->np) &&
> - (ofdma->of_dma_nbcells == dma_spec->args_count))
> + if (ofdma->of_node == dma_spec->np)
> return ofdma;
Other device-tree functions perform similar tests to this such as ...
static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
{
struct gg_data *gg_data = data;
int ret;
if ((gc->of_node != gg_data->gpiospec.np) ||
(gc->of_gpio_n_cells != gg_data->gpiospec.args_count) ||
(!gc->of_xlate))
return false;
...
Cheers
Jon
On 04/22/2013 10:52 PM, Jon Hunter wrote:
>
> On 04/22/2013 03:33 AM, Lars-Peter Clausen wrote:
>> Both of_dma_nbcells field of the of_dma_controller and the args_count field of
>> the dma_spec are initialized by parsing the #dma-cells attribute of their device
>> tree node. So if the device tree nodes of a DMA controller and the dma_spec
>> match this means that of_dma_nbcells and args_count will also match. So the
>> second test in the of_dma_find_controller loop is redundant because given the
>> first test yields true the second test will also yield true. So we can safely
>> remove the test whether of_dma_nbcells matches args_count. Since this was the
>> last user of the of_dma_nbcells field we can remove it altogether.
>
> This assumes that someone has correctly added the dma information to the
> dma slave binding. I could see systems where different dma controllers
> have different of_dma_nbcells and so someone could put the enter wrong
> number of cells for a dma slave binding. Its really to catch user error.
No, this assumes nothing. The condition will _always_ be true.
dma_spec->args_count is initialized by parsing the #dma-cells attribute of
dma_sepc->np. of_dma->of_dma_nbcells is initialized by parsing the #dma-cells
attribute of of_dma->of_node. If ofdma->of_node equals dma_spec->np then
dma_spec->args_count will also equal of_dma->of_dma_nbcells.
>
>> Signed-off-by: Lars-Peter Clausen <[email protected]>
>> ---
>> drivers/dma/of-dma.c | 14 +-------------
>> include/linux/of_dma.h | 1 -
>> 2 files changed, 1 insertion(+), 14 deletions(-)
>>
>> diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
>> index 268cc8a..75334bd 100644
>> --- a/drivers/dma/of-dma.c
>> +++ b/drivers/dma/of-dma.c
>> @@ -35,8 +35,7 @@ static struct of_dma *of_dma_find_controller(struct of_phandle_args *dma_spec)
>> struct of_dma *ofdma;
>>
>> list_for_each_entry(ofdma, &of_dma_list, of_dma_controllers)
>> - if ((ofdma->of_node == dma_spec->np) &&
>> - (ofdma->of_dma_nbcells == dma_spec->args_count))
>> + if (ofdma->of_node == dma_spec->np)
>> return ofdma;
>
> Other device-tree functions perform similar tests to this such as ...
>
So it's redundant there as well ;)
> static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
> {
> struct gg_data *gg_data = data;
> int ret;
>
> if ((gc->of_node != gg_data->gpiospec.np) ||
> (gc->of_gpio_n_cells != gg_data->gpiospec.args_count) ||
> (!gc->of_xlate))
> return false;
>
> ...
>
> Cheers
> Jon
On 04/22/2013 04:00 PM, Lars-Peter Clausen wrote:
> On 04/22/2013 10:52 PM, Jon Hunter wrote:
>>
>> On 04/22/2013 03:33 AM, Lars-Peter Clausen wrote:
>>> Both of_dma_nbcells field of the of_dma_controller and the args_count field of
>>> the dma_spec are initialized by parsing the #dma-cells attribute of their device
>>> tree node. So if the device tree nodes of a DMA controller and the dma_spec
>>> match this means that of_dma_nbcells and args_count will also match. So the
>>> second test in the of_dma_find_controller loop is redundant because given the
>>> first test yields true the second test will also yield true. So we can safely
>>> remove the test whether of_dma_nbcells matches args_count. Since this was the
>>> last user of the of_dma_nbcells field we can remove it altogether.
>>
>> This assumes that someone has correctly added the dma information to the
>> dma slave binding. I could see systems where different dma controllers
>> have different of_dma_nbcells and so someone could put the enter wrong
>> number of cells for a dma slave binding. Its really to catch user error.
>
> No, this assumes nothing. The condition will _always_ be true.
>
> dma_spec->args_count is initialized by parsing the #dma-cells attribute of
> dma_sepc->np. of_dma->of_dma_nbcells is initialized by parsing the #dma-cells
> attribute of of_dma->of_node. If ofdma->of_node equals dma_spec->np then
> dma_spec->args_count will also equal of_dma->of_dma_nbcells.
Thanks for the clarification. I should have looked more closely at
of_parse_phandle_with_args().
Yes I agree it will always be true if count is less than or equal to
MAX_PHANDLE_ARGS (which defaults to 8). It is very unlikely that someone
would use more than 8 and I guess it does warn on this condition.
if (out_args) {
int i;
if (WARN_ON(count > MAX_PHANDLE_ARGS))
count = MAX_PHANDLE_ARGS;
out_args->np = node;
out_args->args_count = count;
for (i = 0; i < count; i++)
out_args->args[i] = be32_to_cpup(list++);
}
Cheers
Jon
On Mon, Apr 22, 2013 at 10:33:32AM +0200, Lars-Peter Clausen wrote:
> There is no sensible reason why #dma-cells shouldn't be allowed to be 0. It is
> completely up to the DMA controller how many additional parameters, besides the
> phandle, it needs to identify a channel. E.g. for DMA controller with only one
> channel or for DMA controllers which don't have a restriction on which channel
> can be used for which peripheral it completely legitimate to not require any
> additional parameters.
>
> Also fixes the following warning:
> drivers/dma/of-dma.c: In function 'of_dma_controller_register':
> drivers/dma/of-dma.c:67:7: warning: 'nbcells' may be used uninitialized in this function
>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
Applied, both thanks
Sorry looks like I missed applying them earlier...
--
~Vinod
> ---
> drivers/dma/of-dma.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
> index 7aa0864..268cc8a 100644
> --- a/drivers/dma/of-dma.c
> +++ b/drivers/dma/of-dma.c
> @@ -64,7 +64,6 @@ int of_dma_controller_register(struct device_node *np,
> void *data)
> {
> struct of_dma *ofdma;
> - int nbcells;
> const __be32 *prop;
>
> if (!np || !of_dma_xlate) {
> @@ -77,18 +76,16 @@ int of_dma_controller_register(struct device_node *np,
> return -ENOMEM;
>
> prop = of_get_property(np, "#dma-cells", NULL);
> - if (prop)
> - nbcells = be32_to_cpup(prop);
> -
> - if (!prop || !nbcells) {
> - pr_err("%s: #dma-cells property is missing or invalid\n",
> + if (!prop) {
> + pr_err("%s: #dma-cells property is missing\n",
> __func__);
> kfree(ofdma);
> return -EINVAL;
> }
>
> +
> ofdma->of_node = np;
> - ofdma->of_dma_nbcells = nbcells;
> + ofdma->of_dma_nbcells = be32_to_cpup(prop);
> ofdma->of_dma_xlate = of_dma_xlate;
> ofdma->of_dma_data = data;
>
> --
> 1.8.0
>