Patchwork [6/6] mtd: ofpart: add compatible check for child nodes

login
register
mail settings
Submitter Wu, Josh
Date June 10, 2013, 10:26 a.m.
Message ID <1370860014-1770-7-git-send-email-josh.wu@atmel.com>
Download mbox | patch
Permalink /patch/250213/
State New
Headers show

Comments

Wu, Josh - June 10, 2013, 10:26 a.m.
If the child node has compatible property then it is a driver not partition.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
 drivers/mtd/ofpart.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
Sergei Shtylyov - June 10, 2013, 12:35 p.m.
hello.

On 10-06-2013 14:26, Josh Wu wrote:

> If the child node has compatible property then it is a driver not partition.

> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
>   drivers/mtd/ofpart.c |   14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)


> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
> index 553d6d6..61ce1f8 100644
> --- a/drivers/mtd/ofpart.c
> +++ b/drivers/mtd/ofpart.c
[...]
> @@ -40,8 +44,13 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>   	/* First count the subnodes */
>   	pp = NULL;
>   	nr_parts = 0;
> -	while ((pp = of_get_next_child(node, pp)))
> +	while ((pp = of_get_next_child(node, pp))) {

    Assignment in *while*? Is scripts/checkpatch.pl happy about that?

> +		int len;

    Empty line wouldn't hurt here, after declaration.

> +		if (node_has_compatible(pp, &len))
> +			continue;
> +
>   		nr_parts++;
> +	}
>
>   	if (nr_parts == 0)
>   		return 0;

WBR, Sergei
Brian Norris - June 11, 2013, 11:34 p.m.
Hi Josh,

On Mon, Jun 10, 2013 at 3:26 AM, Josh Wu <josh.wu@atmel.com> wrote:
> If the child node has compatible property then it is a driver not partition.
>
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
>  drivers/mtd/ofpart.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
> index 553d6d6..61ce1f8 100644
> --- a/drivers/mtd/ofpart.c
> +++ b/drivers/mtd/ofpart.c
> @@ -20,6 +20,10 @@
>  #include <linux/slab.h>
>  #include <linux/mtd/partitions.h>
>
> +static bool node_has_compatible(struct device_node *pp, int *len)
> +{
> +       return of_get_property(pp, "compatible", len);
> +}

The way the interface is named, it seems like a user only would ever
need the bool return value, not the implicit 'len' return value. So I
would write it like this:

static bool node_has_compatible(struct device_node *pp)
{
       return of_get_property(pp, "compatible", NULL);
}

