2010/9/17 Sergei Shtylyov <sshtylyov@xxxxxxxxxx>:
> Hello.
>
> On 17-09-2010 4:00, Ming Lei wrote:
>
>>>> Suppose the condition of "request->zero&& (request->actual ==
>>>> request->length)" is true now:
>
>>>> - if request->length % musb_ep->packet_sz == 0, the patch can send zlp
>>>> out
>>>> without any problem
>
>>>> -if request->length % musb_ep->packet_sz != 0, in the dma case we may
>>>> set
>>>> txpktrdy to send out short packet since the 2nd condition is triggered,
>
>>> Which 2nd condition?
>
>> (is_dma&& (!dma->desired_mode ||
>> (request->actual&
>> (musb_ep->packet_sz - 1)))
>
>>>> and in
>>>> pio case txstate has set it already if request->actual ==
>>>> request->length;
>
>>> It might have been cleared already by the time you'd set it again.
>>> You'll
>>> then re-trigger empty packet send.
>
>> Yes, your are right, and follows the revised version.
>
>> How about this one?
>
>> diff --git a/drivers/usb/musb/musb_gadget.c
>> b/drivers/usb/musb/musb_gadget.c
>> index 46cf94a..fff2455 100644
>> --- a/drivers/usb/musb/musb_gadget.c
>> +++ b/drivers/usb/musb/musb_gadget.c
>> @@ -477,40 +477,39 @@ void musb_g_tx(struct musb *musb, u8 epnum)
>> epnum, csr, musb_ep->dma->actual_len,
>> request);
>> }
>>
>> - if (is_dma || request->actual == request->length) {
>> - /*
>> - * First, maybe a terminating short packet. Some
>> DMA
>> - * engines might handle this by themselves.
>> - */
>> - if ((request->zero&& request->length
>> - && request->length % musb_ep->packet_sz
>> == 0)
>> + /*
>> + * First, maybe a terminating short packet. Some DMA
>> + * engines might handle this by themselves.
>> + */
>> + if ((request->zero && request->length
>> + && (request->length % musb_ep->packet_sz == 0)
>> + && (request->actual == request->length))
>
> Parens around == not necessary.
>
>> #ifdef CONFIG_USB_INVENTRA_DMA
>> - || (is_dma && (!dma->desired_mode ||
>> - (request->actual &
>> - (musb_ep->packet_sz -
>> 1))))
>> + || (is_dma && (!dma->desired_mode ||
>> + (request->actual &
>> + (musb_ep->packet_sz - 1))))
>> #endif
>> - ) {
>> - /*
>> - * On DMA completion, FIFO may not be
>> - * available yet...
>> - */
>> - if (csr& MUSB_TXCSR_TXPKTRDY)
>> - return;
>> + ) {
>> + /*
>> + * On DMA completion, FIFO may not be
>> + * available yet...
>> + */
>> + if (csr& MUSB_TXCSR_TXPKTRDY)
>> + return;
>>
>> - DBG(4, "sending zero pkt\n");
>> - musb_writew(epio, MUSB_TXCSR,
>> MUSB_TXCSR_MODE
>> - | MUSB_TXCSR_TXPKTRDY);
>> - request->zero = 0;
>> - }
>> + DBG(4, "sending zero pkt\n");
>> + musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE
>> + | MUSB_TXCSR_TXPKTRDY);
>> + request->zero = 0;
>> + }
>>
>> - if (request->actual == request->length) {
>> - musb_g_giveback(musb_ep, request, 0);
>> - request = musb_ep->desc ?
>> next_request(musb_ep) : NULL;
>> - if (!request) {
>> - DBG(4, "%s idle now\n",
>> - musb_ep->end_point.name);
>> - return;
>> - }
>> + if (request->actual == request->length) {
>
> I don't understand why we have to check this twice when only once would
> suffice.
This check can handle both zero and non-zero request, so it is needed.
--
Lei Ming
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
