Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Wed, Sep 03, 2014 at 04:39:48PM +0300, Octavian Purdila wrote: > On Tue, Sep 2, 2014 at 6:23 PM, Johan Hovold wrote: > > That should be possible using the regmap bus read and write operations. > > I took a closer look on the regmap bus read/write operations and I > think they are not fit for what we need in the driver. The driver uses > a request/response model which, IMHO, does not fit well with a > register read/write API. Yes, maybe we can emulate it, but why do > that? > > >> (Also creating a regmap class for a particular device seems over > >> engineering since nobody else is going to use it) > > > > Possibly, but it would allow subdrivers to be implemented using a > > standard interface and also provide register caching for free. > > Using a standard interface is nice, but I think that using the right > interface type is more important. This hardware does not use registers > but a messages to communicate with the OS. You might be right, and as I mentioned, I haven't looked that closely at the protocol yet. I'll take a look at your updated I/O interface and how you use it. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 2, 2014 at 6:23 PM, Johan Hovold wrote: > On Tue, Sep 02, 2014 at 11:45:55AM +0300, Octavian Purdila wrote: >> On Tue, Sep 2, 2014 at 11:00 AM, Lee Jones wrote: >> > On Mon, 01 Sep 2014, Johan Hovold wrote: > >> >> 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? >> > >> Yes, I did. Since this is the first time I am looking at regmap I may >> be wrong but I don't see a way to use it. The dln2 i2c driver needs to >> be able to send and receive arbitrary size buffers and this does not >> seem possible to do with the regmap API. > > That should be possible using the regmap bus read and write operations. I took a closer look on the regmap bus read/write operations and I think they are not fit for what we need in the driver. The driver uses a request/response model which, IMHO, does not fit well with a register read/write API. Yes, maybe we can emulate it, but why do that? >> (Also creating a regmap class for a particular device seems over >> engineering since nobody else is going to use it) > > Possibly, but it would allow subdrivers to be implemented using a > standard interface and also provide register caching for free. > Using a standard interface is nice, but I think that using the right interface type is more important. This hardware does not use registers but a messages to communicate with the OS. Also caching is not useful for i2c and only partially useful for GPIO (to cache the pin direction, and pin status in output mode only) so its not a big win. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 2, 2014 at 6:23 PM, Johan Hovold jo...@kernel.org wrote: On Tue, Sep 02, 2014 at 11:45:55AM +0300, Octavian Purdila wrote: On Tue, Sep 2, 2014 at 11:00 AM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Johan Hovold wrote: 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? Yes, I did. Since this is the first time I am looking at regmap I may be wrong but I don't see a way to use it. The dln2 i2c driver needs to be able to send and receive arbitrary size buffers and this does not seem possible to do with the regmap API. That should be possible using the regmap bus read and write operations. I took a closer look on the regmap bus read/write operations and I think they are not fit for what we need in the driver. The driver uses a request/response model which, IMHO, does not fit well with a register read/write API. Yes, maybe we can emulate it, but why do that? (Also creating a regmap class for a particular device seems over engineering since nobody else is going to use it) Possibly, but it would allow subdrivers to be implemented using a standard interface and also provide register caching for free. Using a standard interface is nice, but I think that using the right interface type is more important. This hardware does not use registers but a messages to communicate with the OS. Also caching is not useful for i2c and only partially useful for GPIO (to cache the pin direction, and pin status in output mode only) so its not a big win. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Wed, Sep 03, 2014 at 04:39:48PM +0300, Octavian Purdila wrote: On Tue, Sep 2, 2014 at 6:23 PM, Johan Hovold jo...@kernel.org wrote: That should be possible using the regmap bus read and write operations. I took a closer look on the regmap bus read/write operations and I think they are not fit for what we need in the driver. The driver uses a request/response model which, IMHO, does not fit well with a register read/write API. Yes, maybe we can emulate it, but why do that? (Also creating a regmap class for a particular device seems over engineering since nobody else is going to use it) Possibly, but it would allow subdrivers to be implemented using a standard interface and also provide register caching for free. Using a standard interface is nice, but I think that using the right interface type is more important. This hardware does not use registers but a messages to communicate with the OS. You might be right, and as I mentioned, I haven't looked that closely at the protocol yet. I'll take a look at your updated I/O interface and how you use it. Johan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 02, 2014 at 11:45:55AM +0300, Octavian Purdila wrote: > On Tue, Sep 2, 2014 at 11:00 AM, Lee Jones wrote: > > On Mon, 01 Sep 2014, Johan Hovold wrote: > >> 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? > > > Yes, I did. Since this is the first time I am looking at regmap I may > be wrong but I don't see a way to use it. The dln2 i2c driver needs to > be able to send and receive arbitrary size buffers and this does not > seem possible to do with the regmap API. That should be possible using the regmap bus read and write operations. > (Also creating a regmap class for a particular device seems over > engineering since nobody else is going to use it) Possibly, but it would allow subdrivers to be implemented using a standard interface and also provide register caching for free. The event callbacks of the device in questions would not fit this scheme though, but perhaps only that part needs to be driver specific. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 02, 2014 at 09:00:10AM +0100, Lee Jones wrote: > On Mon, 01 Sep 2014, Johan Hovold wrote: > > 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? Yes, it's using the low-level USB API, but there's a lot of higher-level interfaces in place for (fairly) standard things such as the USB class drivers or the USB serial subsystem. > 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* Unfortunately, anything that does not already have a driver is likely to use some vendor-specific protocol and therefore must use the low-level API. > > 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. Simple register-based USB MFD devices (e.g. only using control transfers) are conceivable though, and if we start seeing a lot of those (which I doubt) perhaps that part could be refactored as a regmap bus. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 2, 2014 at 11:00 AM, Lee Jones wrote: > 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 wrote: >> > > On Mon, 01 Sep 2014, Octavian Purdila wrote: >> > > >> > >> On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones wrote: >> > >> > On Mon, 01 Sep 2014, Octavian Purdila wrote: >> > >> >> On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones >> > >> >> 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* > Yes, the driver is just using the common USB API to communicate with the device. It is more complicated then the average USB driver because there is a single interface and a single receive endpoint which needs to serve multiple drivers and requests, thus the need to multiplex the communication. >> I haven't looked at the details of the protocol for the device in >> question, but it might even be possible to use regmap
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
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 wrote: > > > On Mon, 01 Sep 2014, Octavian Purdila wrote: > > > > > >> On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones wrote: > > >> > On Mon, 01 Sep 2014, Octavian Purdila wrote: > > >> >> On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones > > >> >> 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
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.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones lee.jo...@linaro.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. 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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 2, 2014 at 11:00 AM, Lee Jones lee.jo...@linaro.org wrote: 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.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones lee.jo...@linaro.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. 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* Yes, the driver is just using the common USB API to communicate with the device. It is more complicated then the average USB driver because there is a single interface and a single receive endpoint which needs to serve multiple drivers and requests, thus the need to multiplex the communication. 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? Yes, I did. Since this is the first time I am looking at regmap I may be wrong but I don't see a way to use it. The dln2 i2c driver needs to be able to send and receive arbitrary
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 02, 2014 at 09:00:10AM +0100, Lee Jones wrote: On Mon, 01 Sep 2014, Johan Hovold wrote: 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? Yes, it's using the low-level USB API, but there's a lot of higher-level interfaces in place for (fairly) standard things such as the USB class drivers or the USB serial subsystem. 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* Unfortunately, anything that does not already have a driver is likely to use some vendor-specific protocol and therefore must use the low-level API. 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. Simple register-based USB MFD devices (e.g. only using control transfers) are conceivable though, and if we start seeing a lot of those (which I doubt) perhaps that part could be refactored as a regmap bus. Johan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Tue, Sep 02, 2014 at 11:45:55AM +0300, Octavian Purdila wrote: On Tue, Sep 2, 2014 at 11:00 AM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Johan Hovold wrote: 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? Yes, I did. Since this is the first time I am looking at regmap I may be wrong but I don't see a way to use it. The dln2 i2c driver needs to be able to send and receive arbitrary size buffers and this does not seem possible to do with the regmap API. That should be possible using the regmap bus read and write operations. (Also creating a regmap class for a particular device seems over engineering since nobody else is going to use it) Possibly, but it would allow subdrivers to be implemented using a standard interface and also provide register caching for free. The event callbacks of the device in questions would not fit this scheme though, but perhaps only that part needs to be driver specific. Johan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 01, 2014 at 07:22:39PM +0300, Octavian Purdila wrote: > On Mon, Sep 1, 2014 at 6:46 PM, Lee Jones wrote: > > On Mon, 01 Sep 2014, Octavian Purdila wrote: > > > >> On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones wrote: > >> > On Mon, 01 Sep 2014, Octavian Purdila wrote: > >> >> On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 1, 2014 at 6:46 PM, Lee Jones wrote: > On Mon, 01 Sep 2014, Octavian Purdila wrote: > >> On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones wrote: >> > On Mon, 01 Sep 2014, Octavian Purdila wrote: >> > >> >> On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones wrote: >> >> > On Mon, 01 Sep 2014, Octavian Purdila wrote: >> >> > >> >> >> On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones >> >> >> 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, 01 Sep 2014, Octavian Purdila wrote: > On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones wrote: > > On Mon, 01 Sep 2014, Octavian Purdila wrote: > > > >> On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones wrote: > >> > On Mon, 01 Sep 2014, Octavian Purdila wrote: > >> > > >> >> On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones wrote: > On Mon, 01 Sep 2014, Octavian Purdila wrote: > >> On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones wrote: >> > On Mon, 01 Sep 2014, Octavian Purdila wrote: >> > >> >> On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, 01 Sep 2014, Octavian Purdila wrote: > On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones wrote: > > On Mon, 01 Sep 2014, Octavian Purdila wrote: > > > >> On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones 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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones wrote: > On Mon, 01 Sep 2014, Octavian Purdila wrote: > >> On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones 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? Thanks, Tavi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, 01 Sep 2014, Octavian Purdila wrote: > On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones 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? > 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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones 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. > Hi Lee, 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. Do you see a better approach? Thanks, Tavi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones lee.jo...@linaro.org 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. Hi Lee, 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. Do you see a better approach? Thanks, Tavi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones lee.jo...@linaro.org 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? 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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones lee.jo...@linaro.org 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? Thanks, Tavi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones lee.jo...@linaro.org 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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones lee.jo...@linaro.org 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones lee.jo...@linaro.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
On Mon, Sep 1, 2014 at 6:46 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 11:37 AM, Lee Jones lee.jo...@linaro.org 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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] mfd: add support for Diolan DLN-2 devices
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.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 2:39 PM, Lee Jones lee.jo...@linaro.org wrote: On Mon, 01 Sep 2014, Octavian Purdila wrote: On Mon, Sep 1, 2014 at 12:51 PM, Lee Jones lee.jo...@linaro.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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
