Re: [PATCH v2 5/5] mtd: ofpart: move ofpart partitions to a dedicated dt node
From: Boris Brezillon <hidden>
Date: 2015-07-31 16:06:23
Also in:
lkml
Hi Michal, On Thu, 30 Jul 2015 12:10:42 +0200 Michal Suchanek [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
+ 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.
quoted hunk ↗ jump to hunk
+ 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.
+ }
}
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.
quoted hunk ↗ jump to hunk
(*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? Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html