Fix: keep trying to read TARGETID for nordic devices #2215

This block of changes makes some assumptions about the nature of the problem which fail to hold properly when scrutinised - and fails to move a comment that should have been, and now doesn't make sense where it's been left.

We would strongly suggest that the TARGETID register decoding be left below outside the loop, and instead the loop focus only on re-reading TARGETID a few times till it's either non-zero or the number of attempts exhausted. If it has come back 0, then dp->target_designer_code is going to be 0! thus it makes most sense to check if targetid == 0U and do this at the top of the loop (so, a while loop, not a do-while).

Please make read_attempts unsigned and only large enough to hold a suitable number of attempts - eg, by making it a uint8_t. Let's make invalid states unrepresentable.

Multi-line comment format is only required for an actually multi-line comment.

Our suggested change here is as follows:

uint32_t targetid = 0U;
uint8_t read_attempts = 0U;
/* Retry reading TARGETID till it's non-zero or we've tried too many times */
while (targetid == 0U) {
	/* TARGETID is on bank 2 */
	adiv5_dp_write(dp, ADIV5_DP_SELECT, ADIV5_DP_BANK2);
	targetid = adiv5_dp_read(dp, ADIV5_DP_TARGETID);
	adiv5_dp_write(dp, ADIV5_DP_SELECT, ADIV5_DP_BANK0);
 if (++read_attempts >= 128U && targetid == 0U) {
 DEBUG_WARN("Failed to read TARGETID after 128 attempts\n");
 break;
 }
}

targetid won't be 0 after the first read, since the designer is part of it which can be read on the first attempt:

Please report unknown device with Designer 0x244 Part ID 0x0

Ah, that wasn't made clear as the way it was conveyed was that the register entirely fails to read (returns 0) - please use comments in the code to convey this. We would still prefer this be a while loop - do note that dp's members are initalised to 0 (guaranteed by calloc) so please make use of that to test dp->target_partno == 0U in the while condition.

Constants are still missing their U suffixes - unsigned variables should be compared with unsigned values so we don't do an oops with signed-unsigned conversions. This suffix is the only way to construct an unsigned constant - without it, they default to being signed, and undergoing implicit conversions.

The spaces this removes are there intentionally and on purpose - please re-insert them.