Re: [PATCH 2/2] mtd: partitions: use DT info for parsing partitions with specified type

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Thu, 24 May 2018 07:50:10 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> On 23.05.2018 20:24, Boris Brezillon wrote:
> > On Wed, 23 May 2018 19:14:48 +0200
> > Rafał Miłecki <zajec5@gmail.com> wrote:
> >   
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> This supports nested partitions in a DT. If selected partition has a
> >> "compatible" property specified it will be parsed looking for
> >> subpartitions.
> >>
> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> >> ---
> >>   drivers/mtd/mtdpart.c | 33 +++++++++++++--------------------
> >>   1 file changed, 13 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> >> index f8d3a015cdad..52e2cb35fc79 100644
> >> --- a/drivers/mtd/mtdpart.c
> >> +++ b/drivers/mtd/mtdpart.c
> >> @@ -322,22 +322,6 @@ static inline void free_partition(struct mtd_part *p)
> >>   	kfree(p);
> >>   }
> >>   
> >> -/**
> >> - * mtd_parse_part - parse MTD partition looking for subpartitions
> >> - *
> >> - * @slave: part that is supposed to be a container and should be parsed
> >> - * @types: NULL-terminated array with names of partition parsers to try
> >> - *
> >> - * Some partitions are kind of containers with extra subpartitions (volumes).
> >> - * There can be various formats of such containers. This function tries to use
> >> - * specified parsers to analyze given partition and registers found
> >> - * subpartitions on success.
> >> - */
> >> -static int mtd_parse_part(struct mtd_part *slave, const char *const *types)
> >> -{
> >> -	return parse_mtd_partitions(&slave->mtd, types, NULL);
> >> -}
> >> -
> >>   static struct mtd_part *allocate_partition(struct mtd_info *parent,
> >>   			const struct mtd_partition *part, int partno,
> >>   			uint64_t cur_offset)
> >> @@ -735,8 +719,8 @@ int add_mtd_partitions(struct mtd_info *master,
> >>   
> >>   		add_mtd_device(&slave->mtd);
> >>   		mtd_add_partition_attrs(slave);
> >> -		if (parts[i].types)
> >> -			mtd_parse_part(slave, parts[i].types);
> >> +		/* Look for subpartitions */
> >> +		parse_mtd_partitions(&slave->mtd, parts[i].types, NULL);
> >>   
> >>   		cur_offset = slave->offset + slave->mtd.size;
> >>   	}
> >> @@ -812,6 +796,12 @@ static const char * const default_mtd_part_types[] = {
> >>   	NULL
> >>   };
> >>   
> >> +/* Check DT only when looking for subpartitions. */
> >> +static const char * const default_subpartition_types[] = {
> >> +	"ofpart",
> >> +	NULL
> >> +};
> >> +
> >>   static int mtd_part_do_parse(struct mtd_part_parser *parser,
> >>   			     struct mtd_info *master,
> >>   			     struct mtd_partitions *pparts,
> >> @@ -882,7 +872,9 @@ static int mtd_part_of_parse(struct mtd_info *master,
> >>   	const char *fixed = "fixed-partitions";
> >>   	int ret, err = 0;
> >>   
> >> -	np = of_get_child_by_name(mtd_get_of_node(master), "partitions");
> >> +	np = mtd_get_of_node(master);
> >> +	if (!mtd_is_partition(master))
> >> +		np = of_get_child_by_name(np, "partitions");
> >>   	of_property_for_each_string(np, "compatible", prop, compat) {
> >>   		parser = mtd_part_get_compatible_parser(compat);
> >>   		if (!parser)
> >> @@ -945,7 +937,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
> >>   	int ret, err = 0;
> >>   
> >>   	if (!types)
> >> -		types = default_mtd_part_types;
> >> +		types = mtd_is_partition(master) ? default_subpartition_types :
> >> +			default_mtd_part_types;  
> > 
> > Hm, that means the subparts inherit the parser types from their parent
> > if types != NULL? Is that really what we want? And if that's what we
> > want, why don't we do the same for types == NULL?  
> 
> No, unless I'm missing something. In add_mtd_partitions() there is now
> a following call:
> parse_mtd_partitions(&slave->mtd, parts[i].types, NULL);
> 
> In most cases parts[i].types is NULL, unless you're using some
> exceptional driver (like bcm47xxpart) which sets "types".
> 
> So for each partition parse_mtd_partitions() will be called with "types"
> argument almost always set to NULL (no inheriting). That will result in
> picking default_subpartition_types.

And that's the "almost always" I'm not happy with. Either you don't
want to inherit the types from the parent and you do that for both !=
NULL and == NULL, or you inherit it for both.

Also, I'd really like to improve the 'part parser types' selection logic
by letting the boards select the part parsers instead of having the MTD
drivers do it.

Maybe we could have an approach similar to the gpio-lookup-table where
board files could assign a part-parser list to an MTD name, and then let
the core find whether there's a part parser list attached to the MTD
dev at registration time. And for DT-based setup, this information
would just be extracted from the compatible of the 'partitions' node (or
the node directly if the MTD device is a partition).

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/