On Wed, Aug 07, 2013 at 03:00:05PM +0800, Peter Chen wrote: > On Wed, Aug 07, 2013 at 10:15:22AM +0200, Sascha Hauer wrote: > > On Wed, Aug 07, 2013 at 01:34:37PM +0800, Peter Chen wrote: > > > At former design, both ci_hdrc_imx and usbmisc_imx are individual > > > module, ci_hdrc_imx is glue layer for imx usb driver. usbmisc_imx > > > handles non-core registers which has different register layout > > > for imx SoC serials, it usually supplies interface for wakeup > > > setting, PHY setting, etc. > > > > > > usbmisc_imx uses symbols from ci_hdrc_imx, So, when we combile both > > > of drivers as loadable module, usbmisc is needed to be unload first. > > > However at ci_hdrc_imx, it needs to call usbmisc_imx's interface > > > at its removal function once we enable wakeup/runtime pm function, so > > > keeping two drivers can't work well at loadable module use case. > > > > > > To fix loadable module use case, we need to build usbmisc_imx into > > > ci_hdrc_imx. The usbmisc_imx still handles misc setting for controllers, > > > and will be included at ci_hdrc_imx. > > > > > > There is a short discussion for it: > > > http://marc.info/?l=linux-usb&m=136861599423172&w=2 > > > > > > Besides, we update dts and binding doc for at this commit > > > > This patch does far too many things at once. > > > > - Convert imx-usb-misc from a driver into something which is called > > directly > > - add aliases > > - change devicetree bindings (which causes pain and it's not explained why > > this is necessary at all) > > Yes, I need to split doc from implementation. No, you don't. You just have to explain *why* the bindings need to be chaged. > > > - converts the misc stuff to regmap > > > > Please split this up so that it can be reviewed. > > The changes are all related to convert usbmisc_imx from a driver to something > which can be called directly. > > - usbmisc has #index-cells to indicate controller number, now, it is not > a driver, we use aliases at controller driver to instead of it. Why? > - When usbmisc_imx is a device, the register maps only one time. > But ci_hdrc_imx will be several devices, the non-core register > will be mapped several times, we have to use syscon to visit one register > region among several devices. Converting the imx-usb misc driver to regmap is fine, but I see no reason to not make this a separate patch. This would make this much easier to read. > > > > > > > > > > #include "ci.h" > > > #include "ci_hdrc_imx.h" > > > +#include "usbmisc_imx.c" > > > > Don't include C files. > > > > Yes, it is not a good practice, but I want to keep ci_hdrc_imx.c > as clean as possible. The SoC related implementation will be more > and more in the future after we add more USB functions. Just because you want to have the binary code in a single module doesn't mean the source code has to be in a single C file. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
