Optional DTB #186

lool · 2025-10-17T19:39:27Z

Make DTB optional as to allow kernels with less support than all known
boards – this will allow listing boards supported only by qcom-next or
mainline that are not supported with the current stable linux-qcom
kernel.

Update docs for new option and use this opportunity for cleanups.

feat(debos/flash): Add target boards flag
feat: Skip boards expecting a dtb we don't have
docs(README)! add target_boards; deprecate build_*
docs(README): Drop example with experimentalkernel
docs(README): Fix misc typos and improve style

github-actions · 2025-10-17T20:38:27Z

Test Results

2 files ±0 6 suites ±0 3m 4s ⏱️ -17s
20 tests ±0 20 ✅ ±0 0 💤 ±0 0 ❌ ±0
64 runs ±0 64 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 64079e3. ± Comparison against base commit f87cc7b.

♻️ This comment has been updated with latest results.

github-actions · 2025-10-17T20:38:28Z

Test jobs for commit `6883f28`

basak-qcom

Some minor comments inline. I think I understand what you're doing here, but I'm not sure I completely grasp it. I understand what it means to not have a particular board supported by the upstream kernel, and therefore not appear in dtbs.tar.gz. But:

...this will allow listing boards supported only by qcom-next or mainline that are not supported with the current stable linux-qcom kernel

...what does "listing boards" mean?

debos-recipes/qualcomm-linux-debian-flash.yaml

basak-qcom · 2025-10-24T16:10:32Z

debos-recipes/qualcomm-linux-debian-flash.yaml

+      rm -f "${targets_file}"
+      case "{{ $target_boards }}" in
+        all)
+          # no override; build all possible boards (default)


You already have $boards defined, right? Could you make the later code simpler by unconditionally writing build/targets.txt, so the "all" magic is restricted to here, rather than needing to be understood elsewhere? I think that would avoid surprising semantics of when build/targets.txt doesn't exist.

Let me try to repeat what I understand you're suggesting: have the go template logic at the top to generate a targets.txt table based on the data from $boards, then have a pure shell script reading through that table afterwards. Is that correct?

I think that matches, yes, if by "at the top" you mean in the code area highlighted here?

I think you could write $board.name for $board in $boards to build/targets.txt in the area of code highlighted here in the case that $target_boards is set to "all". Then below the if [ -e "${targets_file}" ]; then can go away - it'd be unconditionally setting skip_board to true if $board.name (in the iteration below) isn't present in build/targets.txt.

This would take away the special handling when build/targets.txt isn't present, because it'd always be present, making the logic simpler and less surprising from my POV.

Done, looks like this:

+ touch "${targets_file}" +{{- range $board := $boards }} + echo "{{ $board.name }}" >>"${targets_file}" +{{- end }}

and that loop drops one if statement/level afterwards

basak-qcom · 2025-10-24T16:12:08Z

debos-recipes/qualcomm-linux-debian-flash.yaml

          "${QCOM_PTOOL}/ptool.py" -x ptool-partitions.xml
      )

+      if [ "${skip_board}" = false ]; then


Do the following lines need indenting, or is GitHub hiding that from me? Same question for the other if statements below.

It does, but then I have to indent the whole file, becoming hard to read and review :)

happy to do that though!

My opinion: especially because there are many matching if/fi close together, it's easier to read with the expected indentation and if someone is debugging in this area then your intent will be clearer.

Factoring some of these into functions might help with readability too, but that's another can of worms so maybe not worth it.

This is subjective of course so I'll leave it to you.

Reindented, but as a separate commit

lool · 2025-10-24T16:19:49Z

Some minor comments inline. I think I understand what you're doing here, but I'm not sure I completely grasp it. I understand what it means to not have a particular board supported by the upstream kernel, and therefore not appear in dtbs.tar.gz. But:

...this will allow listing boards supported only by qcom-next or mainline that are not supported with the current stable linux-qcom kernel

...what does "listing boards" mean?

I mean listing them in our known boards in the long yaml table in flash.yaml

debos-recipes/qualcomm-linux-debian-flash.yaml

