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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Michal,

On Thu, 30 Jul 2015 12:10:42 +0200
Michal Suchanek <hramrach@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@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. 

> +	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.

> +		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.

>  		(*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