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

[Felipe Balbi]

Hi,

On Tue, Aug 23, 2011 at 03:48:30PM +0200, Michal Nazarewicz wrote:
> Sorry, I somehow missed this mail before.

np

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

my suggestion was exactly to change that :-) Speed is something
functions support. composite.c shouldn't dictate which speed functions
should use, rather composite.c should use the maximum speed which all
functions support.

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

I wouldn't say that, actually. Just replacing is_dualspeed with
max_speed isn't changing much and if you want to make that part of the
code better, why not doing The Right Thing(TM) ?

All of the speed negotiation between composite.c and f_*.c should happen
before even connecting to host (before attaching data pullups, enabling
IRQs, etc), that's exactly why me and Sebastian have decided (at that
time off list) to add udc_start()/udc_stop() methods. One of the reason
was that it would be a quite intrusive change to all UDC drivers, second
we wanted to give maintainers/authors of those UDC drivers some grace
period for the change, third  when all UDCs are converted, it allow us
to do the speed negotiation before connecting to host.

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

you already maximum_speed (below) and speed alone looses some extra
hint of what kind of information will be there. I think it's better to
change this to current_speed and make a symbolic link called 'speed'
which we can keep for the next 5 years and remove it in e.g. Linux v5.0

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

I rather have the announcement. See above that you can make a symbolic
link with the name 'speed' for the time being.

-- 
balbi

Attachment: signature.asc
Description: Digital signature