[PATCH] ARM: OMAP: Fix warning in plat_uds.c

[PATCH] ARM: OMAP: Fix warning in plat_uds.c

[Dirk Behme]

[-- Attachment #1: Type: text/plain, Size: 197 bytes --]


Fix warning

drivers/usb/musb/plat_uds.c:1731: warning: 'dev_attr_srp' defined but 
not used

if CONFIG_USB_MUSB_OTG isn't set (DaVinci use case)

Signed-off-by: Dirk Behme <dirk.behme@gmail.com>

[-- Attachment #2: plat_uds_warning_fix.txt --]
[-- Type: text/plain, Size: 754 bytes --]

Index: linux-davinci/drivers/usb/musb/plat_uds.c
===================================================================
--- linux-davinci.orig/drivers/usb/musb/plat_uds.c
+++ linux-davinci/drivers/usb/musb/plat_uds.c
@@ -1707,6 +1707,7 @@ musb_vbus_show(struct device *dev, struc
 }
 static DEVICE_ATTR(vbus, 0644, musb_vbus_show, musb_vbus_store);
 
+#ifdef CONFIG_USB_MUSB_OTG
 static ssize_t
 musb_srp_store(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t n)
@@ -1728,8 +1729,11 @@ musb_srp_store(struct device *dev, struc
 
 	return n;
 }
+
+
 static DEVICE_ATTR(srp, 0644, NULL, musb_srp_store);
 #endif
+#endif
 
 /* Only used to provide cable state change events */
 static void musb_irq_work(struct work_struct *data)


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[Felipe Balbi]

Hi,

On 8/17/07, Dirk Behme <dirk.behme@googlemail.com> wrote:
>
> Fix warning
>
> drivers/usb/musb/plat_uds.c:1731: warning: 'dev_attr_srp' defined but
> not used
>
> if CONFIG_USB_MUSB_OTG isn't set (DaVinci use case)
>
> Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
>
> Index: linux-davinci/drivers/usb/musb/plat_uds.c
> ===================================================================
> --- linux-davinci.orig/drivers/usb/musb/plat_uds.c
> +++ linux-davinci/drivers/usb/musb/plat_uds.c
> @@ -1707,6 +1707,7 @@ musb_vbus_show(struct device *dev, struc
>  }
>  static DEVICE_ATTR(vbus, 0644, musb_vbus_show, musb_vbus_store);
>
> +#ifdef CONFIG_USB_MUSB_OTG
>  static ssize_t
>  musb_srp_store(struct device *dev, struct device_attribute *attr,
>                 const char *buf, size_t n)
> @@ -1728,8 +1729,11 @@ musb_srp_store(struct device *dev, struc
>
>         return n;
>  }
> +
> +
>  static DEVICE_ATTR(srp, 0644, NULL, musb_srp_store);
>  #endif
> +#endif

this doesn't look right since any device can issue SRP. It's better
for you to use it somewhere on DaVinci.

>
>  /* Only used to provide cable state change events */
>  static void musb_irq_work(struct work_struct *data)
>
>
> _______________________________________________
> Linux-omap-open-source mailing list
> Linux-omap-open-source@linux.omap.com
> http://linux.omap.com/mailman/listinfo/linux-omap-open-source
>
>


-- 
Best Regards,

Felipe Balbi
felipebalbi@users.sourceforge.net

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[Dirk Behme]

Felipe Balbi wrote:
> Hi,

Thanks for review!

> On 8/17/07, Dirk Behme <dirk.behme@googlemail.com> wrote:
> 
>>Fix warning
>>
>>drivers/usb/musb/plat_uds.c:1731: warning: 'dev_attr_srp' defined but
>>not used
>>
>>if CONFIG_USB_MUSB_OTG isn't set (DaVinci use case)
>>
>>Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
>>
>>Index: linux-davinci/drivers/usb/musb/plat_uds.c
>>===================================================================
>>--- linux-davinci.orig/drivers/usb/musb/plat_uds.c
>>+++ linux-davinci/drivers/usb/musb/plat_uds.c
>>@@ -1707,6 +1707,7 @@ musb_vbus_show(struct device *dev, struc
>> }
>> static DEVICE_ATTR(vbus, 0644, musb_vbus_show, musb_vbus_store);
>>
>>+#ifdef CONFIG_USB_MUSB_OTG
>> static ssize_t
>> musb_srp_store(struct device *dev, struct device_attribute *attr,
>>                const char *buf, size_t n)
>>@@ -1728,8 +1729,11 @@ musb_srp_store(struct device *dev, struc
>>
>>        return n;
>> }
>>+
>>+
>> static DEVICE_ATTR(srp, 0644, NULL, musb_srp_store);
>> #endif
>>+#endif
> 
> 
> this doesn't look right since any device can issue SRP. It's better
> for you to use it somewhere on DaVinci.

Sorry, if I missed anything. Looking to recent plat_uds.c of OMAP, 
dev_attr_srp is used at two places encapsulated by CONFIG_USB_MUSB_OTG:

#ifdef CONFIG_USB_MUSB_OTG
	status = device_create_file(dev, &dev_attr_srp);
#endif /* CONFIG_USB_MUSB_OTG */

#ifdef CONFIG_USB_MUSB_OTG
	device_remove_file(musb->controller, &dev_attr_srp);
#endif

I think that's the reason for warning

drivers/usb/musb/plat_uds.c:1731: warning: 'dev_attr_srp' defined but 
not used

if CONFIG_USB_MUSB_OTG isn't set? What do do you propose instead to 
get rid of this warning on OMAP?

Dirk

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[David Brownell]

> this doesn't look right since any device can issue SRP. It's better
> for you to use it somewhere on DaVinci.

That "issue SRP" attribute has no business being specific to any
particular platform.

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[Felipe Balbi]

Hi,

On 8/17/07, David Brownell <david-b@pacbell.net> wrote:
> > this doesn't look right since any device can issue SRP. It's better
> > for you to use it somewhere on DaVinci.
>
> That "issue SRP" attribute has no business being specific to any
> particular platform.
>

This is needed during USB OTG Compliance program... and it looks to me
really architecture dependent, if you have a better way for doing
this, please tell me and I'll try it.


-- 
Best Regards,

Felipe Balbi
felipebalbi@users.sourceforge.net

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[David Brownell]

On Friday 17 August 2007, Felipe Balbi wrote:
> Hi,
> 
> On 8/17/07, David Brownell <david-b@pacbell.net> wrote:
> > > this doesn't look right since any device can issue SRP. It's better
> > > for you to use it somewhere on DaVinci.
> >
> > That "issue SRP" attribute has no business being specific to any
> > particular platform.
> >
> 
> This is needed during USB OTG Compliance program... and it looks to me
> really architecture dependent, if you have a better way for doing
> this, please tell me and I'll try it.

Pure devices can issue SRP, hardware allowing.  So it's incorrect
to have this #ifdeffed for OTG.  That's in addition to the compiler
warnings you get when building with a pure host...

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[Felipe Balbi]

Hi,

On 8/17/07, David Brownell <david-b@pacbell.net> wrote:
> On Friday 17 August 2007, Felipe Balbi wrote:
> > Hi,
> >
> > On 8/17/07, David Brownell <david-b@pacbell.net> wrote:
> > > > this doesn't look right since any device can issue SRP. It's better
> > > > for you to use it somewhere on DaVinci.
> > >
> > > That "issue SRP" attribute has no business being specific to any
> > > particular platform.
> > >
> >
> > This is needed during USB OTG Compliance program... and it looks to me
> > really architecture dependent, if you have a better way for doing
> > this, please tell me and I'll try it.
>
> Pure devices can issue SRP, hardware allowing.  So it's incorrect
> to have this #ifdeffed for OTG.  That's in addition to the compiler
> warnings you get when building with a pure

Maybe my previous mail didn't get to list yet but I sent an email
telling Dirk to get rid of the ifdef CONFIG_USB_MUSB_OTG.

Does that seem better for you?


Thanks

>


-- 
Best Regards,

Felipe Balbi
felipebalbi@users.sourceforge.net

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[David Brownell]

On Friday 17 August 2007, Felipe Balbi wrote:
> Maybe my previous mail didn't get to list yet but I sent an email
> telling Dirk to get rid of the ifdef CONFIG_USB_MUSB_OTG.
> 
> Does that seem better for you?

Host-only systems shouldn't have that SRP support.

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[Dirk Behme]

David Brownell wrote:
> On Friday 17 August 2007, Felipe Balbi wrote:
> 
>>Maybe my previous mail didn't get to list yet but I sent an email
>>telling Dirk to get rid of the ifdef CONFIG_USB_MUSB_OTG.
>>
>>Does that seem better for you?
> 
> Host-only systems shouldn't have that SRP support.

Sorry for stupid question from a non-USB expert: But what is the 
proposed fix for this now? ;)

Regards

Dirk

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[David Brownell]

On Sunday 19 August 2007, Dirk Behme wrote:
> 
> Sorry for stupid question from a non-USB expert: But what is the 
> proposed fix for this now? ;)

