usb: phy: msm: make it depend on RESET_CONTROLLER

Message ID 1399993327-12543-1-git-send-email-balbi@ti.com
State New
Headers show

Commit Message

Felipe Balbi May 13, 2014, 3:02 p.m.
drivers/reset/core.c does not provide an empty
stub for cases when CONFIG_RESET_CONTROLLER isn't
enabled, which might break build of the MSM PHY
driver if that driver is enabled but
CONFIG_RESET_CONTROLLER isn't.

We make the driver depend on CONFIG_RESET_CONTROLLER
so we can never have such a case

Cc: Ivan T. Ivanov <iivanov@mm-sol.com>
Cc: Tim Bird <tim.bird@sonymobile.com>
Reported-by: Jim Davis <jim.epost@gmail.com>
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/usb/phy/Kconfig | 1 +
 1 file changed, 1 insertion(+)

Comments

Alan Stern May 13, 2014, 3:13 p.m. | #1
On Tue, 13 May 2014, Felipe Balbi wrote:

> drivers/reset/core.c does not provide an empty
> stub for cases when CONFIG_RESET_CONTROLLER isn't
> enabled, which might break build of the MSM PHY
> driver if that driver is enabled but
> CONFIG_RESET_CONTROLLER isn't.
> 
> We make the driver depend on CONFIG_RESET_CONTROLLER
> so we can never have such a case

This may be a dumb comment...  Would it be better to add the 
appropriate stub routines instead?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi May 13, 2014, 3:30 p.m. | #2
On Tue, May 13, 2014 at 11:13:58AM -0400, Alan Stern wrote:
> On Tue, 13 May 2014, Felipe Balbi wrote:
> 
> > drivers/reset/core.c does not provide an empty
> > stub for cases when CONFIG_RESET_CONTROLLER isn't
> > enabled, which might break build of the MSM PHY
> > driver if that driver is enabled but
> > CONFIG_RESET_CONTROLLER isn't.
> > 
> > We make the driver depend on CONFIG_RESET_CONTROLLER
> > so we can never have such a case
> 
> This may be a dumb comment...  Would it be better to add the 
> appropriate stub routines instead?

maybe...
Bird, Tim May 13, 2014, 3:49 p.m. | #3
On Tuesday, May 13, 2014 8:30 AM, Felipe Balbi wrote:
> 
> On Tue, May 13, 2014 at 11:13:58AM -0400, Alan Stern wrote:
> > On Tue, 13 May 2014, Felipe Balbi wrote:
> >
> > > drivers/reset/core.c does not provide an empty
> > > stub for cases when CONFIG_RESET_CONTROLLER isn't
> > > enabled, which might break build of the MSM PHY
> > > driver if that driver is enabled but
> > > CONFIG_RESET_CONTROLLER isn't.
> > >
> > > We make the driver depend on CONFIG_RESET_CONTROLLER
> > > so we can never have such a case
> >
> > This may be a dumb comment...  Would it be better to add the
> > appropriate stub routines instead?
> 
> maybe...

I don't think so.  I haven't tested, but I believe the driver now relies
on operational (not stub) resets.  I think this comment is a bit off.
I think it would be better to just say that the driver now depends on the
reset sub-system, and thus needs the config dependency.

Maybe there's a case where the driver works fine even without
the resets, but if so it's not obvious to me.

Out of curiosity - is this something that many sub-systems do:
provide stubs for compilation, and support operating with or without
real functionality.  This seems kind of odd.

I admit to being a bit sensitive about this, since I got bit by
a situation where the driver relied on the firmware to get the hardware
into a proper state. :-)
 -- Tim
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi May 13, 2014, 3:52 p.m. | #4
On Tue, May 13, 2014 at 05:49:06PM +0200, Bird, Tim wrote:
> On Tuesday, May 13, 2014 8:30 AM, Felipe Balbi wrote:
> > 
> > On Tue, May 13, 2014 at 11:13:58AM -0400, Alan Stern wrote:
> > > On Tue, 13 May 2014, Felipe Balbi wrote:
> > >
> > > > drivers/reset/core.c does not provide an empty
> > > > stub for cases when CONFIG_RESET_CONTROLLER isn't
> > > > enabled, which might break build of the MSM PHY
> > > > driver if that driver is enabled but
> > > > CONFIG_RESET_CONTROLLER isn't.
> > > >
> > > > We make the driver depend on CONFIG_RESET_CONTROLLER
> > > > so we can never have such a case
> > >
> > > This may be a dumb comment...  Would it be better to add the
> > > appropriate stub routines instead?
> > 
> > maybe...
> 
> I don't think so.  I haven't tested, but I believe the driver now relies
> on operational (not stub) resets.  I think this comment is a bit off.
> I think it would be better to just say that the driver now depends on the
> reset sub-system, and thus needs the config dependency.
> 
> Maybe there's a case where the driver works fine even without
> the resets, but if so it's not obvious to me.
> 
> Out of curiosity - is this something that many sub-systems do:
> provide stubs for compilation, and support operating with or without
> real functionality.  This seems kind of odd.

heh, stubs are only stubs... they're there so the driver can still build
otherwise every module's dependency list would be gigantic. IOW, stubs
are only for randconfig's sake.
Alan Stern May 13, 2014, 4:07 p.m. | #5
On Tue, 13 May 2014, Bird, Tim wrote:

> I don't think so.  I haven't tested, but I believe the driver now relies
> on operational (not stub) resets.  I think this comment is a bit off.
> I think it would be better to just say that the driver now depends on the
> reset sub-system, and thus needs the config dependency.
> 
> Maybe there's a case where the driver works fine even without
> the resets, but if so it's not obvious to me.
> 
> Out of curiosity - is this something that many sub-systems do:
> provide stubs for compilation, and support operating with or without
> real functionality.  This seems kind of odd.

Some subsystems do it when the functionality they provide is optional.  
Runtime PM is a good example.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan T. Ivanov May 14, 2014, 6:57 a.m. | #6
On Tue, 2014-05-13 at 12:07 -0400, Alan Stern wrote:
> On Tue, 13 May 2014, Bird, Tim wrote:
> 
> > I don't think so.  I haven't tested, but I believe the driver now relies
> > on operational (not stub) resets.  I think this comment is a bit off.
> > I think it would be better to just say that the driver now depends on the
> > reset sub-system, and thus needs the config dependency.
> > 
> > Maybe there's a case where the driver works fine even without
> > the resets, but if so it's not obvious to me.
> > 
> > Out of curiosity - is this something that many sub-systems do:
> > provide stubs for compilation, and support operating with or without
> > real functionality.  This seems kind of odd.
> 
> Some subsystems do it when the functionality they provide is optional.  
> Runtime PM is a good example.

If I could add. clk and regulator frameworks also provide stub API's,
why reset framework should be different? Probably none of the drivers,
which use clk or regulator, did not have dependency to these frameworks. 
I am pretty sure that these drivers will not work properly without
proper back end implementation.

Regards,
Ivan


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index fbbced8..ec531a4 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -173,6 +173,7 @@  config USB_ISP1301
 config USB_MSM_OTG
 	tristate "Qualcomm on-chip USB OTG controller support"
 	depends on (USB || USB_GADGET) && (ARCH_MSM || ARCH_QCOM || COMPILE_TEST)
+	depends on RESET_CONTROLLER
 	select USB_PHY
 	help
 	  Enable this to support the USB OTG transceiver on Qualcomm chips. It