[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] gnu: Add u-boot.
From: |
Ludovic Courtès |
Subject: |
Re: [PATCH] gnu: Add u-boot. |
Date: |
Wed, 31 Aug 2016 22:40:57 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
David Craven <address@hidden> skribis:
> From: Danny Milosavljevic <address@hidden>
>
> * gnu/packages/u-boot.scm (u-boot, make-u-boot-package,
> u-boot-vexpress_ca9x4, u-boot-malta armhf-linux-uboot,
> mips64el-linux-uboot): New variables.
>
> Co-authored-by: David Craven <address@hidden>
Cool, thanks for picking it up!
Overall this LGTM. I guess Danny’s comments should be taken into
account. Other than that, I only have cosmetic suggestions:
> + (synopsis "ARM Universal Bootloader")
“ARM bootloader” (or “ARM universal bootloader” if you insist).
> + (description "U-Boot is an universal bootloader mostly used for ARM
> boards.
> +It also initializes the boards (RAM etc).")
I also think “universal” can be removed.
> +(define (make-u-boot-package board triplet xgcc)
A docstring would be nice here.
I think you could remove the ‘xgcc’ parameter and simply do:
(let ((xgcc (cross-gcc triplet)))
(package
…))
> + (lambda* (#:key outputs make-flags #:allow-other-keys)
> + (let ((configname (string-append ,board "_defconfig")))
Should be ‘config-name’ per our conventions, but ‘config’ is probably
enough.
> + (if (file-exists? (string-append "configs/" configname))
> + (zero? (apply system* "make" `(,@make-flags ,configname)))
> + (begin
> + (display "Invalid boardname. Valid boardnames would
> have been:")
> + (newline)
> + (system* "ls" "-1" "configs")
> + #f)))))
“board name” (two words).
> + (replace 'install
> + (lambda* (#:key outputs make-flags #:allow-other-keys)
> + (let* ((out (assoc-ref outputs "out"))
> + (libexec (string-append out "/libexec"))
> + (uboot-files (find-files "." ".*\\.(bin|efi)$")))
> + (mkdir-p libexec)
> + (for-each
> + (lambda (file-path)
> + (let ((target-file-path (string-append libexec "/"
> file-path)))
> + (mkdir-p (dirname target-file-path))
> + (copy-file file-path target-file-path)))
> + uboot-files)))))))))
s/-path//
> +
> +(define-public u-boot-vexpress_ca9x4
> + (make-u-boot-package "vexpress_ca9x4" "arm-linux-gnueabihf" xgcc-armhf))
> +
> +(define-public u-boot-malta
> + (make-u-boot-package "malta" "mips64el-linux-gnuabi64" xgcc-mips64el))
> +
> +(define-public armhf-linux-uboot
> + u-boot-vexpress_ca9x4)
> +
> +(define-public mips64el-linux-uboot
> + u-boot-malta)
I think these two aliases can be removed, no?
Thanks!
Ludo’.