Re: [PATCH 01/11] mfd: Eberspaecher Flexcard PMC II Carrier Board support

[Lee Jones <lee.jones@linaro.org>]

> From: Benedikt Spranger <b.spranger@linutronix.de>
> 
> The Eberspaecher Flexcard PMC II is a PMC (PCI Mezzanine Card) II
> carrier board. The carrier board can take up to 4 exchangeable physical
> layer boards for e.g. CAN, FlexRay or Ethernet.
> 
> Signed-off-by: Holger Dengler <dengler@linutronix.de>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> cc: Samuel Ortiz <sameo@linux.intel.com>
> cc: Lee Jones <lee.jones@linaro.org>

When you're providing a patch-set such as this, where all of the
patches spread across the various subsystems are related, it's better
to just send the whole thing to everyone.  Adding maintainers in this
way means that some of us are now missing come context.  More
importantly I'm missing [PATCH 00/11].

After looking the cover-letter up on the interweb, I notice this:

"According to the comments regarding our last posting, the MFD driver
patchset has been split up into separate functional parts."

Where did your last posting go?  I can't seem to look it up, either on
the MLs or via Google.  Who asked you to split up the MFD patches?  Do
you have a link to any previous conversation surrounding this set?

> ---
>  drivers/mfd/Kconfig           |  10 +++
>  drivers/mfd/Makefile          |   2 +
>  drivers/mfd/flexcard/Makefile |   2 +

This is worrying.  What makes flexcard special enough to require a
directory?  There are currently no other directories in drivers/mfd.

>  drivers/mfd/flexcard/core.c   | 193 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/flexcard.h  |  34 ++++++++
>  include/uapi/linux/flexcard.h | 125 +++++++++++++++++++++++++++
>  6 files changed, 366 insertions(+)
>  create mode 100644 drivers/mfd/flexcard/Makefile
>  create mode 100644 drivers/mfd/flexcard/core.c
>  create mode 100644 include/linux/mfd/flexcard.h
>  create mode 100644 include/uapi/linux/flexcard.h

[...]

> diff --git a/drivers/mfd/flexcard/core.c b/drivers/mfd/flexcard/core.c
> new file mode 100644
> index 0000000..99df3d5
> --- /dev/null
> +++ b/drivers/mfd/flexcard/core.c
> @@ -0,0 +1,193 @@
> +/*
> + * Eberspaecher Flexcard PMC II Carrier Board PCI Driver
> + *
> + * Copyright (c) 2014,2015 Linutronix GmbH
> + * Author: Holger Dengler
> + *         Benedikt Spranger

Full email addresses of the authors please.

> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/flexcard.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/flexcard.h>

Alphabetical.

> +static const char drv_name[] = "flexcard";

I've never really liked these.  Just use "flexcard" where required.

[...]

> +static int flexcard_tiny_probe(struct flexcard_device *priv)
> +{
> +	u32 fc_slic0 = priv->conf->fc_slic[0];
> +	struct pci_dev *pdev = priv->pdev;
> +	u8 nr_can, nr_fr, nr;
> +	u32 offset = 0;
> +	int i, ret;
> +
> +	nr_can = (fc_slic0 >> 4) & 0xf;
> +	nr_fr = fc_slic0 & 0xf;
> +	nr = nr_can + nr_fr;

You need to make this more easily/instantly readable.  Insert a
comment explaining what's happening and consider renaming the local
variable to something more friendly/explanatory.

[...]

> diff --git a/include/linux/mfd/flexcard.h b/include/linux/mfd/flexcard.h
> new file mode 100644
> index 0000000..20d0f40
> --- /dev/null
> +++ b/include/linux/mfd/flexcard.h
> @@ -0,0 +1,34 @@
> +/*
> + * Common Definitions for Eberspaecher Flexcard PMC II
> + *
> + * Copyright (c) 2014,2015 Linutronix GmbH
> + * Author: Holger Dengler
> + *         Benedikt Spranger
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#ifndef FLEXCARD_H
> +#define FLEXCARD_H
> +
> +#define FLEXCARD_CAN_OFFSET	0x2000
> +#define FLEXCARD_CAN_SIZE	0x2000
> +
> +#define FLEXCARD_FR_OFFSET	0x4000
> +#define FLEXCARD_FR_SIZE	0x2000

Are these used anywhere else except core.c?  If not, please move them
into there.  Same with any other variable/struct/define which is used
in only a single file.

> +struct flexcard_device {
> +	struct pci_dev *pdev;
> +	struct fc_conf_bar __iomem *conf;
> +	struct mfd_cell *cells;
> +	struct resource *res;
> +};
> +
> +enum flexcard_cell_id {
> +	FLEXCARD_CELL_CAN,
> +	FLEXCARD_CELL_FLEXRAY,
> +};
> +
> +#endif /* FLEXCARD_H */

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog