Re: [PATCH] musb_hdrc: Add SRP Interface and control it through sysfs

David Brownell
2007-05-29T17:07:03Z

On Tuesday 29 May 2007, Felipe Balbi wrote:
> --- linux-omap-2.6.orig/drivers/usb/musb/musb_gadget.c 2007-05-28 01:40:16.000000000 +0300
> +++ linux-omap-2.6/drivers/usb/musb/musb_gadget.c 2007-05-28 13:18:47.000000000 +0300
> @@ -2056,3 +2056,29 @@
> (void) musb_gadget_vbus_draw(&musb->g,
> is_otg_enabled(musb) ? 8 : 100);
> }
> +
> +/* called when we want to send a Session Request. Caller must hold the lock */
> +void musb_g_srp(struct musb *musb)

This method should not be needed. When you
invoke usb_gadget_wakeup(), that's supposed
to trigger SRP when there's no existing
power session.

Note also that SRP is *NOT* an OTG-only
capability ... so it's incorrect to wrap
calls to such a routine in an OTG #ifdef.
Any peripheral is allowed to issue SRP.

> --- linux-omap-2.6.orig/drivers/usb/musb/plat_uds.c 2007-05-28 01:40:16.000000000 +0300
> +++ linux-omap-2.6/drivers/usb/musb/plat_uds.c 2007-05-28 13:18:47.000000000 +0300
> @@ -1579,6 +1579,32 @@
> }
> static DEVICE_ATTR(cable, S_IRUGO, musb_cable_show, NULL);
>
> +#ifdef CONFIG_USB_MUSB_OTG
> +static ssize_t
> +musb_srp_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t n)
> +{
> + struct musb *musb=dev_to_musb(dev);
> + unsigned long flags;
> + unsigned short srp;
> +
> + if (sscanf(buf, "%hu", &srp) != 1
> + || (srp != 1)) {
> + printk (KERN_ERR "SRP: Value must be 1\n");
> + return -EINVAL;
> + }

I see no point to such input validation. Why even bother?

> +
> + spin_lock_irqsave(&musb->Lock, flags);
> + if (srp == 1){
> + musb_g_srp(musb);

Call the "resume" method, not a special SRP-only one.

> + }
> + spin_unlock_irqrestore(&musb->Lock, flags);
> +
> + return n;
> +}
> +static DEVICE_ATTR(srp, 0644, NULL, musb_srp_store);
> +#endif /* CONFIG_USB_MUSB_OTG */
> +
> #endif
>
> /* Only used to provide cable state change events */
> @@ -1648,6 +1674,9 @@
> #ifdef CONFIG_SYSFS
> device_remove_file(musb->controller, &dev_attr_mode);
> device_remove_file(musb->controller, &dev_attr_cable);
> +#ifdef CONFIG_USB_MUSB_OTG

This, and its sibling later, and the code above, should
all be #ifdeffed CONFIG_USB_GADGET_MUSB_HDRC ... since
any peripheral may issue SRP, not only OTG-capable ones.

> + device_remove_file(musb->controller, &dev_attr_srp);
> +#endif
> #endif
>
> #ifdef CONFIG_USB_GADGET_MUSB_HDRC
> @@ -1876,6 +1905,9 @@
> #ifdef CONFIG_SYSFS
> status = device_create_file(dev, &dev_attr_mode);
> status = device_create_file(dev, &dev_attr_cable);
> +#ifdef CONFIG_USB_MUSB_OTG
> + status = device_create_file(dev, &dev_attr_srp);
> +#endif /* CONFIG_USB_MUSB_OTG */
> status = 0;
> #endif
>
> _______________________________________________
> Linux-omap-open-source mailing list
> Linux-omap-open-source@linux.omap.com
> http://linux.omap.com/mailman/listinfo/linux-omap-open-source
>