[Buildroot] [PATCH 1/3] fmlib: new package

[Matthew Weber <matthew.weber@rockwellcollins.com>]

Thomas, thanks for the review.

On Tue, Jun 10, 2014 at 3:01 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Matt Weber,
>
> On Tue, 10 Jun 2014 08:02:04 -0700, Matt Weber wrote:
>
>> diff --git a/package/fmlib/Config.in b/package/fmlib/Config.in
>> new file mode 100644
>> index 0000000..43f7aa3
>> --- /dev/null
>> +++ b/package/fmlib/Config.in
>> @@ -0,0 +1,9 @@
>> +config BR2_PACKAGE_FMLIB
>> +     bool "fmlib"
>> +     depends on BR2_powerpc_e500mc
>
> If your package has 'linux' in its dependencies, then it should
> 'depends on BR2_LINUX_KERNEL' here, and have a comment about this. See
> package/freescale-imx/imx-lib/Config.in for an example.
Agree.

>
>> +     help
>> +       The Frame Manager library provides an API on top of the Frame Manager driver
>> +       ioctl calls, that provides a user space application with a simple way
>> +       to configure driver parameters and PCD (parse - classify - distribute) rules.
>
> I think the lines are too long here. Wrap to 72 max.
Agree.

>
>> +FMLIB_VERSION = fsl-sdk-v1.5-rc3
>> +FMLIB_SITE = git://git.freescale.com/ppc/sdk/fmlib.git
>> +FMLIB_LICENSE = BSD-c3
>
> We actually use BSD-3c and not BSD-c3. Also, the license should
> actually be:
>
>         BSD-3c or GPLv2+
Good catch.

>
> See the COPYING file of the project for details.
>
>> +FMLIB_LICENSE_FILES = COPYING
>> +FMLIB_DEPENDENCIES = linux
>> +FMLIB_INSTALL_STAGING = YES
>> +FMLIB_INSTALL_TARGET = NO
>
> Please add a comment above this like:
>
> # This package installs a static library only, so there's nothing to
> # install to the target
Sure.

>
>> +FMLIB_MAKE_OPTS = \
>> +     CC="$(TARGET_CC)" \
>> +     CROSS_COMPILE="$(TARGET_CROSS)" \
>> +     KERNEL_SRC="$(LINUX_DIR)" \
>> +     PREFIX="$(STAGING_DIR)/usr"
>> +
>> +ifeq ($(BR2_powerpc_e500mc), y)
>
> No space between , and y
Sure.

>
>> +     FMLIB_ARCHTYPE = ppce500mc
>> +endif
>
> That being said, since the package can only be enabled on ppce500mc, do
> we really need this condition? Or maybe you're making the thing a bit
> more future proof to support the other variants?
(future proof)  I don't have the necessary target configurations to
test more then the
ppce500mc configuration at this point.  However it is capable of
building a number
of ppc and ppc64 libs.

<snip>

-- 
Matthew L Weber / Sr Software Engineer / Platform Software
matthew.weber at rockwellcollins.com
www.rockwellcollins.com