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

[Michal Nazarewicz]

Hi,

Sorry, I somehow missed this mail before.

> On Sat, Aug 20, 2011 at 12:33:00AM +0200, Michal Nazarewicz wrote:
> > 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.

On Sat, 20 Aug 2011 01:28:06 +0200, Felipe Balbi <balbi@xxxxxx> wrote:
> 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);

This is actually not how composite works at the moment.  Currently,
a composite gadget can declare a maximum speed of say âhighâ even if
all the functions do not support that speed.  Of course when host asks
about descriptors for given speed, only functions that support that
speed will be returned (and hence only configurations that have at
least one function supporting that speed).

Whether the behaviour should be changed is, in my opinion, issue separate
from the patchset that I'm sending.

> > 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" ?

Is there a big advantage?  That would change external interface and I don't
see reason to do so.  Of course, udc class is quite recent so if you feel  
we can
ignore this issue I can go forward with that change.

> > +static USB_UDC_SPEED_ATTR(max_speed);
>
> in this case, humans will be reading sysfs, so maximum_speed will look
> nicer, IMHO.

I don't care much.  Will change.

> > +/* 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 ??

Sure.

Also, if we decide to change âspeedâ to âcurrent_speedâ, we could just drop
this without the announcement.

--
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@xxxxxxxxxx>-----ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/