[Pdbg] [PATCH 04/10] path: Add device tree path based targeting

[Alistair Popple]

On Thursday, 1 November 2018 2:06:50 PM AEDT Amitay Isaacs wrote:
> On Thu, 2018-11-01 at 12:56 +1100, Alistair Popple wrote:
> > > +/**
> > > + * @brief Iterator for list of path targets
> > > + *
> > > + * @param[in]  prev The last pdbg target
> > > + * @param[in]  only_enabled Whether to list enabled targets or all
> > 
> > Just a minor documentation nit-pick ... reading that makes me think
> > it would
> > just check the status. However it also has the side-effect of trying
> > to probe
> > the device first.
> 
> Hmm..  I added probing in line with the current code.  But now I think
> that this code should not probe targets at all.  That way there are no
> side-effects.
> 
> I think we should add explicit probing before running the command and
> explicit release after the command has executed.  That way you don't
> need incremental probing as done in the for_each_target and
> for_each_child_target macros.
> 
> How about something like:
> 
>    for_each_path_target(target)
>       pdbg_target_probe(target);
> 
>    rc = cmd(args, flags);
> 
>    for_each_path_target(target)
>       pdbg_target_release(target);
> 
> Then each command does not have to worry about the probing targets and
> only use for_each_path_target_enabled() or
> for_each_path_target_class().  May be we will also need
> for_each_path_target_class_enabled().

Yep. I think that makes more sense. From memory you only use 
only_enabled==true in one to two locations so hopefully changing it won't 
cause too much churn.

- Alistair

> > In general I think it would be nicer to have seperate macros/calls
> > for probed
> > and enabled only vs. iterating over all rather than a bool flag whos
> > meaning is
> > a little less obvious when reading the calls in code.
> 
> Sure.
> 
> Amitay.