| Message ID | 20171117144515.10870-4-robh@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Another batch of dtc checks | expand |
Hi, On 17/11/17 14:45, Rob Herring wrote: > Add a string list check for common properties ending in "-names" such as > reg-names or interrupt-names. > > Signed-off-by: Rob Herring <robh@kernel.org> > --- > checks.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/checks.c b/checks.c > index 4e23f29486bb..346b0256f9cb 100644 > --- a/checks.c > +++ b/checks.c > @@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path"); > > WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible"); > > +static void check_names_is_string_list(struct check *c, struct dt_info *dti, > + struct node *node) > +{ > + struct property *prop; > + > + for_each_property(node, prop) { > + if (!strstr(prop->name, "-names")) But that would match "actually-names-dont-matter" as well, resulting in a false positive? Should we check if the string appears at the *end* of the property name? Cheers, Andre. > + continue; > + > + c->data = prop->name; > + check_is_string_list(c, dti, node); > + } > +} > +WARNING(names_is_string_list, check_names_is_string_list, NULL); > + > static void fixup_addr_size_cells(struct check *c, struct dt_info *dti, > struct node *node) > { > @@ -1273,7 +1288,7 @@ static struct check *check_table[] = { > &device_type_is_string, &model_is_string, &status_is_string, > &label_is_string, &bootargs_is_string, &stdout_path_is_string, > > - &compatible_is_string_list, > + &compatible_is_string_list, &names_is_string_list, > > &property_name_chars_strict, > &node_name_chars_strict, > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 17, 2017 at 9:12 AM, Andre Przywara <andre.przywara@arm.com> wrote: > Hi, > > On 17/11/17 14:45, Rob Herring wrote: >> Add a string list check for common properties ending in "-names" such as >> reg-names or interrupt-names. >> >> Signed-off-by: Rob Herring <robh@kernel.org> >> --- >> checks.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/checks.c b/checks.c >> index 4e23f29486bb..346b0256f9cb 100644 >> --- a/checks.c >> +++ b/checks.c >> @@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path"); >> >> WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible"); >> >> +static void check_names_is_string_list(struct check *c, struct dt_info *dti, >> + struct node *node) >> +{ >> + struct property *prop; >> + >> + for_each_property(node, prop) { >> + if (!strstr(prop->name, "-names")) > > But that would match "actually-names-dont-matter" as well, resulting in > a false positive? Should we check if the string appears at the *end* of > the property name? Perhaps. IMO, once a word is used, it needs to be reserved for that purpose. For example, the gpio hogs binding use of "gpios" with just numbers and no phandle is bad because we have a mixture of types for a given property name or suffix. So we should really enforce that "-names" only appears as a suffix and use anywhere else is a warning. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 17, 2017 at 03:12:02PM +0000, Andre Przywara wrote: > Hi, > > On 17/11/17 14:45, Rob Herring wrote: > > Add a string list check for common properties ending in "-names" such as > > reg-names or interrupt-names. > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > checks.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/checks.c b/checks.c > > index 4e23f29486bb..346b0256f9cb 100644 > > --- a/checks.c > > +++ b/checks.c > > @@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path"); > > > > WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible"); > > > > +static void check_names_is_string_list(struct check *c, struct dt_info *dti, > > + struct node *node) > > +{ > > + struct property *prop; > > + > > + for_each_property(node, prop) { > > + if (!strstr(prop->name, "-names")) > > But that would match "actually-names-dont-matter" as well, resulting in > a false positive? Should we check if the string appears at the *end* of > the property name? Yes, we should. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Fri, Nov 17, 2017 at 12:07:48PM -0600, Rob Herring wrote: > On Fri, Nov 17, 2017 at 9:12 AM, Andre Przywara <andre.przywara@arm.com> wrote: > > Hi, > > > > On 17/11/17 14:45, Rob Herring wrote: > >> Add a string list check for common properties ending in "-names" such as > >> reg-names or interrupt-names. > >> > >> Signed-off-by: Rob Herring <robh@kernel.org> > >> --- > >> checks.c | 17 ++++++++++++++++- > >> 1 file changed, 16 insertions(+), 1 deletion(-) > >> > >> diff --git a/checks.c b/checks.c > >> index 4e23f29486bb..346b0256f9cb 100644 > >> --- a/checks.c > >> +++ b/checks.c > >> @@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path"); > >> > >> WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible"); > >> > >> +static void check_names_is_string_list(struct check *c, struct dt_info *dti, > >> + struct node *node) > >> +{ > >> + struct property *prop; > >> + > >> + for_each_property(node, prop) { > >> + if (!strstr(prop->name, "-names")) > > > > But that would match "actually-names-dont-matter" as well, resulting in > > a false positive? Should we check if the string appears at the *end* of > > the property name? > > Perhaps. IMO, once a word is used, it needs to be reserved for that > purpose. For example, the gpio hogs binding use of "gpios" with just > numbers and no phandle is bad because we have a mixture of types for a > given property name or suffix. So we should really enforce that > "-names" only appears as a suffix and use anywhere else is a warning. That sounds... overly restrictive to me. The grabbing of a whole bunch of gpios words is an example of poor - or at least nonstandard - binding design IMO. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
diff --git a/checks.c b/checks.c index 4e23f29486bb..346b0256f9cb 100644 --- a/checks.c +++ b/checks.c @@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path"); WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible"); +static void check_names_is_string_list(struct check *c, struct dt_info *dti, + struct node *node) +{ + struct property *prop; + + for_each_property(node, prop) { + if (!strstr(prop->name, "-names")) + continue; + + c->data = prop->name; + check_is_string_list(c, dti, node); + } +} +WARNING(names_is_string_list, check_names_is_string_list, NULL); + static void fixup_addr_size_cells(struct check *c, struct dt_info *dti, struct node *node) { @@ -1273,7 +1288,7 @@ static struct check *check_table[] = { &device_type_is_string, &model_is_string, &status_is_string, &label_is_string, &bootargs_is_string, &stdout_path_is_string, - &compatible_is_string_list, + &compatible_is_string_list, &names_is_string_list, &property_name_chars_strict, &node_name_chars_strict,
Add a string list check for common properties ending in "-names" such as reg-names or interrupt-names. Signed-off-by: Rob Herring <robh@kernel.org> --- checks.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) -- 2.14.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html