Re: [PATCH] hyplnk-lld: Add new recipe for Hyperlink lld

[Denys Dmytriyenko <denys@ti.com>]

Sam,

This patch is not as clean as it could have been...

First of all, I see it's split into 2 separate recipes - one to build a 
library and another to build a test/example app using that library. While 
there are some valid reasons for such split, like when one component builds 
both kernel and user-space parts, or when half of the component can be 
replaced by a different provider; it is probably not the best approach in this 
case. It would be better to have a single recipe that builds everything and 
then packages its components into separate binary packages. Again, unless you 
envision building a test/example app w/o building its library, due to a 
replacement option. Or, if test/example app takes very long to build and is 
an optional piece and not being built by default... Please provide some more 
details here.

Next, I understand that you have to work with the provided Makefiles as is, 
but usually for things like building the dynamic and static versions of the 
library one would use separate Makefile targets and not the variable. That way 
only dynamic, only static, or both can be done in one command, instead of 
calling the same command twice with the variable set to "yes" or "no" - not 
very effective.

Also, please try to follow OpenEmbedded coding guidelines, like using spaces 
around = sign. I.e. VAR = "VAL" instead of VAR="VAL". I understand it's a 
nitpick and not critical, but still.

And since I'm not commenting inline, please be specific when using Bitbake 
variables vs. shell variables. I.e. in for-loops below, it should be $device 
and $choice instead of ${device} and ${choice}. In this case those are shell 
variables and there is no reason to try to confuse Bitbake, although it would 
fail to resolve them and leave as is to pick by shell...

One more - HYPLNK_SRC_DIR is not being passed to all make targets and when 
it's passed, it's sometimes ${S}, but sometimes ${PWD}. Please check the code 
and clean it up.

BTW, what does PDK_INSTALL_PATH variable specify? Is there any other 
dependency besides common-csl-ip?

Thanks for your submission! Please work with me and we'll get it cleared 
quickly. Thanks.

-- 
Denys


On Wed, Oct 08, 2014 at 12:23:49PM -0400, Sam Nelson wrote:
> - Provides low level driver for Hyperlink module
> - Test recipe is used to build tests and examples using
>   the hyperlink library.
> - Supports k2h, k2k & k2e
> 
> Signed-off-by: Sam Nelson <sam.nelson@ti.com>
> ---
>  recipes-bsp/hyplnk-lld/hyplnk-lld-test_git.bb |   25 +++++++++++++++++++++++++
>  recipes-bsp/hyplnk-lld/hyplnk-lld.inc         |   24 ++++++++++++++++++++++++
>  recipes-bsp/hyplnk-lld/hyplnk-lld_git.bb      |   18 ++++++++++++++++++
>  3 files changed, 67 insertions(+)
>  create mode 100755 recipes-bsp/hyplnk-lld/hyplnk-lld-test_git.bb
>  create mode 100644 recipes-bsp/hyplnk-lld/hyplnk-lld.inc
>  create mode 100755 recipes-bsp/hyplnk-lld/hyplnk-lld_git.bb
> 
> diff --git a/recipes-bsp/hyplnk-lld/hyplnk-lld-test_git.bb b/recipes-bsp/hyplnk-lld/hyplnk-lld-test_git.bb
> new file mode 100755
> index 0000000..49d11f0
> --- /dev/null
> +++ b/recipes-bsp/hyplnk-lld/hyplnk-lld-test_git.bb
> @@ -0,0 +1,25 @@
> +include hyplnk-lld.inc
> +
> +DEPENDS="common-csl-ip hyplnk-lld"
> +
> +CHOICELIST = " yes \
> +               no  \
> +"
> +
> +do_compile () {
> +	make -f makefile_armv7 clean PDK_INSTALL_PATH=${STAGING_INCDIR} HYPLNK_SRC_DIR=${PWD}
> +	for device in ${DEVICELIST}
> +	do
> +		for choice in ${CHOICELIST}
> +		do
> +			make -f makefile_armv7 tests examples PDK_INSTALL_PATH=${STAGING_INCDIR} DEVICE="${device}" HYPLNK_SRC_DIR=${S} USEDYNAMIC_LIB="${choice}"
> +		done
> +	done
> +}
> +
> +do_install () {
> +    for device in ${DEVICELIST}
> +	do
> +	    make -f makefile_armv7 installbin PDK_INSTALL_PATH=${STAGING_INCDIR} DEVICE="${device}" HYPLNK_SRC_DIR=${S} INSTALL_BIN_BASE_DIR=${D}/${bindir}
> +	done
> +}
> diff --git a/recipes-bsp/hyplnk-lld/hyplnk-lld.inc b/recipes-bsp/hyplnk-lld/hyplnk-lld.inc
> new file mode 100644
> index 0000000..4c8bb32
> --- /dev/null
> +++ b/recipes-bsp/hyplnk-lld/hyplnk-lld.inc
> @@ -0,0 +1,24 @@
> +DESCRIPTION = "TI Hyperlink Low Level Driver"
> +LICENSE = "BSD-3-Clause"
> +
> +COMPATIBLE_MACHINE = "keystone"
> +
> +LLDNAME="hyplnk"
> +
> +LIC_FILES_CHKSUM = "file://${WORKDIR}/git/ti/drv/${LLDNAME}/COPYING.txt;md5=623325cc19e613a4e770fbb749922592"
> +
> +BRANCH="master"
> +SRC_URI = "git://git.ti.com/keystone-rtos/hyplnk-lld.git;destsuffix=git/ti/drv/${LLDNAME};protocol=git;branch=${BRANCH}"
> +# Following commit corresponds to tag DEV.HYPLNK_LLD.02.01.00.01
> +SRCREV = "6910da379501984ecf27f8d23ba6fc6310fe387e"
> +
> +PV = "2.1.0"
> +PR = "r0"
> +
> +DEVICELIST = " k2h \
> +               k2k \
> +               k2e \
> +"
> +
> +BASEDIR = "${WORKDIR}/git"
> +S = "${BASEDIR}/ti/drv/${LLDNAME}"
> diff --git a/recipes-bsp/hyplnk-lld/hyplnk-lld_git.bb b/recipes-bsp/hyplnk-lld/hyplnk-lld_git.bb
> new file mode 100755
> index 0000000..3b9bc8a
> --- /dev/null
> +++ b/recipes-bsp/hyplnk-lld/hyplnk-lld_git.bb
> @@ -0,0 +1,18 @@
> +include hyplnk-lld.inc
> +
> +DEPENDS="common-csl-ip"
> +
> +CHOICELIST = " yes \
> +               no  \
> +"
> +do_compile () {
> +	make -f makefile_armv7 clean PDK_INSTALL_PATH=${STAGING_INCDIR} HYPLNK_SRC_DIR=${PWD}
> +	for device in ${DEVICELIST}
> +	do
> +		make -f makefile_armv7 lib PDK_INSTALL_PATH=${STAGING_INCDIR}  DEVICE="${device}"
> +	done
> +}
> +
> +do_install () {
> +	make -f makefile_armv7 install PDK_INSTALL_PATH=${STAGING_INCDIR} INSTALL_INC_BASE_DIR=${D}/${includedir} INSTALL_LIB_BASE_DIR=${D}${libdir}
> +}
> -- 
> 1.7.9.5
> 
> -- 
> _______________________________________________
> meta-ti mailing list
> meta-ti@yoctoproject.org
> https://lists.yoctoproject.org/listinfo/meta-ti