[1/2] mtd: move code adding master MTD out of mtd_add_device_partitions

Message ID 20180112144034.6655-2-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 change is a small cleanup of mtd_device_parse_register. When using
MTD_PARTITIONED_MASTER it makes sure a master MTD is registered before
dealing with partitions. There are 2 advantages of this:
1) Not mixing code handling master MTD with code handling partitions
2) Possibility of reusing mtd_parse_part in the future to avoid some
   code duplication.

This commit doesn't change any behavior except from a slightly different
failure code path. The new code may need to call del_mtd_device when
something goes wrong.

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

Comments

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

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This change is a small cleanup of mtd_device_parse_register. When using
> MTD_PARTITIONED_MASTER it makes sure a master MTD is registered before
> dealing with partitions. There are 2 advantages of this:
> 1) Not mixing code handling master MTD with code handling partitions
> 2) Possibility of reusing mtd_parse_part in the future to avoid some
>    code duplication.
> 
> This commit doesn't change any behavior except from a slightly different
> failure code path. The new code may need to call del_mtd_device when
> something goes wrong.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/mtd/mtdcore.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index f80e911b8843..f6460862e2ad 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -641,20 +641,12 @@ static int mtd_add_device_partitions(struct mtd_info *mtd,
>  {
>  	const struct mtd_partition *real_parts = parts->parts;
>  	int nbparts = parts->nr_parts;
> -	int ret;
>  
> -	if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
> -		ret = add_mtd_device(mtd);
> -		if (ret)
> -			return ret;
> -	}
> +	if (nbparts == 0 && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
> +		return add_mtd_device(mtd);

Why not moving this part in mtd_device_parse_register() as well.

And I'd prefer something like:

	if (!nbparts && !device_is_registered(&mtd->dev))
		...

>  
> -	if (nbparts > 0) {
> -		ret = add_mtd_partitions(mtd, real_parts, nbparts);
> -		if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
> -			del_mtd_device(mtd);
> -		return ret;
> -	}
> +	if (nbparts > 0)
> +		return add_mtd_partitions(mtd, real_parts, nbparts);
>  
>  	return 0;
>  }
> @@ -714,6 +706,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  
>  	mtd_set_dev_defaults(mtd);
>  
> +	if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
> +		ret = add_mtd_device(mtd);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	memset(&parsed, 0, sizeof(parsed));
>  
>  	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
> @@ -753,6 +751,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  out:
>  	/* Cleanup any parsed partitions */
>  	mtd_part_parser_cleanup(&parsed);
> +	if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))

	if (ret && device_is_registered(&mtd->dev))

> +		del_mtd_device(mtd);

Blank line here, please.

>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(mtd_device_parse_register);
Rafał Miłecki Jan. 15, 2018, 10:58 a.m. | #2
On 12 January 2018 at 16:01, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index f80e911b8843..f6460862e2ad 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -641,20 +641,12 @@ static int mtd_add_device_partitions(struct mtd_info *mtd,
>>  {
>>       const struct mtd_partition *real_parts = parts->parts;
>>       int nbparts = parts->nr_parts;
>> -     int ret;
>>
>> -     if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
>> -             ret = add_mtd_device(mtd);
>> -             if (ret)
>> -                     return ret;
>> -     }
>> +     if (nbparts == 0 && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
>> +             return add_mtd_device(mtd);
>
> Why not moving this part in mtd_device_parse_register() as well.

I move that in 2/2. I prefer to handle these changes in 2 steps as it
should be quite easier to review it that way.


> And I'd prefer something like:
>
>         if (!nbparts && !device_is_registered(&mtd->dev))
>                 ...

Nice idea with that device_is_registered.

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index f80e911b8843..f6460862e2ad 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -641,20 +641,12 @@  static int mtd_add_device_partitions(struct mtd_info *mtd,
 {
 	const struct mtd_partition *real_parts = parts->parts;
 	int nbparts = parts->nr_parts;
-	int ret;
 
-	if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
-		ret = add_mtd_device(mtd);
-		if (ret)
-			return ret;
-	}
+	if (nbparts == 0 && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
+		return add_mtd_device(mtd);
 
-	if (nbparts > 0) {
-		ret = add_mtd_partitions(mtd, real_parts, nbparts);
-		if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
-			del_mtd_device(mtd);
-		return ret;
-	}
+	if (nbparts > 0)
+		return add_mtd_partitions(mtd, real_parts, nbparts);
 
 	return 0;
 }
@@ -714,6 +706,12 @@  int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 
 	mtd_set_dev_defaults(mtd);
 
+	if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
+		ret = add_mtd_device(mtd);
+		if (ret)
+			return ret;
+	}
+
 	memset(&parsed, 0, sizeof(parsed));
 
 	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
@@ -753,6 +751,8 @@  int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 out:
 	/* Cleanup any parsed partitions */
 	mtd_part_parser_cleanup(&parsed);
+	if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
+		del_mtd_device(mtd);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(mtd_device_parse_register);