[2/2] mtd: get rid of the mtd_add_device_partitions function

Message ID 20180112144034.6655-3-zajec5@gmail.com
State Changes Requested
Headers show
Series
  • mtd: simplify mtd_device_parse_register code
Related show

Commit Message

Rafał Miłecki Jan. 12, 2018, 2:40 p.m.
From: Rafał Miłecki <rafal@milecki.pl>

This simplifies code a bit by:
1) Avoiding an extra (tiny) function
2) Checking for amount of parsed (found) partitions just once
3) Avoiding clearing/filling struct mtd_partitions manually

With this commit a proper functions are called directly from the
mtd_device_parse_register. It doesn't need to use minor tricks like
memsetting struct to 0 to trigger an expected mtd_add_device_partitions
behavior.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/mtdcore.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

Comments

Boris Brezillon Jan. 12, 2018, 3:11 p.m. | #1
On Fri, 12 Jan 2018 15:40:34 +0100
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This simplifies code a bit by:
> 1) Avoiding an extra (tiny) function
> 2) Checking for amount of parsed (found) partitions just once
> 3) Avoiding clearing/filling struct mtd_partitions manually
> 
> With this commit a proper functions are called directly from the
> mtd_device_parse_register. It doesn't need to use minor tricks like
> memsetting struct to 0 to trigger an expected mtd_add_device_partitions
> behavior.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/mtd/mtdcore.c | 29 +++++++----------------------
>  1 file changed, 7 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index f6460862e2ad..0a414750bc8b 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -636,21 +636,6 @@ int del_mtd_device(struct mtd_info *mtd)
>  	return ret;
>  }
>  
> -static int mtd_add_device_partitions(struct mtd_info *mtd,
> -				     struct mtd_partitions *parts)
> -{
> -	const struct mtd_partition *real_parts = parts->parts;
> -	int nbparts = parts->nr_parts;
> -
> -	if (nbparts == 0 && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
> -		return add_mtd_device(mtd);
> -
> -	if (nbparts > 0)
> -		return add_mtd_partitions(mtd, real_parts, nbparts);
> -
> -	return 0;
> -}
> -
>  /*
>   * Set a few defaults based on the parent devices, if not provided by the
>   * driver
> @@ -717,19 +702,19 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
>  	if ((ret < 0 || parsed.nr_parts == 0) && parts && nr_parts) {
>  		/* Fall back to driver-provided partitions */
> -		parsed = (struct mtd_partitions){
> -			.parts		= parts,
> -			.nr_parts	= nr_parts,
> -		};
> +		ret = add_mtd_partitions(mtd, parts, nr_parts);
>  	} else if (ret < 0) {
>  		/* Didn't come up with parsed OR fallback partitions */
>  		pr_info("mtd: failed to find partitions; one or more parsers reports errors (%d)\n",
>  			ret);
>  		/* Don't abort on errors; we can still use unpartitioned MTD */
> -		memset(&parsed, 0, sizeof(parsed));
> +		if (!IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
> +			ret = add_mtd_device(mtd);
> +		else
> +			ret = 0;
> +	} else {
> +		ret = add_mtd_partitions(mtd, parsed.parts, parsed.nr_parts);
>  	}

How about:

	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
	if (!ret && parsed.nr_parts) {
		parts = parsed.parts;
		nr_parts = parsed.nr_parts;
	}

	if (nr_parts)
		ret = add_mtd_partitions(mtd, parts, nr_parts);
	else if (!device_is_registered(&mtd->dev))
		ret = add_mtd_device(mtd);
	else
		ret = 0;

	...
> -
> -	ret = mtd_add_device_partitions(mtd, &parsed);
>  	if (ret)
>  		goto out;
>
Rafał Miłecki Jan. 15, 2018, 12:56 p.m. | #2
On 12 January 2018 at 16:11, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
>> @@ -717,19 +702,19 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>>       ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
>>       if ((ret < 0 || parsed.nr_parts == 0) && parts && nr_parts) {
>>               /* Fall back to driver-provided partitions */
>> -             parsed = (struct mtd_partitions){
>> -                     .parts          = parts,
>> -                     .nr_parts       = nr_parts,
>> -             };
>> +             ret = add_mtd_partitions(mtd, parts, nr_parts);
>>       } else if (ret < 0) {
>>               /* Didn't come up with parsed OR fallback partitions */
>>               pr_info("mtd: failed to find partitions; one or more parsers reports errors (%d)\n",
>>                       ret);
>>               /* Don't abort on errors; we can still use unpartitioned MTD */
>> -             memset(&parsed, 0, sizeof(parsed));
>> +             if (!IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
>> +                     ret = add_mtd_device(mtd);
>> +             else
>> +                     ret = 0;
>> +     } else {
>> +             ret = add_mtd_partitions(mtd, parsed.parts, parsed.nr_parts);
>>       }
>
> How about:
>
>         ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
>         if (!ret && parsed.nr_parts) {
>                 parts = parsed.parts;
>                 nr_parts = parsed.nr_parts;
>         }
>
>         if (nr_parts)
>                 ret = add_mtd_partitions(mtd, parts, nr_parts);
>         else if (!device_is_registered(&mtd->dev))
>                 ret = add_mtd_device(mtd);
>         else
>                 ret = 0;

I spent a moment writing possible cases on a paper and your solution
should keep the same logic. Looks good!

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index f6460862e2ad..0a414750bc8b 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -636,21 +636,6 @@  int del_mtd_device(struct mtd_info *mtd)
 	return ret;
 }
 
-static int mtd_add_device_partitions(struct mtd_info *mtd,
-				     struct mtd_partitions *parts)
-{
-	const struct mtd_partition *real_parts = parts->parts;
-	int nbparts = parts->nr_parts;
-
-	if (nbparts == 0 && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
-		return add_mtd_device(mtd);
-
-	if (nbparts > 0)
-		return add_mtd_partitions(mtd, real_parts, nbparts);
-
-	return 0;
-}
-
 /*
  * Set a few defaults based on the parent devices, if not provided by the
  * driver
@@ -717,19 +702,19 @@  int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
 	if ((ret < 0 || parsed.nr_parts == 0) && parts && nr_parts) {
 		/* Fall back to driver-provided partitions */
-		parsed = (struct mtd_partitions){
-			.parts		= parts,
-			.nr_parts	= nr_parts,
-		};
+		ret = add_mtd_partitions(mtd, parts, nr_parts);
 	} else if (ret < 0) {
 		/* Didn't come up with parsed OR fallback partitions */
 		pr_info("mtd: failed to find partitions; one or more parsers reports errors (%d)\n",
 			ret);
 		/* Don't abort on errors; we can still use unpartitioned MTD */
-		memset(&parsed, 0, sizeof(parsed));
+		if (!IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
+			ret = add_mtd_device(mtd);
+		else
+			ret = 0;
+	} else {
+		ret = add_mtd_partitions(mtd, parsed.parts, parsed.nr_parts);
 	}
-
-	ret = mtd_add_device_partitions(mtd, &parsed);
 	if (ret)
 		goto out;