speedtch usbatm.c,1.20,1.21 usbatm.h,1.13,1.14

[Roman Kagan]

On Tue, Apr 26, 2005 at 11:39:02PM +0200, Duncan Sands wrote:
> Hi Roman, the channel initialization in usbatm_usb_probe seems messy.

Yes this is the part I'm the least happy myself.

> Near the start, you do:
> 
>         instance->rx_channel.endpoint = usb_rcvbulkpipe(usb_dev, driver->in);
>         instance->tx_channel.endpoint = usb_sndbulkpipe(usb_dev, driver->out);
>         instance->rx_channel.stride = ATM_CELL_SIZE + driver->rx_padding;
>         instance->tx_channel.stride = ATM_CELL_SIZE + driver->tx_padding;
>         instance->rx_channel.buf_size = rcv_buf_size * instance->rx_channel.stride;
>         instance->tx_channel.buf_size = snd_buf_size * instance->tx_channel.stride;
> 
> This is before calling bind.  After bind you do:
> 
>         tasklet_init(&instance->rx_channel.tasklet, usbatm_rx_process, (unsigned long)instance);
>         tasklet_init(&instance->tx_channel.tasklet, usbatm_tx_process, (unsigned long)instance);
>         usbatm_init_channel(&instance->rx_channel);
>         usbatm_init_channel(&instance->tx_channel);
>         instance->rx_channel.usbatm = instance->tx_channel.usbatm = instance;
> 
> Why do these in different places?

In some sense it's historical: this is where the corresponding bits were
initialized before.  I think most of the before-bind stuff can safely go
to after-bind.

One thing is special, however: the endpoints are initialized to bulk
pipes from usbatm_driver.in and .out, and I assumed that the subdriver
could override it by an iso pipe, if needed.  I guess I should better
check the endpoint descriptor instead to determine if it's bulk or iso,
and this all can (and should) be done after bind.

> Also, you'd expect something called usbatm_init_channel
> to initialize the whole thing, but in fact it only initializes a bit of it (just as well,
> since otherwise it would wipe out the fields you already initialized).  Why group this
> particular bit of initialization in a procedure, and not all, or other bits - what is the
> logic?

I wouldn't call it "logic" - I just didn't think well about it :)  At
some point I was scared that usbatm_init_channel() will turn into a
ten-argument function, most of which would be a simple assignment from
an argument to a field in the structure.   That's certainly not true so
I'd better merge it into one function.

Cheers,
  Roman.