Re: [PATCH 1/1] ti-cmem: New recipes for ti-cmem

[Denys Dmytriyenko <denys@ti.com>]

Sam,

First, thanks for the submission!

Second, please drop "ti-" prefix from the recipes - it's redundant. Plus, 
everywhere else it is called just "cmem":

> "The cmem component module for contiguous memory allocation from userspace "
> "The cmem component supports contiguous memory allocation from userspace "
> "http://processors.wiki.ti.com/index.php/Category:CMEM"

For the rest of the comments, please see inline. But mostly everything looks 
fine, just a few minor questions/concerns.



On Wed, Oct 30, 2013 at 03:07:35PM -0400, Sam Nelson wrote:
> - Recipes for ti-cmem which provides user space api for contigous memory allocaion
> - ti-cmem_mod provides the kernel module
> - ti-mem provides the user space library
> 
> Signed-off-by: Sam Nelson <sam.nelson@ti.com>
> ---
>  .../recipes-bsp/ti-cmem/ti-cmem-mod.bb             |   40 ++++++++++++++++++++
>  meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem.bb   |   20 ++++++++++
>  meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem.inc  |   10 +++++
>  3 files changed, 70 insertions(+)
>  create mode 100755 meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem-mod.bb
>  create mode 100644 meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem.bb
>  create mode 100644 meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem.inc
> 
> diff --git a/meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem-mod.bb b/meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem-mod.bb
> new file mode 100755
> index 0000000..0839c20
> --- /dev/null
> +++ b/meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem-mod.bb
> @@ -0,0 +1,40 @@
> +DESCRIPTION="The cmem component module for contiguous memory allocation from userspace "
> +
> +HOMEPAGE="http://processors.wiki.ti.com/index.php/Category:CMEM"

You duplicate HOMEPAGE in all recipes and .inc file...


> +LICENSE = "GPLv2"

Can you move LICENSE field in the .inc file, since you already have 
LIC_FILES_CHKSUM there...


> +include ti-cmem.inc
> +
> +RDEPENDS_${PN} += "kernel (=${KERNEL_VERSION})"

Is it really tied to a specific kernel version? What if it's slightly 
different, i.e. with a minor update? Would using ">=" be better here?


> +DEPENDS += "virtual/kernel"

module.bbclass already does it for you.


> +NAME="cmem"
> +MODULE_BUILD_DIR ="src/cmem/module"
> +
> +# This package builds a kernel module, use kernel PR as base and append a local
> +PR = "${MACHINE_KERNEL_PR}"
> +PR_append = "a"
> +
> +PKG_${PN} = "kernel-module-${PN}"
> +
> +KERNEL_VERSION = "${@base_read_file('${STAGING_KERNEL_DIR}/kernel-abiversion')}"

This one is also defined by module.bbclass


> +S = "${WORKDIR}/git"
> +
> +inherit module
> +
> +do_compile () {
> +    cd ${S}
> +    make -f lu.mak module_clean
> +    make -f lu.mak module KERNEL_INSTALL_DIR=${STAGING_KERNEL_DIR} TOOLCHAIN_PREFIX=${CROSS_COMPILE} 
> +}
> +
> +do_install () {
> +    cd ${S}
> +    make -f lu.mak module_install KERNEL_INSTALL_DIR="${STAGING_KERNEL_DIR}" EXEC_DIR="${D}/lib/modules/${KERNEL_VERSION}/extra" INSTALL_MOD_PATH="${D}"
> +}
> +
> +PACKAGE_STRIP = "no"

Why this?


> diff --git a/meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem.bb b/meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem.bb
> new file mode 100644
> index 0000000..a106aa0
> --- /dev/null
> +++ b/meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem.bb
> @@ -0,0 +1,20 @@
> +DESCRIPTION="The cmem component supports contiguous memory allocation from userspace "
> +
> +HOMEPAGE="http://processors.wiki.ti.com/index.php/Category:CMEM"
> +
> +LICENSE = "GPLv2"
> +
> +include ti-cmem.inc
> +
> +DEPENDS = "ti-cmem-mod" 

Do you mean RDEPENDS here? I don't think you have a build-time dependency 
here...


> +S = "${WORKDIR}/git"
> +
> +PR = "r1"
> +
> +PACKAGES =+ "${PN}-test"
> +
> +FILES_${PN}-test = "${bindir}/*"

So, all the binaries will go into cmem-test, what would remain in the main 
cmem package?


> +inherit autotools
> +
> diff --git a/meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem.inc b/meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem.inc
> new file mode 100644
> index 0000000..9308b2d
> --- /dev/null
> +++ b/meta-arago-extras/recipes-bsp/ti-cmem/ti-cmem.inc
> @@ -0,0 +1,10 @@
> +HOMEPAGE="http://processors.wiki.ti.com/index.php/Category:CMEM"
> +
> +LIC_FILES_CHKSUM = "file://products.mak;beginline=2;endline=30;md5=195feadf798bb4165bcb1a23ffd50dbb"

I haven't checked yet, but is the Makefile the only and proper place to define 
licenses for the component?


> +COMPATIBLE_MACHINE = "keystone-evm"
> +
> +BRANCH ?= "master"
> +SRCREV = "4.00.00.06"

Any chance you can use a commit ID here?


> +SRC_URI = "git://git.ti.com/ipc/ludev.git;protocol=git;branch=${BRANCH}"
> -- 
> 1.7.9.5
> 
> _______________________________________________
> meta-arago mailing list
> meta-arago@arago-project.org
> http://arago-project.org/cgi-bin/mailman/listinfo/meta-arago