| Message ID | 1226533133-7405-1-git-send-email-vapier@gentoo.org |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 12 Nov 2008 18:38:53 -0500, Mike Frysinger <vapier@gentoo.org> wrote: > Rather than having every map/mtd driver doing the same sequence of > registering partitions and/or devices, implement common parse_mtd(). > > Signed-off-by: Mike Frysinger <vapier@gentoo.org> > --- > drivers/mtd/mtdcore.c | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/mtd/mtd.h | 3 +++ > 2 files changed, 40 insertions(+), 0 deletions(-) I like this idea. > +int parse_mtd(struct mtd_info *mtd, const char **probe_types, > + struct mtd_partition *parts, int nr_parts) > +{ > +#ifdef CONFIG_MTD_PARTITIONS > + const char *default_part_probe_types[] = { > + "cmdlinepart", > + "RedBoot", > + NULL > + }; But I'm not sure this default_part_probe_types is appropriate. How about enclose each parser with #ifdefs? const char *default_part_probe_types[] = { #ifdef CONFIG_MTD_CMDLINE_PARTS "cmdlinepart", #endif #ifdef CONFIG_MTD_REDBOOT_PARTS "RedBoot", #endif NULL }; This get rid of "RedBoot partition parsing not available" noise in boot message when you use default_part_probe_types without MTD_REDBOOT_PARTS. --- Atsushi Nemoto
On Thu, 13 Nov 2008 22:43:50 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote: > On Wed, 12 Nov 2008 18:38:53 -0500, Mike Frysinger <vapier@gentoo.org> wrote: > > Rather than having every map/mtd driver doing the same sequence of > > registering partitions and/or devices, implement common parse_mtd(). > > > > Signed-off-by: Mike Frysinger <vapier@gentoo.org> > > --- > > drivers/mtd/mtdcore.c | 37 +++++++++++++++++++++++++++++++++++++ > > include/linux/mtd/mtd.h | 3 +++ > > 2 files changed, 40 insertions(+), 0 deletions(-) > > I like this idea. Some drivers call both add_mtd_device() and add_mtd_partitions(). The mtd_device is used to access whole flash area (like /dev/hda). How do you think of these usage patterns? maps/edb7312.c maps/mbx860.c maps/plat-ram.c nand/cafe_nand.c nand/diskonchip.c nand/ndfc.c Automatic fallback to mtd_device in parse_mtd() is convenient but somewhat unflexible... --- Atsushi Nemoto
On Thu, Nov 13, 2008 at 08:43, Atsushi Nemoto wrote: > On Wed, 12 Nov 2008 18:38:53 -0500, Mike Frysinger wrote: >> +int parse_mtd(struct mtd_info *mtd, const char **probe_types, >> + struct mtd_partition *parts, int nr_parts) >> +{ >> +#ifdef CONFIG_MTD_PARTITIONS >> + const char *default_part_probe_types[] = { >> + "cmdlinepart", >> + "RedBoot", >> + NULL >> + }; > > But I'm not sure this default_part_probe_types is appropriate. > > How about enclose each parser with #ifdefs? > > const char *default_part_probe_types[] = { > #ifdef CONFIG_MTD_CMDLINE_PARTS > "cmdlinepart", > #endif > #ifdef CONFIG_MTD_REDBOOT_PARTS > "RedBoot", > #endif > NULL > }; > > This get rid of "RedBoot partition parsing not available" noise in > boot message when you use default_part_probe_types without > MTD_REDBOOT_PARTS. yeah, that parsing thing is annoying, but i didnt think enough so to add ifdef's. i'm fine with it either way. -mike
On Thu, Nov 13, 2008 at 09:28, Atsushi Nemoto wrote: > On Thu, 13 Nov 2008 22:43:50 +0900 (JST), Atsushi Nemoto wrote: >> On Wed, 12 Nov 2008 18:38:53 -0500, Mike Frysinger wrote: >> > Rather than having every map/mtd driver doing the same sequence of >> > registering partitions and/or devices, implement common parse_mtd(). >> > >> > Signed-off-by: Mike Frysinger <vapier@gentoo.org> >> > --- >> > drivers/mtd/mtdcore.c | 37 +++++++++++++++++++++++++++++++++++++ >> > include/linux/mtd/mtd.h | 3 +++ >> > 2 files changed, 40 insertions(+), 0 deletions(-) >> >> I like this idea. > > Some drivers call both add_mtd_device() and add_mtd_partitions(). The > mtd_device is used to access whole flash area (like /dev/hda). How do > you think of these usage patterns? > > maps/edb7312.c > maps/mbx860.c > maps/plat-ram.c > nand/cafe_nand.c > nand/diskonchip.c > nand/ndfc.c > > Automatic fallback to mtd_device in parse_mtd() is convenient but > somewhat unflexible... we could just have it do it all the time. i dont see a problem with exposing the entire block device the whole time ? i know for the driver or two of mine, i'm fine with it. ... ret = parse_mtd_partitions(mtd, probe_types, &parts, 0); if (ret > 0) { ret = add_mtd_partitions(mtd, parts, ret); kfree(parts); } else if (nr_parts) ret = add_mtd_partitions(mtd, parts, nr_parts); if (ret) return ret; #endif return add_mtd_device(mtd); -mike
On Thu, 13 Nov 2008 09:51:01 -0500, "Mike Frysinger" <vapier.adi@gmail.com> wrote: > > Automatic fallback to mtd_device in parse_mtd() is convenient but > > somewhat unflexible... > > we could just have it do it all the time. i dont see a problem with > exposing the entire block device the whole time ? i know for the > driver or two of mine, i'm fine with it. > > ... > ret = parse_mtd_partitions(mtd, probe_types, &parts, 0); > if (ret > 0) { > ret = add_mtd_partitions(mtd, parts, ret); > kfree(parts); > } else if (nr_parts) > ret = add_mtd_partitions(mtd, parts, nr_parts); > if (ret) > return ret; > #endif > > return add_mtd_device(mtd); I'm OK with this. But not sure all current mtd partition users. Are you going to convert all parse_mtd_partitions() call? Maybe some people do not want to change /dev/mtd numbers... --- Atsushi Nemoto
On Thu, Nov 13, 2008 at 10:51, Atsushi Nemoto wrote: > On Thu, 13 Nov 2008 09:51:01 -0500, "Mike Frysinger" wrote: >> > Automatic fallback to mtd_device in parse_mtd() is convenient but >> > somewhat unflexible... >> >> we could just have it do it all the time. i dont see a problem with >> exposing the entire block device the whole time ? i know for the >> driver or two of mine, i'm fine with it. >> >> ... >> ret = parse_mtd_partitions(mtd, probe_types, &parts, 0); >> if (ret > 0) { >> ret = add_mtd_partitions(mtd, parts, ret); >> kfree(parts); >> } else if (nr_parts) >> ret = add_mtd_partitions(mtd, parts, nr_parts); >> if (ret) >> return ret; >> #endif >> >> return add_mtd_device(mtd); > > I'm OK with this. But not sure all current mtd partition users. > > Are you going to convert all parse_mtd_partitions() call? Maybe some > people do not want to change /dev/mtd numbers... if it's a real concern, we can make it optional. i'll post patches to convert physmap and my drivers as those are the only ones i can actually test. -mike
On Wed, 12 November 2008 18:38:53 -0500, Mike Frysinger wrote: > > Rather than having every map/mtd driver doing the same sequence of > registering partitions and/or devices, implement common parse_mtd(). I just added partitioning support to block2mtd. Looks like I might rethink the patch. :) > @@ -292,6 +292,43 @@ void put_mtd_device(struct mtd_info *mtd) > module_put(mtd->owner); > } > > +#include <linux/mtd/partitions.h> #include in the middle of the code? Jörn
On Sunday 16 November 2008 12:34:54 Jörn Engel wrote: > On Wed, 12 November 2008 18:38:53 -0500, Mike Frysinger wrote: > > Rather than having every map/mtd driver doing the same sequence of > > registering partitions and/or devices, implement common parse_mtd(). > > I just added partitioning support to block2mtd. Looks like I might > rethink the patch. :) > > > @@ -292,6 +292,43 @@ void put_mtd_device(struct mtd_info *mtd) > > module_put(mtd->owner); > > } > > > > +#include <linux/mtd/partitions.h> > > #include in the middle of the code? maybe i'll stick a comment above it ... the reason was to be proactive in preventing mtd partitioning code bleeding into the core. maybe i'm just paranoid. -mike
On Sun, 16 November 2008 12:55:17 -0500, Mike Frysinger wrote: > > maybe i'll stick a comment above it ... the reason was to be proactive in > preventing mtd partitioning code bleeding into the core. maybe i'm just > paranoid. Sounds a bit silly to me. *shrugs* Jörn
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index a9d2469..654ec1b 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -292,6 +292,43 @@ void put_mtd_device(struct mtd_info *mtd) module_put(mtd->owner); } +#include <linux/mtd/partitions.h> + +/** + * parse_mtd - add partitions / devices + * + * If partitioning support is enabled, attempt to call parse_mtd_partitions() + * and add_mtd_partitions() with all available parsers. Otherwise just add + * the MTD device. + */ + +int parse_mtd(struct mtd_info *mtd, const char **probe_types, + struct mtd_partition *parts, int nr_parts) +{ +#ifdef CONFIG_MTD_PARTITIONS + const char *default_part_probe_types[] = { + "cmdlinepart", + "RedBoot", + NULL + }; + int ret; + + if (!probe_types) + probe_types = default_part_probe_types; + + ret = parse_mtd_partitions(mtd, probe_types, &parts, 0); + if (ret > 0) { + ret = add_mtd_partitions(mtd, parts, ret); + kfree(parts); + return ret; + } else if (nr_parts) + return add_mtd_partitions(mtd, parts, nr_parts); +#endif + + return add_mtd_device(mtd); +} +EXPORT_SYMBOL(parse_mtd); + /* default_mtd_writev - default mtd writev method for MTD devices that * don't implement their own */ diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index eae26bb..dec3456 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -232,6 +232,9 @@ extern struct mtd_info *get_mtd_device_nm(const char *name); extern void put_mtd_device(struct mtd_info *mtd); +struct mtd_partition; +int parse_mtd(struct mtd_info *mtd, const char **probe_types, + struct mtd_partition *parts, int nr_parts); struct mtd_notifier { void (*add)(struct mtd_info *mtd);
Rather than having every map/mtd driver doing the same sequence of registering partitions and/or devices, implement common parse_mtd(). Signed-off-by: Mike Frysinger <vapier@gentoo.org> --- drivers/mtd/mtdcore.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/mtd/mtd.h | 3 +++ 2 files changed, 40 insertions(+), 0 deletions(-)