[PATCH linux] FSI: Initial stubs for device driver

[Joel Stanley]

Hi Chris,

Thanks for the patch, it's a good first step.

In the future you will need to use a real SMTP server with git
send-email to submit your patches. Notes doesn't quite do the trick.
I'll ping you with some suggestions.

On Wed, Jun 22, 2016 at 12:51 AM, Christopher Bostic <cbostic at us.ibm.com> wrote:
>
> From: Christopher Bostic <cbostic at us.ibm.com>
> Date: Thu, 9 Jun 2016 13:42:59 -0500
>
> Initial FSI device driver stubs

When writing commit messages it's convention to indicate the area of
the kernel that they touch. In this case you might do:

drivers/fsi: FSI device driver stubs

Then you want some description of what the patches do. Perhaps you
could describe what FSI is, what it does and why we want it in the
kernel.

Finally, the commit message must carry a developers certificate of
origin. You can read about that here:

 http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n409

On to the code.

>
> ---
> drivers/Makefile      |  1 +
> drivers/fsi/Makefile  |  5 +++++
> drivers/fsi/fsiinit.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/fsi/fsiinit.h | 45 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 105 insertions(+)
> create mode 100644 drivers/fsi/Makefile
> create mode 100644 drivers/fsi/fsiinit.c
> create mode 100644 drivers/fsi/fsiinit.h
>
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 795d0ca..8ea7c58 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -172,3 +172,4 @@ obj-$(CONFIG_STM)        += hwtracing/stm/
> obj-$(CONFIG_ANDROID)        += android/
> obj-$(CONFIG_NVMEM)        += nvmem/
> obj-$(CONFIG_FPGA)        += fpga/
> +obj-$(CONFIG_FSI)        += fsi/
> diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
> new file mode 100644
> index 0000000..f547c08
> --- /dev/null
> +++ b/drivers/fsi/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for the FSI bus specific drivers.
> +#
> +
> +obj-y        += fsiinit.o
> diff --git a/drivers/fsi/fsiinit.c b/drivers/fsi/fsiinit.c
> new file mode 100644
> index 0000000..d7902d8
> --- /dev/null
> +++ b/drivers/fsi/fsiinit.c
> @@ -0,0 +1,54 @@
> +/*
> + * Copyright (c) International Business Machines Corp., 2012
> + *
> + * FSI Master device driver
> + *
> + * Thomas Richter, IBM Boeblingen, Germany, 17-Jun-2010
> + * Christopher Bostic, IBM Austin, TX  2016
> + *
> + */

Take a look at some of the modern files contributed by IMBers to the
kernel for how to strucutre the copyright and licence declerations.

eg. http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/platforms/powernv/opal.c

It's up to the developer to put their name and email address at the
top of the file. Some developers steer clear as the information can
become outdated over time, and it's contained in the git commit logs
anyway. If you do chose to keep it in I'd perhaps put names and email
addresses, instead of work locations.

> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include "fsiinit.h"
> +
> +MODULE_AUTHOR("Christopher Bostic cbostic at us.ibm.com");
> +MODULE_DESCRIPTION("FSI master device driver");
> +
> +#define FSIDD_MAJOR    0        /* FSI device driver major */
> +#define    FSIDD_TOSTR(x)    #x
> +#define    FSIDD_VERNO    4000
> +#define    FSIDD_VER(x)    FSIDD_TOSTR(x)
> +
> +struct fsidd fsidd = {        /* FSI device driver structure definition */
> +    .magic = FSI_DD_MAGIC,
> +    .strno = FSI_DD_STRNO,
> +};
> +
> +
> +static int fsi_start(void)
> +{
> +    int rc = 0;
> +
> +
> +    printk("FSI DD %s installation ok\n", fsidd.vstr);
> +    return rc;
> +}
> +
> +static void fsi_exit(void)
> +{
> +
> +}
> +
> +static int __init fsi_init(void)
> +{
> +    int rc = 0;
> +
> +    /* Set up the host controller */
> +    fsi_start();
> +    return rc;
> +}

These stubs are very minimal. Hard to procide much feedback at this point :)

> +
> +module_init(fsi_init);
> +module_exit(fsi_exit);
> +
> diff --git a/drivers/fsi/fsiinit.h b/drivers/fsi/fsiinit.h
> new file mode 100644
> index 0000000..2383610
> --- /dev/null
> +++ b/drivers/fsi/fsiinit.h
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright (c) International Business Machines Corp., 2010
> + *
> + * FSI Master device driver
> + *
> + * Thomas Richter, IBM Boeblingen, Germany, Jun-2010
> + * Christopher Bostic, IBM Austin, TX 2013

Similar to the comments above for the copyright and licence headers.

> + *
> + *
> + * Structure definitions and defines.
> + */
> +
> +#ifndef DRIVERS_FSIINIT_H
> +#define DRIVERS_FSIINIT_H
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/kobject.h>
> +#include <linux/workqueue.h>
> +#include <linux/hrtimer.h>
> +
> +#define VSTRLEN        32        /* Version string length */
> +
> +enum {
> +    FSI_DD_MAGIC = 0x64644632,    /* ddF2 */
> +    FSI_DD_STRNO = 0x1        /* Structure version number */
> +};

These arent an enumeration of values. Make them #defines.

> +
> +struct fsidd {                /* FSI Main structure */
> +    unsigned long magic;        /* Magic number */
> +    unsigned long strno;        /* Structure version number */

What purpose does this serve?

> +    char vstr[VSTRLEN];        /* Version string */

What's the version string used for? Is it really necessary?

> +    dev_t major;            /* Major number of device */
> +    struct workqueue_struct *hotp_wq;    /* Hot plug work queue */
> +    wait_queue_head_t error_wq;    /* Wait queue for port errors */
> +    wait_queue_head_t lbus_wq;    /* Wait queue for lbus events */
> +    wait_queue_head_t irq_wq;    /* Wait queue for IRQ loops */
> +    wait_queue_head_t link_wq;    /* Wait queue for link changes */
> +    unsigned long state;        /* State driver is in */
> +    struct kobject kobj;        /* Anchor point in /sys/kernel */

I would expect the structure to contain a struct device, which itself
contains a struct kobject.

I assume this code will create a struct bus_type for fsi?

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-model/bus.txt

Cheers,

Joel