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