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

[Lee Jones]

On Mon, 01 Sep 2014, Octavian Purdila wrote:

> On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > On Mon, 01 Sep 2014, Octavian Purdila wrote:
> >
> >> On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> >> > On Mon, 01 Sep 2014, Octavian Purdila wrote:
> >> >
> >> >> On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> >> >> > On Sat, 30 Aug 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.
> >> >> >>
> >> >> >> 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
> >> >> >
> >> >> > MFD is not a dumping ground for misfit h/w.  Almost all of this code
> >> >> > looks like it belongs in drivers/usb.  Please move it there.
> >> >> >
> >> >>
> >> >> We initially submitted this driver as a pure USB driver, with our own
> >> >> module registration mechanism, but during the first round of reviews
> >> >> people pointed out that a MFD driver is the better approach, and I
> >> >> agree. I also see that there are already a couple of USB drivers
> >> >> implemented as MFD drivers.
> >> >
> >> > Can you link me to your previous submission please?
> >>
> >> Sure, here it is:
> >>
> >> https://lkml.org/lkml/2014/8/20/228
> >>
> >> >
> >> >> Do you see a better approach?
> >> >
> >> > You should have a small MFD driver which controls resources and
> >> > registers children.  All other functionality should live in their
> >> > respective drivers/X locations i.e. USB functionallity should normally
> >> > live in drivers/usb.
> >> >
> >>
> >> OK, that sounds better. I am not sure how to handle the registration
> >> part though, since in this case we need to create the children at
> >> runtime, from the usb probe routine.
> >>
> >> The only solution I see is to move the driver completely to
> >> usb/drivers and continue to use the MFD infrastructure. Does that
> >> sound OK to you?
> >
> > I have no problem with that.  If this is an MFD driver, it _should_
> > live in drivers/mfd.  However, all of that USB specific stuff
> > defiantly should not.
> >
> 
> It is a multi-function driver which is using the USB interface, so I
> am not sure where it belongs. The only driver that calls
> mfd_add_devices and is not in drivers/mfd is the hid sensor hub
> driver.
> 
> BTW, the mfd/viperboard.c driver is very similar with this driver. It
> has less USB specific stuff because the protocol is simpler, but still
> has some.

Looking at viperboard.c, it seems to use some basic generic framework
calls to obtain some information about the device information like
version numbers.  Your driver is leaps and bounds more USB centric.

Your MFD driver should know about things like; regmap, platform data,
memory allocation, same-chip devices (children), etc.  Your MFD driver
should not need to concern itself with; endpoints, slots, URBs, USB
device IDs and the like.  The later knowledge belongs in drivers/usb. 

You should be calling mfd_add_devices() from within the MFD driver.
At a guess, I would say that you need a new entry for the USB stuff in
your mfd_cells structure.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/