| Message ID | 20180112144034.6655-2-zajec5@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mtd: simplify mtd_device_parse_register code | expand |
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);
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.
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);