Comment 5 for bug 2037407

Revision history for this message
Dave Jones (waveform) wrote :

I've had a look at the devel patch and there's a few things I'm a bit concerned about:

In the new all.db entry the "Machine:" entries both include * wildcards. The all.db database is organized to be in strictly alphabetical order and that means that entries with * wildcards (or really, any accepted wildcard) should never be the first Machine: entry because their sorting order is ambiguous. I realize we've already got several such entries, but I'd really like to see all of these revised to be more like the Pi entries with wildcards, e.g.:

Machine: Raspberry Pi 3 Model B
Machine: Raspberry Pi 3 Model B Rev 1.2
Machine: Raspberry Pi 3 Model B Rev *

Here the first entry has no wildcards and hence can be used to enforce the sort order without ambiguity. They also serve to document the strings that the wildcard is expected to match. In the proposed patch, I'm not sure what sort of strings the "ZynqMP *K24*" and "ZynqMP *KD240*" are going to match.

Secondly, in the revised u-boot script, the support list of Kria boards at the top appears unchanged, but I'd assume "sck-kd-g" should appear in there somewhere?

Continuing with the revised u-boot script, $k24_starter is set to "SMK-K24-" if it's not already in the environment. I'm not entirely clear on what this variable is meant to represent and why the environment's providing it? It would be nice to see this (and, ideally, $card1_name) documented as the "variables expected to be in the environment".

Finally, the patch adds the following test:

  if setexpr model gsub .*$k24_starter.* $model

This doesn't make sense unless $model is also set in the environment. It stands in contrast to the tests immediately following it that rely on $model_test, which is retrieved from the device-tree prior to the tests. Are we sure this is correct?

Leaving as "Incomplete" and unsubscribing ubuntu-sponsors pending correct and clarification of the above.