Re: [PATCH v2] ti-rm: provides ti resouce manager recipe for KeyStone devices

[Denys Dmytriyenko <denys@ti.com>]

On Fri, Feb 21, 2014 at 08:07:41PM -0500, Aravind Batni wrote:
> - TI Resource Manager Low Level Driver
> 
> Signed-off-by: Aravind Batni <aravindbr@ti.com>
> 
> ---
> Changes from last patch:
> * Removed ti-rm.inc file
> * Added destsuffix in the SRCURI to clone RM git under git/ti/drv/rm folder
> * Removed the symbolic link workaround
> * Added PV
> * Added for loop for multiple devices and choices for the test binaries

Thanks! That's a good list of fixes and the patch now looks much better and 
cleaner. Please see few minor comments inline.


> ---
> ---
>  meta-arago-extras/recipes-bsp/ti-rm/ti-rm_git.bb |   53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100755 meta-arago-extras/recipes-bsp/ti-rm/ti-rm_git.bb
> 
> diff --git a/meta-arago-extras/recipes-bsp/ti-rm/ti-rm_git.bb b/meta-arago-extras/recipes-bsp/ti-rm/ti-rm_git.bb
> new file mode 100755
> index 0000000..761a722
> --- /dev/null
> +++ b/meta-arago-extras/recipes-bsp/ti-rm/ti-rm_git.bb
> @@ -0,0 +1,53 @@
> +DESCRIPTION = "TI Resource Manager Low Level Driver"
> +LICENSE = "TI BSD"
> +LIC_FILES_CHKSUM = "file://${WORKDIR}/git/ti/drv/rm/COPYING.txt;md5=dc61631b65360e6beb73b6c337800afc"
> +
> +BRANCH="master"
> +SRC_URI = "git://git.ti.com/keystone-rtos/rm-lld.git;destsuffix=git/ti/drv/rm;protocol=git;branch=${BRANCH}"
> +# Below commit ID corresponds to DEV.RM_LLD.02.00.00.08
> +SRCREV = "3a73cfe015214ff0401639f85fa5e52ea610e59d"
> +PR = "r0"
> +PV = "02.00.00.08"
> +
> +COMPATIBLE_MACHINE = "keystone"
> +
> +DEPENDS="ti-ipc"

1. It depends on ti-ipc, so will have to wait for that one to get merged 
first.
2. Not critical at all, but would be nice to have a single space before and 
after = sign... :)


> +PACKAGES =+ "${PN}-test"
> +
> +FILES_${PN}-test = "${bindir}/rmDspClientTest_*.out \
> +                    ${bindir}/rmLinuxClientTest_*.out \
> +                    ${bindir}/ti/drv/rm/test/dts_files/*.dtb"
> +
> +DEVICELIST = " k2h \
> +               k2k \
> +"
> +
> +CHOICELIST = " yes \
> +               no  \
> +"

Since you are using above 2 vars in a "for" loop, they can be simple one-line 
strings like "k2h k2k" and "yes no". But it's Ok to split them in multiple 
lines too - it's up to you.


> +BASEDIR = "${WORKDIR}/git"
> +S = "${BASEDIR}/ti/drv/rm"
> +
> +do_compile () {
> +#   Now build the lld in the updated directory	
> +    make -f makefile_armv7 clean lib PDK_INSTALL_PATH=${STAGING_INCDIR} RM_SRC_DIR=${S}
> +    for device in ${DEVICELIST}
> +    do
> +      for choice in ${CHOICELIST}
> +      do
> +        make -f makefile_armv7 tests IPC_DEVKIT_INSTALL_PATH=${STAGING_INCDIR} PDK_INSTALL_PATH=${BASEDIR} DEVICE="$device" USEDYNAMIC_LIB="$choice"
> +      done
> +    done	
> +}
> +
> +do_install () {
> +	install -d ${D}/${includedir}/ti/drv/rm

There seems to be an inconsistent indentation here - the above line uses tab, 
while other lines use spaces. Either way is fine, as long as they are 
consistent.


> +    install -d ${D}/${libdir}
> +    install -d ${D}/${bindir}	

There shouldn't be / between ${D} and ${libdir}. All the ${*dir} variables are 
defined from root, i.e. starting with / and hence you'd end up with double-/ 
in the code...


> +    for device in ${DEVICELIST}
> +    do	
> +      make -f makefile_armv7 install installbin installbin_test INSTALL_INC_BASE_DIR=${D}/${includedir} INSTALL_LIB_BASE_DIR=${D}/${libdir} INSTALL_BIN_BASE_DIR=${D}/${bindir} DEVICE="$device"

Same comment about ${D}/${*dir} as above.


> +    done	
> +}
> -- 
> 1.7.9.5
> 
> _______________________________________________
> meta-arago mailing list
> meta-arago@arago-project.org
> http://arago-project.org/cgi-bin/mailman/listinfo/meta-arago