basak-qcom · 2025-10-27T09:41:44Z

debos-recipes/qualcomm-linux-debian-flash.yaml

+      rm -f "${targets_file}"
+      case "{{ $target_boards }}" in
+        all)
+          # no override; build all possible boards (default)


I think that matches, yes, if by "at the top" you mean in the code area highlighted here?

I think you could write $board.name for $board in $boards to build/targets.txt in the area of code highlighted here in the case that $target_boards is set to "all". Then below the if [ -e "${targets_file}" ]; then can go away - it'd be unconditionally setting skip_board to true if $board.name (in the iteration below) isn't present in build/targets.txt.

This would take away the special handling when build/targets.txt isn't present, because it'd always be present, making the logic simpler and less surprising from my POV.

basak-qcom · 2025-10-27T09:44:47Z

debos-recipes/qualcomm-linux-debian-flash.yaml

          "${QCOM_PTOOL}/ptool.py" -x ptool-partitions.xml
      )

+      if [ "${skip_board}" = false ]; then


My opinion: especially because there are many matching if/fi close together, it's easier to read with the expected indentation and if someone is debugging in this area then your intent will be clearer.

Factoring some of these into functions might help with readability too, but that's another can of worms so maybe not worth it.

This is subjective of course so I'll leave it to you.

basak-qcom · 2025-10-27T09:48:35Z

Approved with inline changes clarified, I think? Everything is subjective so it's up to you if you want to change anything or not. I don't think I need to review again.

Add a config to select a list of target boards to build. The default is still to build for all possible boards. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

Build a list of dtb files from dtbs.tar.gz and skip boards that use a dtb that isn't present. This allows listing all supported boards/dtbs, while building with kernels that don't support all boards. Missing dtbs/boards get automatically skipped. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

Document new target_boards option for the flash recipe and deprecate the build_* options. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

Option was removed some time ago. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

Misc rewording from spellcheck. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

Reindent if statements for skip_board sections. This is split out to make review easier. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

github-actions · 2025-12-02T16:45:28Z

Test jobs for commit `e23c17c`

github-actions · 2025-12-03T00:05:52Z

Test jobs for commit `64079e3`

basak-qcom reviewed Oct 24, 2025

View reviewed changes

basak-qcom approved these changes Oct 27, 2025

View reviewed changes

lool mentioned this pull request Dec 2, 2025

Ptool update to bring Lemans EVK support #206

Merged

lool added 5 commits December 2, 2025 15:49

feat(debos/flash): Add target boards flag

aa9799a

Add a config to select a list of target boards to build. The default is still to build for all possible boards. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

docs(README)! add target_boards; deprecate build_*

bdf08d4

Document new target_boards option for the flash recipe and deprecate the build_* options. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

docs(README): Drop example with experimentalkernel

d58ef21

Option was removed some time ago. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

docs(README): Fix misc typos and improve style

e23c17c

Misc rewording from spellcheck. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

lool force-pushed the optional-dtb branch from 6883f28 to e23c17c Compare December 2, 2025 15:49

refactor(debos): Reindent skip_board sections

64079e3

Reindent if statements for skip_board sections. This is split out to make review easier. Signed-off-by: Loïc Minier <loic.minier@oss.qualcomm.com>

lool merged commit b018f1d into qualcomm-linux:main Dec 3, 2025
15 of 16 checks passed

Optional DTB #186

Optional DTB #186

Uh oh!

Conversation

lool commented Oct 17, 2025

Uh oh!

github-actions bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

github-actions bot commented Oct 17, 2025

Test jobs for commit 6883f28

Uh oh!

basak-qcom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lool commented Oct 24, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basak-qcom commented Oct 27, 2025

Uh oh!

github-actions bot commented Dec 2, 2025

Test jobs for commit e23c17c

Uh oh!

Uh oh!

github-actions bot commented Dec 3, 2025

Test jobs for commit 64079e3

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Oct 17, 2025 •

edited

Loading

Test jobs for commit `6883f28`

Test jobs for commit `e23c17c`

Test jobs for commit `64079e3`