Message-ID: <op.v0n67imkvgw7ix@mnazarewicz-glaptop>
Date:	Tue, 23 Aug 2011 16:15:08 +0200
From:	"Michal Nazarewicz" <mnazarewicz@...gle.com>
To:	"Felipe Balbi" <balbi@...com>
Cc:	"Alan Stern" <stern@...land.harvard.edu>,
	"Sebastian Andrzej Siewior" <bigeasy@...utronix.de>,
	"Yang Rui Rui" <ruirui.r.yang@...to.com>,
	"Dave Young" <hidave.darkstar@...il.com>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with
 "max_speed"

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

>> On Sat, 20 Aug 2011 01:28:06 +0200, Felipe Balbi <balbi@...com> 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@...gle.com>-----ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists