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

From: Michal Nazarewicz
Date: Tue Aug 23 2011 - 10:15:38 EST


On Tue, 23 Aug 2011 15:58:17 +0200, Felipe Balbi <balbi@xxxxxx> wrote:

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.

Strictly speaking, composite.c does not dictate anything. It just copies
what the composite gadget driver declared as maximum speed.

My understanding was that one could consciously create a composite gadget
such that not all of the functions support all of the speeds.

I would suggest leaving everything as is, expect if usb_composite_driver's
max_speed is set to USB_SPEED_UNKNOWN in which case composite would iterate
over all the functions and figure out the maximum speed that all of the
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) ?

I'm just saying that the main reason for this patchset is to get rid of
the USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig options. So
the purpose of changing is_dualspeed with max_speed is to be able to
check if gadget is super speed at run-time so that gadget_is_superspeed()
can be implemented.

So I would like to just get this done and then figure out what to do with
composite.c. How does that sound?

All of the speed negotiation between composite.c and f_*.c should happen
before even connecting to host

Yep, obviously. The usb_gadget_probe_driver() is called at the very and
once all the functions and everything is added so composite.c can do all
the analysis it wants and figure out the maximum speed.

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

I don't really follow why those would be needed...

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.

Again, I don't follow. We can figure out the max_speed before calling
usb_gadget_probe_driver() just fine. We don't even have to have UDC
to figure that out (ie. gadget driver's max_speed does not change
depending on UDC, right?).

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

OK, I'll do that (as soon as I figure out/recall how to make symlinks that
is ;) ).

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