On Mon, 01 Sep 2014, Johan Hovold wrote: > On Mon, Sep 01, 2014 at 07:22:39PM +0300, Octavian Purdila wrote: > > 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: > > > >> >> > 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? > > No, no. USB is not a function of the MFD device, it's the transport. > Thus there should be no USB MFD-cell. No subdriver can work without it. > > And the USB id belongs in the MFD-driver in the same way that an > i2c id (address) does. > > Just like an MFD device with i2c as a transport, this driver would > function as an arbiter to a shared resource (i.e. the register space). > The reason it seems much more USB-centric than an i2c-mfd driver is that > that transport API is simpler and some code have also already been > generalised (e.g. regmap), whereas we appear to have only two USB > mfd-drivers thus far. > > The viperboard is perhaps a bad example in so far that it has pushed the > transport details down into the subdrivers (and thus into gpio, i2c and > iio subsystems) instead of handling it one place. Thanks for your explanation. I take your point about the USB ID and I did say I was guessing that the USB part should exist as a child device. So after your comments I decided to do a little investigation. It appears that this MFD driver is _just_ using the common API which all other devices utilising USB comms are forced to use. Is that correct? If so, I have a question. Is there no way to hide more of the USB specifics inside a better, simpler API? It looks like the drivers which use USB are subjected to a lot (too much) of what might be considered internals. Or is it just that the client has to tinker with too many dials to get anything sensible out? *shudders* > I haven't looked at the details of the protocol for the device in > question, but it might even be possible to use regmap here (as I > mentioned in my comments on v1). Obviously that would be preferred. Octavian, did you look into that? -- 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-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
