ARM: OMAP: Fix warning in plat_uds.c

PATCH In review Dirk Behme 4 participants 13 messages Aug 17, 2007 – Aug 21, 2007

Subsystems (2)

USB MUSB multipoint high speed dual-role controller

lore

Share & tools

lore

Dirk Behme

author

2007-08-17 15:43 UTC #0 lore raw

PATCH 1/1 ARM: OMAP: Fix warning in plat_uds.c

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 <[email protected]>

drivers/usb/musb/plat_uds.c +4 -0

Index: 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)

Felipe Balbi

2007-08-17 16:12 UTC #1 lore raw

Hi,

Quoted 23 lines · Dirk Behme

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 <[email protected]>
Index: linux-davinci/drivers/usb/musb/plat_uds.c
===================================================================
drivers/usb/musb/plat_uds.c +1 -0
@@ -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)

drivers/usb/musb/plat_uds.c musb_srp_store(struct device *dev, struc · line 1736

Quoted 8 lines

drivers/usb/musb/plat_uds.c +3 -0

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

Quoted 11 lines

drivers/usb/musb/plat_uds.c +0 -0
 /* Only used to provide cable state change events */
 static void musb_irq_work(struct work_struct *data)
 
_______________________________________________
Linux-omap-open-source mailing list
[email protected]
http://linux.omap.com/mailman/listinfo/linux-omap-open-source

Signature · 4 lines

Best Regards,

Felipe Balbi
[email protected]

Dirk Behme

author

2007-08-17 16:39 UTC #2 lore raw

Quoted 1 line · Felipe Balbi

Hi,

Thanks for review!

Quoted 25 lines · Dirk Behme

Quoted 24 lines
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 <[email protected]>
Index: linux-davinci/drivers/usb/musb/plat_uds.c
===================================================================
drivers/usb/musb/plat_uds.c +1 -0
@@ -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)
drivers/usb/musb/plat_uds.c +0 -0
@@ -1728,8 +1729,11 @@ musb_srp_store(struct device *dev, struc
 

Quoted 11 lines

drivers/usb/musb/plat_uds.c musb_srp_store(struct device *dev, struc · line 1736
Quoted 7 lines
drivers/usb/musb/plat_uds.c +3 -0
       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

David Brownell

2007-08-17 16:57 UTC #3 lore raw

Quoted 2 lines

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.

Felipe Balbi

2007-08-17 18:14 UTC #4 lore raw

Hi,

Quoted 6 lines · David Brownell

Quoted 2 lines
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.

Signature · 4 lines

Best Regards,

Felipe Balbi
[email protected]

David Brownell

2007-08-17 18:30 UTC #5 lore raw

Quoted 6 lines · Felipe Balbi

Hi,
Quoted 3 lines · David Brownell
Quoted 2 lines
this doesn't look right since any device can issue SRP. It's better for you to use it somewhere on DaVinci.

Quoted 7 lines

Quoted 3 lines
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...

Felipe Balbi

2007-08-17 20:36 UTC #6 lore raw

Hi,

Quoted 11 lines · David Brownell

Quoted 10 lines · Felipe Balbi
Hi,
Quoted 6 lines · David Brownell
Quoted 2 lines
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.

Quoted 7 lines

Quoted 3 lines
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

Quoted 1 line

Signature · 4 lines

Best Regards,

Felipe Balbi
[email protected]

David Brownell

2007-08-17 21:02 UTC #7 lore raw

Quoted 4 lines · Felipe Balbi

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.

Dirk Behme

author

2007-08-20 06:51 UTC #8 lore raw

Quoted 8 lines · David Brownell

Quoted 4 lines · Felipe Balbi
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

David Brownell

2007-08-20 08:41 UTC #9 lore raw

Quoted 3 lines · Dirk Behme

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 <[email protected]>

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

drivers/usb/musb/plat_uds.c +17 ~~-11~~

@@ -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

drivers/usb/musb/musbdefs.h +2 -2

@@ -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;

drivers/usb/musb/tusb6010.c +0 -4

@@ -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

Dirk Behme

author

2007-08-20 09:59 UTC #10 lore raw

Quoted 5 lines · David Brownell

Quoted 1 line · Dirk Behme
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

David Brownell

2007-08-20 18:10 UTC #11 lore raw

Quoted 2 lines · Dirk Behme

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 <[email protected]>

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

drivers/usb/musb/plat_uds.c +13 ~~-10~~

@@ -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

drivers/usb/musb/musbdefs.h +2 -2

@@ -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;

drivers/usb/musb/tusb6010.c +0 -4

@@ -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

Tony Lindgren

2007-08-21 07:24 UTC #12 lore raw

Quoted 8 lines · David Brownell

Quoted 2 lines · Dirk Behme
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.

Quoted 5 lines

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

Thanks, pushing today.

Tony