On Mon, Sep 1, 2014 at 6:46 PM, Lee Jones <lee.jones@xxxxxxxxxx> wrote: > 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. > Makes sense, thanks for making clearing up what the MFD part of the driver should do. Here is how I think it could work: * keep the usb probe routine in the MFD driver (and keep it a usb driver) * add a new cell for the usb part * pass the usb_interface via platform_data to the USB sub-driver's platform_device probe routine and continue the USB setup there Lee, USB folks, is this acceptable? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
