Re: [v2,1/9] fsl/fman: Add the FMan FLIB

[Scott Wood <scottwood@freescale.com>]

On Wed, 2015-06-24 at 22:33 +0300, igal.liberman@freescale.com wrote:
> From: Igal Liberman <Igal.Liberman@freescale.com>
> 
> The FMan FLib provides the basic API used by the FMan drivers to
> > configure and control the FMan hardware.
> 
> Signed-off-by: Igal Liberman <Igal.Liberman@freescale.com>

Again, what is an FLib?  What determines whether content should go in 
the "flib" directory?

The patch title says "Add the FMan FLIB", but there's more code added 
outside the "flib" directory than inside.
 
"FMan drivers"?  There's more than one?  What is 
"drivers/net/ethernet/freescale/fman/fman.c" if not "the FMan driver"? 
 What is "the FMan driver" if not the code "to configure and control 
the FMan hardware"?  If this is a public API, where's the 
documentation?

---
>  drivers/net/ethernet/freescale/Kconfig             |    1 +
>  drivers/net/ethernet/freescale/Makefile            |    2 +
>  drivers/net/ethernet/freescale/fman/Kconfig        |    7 +
>  drivers/net/ethernet/freescale/fman/Makefile       |    5 +
>  .../net/ethernet/freescale/fman/flib/fsl_fman.h    |  608 
> ++++++++++++
>  drivers/net/ethernet/freescale/fman/fman.c         |  975 
> ++++++++++++++++++++
>  6 files changed, 1598 insertions(+)
>  create mode 100644 drivers/net/ethernet/freescale/fman/Kconfig
>  create mode 100644 drivers/net/ethernet/freescale/fman/Makefile
>  create mode 100644 
> drivers/net/ethernet/freescale/fman/flib/fsl_fman.h
>  create mode 100644 drivers/net/ethernet/freescale/fman/fman.c
> 
> diff --git a/drivers/net/ethernet/freescale/Kconfig 
> b/drivers/net/ethernet/freescale/Kconfig
> index 25e3425..24e938d 100644
> --- a/drivers/net/ethernet/freescale/Kconfig
> +++ b/drivers/net/ethernet/freescale/Kconfig
> @@ -55,6 +55,7 @@ config FEC_MPC52xx_MDIO
>         If compiled as module, it will be called fec_mpc52xx_phy.
>  
>  source "drivers/net/ethernet/freescale/fs_enet/Kconfig"
> +source "drivers/net/ethernet/freescale/fman/Kconfig"
>  
>  config FSL_PQ_MDIO
>       tristate "Freescale PQ MDIO"
> diff --git a/drivers/net/ethernet/freescale/Makefile 
> b/drivers/net/ethernet/freescale/Makefile
> index 71debd1..4097c58 100644
> --- a/drivers/net/ethernet/freescale/Makefile
> +++ b/drivers/net/ethernet/freescale/Makefile
> @@ -17,3 +17,5 @@ gianfar_driver-objs := gianfar.o \
>               gianfar_ethtool.o
>  obj-$(CONFIG_UCC_GETH) += ucc_geth_driver.o
>  ucc_geth_driver-objs := ucc_geth.o ucc_geth_ethtool.o
> +
> +obj-$(CONFIG_FSL_FMAN) += fman/
> diff --git a/drivers/net/ethernet/freescale/fman/Kconfig 
> b/drivers/net/ethernet/freescale/fman/Kconfig
> new file mode 100644
> index 0000000..8aeae29
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/fman/Kconfig
> @@ -0,0 +1,7 @@
> +config FSL_FMAN
> +     bool "FMan support"
> +     depends on FSL_SOC || COMPILE_TEST
> +     default n
> +     help
> +             Freescale Data-Path Acceleration Architecture Frame Manager
> +             (FMan) support

"default n" is a no-op.

What does enabling this option actually do, in terms of user-visible 
features?

> +typedef struct fm_prs_result_t fm_prs_result;
> +typedef enum e_enet_mode enet_mode_t;

This use of typedef is contrary to kernel coding style.

-Scott