Something not totally unlike the appended.  Build-tested only.

==============================================	CUT HERE
Fix various build warnings for peripheral-only builds, and some
linewrap bugs.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/usb/musb/musbdefs.h |    4 ++--
 drivers/usb/musb/plat_uds.c |   28 +++++++++++++++++-----------
 drivers/usb/musb/tusb6010.c |    4 ----
 3 files changed, 19 insertions(+), 17 deletions(-)

--- o26.orig/drivers/usb/musb/plat_uds.c	2007-08-20 00:28:38.000000000 -0700
+++ o26/drivers/usb/musb/plat_uds.c	2007-08-20 01:39:59.000000000 -0700
@@ -364,9 +364,7 @@ static irqreturn_t musb_stage0_irq(struc
 				u8 devctl, u8 power)
 {
 	irqreturn_t handled = IRQ_NONE;
-#ifdef CONFIG_USB_MUSB_HDRC_HCD
 	void __iomem *mbase = musb->mregs;
-#endif
 
 	DBG(3, "<== Power=%02x, DevCtl=%02x, int_usb=0x%x\n", power, devctl,
 		int_usb);
@@ -604,18 +602,18 @@ static irqreturn_t musb_stage0_irq(struc
 	 * only host sees babble; only peripheral sees bus reset.
 	 */
 	if (int_usb & MUSB_INTR_RESET) {
-		if (devctl & MUSB_DEVCTL_HM) {
+		if (is_host_capable() && (devctl & MUSB_DEVCTL_HM) != 0) {
 			/*
 			 * Looks like non-HS BABBLE can be ignored, but
-			 * HS BABBLE is an error condition. For HS the solution
-			 * is to avoid babble in the first place and fix whatever
-			 * causes BABBLE. When HS BABBLE happens we can only stop
-			 * the session.
+			 * HS BABBLE is an error condition. For HS the
+			 * solution is to avoid babble in the first place
+			 * and fix whatever causes BABBLE. When HS BABBLE
+			 * happens we can only stop the session.
 			 */
 			if (devctl & (MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV))
 				DBG(1, "BABBLE devctl: %02x\n", devctl);
 			else {
-				ERR("Stopping host session because of babble\n");
+				ERR("Stopping session because of babble\n");
 				musb_writeb(mbase, MUSB_DEVCTL, 0);
 			}
 		} else {
@@ -1705,6 +1703,11 @@ musb_vbus_show(struct device *dev, struc
 }
 static DEVICE_ATTR(vbus, 0644, musb_vbus_show, musb_vbus_store);
 
+#ifdef CONFIG_USB_GADGET_MUSB_HDRC
+
+/* Gadget drivers can't know that a host is connected so they might want
+ * to start SRP, but users can.  This allows userspace to trigger SRP.
+ */
 static ssize_t
 musb_srp_store(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t n)
@@ -1727,7 +1730,10 @@ musb_srp_store(struct device *dev, struc
 	return n;
 }
 static DEVICE_ATTR(srp, 0644, NULL, musb_srp_store);
-#endif
+
+#endif /* CONFIG_USB_GADGET_MUSB_HDRC */
+
+#endif	/* sysfs */
 
 /* Only used to provide cable state change events */
 static void musb_irq_work(struct work_struct *data)
@@ -2029,9 +2035,9 @@ fail:
 	status = device_create_file(dev, &dev_attr_mode);
 	status = device_create_file(dev, &dev_attr_cable);
 	status = device_create_file(dev, &dev_attr_vbus);
-#ifdef CONFIG_USB_MUSB_OTG
+#ifdef CONFIG_USB_GADGET_MUSB_HDRC
 	status = device_create_file(dev, &dev_attr_srp);
-#endif /* CONFIG_USB_MUSB_OTG */
+#endif /* CONFIG_USB_GADGET_MUSB_HDRC */
 	status = 0;
 #endif
 
--- o26.orig/drivers/usb/musb/musbdefs.h	2007-08-20 00:28:37.000000000 -0700
+++ o26/drivers/usb/musb/musbdefs.h	2007-08-20 00:44:33.000000000 -0700
@@ -328,12 +328,12 @@ struct musb {
 	irqreturn_t		(*isr)(int, void *);
 	struct work_struct	irq_work;
 
-#ifdef CONFIG_USB_MUSB_HDRC_HCD
-
 /* this hub status bit is reserved by USB 2.0 and not seen by usbcore */
 #define MUSB_PORT_STAT_RESUME	(1 << 31)
 
 	u32			port1_status;
+
+#ifdef CONFIG_USB_MUSB_HDRC_HCD
 	unsigned long		rh_timer;
 
 	enum musb_h_ep0_state	ep0_stage;
--- o26.orig/drivers/usb/musb/tusb6010.c	2007-08-20 00:28:38.000000000 -0700
+++ o26/drivers/usb/musb/tusb6010.c	2007-08-20 00:46:18.000000000 -0700
@@ -438,11 +438,9 @@ static void musb_do_idle(unsigned long _
 	if (!musb->is_active) {
 		u32	wakeups;
 
-#ifdef CONFIG_USB_MUSB_HDRC_HCD
 		/* wait until khubd handles port change status */
 		if (is_host_active(musb) && (musb->port1_status >> 16))
 			goto done;
-#endif
 
 #ifdef CONFIG_USB_GADGET_MUSB_HDRC
 		if (is_peripheral_enabled(musb) && !musb->gadget_driver)
@@ -586,7 +584,6 @@ void musb_platform_set_mode(struct musb 
 {
 	void __iomem	*base = musb->ctrl_base;
 	u32		otg_stat, phy_otg_ctrl, phy_otg_ena, dev_conf;
-	int		vbus = 0;
 
 	if (musb->board_mode != MUSB_OTG) {
 		ERR("Changing mode currently only supported in OTG mode\n");
@@ -606,7 +603,6 @@ void musb_platform_set_mode(struct musb 
 		phy_otg_ena |= TUSB_PHY_OTG_CTRL_OTG_ID_PULLUP;
 		dev_conf |= TUSB_DEV_CONF_ID_SEL;
 		dev_conf &= ~TUSB_DEV_CONF_SOFT_ID;
-		vbus = 1;
 		break;
 #endif
 

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[Dirk Behme]

David Brownell wrote:
> On Sunday 19 August 2007, Dirk Behme wrote:
> 
>>Sorry for stupid question from a non-USB expert: But what is the 
>>proposed fix for this now? ;)
>  
> Something not totally unlike the appended.  Build-tested only.

Thanks! Build-tested on DaVinci.

On OMAP seems that this conflicts with Kevins patch Tony applied some 
minutes ago:

http://source.mvista.com/git/?p=linux-omap-2.6.git;a=commitdiff;h=84e28a2065a57efa7a1e82650949128bcd57e31a

Regards

Dirk

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[David Brownell]

On Monday 20 August 2007, Dirk Behme wrote:
> this conflicts with Kevins patch Tony applied some 
> minutes ago:

Update appended.  Too much of the current #ifdeffery
is trivially replaced by using the already-defined
predicates to test e.g. the available modes.

- Dave

======	SNIP!
(Updated) Fix various build warnings for peripheral-only builds,
and some linewrap bugs.  Remove some inappropriate #ifdefs.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/usb/musb/musbdefs.h |    4 ++--
 drivers/usb/musb/plat_uds.c |   23 +++++++++++++----------
 drivers/usb/musb/tusb6010.c |    4 ----
 3 files changed, 15 insertions(+), 16 deletions(-)

--- h4.orig/drivers/usb/musb/plat_uds.c	2007-08-20 10:58:35.000000000 -0700
+++ h4/drivers/usb/musb/plat_uds.c	2007-08-20 11:02:48.000000000 -0700
@@ -364,9 +364,7 @@ static irqreturn_t musb_stage0_irq(struc
 				u8 devctl, u8 power)
 {
 	irqreturn_t handled = IRQ_NONE;
-#ifdef CONFIG_USB_MUSB_HDRC_HCD
 	void __iomem *mbase = musb->mregs;
-#endif
 
 	DBG(3, "<== Power=%02x, DevCtl=%02x, int_usb=0x%x\n", power, devctl,
 		int_usb);
@@ -603,8 +601,7 @@ static irqreturn_t musb_stage0_irq(struc
 	 * only host sees babble; only peripheral sees bus reset.
 	 */
 	if (int_usb & MUSB_INTR_RESET) {
-#ifdef CONFIG_USB_MUSB_HDRC_HCD
-		if (devctl & MUSB_DEVCTL_HM) {
+		if (is_host_capable() && (devctl & MUSB_DEVCTL_HM) != 0) {
 			/*
 			 * Looks like non-HS BABBLE can be ignored, but
 			 * HS BABBLE is an error condition. For HS the solution
@@ -618,9 +615,7 @@ static irqreturn_t musb_stage0_irq(struc
 				ERR("Stopping host session because of babble\n");
 				musb_writeb(mbase, MUSB_DEVCTL, 0);
 			}
-		} else
-#endif	/* CONFIG_USB_MUSB_HDRC_HCD */
-		{
+		} else if (is_peripheral_capable()) {
 			DBG(1, "BUS RESET\n");
 
 			musb_g_reset(musb);
@@ -1707,6 +1702,11 @@ musb_vbus_show(struct device *dev, struc
 }
 static DEVICE_ATTR(vbus, 0644, musb_vbus_show, musb_vbus_store);
 
+#ifdef CONFIG_USB_GADGET_MUSB_HDRC
+
+/* Gadget drivers can't know that a host is connected so they might want
+ * to start SRP, but users can.  This allows userspace to trigger SRP.
+ */
 static ssize_t
 musb_srp_store(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t n)
@@ -1726,7 +1726,10 @@ musb_srp_store(struct device *dev, struc
 	return n;
 }
 static DEVICE_ATTR(srp, 0644, NULL, musb_srp_store);
-#endif
+
+#endif /* CONFIG_USB_GADGET_MUSB_HDRC */
+
+#endif	/* sysfs */
 
 /* Only used to provide cable state change events */
 static void musb_irq_work(struct work_struct *data)
@@ -2028,9 +2031,9 @@ fail:
 	status = device_create_file(dev, &dev_attr_mode);
 	status = device_create_file(dev, &dev_attr_cable);
 	status = device_create_file(dev, &dev_attr_vbus);
-#ifdef CONFIG_USB_MUSB_OTG
+#ifdef CONFIG_USB_GADGET_MUSB_HDRC
 	status = device_create_file(dev, &dev_attr_srp);
-#endif /* CONFIG_USB_MUSB_OTG */
+#endif /* CONFIG_USB_GADGET_MUSB_HDRC */
 	status = 0;
 #endif
 
--- h4.orig/drivers/usb/musb/musbdefs.h	2007-08-20 10:58:35.000000000 -0700
+++ h4/drivers/usb/musb/musbdefs.h	2007-08-20 11:01:08.000000000 -0700
@@ -328,12 +328,12 @@ struct musb {
 	irqreturn_t		(*isr)(int, void *);
 	struct work_struct	irq_work;
 
-#ifdef CONFIG_USB_MUSB_HDRC_HCD
-
 /* this hub status bit is reserved by USB 2.0 and not seen by usbcore */
 #define MUSB_PORT_STAT_RESUME	(1 << 31)
 
 	u32			port1_status;
+
+#ifdef CONFIG_USB_MUSB_HDRC_HCD
 	unsigned long		rh_timer;
 
 	enum musb_h_ep0_state	ep0_stage;
--- h4.orig/drivers/usb/musb/tusb6010.c	2007-08-20 10:58:35.000000000 -0700
+++ h4/drivers/usb/musb/tusb6010.c	2007-08-20 11:01:08.000000000 -0700
@@ -438,11 +438,9 @@ static void musb_do_idle(unsigned long _
 	if (!musb->is_active) {
 		u32	wakeups;
 
-#ifdef CONFIG_USB_MUSB_HDRC_HCD
 		/* wait until khubd handles port change status */
 		if (is_host_active(musb) && (musb->port1_status >> 16))
 			goto done;
-#endif
 
 #ifdef CONFIG_USB_GADGET_MUSB_HDRC
 		if (is_peripheral_enabled(musb) && !musb->gadget_driver)
@@ -586,7 +584,6 @@ void musb_platform_set_mode(struct musb 
 {
 	void __iomem	*base = musb->ctrl_base;
 	u32		otg_stat, phy_otg_ctrl, phy_otg_ena, dev_conf;
-	int		vbus = 0;
 
 	if (musb->board_mode != MUSB_OTG) {
 		ERR("Changing mode currently only supported in OTG mode\n");
@@ -606,7 +603,6 @@ void musb_platform_set_mode(struct musb 
 		phy_otg_ena |= TUSB_PHY_OTG_CTRL_OTG_ID_PULLUP;
 		dev_conf |= TUSB_DEV_CONF_ID_SEL;
 		dev_conf &= ~TUSB_DEV_CONF_SOFT_ID;
-		vbus = 1;
 		break;
 #endif
 

Re: [PATCH] ARM: OMAP: Fix warning in plat_uds.c

[Tony Lindgren]

* David Brownell <david-b@pacbell.net> [070820 11:11]:
> On Monday 20 August 2007, Dirk Behme wrote:
> > this conflicts with Kevins patch Tony applied some 
> > minutes ago:
> 
> Update appended.  Too much of the current #ifdeffery
> is trivially replaced by using the already-defined
> predicates to test e.g. the available modes.
> 
> - Dave
> 
> ======	SNIP!
> (Updated) Fix various build warnings for peripheral-only builds,
> and some linewrap bugs.  Remove some inappropriate #ifdefs.

Thanks, pushing today.

Tony