Re: [PATCH 0/6] Composite Gadget Support

On Sunday 29 July 2007, Felipe Balbi wrote:
> Hi dave,
> 
> first of all, thanks for your comments...
> 
> We'll work on them and send a newer version of the composite gadget support.

I'm sending a snapshot of my current code shortly, along
with a second patch partially converting gadget zero.
(The lack of such conversion was a problem with the first
version of that infrastructure patch -- no examples!)

That merges some stuff from your patch #3 and changes a few
things around to work more smoothly ... and also made me
think one concept particularly needs refactoring.


> See some comments below.
> 
> On 7/29/07, David Brownell <[email protected]> wrote:
> > On Saturday 19 May 2007, Ragner Magalhaes wrote:
> > > The following series implements USB Composite Gadget Support.
> >
> > And I'm finally looking at this again.  :)
> >
> > These patches seem to be "step one", just starting to
> > make conversions.  I'm assuming that you're circulating
> > them to get feedback (which, sorry, has been a bit hard
> > to come by) before later steps...
> >
> >
> > > [PATCH 1/6] USB gadget driver.
> >
> > This is badly misnamed -- "gadget.c" implies that it's the
> > one and only gadget driver.  But it isn't; a "gadget" is
> > whatever talks to the abstracted hardware.  And gadgets
> > aren't required to use this utility code.
> >
> > This is instead a composite gadget driver optimized to work
> > with a *single* function driver.  It needs to be renamed.
> > I'd suggest "single.c", "singlefunc.c", or somesuch.
> 
> We'll we were thinking about converting {serial, ether.
> file_storage}.c into usb_functions for good, and use gadget.c built
> with one of those function drivers to create a gadget driver.
> 
> Do you have a better solution here?

I'm pretty sure that's what I suggested already, in
the text you quoted.  :)

But, details to be worked.  I thought of having a convention
for the name of the gadget driver startup code, and having
those "standalone" gadget drivers use weak symbol declarations.
Then the truly "composite" gadgets would use a non-weak symbol.
That'd probably work, but it might be a bit icky.
 

> > > [PATCH 2/6] Composite gadget driver.
> >
> > This should actually be patch #1.  Among other things, it
> > defines the <linux/usb/composite.h> header used in your
> > patch #1!
> >
> > Now, this patch is basically a version of something I sent
> > some time back. Explicitly *without* a signed-off-by.  So
> > it's wrong for you to both (a) list it as "From:" you, and
> > also to (b) add my signed-off-by line.
> 
> Sorry about that... git mis-using here... fixing right away
> 
> >
> >
> > > [PATCH 3/6] Composite gadget driver upgrade.
> >
> > I'm reviewing this with the expectation that the good
> > bits will merge directly into your #2.  In fact, since
> > you're including an older version of my patch, some
> > of that has already been done...

Yep, I merged some of that.


> > > [PATCH 4/6] Kconfig modifications for USB Composite gadget support.
> >
> > Not really ... there's (a) stuff to switch two gadget
> > drivers over so they're "function" drivers using the
> > "single.c" glue code, and (b) stuff to build all the
> > composite gadget utility code into one module, but it's
> > missing (c) an actual composite gadget.
> 
> Yeah... we still didn't figure how to do this one...
> Could any of you comment here? Dave? Alan?

The *original* notion, which you did not follow, had
the composite driver making explicit calls to glue the
function drivers together.  It kind of needs to do
that, since it must provide product-specific IDs
and descriptors, and so forth.

I don't see a need to change that.  I think that it
may be practical to have single-function drivers
provide their own "weak" versions of functions so
that linking them together with composite glue would
just make things vanish.


> > I know you had an actual composite gadget in earlier
> > patches ... you should include one, so that we can
> > see that it all works.
> >
> > You seem to be assuming that for example that a g_ether
> > module would be an Ethernet gadget driver module if
> > there's no composite gadget defined, else it would be
> > a function module.  That's the wrong way to do things.
> 
> not really, it'll always be a function module/driver, when you load it
> with gadget.c it'll become a gadget module, when you load it with
> composite.c it'll become part of a composite gadget.

Like I said.  In the one case it's no longer a gadget,
it's just "part" of one.


> > The point of the "g_*" naming convention, and also the
> > Kconfig, is that when you select "Ethernet Gadget" you
> > get an Ethernet gadget module.
> >
> > The way to assemble a given composite gadget, let's call
> > it "Fred" and named "g_fred", should be to link all the
> > functions together into that "g_fred.ko" module.  It
> > must be possible to modprobe "g_ether" *OR* "g_fred"...
> >
> >
> > > [PATCH 5/6] Composite File Storage gadget support.
> > > [PATCH 6/6] Composite Ether gadget support.
> >
> > #5 and #6 just convert those gadget drivers to "function"
> > drivers.  But I see that they still have most of the code
> > which should have been eliminated by such a conversion ...
> 
> yeah... we still need to work on a lot of code...

Maybe that's a better place to start:  a more complete
conversion.  For example, you were still assiging IDs
statically.
 

> > > The Composite Gadget can handle one or two configurations.
> > > When the first usb_function modprobe'ed
> >
> > Don't assume functions can be modprobed by themselves
> > to mix'n'match.  There needs to be "glue" code which
> > understands that for example a specific combination has
> > particular product and vendor IDs, and in general uses
> > particular device descriptors.  The device class info
> > can matter a lot, for example...
> 
> this was our first shot while thinking a way to build all the stuff
> together without messing the driver up.

I see.  My next patches are a slightly different first shot.  :)

- Dave



> > > has two configurations
> > > the Composite Gadget will have two configurations, for the
> > > other functions modprobe'ed after will be used the selected or
> > > standard configuration only, their interfaces will be part
> > > of the Composite's Configurations.
> > > Then exchanging configs in the Composite will only affect the
> > > first function.
> > > In case the first modprobe'ed function has only one configure,
> > > the Composite Gadget WILL have only one configuration.
> > > This behavior is useful when modprobe'ing g_ether as the first
> > > usb_function due to the RNDIS and CDC Configurations.
> >
> > That seems like a fair approach to take, at least in terms
> > of managing configurations.  Although I don't see why only
> > the first function should be special.
> >
> > On the other hand, folk are finding that RNDIS is significantly
> > less than Microsoft has promised, so maybe we shouldn't care
> > so much about that.  Wouldn't it simplify things if we didn't
> > have to handle that case?
> >
> > ... and then there's the whole "other speed config" thing.
> > There need to be both full speed and high speed configs, if
> > the hardware supports high speed operation...
> >
> >
> >
> > > When the ether is modprobe'ed first:
> > >
> > >                    (Device Decriptor)
> > >                  /                \
> > >             (Config 0)             (Config 1)
> > > (eth config 0 + fsg config)  (eth config 1 + fsg config)
> >
> > Did you test that with MS-Windows?  The point about multiple
> > configs that Microsoft demands that the first listed config
> > be RNDIS.  (That's not necessarily config 0!!)  I don't recall
> > any expectation that "RNDIS + mass storage" could work...
> >
> >
> > 

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel