On Wed, Jun 9, 2021 at 1:55 AM Loïc Minier <email address hidden>
wrote:

> thanks for the patch!
>

Thanks for the review!


>
> this might be easier to review as smaller atomic changes, but perhaps I
> can still follow around the debdiff :)
>

I have this branch based on the auto-imported ubuntu repos (focal branch)
that probably will make your review easier:
https://code.launchpad.net/~alfonsosanchezbeato/ubuntu/+source/flash-kernel/+git/flash-kernel/+ref/kria-support
There is now a commit with the changes you have requested. I have also
created an MP, purely to help with the review (obviously not for merging to
the auto-imported repo):
https://code.launchpad.net/~alfonsosanchezbeato/ubuntu/+source/flash-kernel/+git/flash-kernel/+merge/403942
When things are ready, I will create a new debdiff for impish.


>
> 1) The its files are data, so they should to /usr/share; I wouldn't
> encourage local customizations by dropping them in /etc unless that's an
> important function; a way to give control would be to install a local
> flash-kernel database entry that overrides the shipped defaults for that
> hardware; this should be possible with /etc/flash-kernel/db?
>

I actually fully agree with you, the reason that I've put the "its" file
under /etc is just to be coherent as that's where the boot script files are
stored (/etc/flash-kernel/bootscript/) too. And I think that /usr/share/
would be a better place for those files as well.

So, should I put the "its" file in /usr/share and maybe move the bootscript
files with another debdiff?


>
> 2) would you add the new Boot-FIT-Path and Boot-ITS-Path to README?
>

Done.


>
> 3) seems "Boot-ITS-Path: /.../foo.its" should be written "U-Boot-ITS-
> Name: foo.its" and should be searched in the default flash-kernel
> directory
>

Done.


>
> 4) would it be possible to avoid the cp calls for kernel and initrd and
> just generate the fit image from their locations? the kernels / initrds
> tend to be big, so I suspect this adds non-trivial time to the run on SD
> cards
>

I have now changed things to use macros in the "its" files (@@XXX@@) and
replace them with the file names using sed, so no copy is necessary anymore.


>
> 5) mkimage_fit(): why is padding needed? perhaps deserves a comment.
> Should this be a config?
>

Added Mkimage-FIT-Options option now.


>
> 6) the full path name in DTB-Id is the first one in the database,
> perhaps this suggests the corresponding deb should be in Required-
> Packages? would help with d-i usage
>

I've added xlnx-kria-firmware to Required-Packages.


>
> 6) "cd" looks pretty singular compared to rest of functions; could it be
> avoided?
>

The problem here was that mkimage was searching the files defined in the
.its in the current folder, but with the macros and using absolute paths
this is not needed anymore.


>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1931278
>
> Title:
>   Creating FIT images is not supported
>
> Status in flash-kernel package in Ubuntu:
>   New
>
> Bug description:
>   flash-kernel does currently not support creating FIT images method.
>   The attached debdiff (for impish) supports that, and additionally add
>   supports for the Kria SOM platform.
>
> To manage notifications about this bug go to:
>
> https://bugs.launchpad.net/ubuntu/+source/flash-kernel/+bug/1931278/+subscriptions
>