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
