Thanks Denys, I implemented all of your minor comments and sent the next patch for review.
-Best Regards, Aravind > -----Original Message----- > From: Dmytriyenko, Denys > Sent: Wednesday, February 26, 2014 5:12 PM > To: Aravind Batni > Cc: [email protected] > Subject: Re: [meta-arago] [PATCH v2] ti-rm: provides ti resouce manager > recipe for KeyStone devices > > On Fri, Feb 21, 2014 at 08:07:41PM -0500, Aravind Batni wrote: > > - TI Resource Manager Low Level Driver > > > > Signed-off-by: Aravind Batni <[email protected]> > > > > --- > > 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=dc61631b65360e6beb73 > b6c337800afc" > > + > > +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 > > [email protected] > > http://arago-project.org/cgi-bin/mailman/listinfo/meta-arago _______________________________________________ meta-arago mailing list [email protected] http://arago-project.org/cgi-bin/mailman/listinfo/meta-arago
