Patchwork mtd: unify mtd partition/device registration

login
register
mail settings
Submitter Mike Frysinger
Date Nov. 12, 2008, 11:38 p.m.
Message ID <1226533133-7405-1-git-send-email-vapier@gentoo.org>
Download mbox | patch
Permalink /patch/8496/
State New, archived
Headers show

Comments

Mike Frysinger - Nov. 12, 2008, 11:38 p.m.
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(-)
Atsushi Nemoto - Nov. 13, 2008, 1:43 p.m.
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
Atsushi Nemoto - Nov. 13, 2008, 2:28 p.m.
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
Mike Frysinger - Nov. 13, 2008, 2:47 p.m.
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
Mike Frysinger - Nov. 13, 2008, 2:51 p.m.
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
Atsushi Nemoto - Nov. 13, 2008, 3:51 p.m.
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
Mike Frysinger - Nov. 13, 2008, 3:55 p.m.
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
Jörn Engel - Nov. 16, 2008, 5:34 p.m.
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
Mike Frysinger - Nov. 16, 2008, 5:55 p.m.
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
Jörn Engel - Nov. 16, 2008, 6:02 p.m.
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

Patch

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);