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

[Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Mon, 01 Sep 2014, Octavian Purdila wrote:
> >
> >> On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >> > On Mon, 01 Sep 2014, Octavian Purdila wrote:
> >> >> On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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.

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).

Johan