>  static int parse_ofpart_partitions(struct mtd_info *master,
>                                    struct mtd_partition **pparts,
>                                    struct mtd_part_parser_data *data)
> @@ -40,8 +44,13 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>         /* First count the subnodes */
>         pp = NULL;
>         nr_parts = 0;
> -       while ((pp = of_get_next_child(node, pp)))
> +       while ((pp = of_get_next_child(node, pp))) {
> +               int len;
> +               if (node_has_compatible(pp, &len))

Then this would just be:

if (node_has_compatible(pp))
...
etc. (rest snipped)

On a more important question, why does a NAND device node have
sub-nodes that are not partitions? And if such devices exist, wouldn't
it be more foolproof to establish a "compatible" property for ofpart
partitions? Of course, we'd still have to retain
backward-compatibility and allow partitions without a "compatible"
prop. But this question probably should be addressed in the commit log
and documented in Documentation/devicetree/bindings/mtd/partition.txt
; either specifying that all sub-nodes without a compatible property
must be partitions, or else some other explanation.

Brian
Wu, Josh - June 13, 2013, 3:38 a.m.
Hi, Brian

On 6/12/2013 7:34 AM, Brian Norris wrote:
> Hi Josh,
>
> On Mon, Jun 10, 2013 at 3:26 AM, Josh Wu <josh.wu@atmel.com> wrote:
>> If the child node has compatible property then it is a driver not partition.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>   drivers/mtd/ofpart.c |   14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
>> index 553d6d6..61ce1f8 100644
>> --- a/drivers/mtd/ofpart.c
>> +++ b/drivers/mtd/ofpart.c
>> @@ -20,6 +20,10 @@
>>   #include <linux/slab.h>
>>   #include <linux/mtd/partitions.h>
>>
>> +static bool node_has_compatible(struct device_node *pp, int *len)
>> +{
>> +       return of_get_property(pp, "compatible", len);
>> +}
> The way the interface is named, it seems like a user only would ever
> need the bool return value, not the implicit 'len' return value. So I
> would write it like this:
>
> static bool node_has_compatible(struct device_node *pp)
> {
>         return of_get_property(pp, "compatible", NULL);
> }

yes, this is exactly is what I want. I didn't check the *len can be NULL 
or not. thanks.

>
>>   static int parse_ofpart_partitions(struct mtd_info *master,
>>                                     struct mtd_partition **pparts,
>>                                     struct mtd_part_parser_data *data)
>> @@ -40,8 +44,13 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>>          /* First count the subnodes */
>>          pp = NULL;
>>          nr_parts = 0;
>> -       while ((pp = of_get_next_child(node, pp)))
>> +       while ((pp = of_get_next_child(node, pp))) {
>> +               int len;
>> +               if (node_has_compatible(pp, &len))
> Then this would just be:
>
> if (node_has_compatible(pp))
> ...
> etc. (rest snipped)
>
> On a more important question, why does a NAND device node have
> sub-nodes that are not partitions? And if such devices exist, wouldn't
> it be more foolproof to establish a "compatible" property for ofpart
> partitions?

In my case, we have a Atmel NAND flash driver/device which is work for 
all series of SAM9/SAMA5D3 chip.
And since in the new chip SAMA5D3 which has a NAND Flash Controller support.

So to enable NAND flash controller, I should either write a compatible 
driver like atmel,sama5-nand, which is
just old device property plus NFC regs and NFC properties, that became 
too big. Or add a NFC sub-node for the NFC support.

IMHO, to make the NFC device as a sub-node is the simplest way to make 
nand driver is back-compatible and easy to enable/disable NFC feature.
Since all NFC related property are grouped it is more readable.

But to implement this, I need to add this patch, otherwise this NFC sub 
node will be recognized as a partition.

> Of course, we'd still have to retain
> backward-compatibility and allow partitions without a "compatible"
> prop. But this question probably should be addressed in the commit log
> and documented in Documentation/devicetree/bindings/mtd/partition.txt
> ; either specifying that all sub-nodes without a compatible property
> must be partitions, or else some other explanation.

yes, sure. I will add an explanation in commit log and 
Documentation/devicetree/bindings/mtd/partition.txt in next version.

>
> Brian

Best Regards,
Josh Wu
Wu, Josh - June 13, 2013, 3:42 a.m.
Hi, Sergei

On 6/10/2013 8:35 PM, Sergei Shtylyov wrote:
> hello.
>
> On 10-06-2013 14:26, Josh Wu wrote:
>
>> If the child node has compatible property then it is a driver not 
>> partition.
>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>   drivers/mtd/ofpart.c |   14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>
>
>> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
>> index 553d6d6..61ce1f8 100644
>> --- a/drivers/mtd/ofpart.c
>> +++ b/drivers/mtd/ofpart.c
> [...]
>> @@ -40,8 +44,13 @@ static int parse_ofpart_partitions(struct mtd_info 
>> *master,
>>       /* First count the subnodes */
>>       pp = NULL;
>>       nr_parts = 0;
>> -    while ((pp = of_get_next_child(node, pp)))
>> +    while ((pp = of_get_next_child(node, pp))) {
>
>    Assignment in *while*? Is scripts/checkpatch.pl happy about that?

yes, checkpatch.pl has no complain  ;-)

>
>> +        int len;
>
>    Empty line wouldn't hurt here, after declaration.

Ok, as Brian said, I will no need to use a 'int len' here. So I will 
remove those declaration.

>
>> +        if (node_has_compatible(pp, &len))
>> +            continue;
>> +
>>           nr_parts++;
>> +    }
>>
>>       if (nr_parts == 0)
>>           return 0;
>
> WBR, Sergei
>

Thanks.
Best Regards,
Josh Wu
Sergei Shtylyov - June 13, 2013, 1:13 p.m.
Hello.

On 13-06-2013 7:42, Josh Wu wrote:

>>> If the child node has compatible property then it is a driver not
>>> partition.

>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>> ---
>>>   drivers/mtd/ofpart.c |   14 +++++++++++++-
>>>   1 file changed, 13 insertions(+), 1 deletion(-)


>>> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
>>> index 553d6d6..61ce1f8 100644
>>> --- a/drivers/mtd/ofpart.c
>>> +++ b/drivers/mtd/ofpart.c
>> [...]
>>> @@ -40,8 +44,13 @@ static int parse_ofpart_partitions(struct mtd_info
>>> *master,
>>>       /* First count the subnodes */
>>>       pp = NULL;
>>>       nr_parts = 0;
>>> -    while ((pp = of_get_next_child(node, pp)))
>>> +    while ((pp = of_get_next_child(node, pp))) {

>>    Assignment in *while*? Is scripts/checkpatch.pl happy about that?

> yes, checkpatch.pl has no complain  ;-)

    It seems checkpatch.pl only complains about *if*. Not very consistent...

WBR, Sergei

Patch

diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index 553d6d6..61ce1f8 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -20,6 +20,10 @@ 
 #include <linux/slab.h>
 #include <linux/mtd/partitions.h>
 
+static bool node_has_compatible(struct device_node *pp, int *len)
+{
+	return of_get_property(pp, "compatible", len);
+}
 static int parse_ofpart_partitions(struct mtd_info *master,
 				   struct mtd_partition **pparts,
 				   struct mtd_part_parser_data *data)
@@ -40,8 +44,13 @@  static int parse_ofpart_partitions(struct mtd_info *master,
 	/* First count the subnodes */
 	pp = NULL;
 	nr_parts = 0;
-	while ((pp = of_get_next_child(node, pp)))
+	while ((pp = of_get_next_child(node, pp))) {
+		int len;
+		if (node_has_compatible(pp, &len))
+			continue;
+
 		nr_parts++;
+	}
 
 	if (nr_parts == 0)
 		return 0;
@@ -57,6 +66,9 @@  static int parse_ofpart_partitions(struct mtd_info *master,
 		int len;
 		int a_cells, s_cells;
 
+		if (node_has_compatible(pp, &len))
+			continue;
+
 		reg = of_get_property(pp, "reg", &len);
 		if (!reg) {
 			nr_parts--;