[PATCH v2 5/5] mtd: ofpart: move ofpart partitions to a dedicated dt node

[Michal Suchanek]

On 31 July 2015 at 18:06, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> Hi Michal,
>
> On Thu, 30 Jul 2015 12:10:42 +0200
> Michal Suchanek <hramrach at gmail.com> wrote:
>
>> Parsing direct subnodes of a mtd device as partitions is unreliable
>> since the mtd device is also part of its bus subsystem and can contain
>> bus data in subnodes.
>>
>> Move ofpart data to a subnode of its own so it is clear which data is
>> part of the partition layout.
>>
>> Signed-off-by: Michal Suchanek <hramrach at gmail.com>
>> ---
>>  drivers/mtd/ofpart.c | 56 +++++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 40 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
>> index aa26c32..2c28aaa 100644
>> --- a/drivers/mtd/ofpart.c
>> +++ b/drivers/mtd/ofpart.c
>> @@ -29,23 +29,33 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>>                                  struct mtd_partition **pparts,
>>                                  struct mtd_part_parser_data *data)
>>  {
>> -     struct device_node *node;
>> +     struct device_node *mtd_node;
>> +     struct device_node *ofpart_node;
>>       const char *partname;
>>       struct device_node *pp;
>>       int nr_parts, i;
>> +     bool dedicated = true;
>>
>>
>>       if (!data)
>>               return 0;
>>
>> -     node = data->of_node;
>> -     if (!node)
>> +     mtd_node = data->of_node;
>> +     if (!mtd_node)
>>               return 0;
>>
>> +     ofpart_node = of_get_child_by_name(mtd_node, "ofpart");
>
> Hm, you should use a more generic name, ofpart of the linux MTD
> DT partition parser, but another operating system might decide to name
> it otherwise. I think "partitions" is more appropriate.

Whatever. Presumably some dt maintainer will look at this and figure
out a name that perfectly fits current dt policy.

>
>> +     if (!ofpart_node) {
>> +             pr_warn("%s: 'ofpart' subnode not found on %s. Trying to parse direct subnodes as partitions.\n",
>> +                     master->name, mtd_node->full_name);
>
> Do we really want to complain here. I mean, a lot of users do not need
> to define their partition in a different node.

Yes, we do.

The binding without subnode is considered defective and obsolete.

>
>> +             ofpart_node = mtd_node;
>> +             dedicated = false;
>> +     }
>> +
>>       /* First count the subnodes */
>>       nr_parts = 0;
>> -     for_each_child_of_node(node,  pp) {
>> -             if (node_has_compatible(pp))
>> +     for_each_child_of_node(ofpart_node,  pp) {
>> +             if (!dedicated && node_has_compatible(pp))
>>                       continue;
>>
>>               nr_parts++;
>> @@ -59,22 +69,36 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>>               return -ENOMEM;
>>
>>       i = 0;
>> -     for_each_child_of_node(node,  pp) {
>> +     for_each_child_of_node(ofpart_node,  pp) {
>>               const __be32 *reg;
>>               int len;
>>               int a_cells, s_cells;
>>
>> -             if (node_has_compatible(pp))
>> -                     continue;
>> +             if (!dedicated && node_has_compatible(pp))
>> +                             continue;
>
> Check your indentation (checkpatch should complain here).
>
>>
>>               reg = of_get_property(pp, "reg", &len);
>>               if (!reg) {
>> +                     if (dedicated) {
>> +                             pr_debug("%s: ofpart partition %s (%s) missing reg property.\n",
>> +                                      master->name, pp->full_name,
>> +                                      mtd_node->full_name);
>> +                             goto ofpart_fail;
>> +                     } else {
>>                       nr_parts--;
>>                       continue;
>
> Ditto.

Well, it does not complain but the indent is definitely off here.

>
>> +                     }
>>               }
>>
>>               a_cells = of_n_addr_cells(pp);
>>               s_cells = of_n_size_cells(pp);
>> +             if (len / 4 != a_cells + s_cells) {
>> +                     pr_debug("%s: ofpart partition %s (%s) error parsing reg property.\n",
>> +                              master->name, pp->full_name,
>> +                              mtd_node->full_name);
>> +                     goto ofpart_fail;
>> +             }
>> +
>
> The above changes have nothing to do with the description you gave in
> your commit message.
>
>>               (*pparts)[i].offset = of_read_number(reg, a_cells);
>>               (*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
>>
>> @@ -92,15 +116,15 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>>               i++;
>>       }
>>
>> -     if (!i) {
>> -             of_node_put(pp);
>> -             pr_err("No valid partition found on %s\n", node->full_name);
>> -             kfree(*pparts);
>> -             *pparts = NULL;
>> -             return -EINVAL;
>> -     }
>> -
>
> Are you sure you can safely remove this check?

Yes. It was incomplete check to reject some partitioning schemes
considered invalid.

Now there is stricter checking above so this can be removed.

Basically the whole point of this patch is to remove this bogus check.

Thanks

Michal