[Pdbg] [PATCH v2 08/15] main: Switch to path based target selection

[Alistair Popple]

On Tuesday, 13 November 2018 3:16:22 PM AEDT Alistair Popple wrote:
> Looks good, happy to see this code start to disappear.
> 
> > @@ -523,19 +519,12 @@ static bool parse_options(int argc, char *argv[])
> > 
> >  void target_select(struct pdbg_target *target)
> >  {
> > 
> > -	/* We abuse the private data pointer atm to indicate the target is
> > -	 * selected */
> > -	pdbg_target_priv_set(target, (void *) 1);
> > -}
> > -
> > -void target_unselect(struct pdbg_target *target)
> > -{
> > -	pdbg_target_priv_set(target, NULL);
> > +	path_target_add(target);
> 
> Do you think there's any value in keeping this function rather than just
> changing the existing code to call path_target_add() directly? It's not part
> of any external API so it seems to just add an unnecessary indirection.

Actually better yet might be to just delete these functions and rename the new 
path_target_add() and path_target_selected() to target_select() and 
target_selected()?

- Alistair

> >  }
> >  
> >  bool target_selected(struct pdbg_target *target)
> >  {
> > 
> > -	return (bool) pdbg_target_priv(target);
> > +	return path_target_selected(target);
> 
> Ditto.
> 
> - Alistair
> 
> >  }
> >  
> >  /* Returns the sum of return codes. This can be used to count how many
> > 
> > targets the callback was run on. */ @@ -554,7 +543,6 @@ int
> > for_each_child_target(char *class, struct pdbg_target *parent,
> > 
> >  		index = pdbg_target_index(target);
> >  		assert(index != -1);
> > 
> > -		pdbg_target_probe(target);
> > 
> >  		status = pdbg_target_status(target);
> >  		if (status != PDBG_TARGET_ENABLED)
> >  		
> >  			continue;
> > 
> > @@ -578,7 +566,6 @@ int for_each_target(char *class, int (*cb)(struct
> > pdbg_target *, uint32_t, uint6
> > 
> >  		index = pdbg_target_index(target);
> >  		assert(index != -1);
> > 
> > -		pdbg_target_probe(target);
> > 
> >  		status = pdbg_target_status(target);
> >  		if (status != PDBG_TARGET_ENABLED)
> >  		
> >  			continue;
> > 
> > @@ -603,8 +590,6 @@ void for_each_target_release(char *class)
> > 
> >  static bool target_selection(void)
> >  {
> > 
> > -	struct pdbg_target *fsi, *pib, *chip, *thread;
> > -
> > 
> >  	switch (backend) {
> >  
> >  #ifdef TARGET_ARM
> >  
> >  	case I2C:
> > @@ -671,63 +656,17 @@ static bool target_selection(void)
> > 
> >  		return false;
> >  	
> >  	}
> > 
> > -	/* At this point we should have a device-tree loaded. We want
> > -	 * to walk the tree and disabled nodes we don't care about
> > -	 * prior to probing. */
> > -	pdbg_for_each_class_target("pib", pib) {
> > -		int proc_index = pdbg_target_index(pib);
> > -
> > -		if (backend == I2C && device_node)
> > -			pdbg_target_set_property(pib, "bus", device_node,
> 
> strlen(device_node) +
> 
> > 1); -
> > -		if (processorsel[proc_index]) {
> > -			target_select(pib);
> > -			pdbg_for_each_target("core", pib, chip) {
> > -				int chip_index = pdbg_target_index(chip);
> > -				if (pdbg_parent_index(chip, "pib") != proc_index)
> > -					continue;
> > -
> > -				if (chipsel[proc_index][chip_index]) {
> > -					target_select(chip);
> > -					pdbg_for_each_target("thread", chip, thread) {
> > -						int thread_index = pdbg_target_index(thread);
> > -						if (threadsel[proc_index][chip_index][thread_index])
> > -							target_select(thread);
> > -						else
> > -							target_unselect(thread);
> > -					}
> > -				} else
> > -					target_unselect(chip);
> > -			}
> > -
> > -			/* This is kinda broken as we're overloading what '-c'
> > -			 * means - it's now up to each command to select targets
> > -			 * based on core/chiplet. We really need a better
> > -			 * solution to target selection. */
> > -			pdbg_for_each_target("chiplet", pib, chip) {
> > -				int chip_index = pdbg_target_index(chip);
> > -				if (chipsel[proc_index][chip_index]) {
> > -					target_select(chip);
> > -				} else
> > -					target_unselect(chip);
> > -			}
> > -		} else
> > -			target_unselect(pib);
> > -	}
> > -
> > -	pdbg_for_each_class_target("fsi", fsi) {
> > -		int index = pdbg_target_index(fsi);
> > -		if (processorsel[index])
> > -			target_select(fsi);
> > -		else
> > -			target_unselect(fsi);
> > -	}
> > -
> > 
> >  	if (pathsel_count) {
> >  	
> >  		if (!path_target_parse(pathsel, pathsel_count))
> >  		
> >  			return false;
> >  	
> >  	}
> > 
> > +	if (!path_target_present()) {
> > +		printf("No valid targets found or specified. Try adding -p/-c/-t
> 
> options
> 
> > to specify a target.\n"); +		printf("Alternatively run 'pdbg -a probe' 
to
> > get a list of all valid targets\n"); +		return false;
> > +	}
> > +
> > 
> >  	return true;
> >  
> >  }
> > 
> > @@ -860,8 +799,5 @@ found_action:
> >  	if (rc > 0)
> >  	
> >  		return 0;
> > 
> > -	printf("No valid targets found or specified. Try adding -p/-c/-t options
> > to specify a target.\n"); -	printf("Alternatively run '%s -a probe' to 
get
> > a list of all valid targets\n", -	       basename(argv[0]));
> > 
> >  	return 1;
> >  
> >  }