Re: [PATCH 1/2] checks: add phandle with arg property checks

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3602 bytes --]

On Tue, Aug 22, 2017 at 09:38:59AM -0500, Rob Herring wrote:
> On Sat, Aug 19, 2017 at 2:33 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Aug 18, 2017 at 11:02:01AM -0500, Rob Herring wrote:
> >> On Thu, Aug 17, 2017 at 11:35 PM, David Gibson
> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> > On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
> >> >
> >> > Yay! Someone actually implementing checks.
> >>
> >> Not like it is my first. I'll celebrate when someone else does.
> >>
> >>
> >> >> Many common bindings follow the same pattern of client properties
> >> >> containing a phandle and N arg cells where N is defined in the provider
> >> >> with a '#<specifier>-cells' property. Add a checks for properties
> >> >> following this pattern.
> >> >
> >> > I think this description would be easier to follow if you led with an example.
> >> >
> >> >>
> >> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> >
> >> > Looks pretty good, though I have some suggestions.
> >> >
> >> >> ---
> >> >>  checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >>  1 file changed, 117 insertions(+)
> >> >>
> >> >> diff --git a/checks.c b/checks.c
> >> >> index afabf64337d5..c0450e118043 100644
> >> >> --- a/checks.c
> >> >> +++ b/checks.c
> >> >> @@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
> >> >>  WARNING(obsolete_chosen_interrupt_controller,
> >> >>       check_obsolete_chosen_interrupt_controller, NULL);
> >> >>
> >> >> +struct provider {
> >> >> +     const char *prop_name;
> >> >> +     const char *cell_name;
> >> >> +     bool optional;
> >> >> +};
> >> >> +
> >> >> +static void check_property_phandle_args(struct check *c,
> >> >> +                                       struct dt_info *dti,
> >> >> +                                       struct node *node,
> >> >> +                                       struct property *prop,
> >> >> +                                       const struct provider *provider)
> >> >> +{
> >> >> +     struct marker *m = prop->val.markers;
> >> >> +
> >> >> +     if (!m) {
> >> >> +             FAIL(c, dti, "Missing phandles in %s:%s",
> >> >> +                  node->fullpath, prop->name);
> >> >> +             return;
> >> >> +     }
> >> >> +     for_each_marker_of_type(m, REF_PHANDLE) {
> >> >
> >> > So going through the markers I think is not the best approach.
> >> > That'll work if the source contains a reference here, which it usually
> >> > will, but there are some circumstances where it could contain a "raw"
> >> > phandle value (the most obvious being when you're decompiling an
> >> > existing dtb).
> >> >
> >> > But I don't think you really need to.  Instead you should be able to
> >> > read the actual value, look it up with get_node_by_phandle().  You can
> >> > make this check dependent on fixup_phandle_references to make sure
> >> > it's executed after the references are resolved.
> >>
> >> That's how I implemented it initially...
> >
> > Ok.. why did you change?
> 
> Just because the code is a bit more compact using markers. But
> decompiling a dtb is a good reason I didn't think of.

Ok.  Well, if you can revert to the first way, that should be good.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]