Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"

[Felipe Balbi <balbi@ti.com>]

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

Hi,

On Sat, Aug 20, 2011 at 12:33:00AM +0200, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
> 
> This commit replaces usb_gadget's is_dualspeed field with
> a max_speed field.
> 
> This change is made so that one will be able to check at
> run-time if given gadget supports super speed.

IMHO the logic is inverted here. It should start from the function
drivers. They should say which USB speeds they support, that would go up
to composite layer and composite would call e.g.
usb_gadget_set_speed(gadget, maximum_speed);

that function will set gadget->maximum_speed and call into the UDC
driver which would write maximum_speed into its internal registers, thus
preventing the core from working at speeds unsupported by the gadget
driver.

All of this should happen before we even connect to the host, so first
all UDCs should have their conversion to udc-core finished and start
using udc_start()/udc_stop(). The driver I just sent [1] illustrates
what udc_start()/udc_stop() should do.

[1] http://marc.info/?l=linux-usb&m=131376669022473&w=2

<snip>

> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index e1ecdbc..25058b4 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
>  }
>  static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);
>  
> -static ssize_t usb_udc_speed_show(struct device *dev,
> +#define USB_UDC_SPEED_ATTR(name)					\
> +ssize_t usb_udc_##name##_show(struct device *dev,			\
> +		struct device_attribute *attr, char *buf)		\
> +{									\
> +	struct usb_udc *udc = container_of(dev, struct usb_udc, dev);	\
> +	return snprintf(buf, PAGE_SIZE, "%s\n",				\
> +			usb_device_speed_name(udc->gadget->name));	\
> +}									\
> +static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
> +
> +static USB_UDC_SPEED_ATTR(speed);

how about "current_speed" ?

> +static USB_UDC_SPEED_ATTR(max_speed);

in this case, humans will be reading sysfs, so maximum_speed will look
nicer, IMHO.

> +/* Provide "is_dualspeed" for backward compatibility. */
> +static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
> -	return snprintf(buf, PAGE_SIZE, "%s\n",
> -			usb_device_speed_name(udc->gadget->speed));
> +	struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			gadget_is_dualspeed(udc->gadget));
>  }
> -static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
> +static DEVICE_ATTR(is_dualspeed, S_IRUSR, usb_udc_is_dualspeed_show, NULL);

maybe deprecate this one on feature-removal-schedule ??

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]