On Thu, 04 Jan 2018 07:32:36 +0100
Rafał Miłecki <rafal@xxxxxxxxxx> wrote:
> On 2018-01-03 19:28, Rob Herring wrote:
> > On Wed, Jan 3, 2018 at 4:55 AM, Rafał Miłecki <zajec5@xxxxxxxxx> wrote:
> >> From: Brian Norris <computersforpeace@xxxxxxxxx>
> >>
> >> Partition parsers can now provide an of_match_table to enable
> >> flash<-->parser matching via device tree as documented in the
> >> mtd/partition.txt.
> >>
> >> It works by looking for a matching parser for every string in the
> >> "compatibility" property (starting with the most specific one).
> >>
> >> This support is currently limited to built-in parsers as it uses
> >> request_module() and friends. This should be sufficient for most cases
> >> though as compiling parsers as modules isn't a common choice.
> >>
> >> Signed-off-by: Brian Norris <computersforpeace@xxxxxxxxx>
> >> Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
> >> ---
> >> This is based on Brian's patches:
> >> [RFC PATCH 4/7] mtd: add of_match_mtd_parser() and
> >> of_mtd_match_mtd_parser() helpers
> >> [RFC PATCH 6/7] RFC: mtd: partitions: enable of_match_table matching
> >>
> >> V1: Put helpers in mtdpart.c instead of drivers/of/of_mtd.c
> >> Merge helpers into a single of_mtd_match_mtd_parser
> >> V3: Add a simple comment to note we will need the best match in the
> >> future
> >> V4: Rework new functions to pick parser with the best match
> >> Move new code in parse_mtd_partitions up so it has precedence over
> >> flash
> >> driver defaults and MTD defaults
> >> V5: Rework matching code to start checking with the most specific
> >> string in the
> >> "compatibility" property.
> >> V6: Initialize "ret" variable in mtd_part_get_parser_by_cp to NULL
> >> ---
> >> drivers/mtd/mtdpart.c | 64
> >> ++++++++++++++++++++++++++++++++++++++++++
> >> include/linux/mtd/partitions.h | 1 +
> >> 2 files changed, 65 insertions(+)
> >>
> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> >> index be088bccd593..b11958adcf45 100644
> >> --- a/drivers/mtd/mtdpart.c
> >> +++ b/drivers/mtd/mtdpart.c
> >> @@ -30,6 +30,7 @@
> >> #include <linux/mtd/mtd.h>
> >> #include <linux/mtd/partitions.h>
> >> #include <linux/err.h>
> >> +#include <linux/of.h>
> >>
> >> #include "mtdcore.h"
> >>
> >> @@ -880,6 +881,48 @@ static int mtd_part_do_parse(struct
> >> mtd_part_parser *parser,
> >> }
> >>
> >> /**
> >> + * mtd_part_get_parser_by_cp - find MTD parser by a compatible string
> >
> > What's "cp"? Perhaps mtd_part_get_compatible_parser.
>
> That's what some base of functions use internally, like
> __of_device_is_compatible (cp)
> of_modalias_node (cplen)
> but using "compatible" for the function name sounds better.
>
>
> >> + *
> >> + * @cp: compatible string describing partitions in a device tree
> >
> > "compat" would be clearer IMO
> >
> >> + *
> >> + * MTD parsers can specify supported partitions by providing a table
> >> of
> >> + * compatibility strings. This function finds a parser that
> >> advertises support
> >> + * for a passed value of "compatible".
> >> + */
> >> +static struct mtd_part_parser *mtd_part_get_parser_by_cp(const char
> >> *cp)
> >> +{
> >> + struct mtd_part_parser *p, *ret = NULL;
> >> +
> >> + spin_lock(&part_parser_lock);
> >> +
> >> + list_for_each_entry(p, &part_parsers, list) {
> >> + const struct of_device_id *matches;
> >> +
> >> + matches = p->of_match_table;
> >> + if (!matches)
> >> + continue;
> >> +
> >> + for (; matches->name[0] || matches->type[0] ||
> >> matches->compatible[0];
> >> + matches++) {
> >> + if (!matches->compatible[0])
> >> + continue;
> >
> > It would be simpler to just ignore name and type. Having either of
> > those would be an error and the only consequence if there are entries
> > without compatible is returning early.
>
> OK, thanks. I guess I misused pattern I saw in the __of_match_node.
>
>
> >> + if (!strcmp(matches->compatible, cp) &&
> >> + try_module_get(p->owner)) {
> >> + ret = p;
> >> + break;
> >> + }
> >> + }
> >> +
> >> + if (ret)
> >> + break;
> >> + }
> >> +
> >> + spin_unlock(&part_parser_lock);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +/**
> >> * parse_mtd_partitions - parse MTD partitions
> >> * @master: the master partition (describes whole MTD device)
> >> * @types: names of partition parsers to try or %NULL
> >> @@ -905,8 +948,29 @@ int parse_mtd_partitions(struct mtd_info *master,
> >> const char *const *types,
> >> struct mtd_part_parser_data *data)
> >> {
> >> struct mtd_part_parser *parser;
> >> + struct device_node *np;
> >> + struct property *prop;
> >> + const char *cp;
> >> int ret, err = 0;
> >>
> >> + np = of_get_child_by_name(mtd_get_of_node(master),
> >> "partitions");
> >> + if (np) {
> >
> > You can drop this. of_property_for_each_string and of_node_put will
> > work if np is NULL.
> >
> >> + of_property_for_each_string(np, "compatible", prop,
> >> cp) {
> >> + parser = mtd_part_get_parser_by_cp(cp);
> >> + if (!parser)
> >> + continue;
> >> + ret = mtd_part_do_parse(parser, master,
> >> pparts, data);
> >> + if (ret > 0) {
> >> + of_node_put(np);
> >> + return 0;
> >> + }
> >> + mtd_part_parser_put(parser);
> >> + if (ret < 0 && !err)
> >> + err = ret;
> >> + }
> >> + of_node_put(np);
> >> + }
> >> +
> >> if (!types)
> >> types = default_mtd_part_types;
> >>
> >> diff --git a/include/linux/mtd/partitions.h
> >> b/include/linux/mtd/partitions.h
> >> index c4beb70dacbd..11cb0c50cd84 100644
> >> --- a/include/linux/mtd/partitions.h
> >> +++ b/include/linux/mtd/partitions.h
> >> @@ -77,6 +77,7 @@ struct mtd_part_parser {
> >> struct list_head list;
> >> struct module *owner;
> >> const char *name;
> >> + const struct of_device_id *of_match_table;
> >
> > I generally like using of_device_id over just an array of strings, but
> > I'm wondering if there's really cases where you have multiple
> > compatible strings for a single parser? Perhaps a single string here
> > would be sufficient. And if you did have multiple matches, how would
> > you distinguish them later on in the parsing functions? Normally we'd
> > use the matched data ptr, but that's not provided here.
>
> I think we may need some flash parsers to support multiple strings in
> the future. For example image tag parser (bcm63xxpart.c) handles
> Broadcom's tag that has few versions (see struct bcm_tag and its
> version field).
>
> Later we may want to add support for matching partitions parsers (like
> trx) which also can have multple versions support. For example TRX v1
> and TRX v2 differ by a single field in the header and it makes sense to
> use the same parser for them.
>
> While this may be not a perfect solution with data ptr, we can always do
> of_device_is_compatible(&mtd->dev, "foo")
Well, if we ever need to extract per-compatible driver data, we could
easily add a field to the mtd_part_parser_data struct (void *of_data).
Rafal, I had a look at your v7 and it looks sane, so I'll queue it for
4.16 unless Rob has other concerns.
Thanks for your patience.
Boris
>
>
> >> int (*parse_fn)(struct mtd_info *, const struct mtd_partition
> >> **,
> >> struct mtd_part_parser_data *);
> >> void (*cleanup)(const struct mtd_partition *pparts, int
> >> nr_parts);
> >> --
> >> 2.11.0
> >>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
