On Sun, 20 Mar 2005 02:15:58 +0100 Oliver Neukum <[EMAIL PROTECTED]> wrote:
> +static void acm_wb_free(struct acm *acm, int wbn)
> +{
> + acm->wb[wbn].use = 0;
> +}
>
> Why isn't this also inline?
Why should it be? I only inline if it's needed.
> +static int acm_wb_is_avail(struct acm *acm)
> +{
> + int i, wbn, n;
> +
> + n = 0;
> + wbn = acm->write_current;
> + for (i = 0; i < ACM_NWB; i++) {
> + if (!acm->wb[wbn].use)
> + n++;
> + wbn = (wbn + 1) % ACM_NWB;
> + }
> + return n;
> +}
>
> Why two counters?
Yes, this can be simplified.
> + if (acm->dev == NULL) {
> + spin_unlock_irqrestore(&acm->write_lock, flags);
> + return -ENODEV;
> + }
>
> Should be tested first.
I suppose it makes sense.
> + acm->write_ready = 0;
> +
> + acm->writeurb->transfer_buffer = wb->buf;
> + acm->writeurb->transfer_dma = wb->dmah;
> + acm->writeurb->transfer_buffer_length = wb->len;
> + acm->writeurb->dev = acm->dev;
> + spin_unlock_irqrestore(&acm->write_lock, flags);
>
> Why is the spinlock released exactly there?
It can be moved a few lines up, as long as write_ready is covered.
> + /*
> + * This is inaccurate (overcounts), but it works.
> + */
> + return (ACM_NWB - acm_wb_is_avail(acm)) * acm->writesize;
>
> Conservative would be to return one buffer size.
I do not understand why. This is use for two things: waiting for flush
(where it's only important if it's nonzero), and for low-water wakeups.
How is it good to error on the low side? You'll get nothing except
extra wakeup loops.
> - acm->write_buffer, acm->writesize, acm_write_bulk,
> acm);
> + NULL, acm->writesize, acm_write_bulk, acm);
>
> Why?
There's no single write buffer anymore.
> + spin_lock_irqsave(&acm->write_lock, flags);
> + acm->write_ready = 1;
> + wbn = acm->write_current;
> + acm_wb_free(acm, wbn);
> + acm->write_current = (wbn + 1) % ACM_NWB;
> + spin_unlock_irqrestore(&acm->write_lock, flags);
>
> Why unconditionally switching to the next buffer?
> It would be more efficient to switch buffers only at need.
Consider this:
- we find and fill 0
- we start 0
- write current is 0
- we find and fill 1
- 0 ends and if doesn't switch.
The very next call to start will not start (without searching, anyway).
Don't forget, I coded this without an access to hardware. I had to do it
right above all. What's little buffer rotation for it? Also, if a bug were
to happen, it would be most likely very hard to find and debug by e-mail.
I'd expect a random output hang.
-- Pete
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
