Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Octavian Purdila]

On Wed, Sep 17, 2014 at 2:21 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
>
> On Tue, 09 Sep 2014, Octavian Purdila wrote:
>
> > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> > Master Adapter DLN-2. Details about the device can be found here:
> >
> > https://www.diolan.com/i2c/i2c_interface.html.
> >
> > Information about the USB protocol can be found in the Programmer's
> > Reference Manual [1], see section 1.7.
>
> This driver really needs a USB Ack before I can accept it.
>

Greg, Johan, is the driver acceptable now?

> > Because the hardware has a single transmit endpoint and a single
> > receive endpoint the communication between the various DLN2 drivers
> > and the hardware will be muxed/demuxed by this driver.
> >
> > Each DLN2 module will be identified by the handle field within the DLN2
> > message header. If a DLN2 module issues multiple commands in parallel
> > they will be identified by the echo counter field in the message header.
> >
> > The DLN2 modules can use the dln2_transfer() function to issue a
> > command and wait for its response. They can also register a callback
> > that is going to be called when a specific event id is generated by
> > the device (e.g. GPIO interrupts). The device uses handle 0 for
> > sending events.
> >
> > [1] https://www.diolan.com/downloads/dln-api-manual.pdf
> >
> > Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxxxx>
> > ---
> >  drivers/mfd/Kconfig      |   9 +
> >  drivers/mfd/Makefile     |   1 +
> >  drivers/mfd/dln2.c       | 681 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/dln2.h |  71 +++++
> >  4 files changed, 762 insertions(+)
> >  create mode 100644 drivers/mfd/dln2.c
> >  create mode 100644 include/linux/mfd/dln2.h
>
> [...]
>
> > diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> > new file mode 100644
> > index 0000000..b551b5e
> > --- /dev/null
> > +++ b/drivers/mfd/dln2.c
> > @@ -0,0 +1,681 @@
> > +/*
> > + * Driver for the Diolan DLN-2 USB adapter
> > + *
> > + * Copyright (c) 2014 Intel Corporation
> > + *
> > + * Derived from:
> > + *  i2c-diolan-u2c.c
> > + *  Copyright (c) 2010-2011 Ericsson AB
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation, version 2.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
>
> What are you using this for?
>

Not needed, I will remove it.

> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/dln2.h>
> > +
> > +#define DRIVER_NAME                  "usb-dln2"
>
> Don't do this, just use "usb-dln2" where it needs to be used.
>

Will do.

> [...]
>
> > +static struct usb_driver dln2_driver = {
> > +     .name = DRIVER_NAME,
> > +     .probe = dln2_probe,
> > +     .disconnect = dln2_disconnect,
> > +     .id_table = dln2_table,
> > +};
> > +
> > +module_usb_driver(dln2_driver);
> > +
> > +MODULE_AUTHOR("Octavian Purdila <octavian.purdila@xxxxxxxxx>");
> > +MODULE_DESCRIPTION(DRIVER_NAME " driver");
>
> This is not a description.
>
> > +MODULE_LICENSE("GPL");
>
> Header says v2.
>

I will change it to GPL v2.

> > diff --git a/include/linux/mfd/dln2.h b/include/linux/mfd/dln2.h
> > new file mode 100644
> > index 0000000..197565d
> > --- /dev/null
> > +++ b/include/linux/mfd/dln2.h
> > @@ -0,0 +1,71 @@
> > +#ifndef __LINUX_USB_DLN2_H
> > +#define __LINUX_USB_DLN2_H
> > +
> > +#define DLN2_CMD(cmd, id)            ((cmd) | ((id) << 8))
> > +
> > +struct dln2_platform_data {
> > +     u16 handle;
> > +     union {
> > +             struct {
> > +                     u8 port;
> > +             } i2c;
> > +     };
> > +};
>
> Why this complexity?
>

There is also an SPI interface on this adapter which will probably the
port information and maybe some additional information. Would you
prefer to add the union later, when we add the SPI driver?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/