| Message ID | 1307026293-8535-7-git-send-email-dbaryshkov@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2011-06-02 at 18:51 +0400, Dmitry Eremin-Solenikov wrote: > Prepare to convert of_mtd_parse_partitions() to usual partitions parser: > 1) Register ofpart parser > 2) Internally don't use passed device for error printing > 3) Add device_node to mtd_info struct > 4) Move of_mtd_parse_partitions from __devinit to common text section > 5) add ofpart to the default list of partition parsers > > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> Good idea, but > #include <linux/mtd/mtd.h> > #include <linux/mtd/partitions.h> > @@ -446,6 +447,10 @@ int mtd_device_register(struct mtd_info *master, > const struct mtd_partition *parts, > int nr_parts) > { > +#ifdef CONFIG_OF > + if (master->node) > + of_node_get(master->node); > +#endif Could all the OF-specific things be done in the ofpart.c ? > --- a/include/linux/mtd/mtd.h > +++ b/include/linux/mtd/mtd.h > @@ -171,6 +171,9 @@ struct mtd_info { > // Kernel-only stuff starts here. > const char *name; > int index; > +#ifdef CONFIG_OF > + struct device_node *node; > +#endif And designe-wise this does not look like a good idea to have such fields in mtd_info ... Who initializes this "node" field?
On Thu, 2011-06-02 at 18:51 +0400, Dmitry Eremin-Solenikov wrote: > Prepare to convert of_mtd_parse_partitions() to usual partitions parser: > 1) Register ofpart parser > 2) Internally don't use passed device for error printing > 3) Add device_node to mtd_info struct > 4) Move of_mtd_parse_partitions from __devinit to common text section > 5) add ofpart to the default list of partition parsers > > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> Do not apply this and further changes so far - I had some questions. Anyway, your series consists of as set of logical steps, which is really nice, so let's finish with the first cmdline consolidation step, and then move on.
On 06.06.2011 11:57, Artem Bityutskiy wrote: > On Thu, 2011-06-02 at 18:51 +0400, Dmitry Eremin-Solenikov wrote: >> Prepare to convert of_mtd_parse_partitions() to usual partitions parser: >> 1) Register ofpart parser >> 2) Internally don't use passed device for error printing >> 3) Add device_node to mtd_info struct >> 4) Move of_mtd_parse_partitions from __devinit to common text section >> 5) add ofpart to the default list of partition parsers >> >> Signed-off-by: Dmitry Eremin-Solenikov<dbaryshkov@gmail.com> > > Good idea, but > >> #include<linux/mtd/mtd.h> >> #include<linux/mtd/partitions.h> >> @@ -446,6 +447,10 @@ int mtd_device_register(struct mtd_info *master, >> const struct mtd_partition *parts, >> int nr_parts) >> { >> +#ifdef CONFIG_OF >> + if (master->node) >> + of_node_get(master->node); >> +#endif > > Could all the OF-specific things be done in the ofpart.c ? Answering both of your questions here: My idea is to have an of node connected to mtd device, if there is any. It can be used by ofpart.c, ofoldpart.c (extract from physmap_of), maybe some other parsers/drivers/etc. The field is populated by the creator of mtd_info. Then the function dedicated to registration of the mtd should get the node and other functions can just use it. Maybe this should be moved to add_mtd_device, but this shouldn't be handled in ofpart.c Another way can be to populate ->node pointer before parsing, then drop it at the end of parsing/registration. Thus we won't have to handle getting/putting the of node, but it won't be available during mtd lifecycle. >> --- a/include/linux/mtd/mtd.h >> +++ b/include/linux/mtd/mtd.h >> @@ -171,6 +171,9 @@ struct mtd_info { >> // Kernel-only stuff starts here. >> const char *name; >> int index; >> +#ifdef CONFIG_OF >> + struct device_node *node; >> +#endif > > And designe-wise this does not look like a good idea to have such fields > in mtd_info ... Who initializes this "node" field? >
On Mon, 2011-06-06 at 12:15 +0400, Dmitry Eremin-Solenikov wrote: > Answering both of your questions here: > My idea is to have an of node connected to mtd device, if there is > any. I just think the connection should be different. Probably the best would be to have an OF-specific object (some struct of_info) containing struct mtd_info inside, and then OF code can use container_of(). > It can be used by ofpart.c, ofoldpart.c (extract from physmap_of), > maybe some other parsers/drivers/etc. I'm still not convinced we need ofoldpart.c - that is a small piece of cade which can be put to ofpart.c.
On 06.06.2011 12:05, Artem Bityutskiy wrote: > On Thu, 2011-06-02 at 18:51 +0400, Dmitry Eremin-Solenikov wrote: >> Prepare to convert of_mtd_parse_partitions() to usual partitions parser: >> 1) Register ofpart parser >> 2) Internally don't use passed device for error printing >> 3) Add device_node to mtd_info struct >> 4) Move of_mtd_parse_partitions from __devinit to common text section >> 5) add ofpart to the default list of partition parsers >> >> Signed-off-by: Dmitry Eremin-Solenikov<dbaryshkov@gmail.com> > > Do not apply this and further changes so far - I had some questions. > > Anyway, your series consists of as set of logical steps, which is really > nice, so let's finish with the first cmdline consolidation step, and > then move on. BTW: what about fixing order of parsing to: cmdlinepart, RedBoot, afs, ofpart/ofoldpart, platform-default-hook. This will allow me to drop "parse order" options from all relevant functions.
On 06.06.2011 12:19, Artem Bityutskiy wrote: > On Mon, 2011-06-06 at 12:15 +0400, Dmitry Eremin-Solenikov wrote: >> Answering both of your questions here: >> My idea is to have an of node connected to mtd device, if there is >> any. > > I just think the connection should be different. Probably the best would > be to have an OF-specific object (some struct of_info) containing struct > mtd_info inside, and then OF code can use container_of(). That will be agains current linux coding practices. E.g. check the gpio, i2c, etc. code. All add device_node field to existing structs. >> It can be used by ofpart.c, ofoldpart.c (extract from physmap_of), >> maybe some other parsers/drivers/etc. > > I'm still not convinced we need ofoldpart.c - that is a small piece of > cade which can be put to ofpart.c. Agreed.
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index c510aff..bfd5a65 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -36,6 +36,7 @@ #include <linux/idr.h> #include <linux/backing-dev.h> #include <linux/gfp.h> +#include <linux/of.h> #include <linux/mtd/mtd.h> #include <linux/mtd/partitions.h> @@ -446,6 +447,10 @@ int mtd_device_register(struct mtd_info *master, const struct mtd_partition *parts, int nr_parts) { +#ifdef CONFIG_OF + if (master->node) + of_node_get(master->node); +#endif return parts ? add_mtd_partitions(master, parts, nr_parts) : add_mtd_device(master); } @@ -461,6 +466,10 @@ int mtd_device_unregister(struct mtd_info *master) { int err; +#ifdef CONFIG_OF + if (master->node) + of_node_put(master->node); +#endif err = del_mtd_partitions(master); if (err) return err; diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 3477e16..0b473ec 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -712,7 +712,7 @@ int deregister_mtd_parser(struct mtd_part_parser *p) } EXPORT_SYMBOL_GPL(deregister_mtd_parser); -static const char *default_mtd_part_types[] = {"cmdlinepart", NULL}; +static const char *default_mtd_part_types[] = {"cmdlinepart", "ofpart", NULL}; int parse_mtd_partitions(struct mtd_info *master, const char **types, struct mtd_partition **pparts, unsigned long origin) diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c index a996718..ff11b85 100644 --- a/drivers/mtd/ofpart.c +++ b/drivers/mtd/ofpart.c @@ -20,7 +20,20 @@ #include <linux/slab.h> #include <linux/mtd/partitions.h> -int __devinit of_mtd_parse_partitions(struct device *dev, +static int parse_ofpart_partitions(struct mtd_info *master, + struct mtd_partition **pparts, + unsigned long origin) +{ + struct device_node *node; + + node = master->node; + if (!node) + return 0; + + return of_mtd_parse_partitions(NULL, node, pparts); +} + +int of_mtd_parse_partitions(struct device *dev, struct device_node *node, struct mtd_partition **pparts) { @@ -69,7 +82,7 @@ int __devinit of_mtd_parse_partitions(struct device *dev, if (!i) { of_node_put(pp); - dev_err(dev, "No valid partition found on %s\n", node->full_name); + pr_err("No valid partition found on %s\n", node->full_name); kfree(*pparts); *pparts = NULL; return -EINVAL; @@ -79,4 +92,17 @@ int __devinit of_mtd_parse_partitions(struct device *dev, } EXPORT_SYMBOL(of_mtd_parse_partitions); +static struct mtd_part_parser ofpart_parser = { + .owner = THIS_MODULE, + .parse_fn = parse_ofpart_partitions, + .name = "ofpart", +}; + +static int __init ofpart_parser_init(void) +{ + return register_mtd_parser(&ofpart_parser); +} + +module_init(ofpart_parser_init); + MODULE_LICENSE("GPL"); diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 2541fb8..14305d0 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -171,6 +171,9 @@ struct mtd_info { // Kernel-only stuff starts here. const char *name; int index; +#ifdef CONFIG_OF + struct device_node *node; +#endif /* ecc layout structure pointer - read only ! */ struct nand_ecclayout *ecclayout; diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h index 08c9c7b..5fde0d8 100644 --- a/include/linux/mtd/partitions.h +++ b/include/linux/mtd/partitions.h @@ -71,7 +71,7 @@ struct device; struct device_node; #ifdef CONFIG_MTD_OF_PARTS -int __devinit of_mtd_parse_partitions(struct device *dev, +int of_mtd_parse_partitions(struct device *dev, struct device_node *node, struct mtd_partition **pparts); #else
Prepare to convert of_mtd_parse_partitions() to usual partitions parser: 1) Register ofpart parser 2) Internally don't use passed device for error printing 3) Add device_node to mtd_info struct 4) Move of_mtd_parse_partitions from __devinit to common text section 5) add ofpart to the default list of partition parsers Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> --- drivers/mtd/mtdcore.c | 9 +++++++++ drivers/mtd/mtdpart.c | 2 +- drivers/mtd/ofpart.c | 30 ++++++++++++++++++++++++++++-- include/linux/mtd/mtd.h | 3 +++ include/linux/mtd/partitions.h | 2 +- 5 files changed, 42 insertions(+), 4 deletions(-)