[1/2] usb: gadget: Add start_frame to usb_request

[1/2] usb: gadget: Add start_frame to usb_request

[Felipe Balbi]

Hi,

Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
> Similar to URB's start_frame, add a field start_frame to the usb_request
> to report the scheduled (micro)frame number of an isochronous transfer.
> This option is useful for debugging purposes.
>
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
>  include/linux/usb/gadget.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index e5cd84a0f84a..ed9dbbce55ee 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -50,6 +50,7 @@ struct usb_ep;
>   * @short_not_ok: When reading data, makes short packets be
>   *     treated as errors (queue stops advancing till cleanup).
>   * @dma_mapped: Indicates if request has been mapped to DMA (internal)
> + * @start_frame: the reported (micro)frame of the scheduled isoc transfer
>   * @complete: Function called when request completes, so this request and
>   *	its buffer may be re-used.  The function will always be called with
>   *	interrupts disabled, and it must not sleep.
> @@ -107,6 +108,8 @@ struct usb_request {
>  	unsigned		short_not_ok:1;
>  	unsigned		dma_mapped:1;
>  
> +	int			start_frame;		/* ISO ONLY */

this name is a bit misleading. I can see functions trying to use it to
request that a particular request start on a specific frame. Would
"started_frame" be a better name, perhaps?

[1/2] usb: gadget: Add start_frame to usb_request

[Felipe Balbi]

Hi,

Thinh Nguyen <thinh.nguyen@synopsys.com> writes:

> Hi Felipe,
>
> On 11/7/2018 11:03 PM, Felipe Balbi wrote:
>> Hi,
>>
>> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>> Similar to URB's start_frame, add a field start_frame to the usb_request
>>> to report the scheduled (micro)frame number of an isochronous transfer.
>>> This option is useful for debugging purposes.
>>>
>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>> ---
>>>  include/linux/usb/gadget.h | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>> index e5cd84a0f84a..ed9dbbce55ee 100644
>>> --- a/include/linux/usb/gadget.h
>>> +++ b/include/linux/usb/gadget.h
>>> @@ -50,6 +50,7 @@ struct usb_ep;
>>>   * @short_not_ok: When reading data, makes short packets be
>>>   *     treated as errors (queue stops advancing till cleanup).
>>>   * @dma_mapped: Indicates if request has been mapped to DMA (internal)
>>> + * @start_frame: the reported (micro)frame of the scheduled isoc transfer
>>>   * @complete: Function called when request completes, so this request and
>>>   *	its buffer may be re-used.  The function will always be called with
>>>   *	interrupts disabled, and it must not sleep.
>>> @@ -107,6 +108,8 @@ struct usb_request {
>>>  	unsigned		short_not_ok:1;
>>>  	unsigned		dma_mapped:1;
>>>  
>>> +	int			start_frame;		/* ISO ONLY */
>> this name is a bit misleading. I can see functions trying to use it to
>> request that a particular request start on a specific frame. Would
>> "started_frame" be a better name, perhaps?
>
> What do you think of changing this field name to "frame_number"? It's
> generic enough so that this field can potentially be updated in the
> future to be used as a starting frame in the future, for synchronization.

I don't mind, but remember changing semantics will mean auditing every
user of this :-)

[1/2] usb: gadget: Add start_frame to usb_request

[Thinh Nguyen]

Hi Felipe,

On 11/7/2018 11:03 PM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>> Similar to URB's start_frame, add a field start_frame to the usb_request
>> to report the scheduled (micro)frame number of an isochronous transfer.
>> This option is useful for debugging purposes.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>>  include/linux/usb/gadget.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index e5cd84a0f84a..ed9dbbce55ee 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -50,6 +50,7 @@ struct usb_ep;
>>   * @short_not_ok: When reading data, makes short packets be
>>   *     treated as errors (queue stops advancing till cleanup).
>>   * @dma_mapped: Indicates if request has been mapped to DMA (internal)
>> + * @start_frame: the reported (micro)frame of the scheduled isoc transfer
>>   * @complete: Function called when request completes, so this request and
>>   *	its buffer may be re-used.  The function will always be called with
>>   *	interrupts disabled, and it must not sleep.
>> @@ -107,6 +108,8 @@ struct usb_request {
>>  	unsigned		short_not_ok:1;
>>  	unsigned		dma_mapped:1;
>>  
>> +	int			start_frame;		/* ISO ONLY */
> this name is a bit misleading. I can see functions trying to use it to
> request that a particular request start on a specific frame. Would
> "started_frame" be a better name, perhaps?

What do you think of changing this field name to "frame_number"? It's
generic enough so that this field can potentially be updated in the
future to be used as a starting frame in the future, for synchronization.

Thinh

[1/2] usb: gadget: Add start_frame to usb_request

[Thinh Nguyen]

Hi,

On 11/7/2018 11:03 PM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>> Similar to URB's start_frame, add a field start_frame to the usb_request
>> to report the scheduled (micro)frame number of an isochronous transfer.
>> This option is useful for debugging purposes.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>>  include/linux/usb/gadget.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index e5cd84a0f84a..ed9dbbce55ee 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -50,6 +50,7 @@ struct usb_ep;
>>   * @short_not_ok: When reading data, makes short packets be
>>   *     treated as errors (queue stops advancing till cleanup).
>>   * @dma_mapped: Indicates if request has been mapped to DMA (internal)
>> + * @start_frame: the reported (micro)frame of the scheduled isoc transfer
>>   * @complete: Function called when request completes, so this request and
>>   *	its buffer may be re-used.  The function will always be called with
>>   *	interrupts disabled, and it must not sleep.
>> @@ -107,6 +108,8 @@ struct usb_request {
>>  	unsigned		short_not_ok:1;
>>  	unsigned		dma_mapped:1;
>>  
>> +	int			start_frame;		/* ISO ONLY */
> this name is a bit misleading. I can see functions trying to use it to
> request that a particular request start on a specific frame. Would
> "started_frame" be a better name, perhaps?
>
Sure. That's more accurate.

Thinh

[1/2] usb: gadget: Add start_frame to usb_request

[Thinh Nguyen]

Similar to URB's start_frame, add a field start_frame to the usb_request
to report the scheduled (micro)frame number of an isochronous transfer.
This option is useful for debugging purposes.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 include/linux/usb/gadget.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a0f84a..ed9dbbce55ee 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -50,6 +50,7 @@ struct usb_ep;
  * @short_not_ok: When reading data, makes short packets be
  *     treated as errors (queue stops advancing till cleanup).
  * @dma_mapped: Indicates if request has been mapped to DMA (internal)
+ * @start_frame: the reported (micro)frame of the scheduled isoc transfer
  * @complete: Function called when request completes, so this request and
  *	its buffer may be re-used.  The function will always be called with
  *	interrupts disabled, and it must not sleep.
@@ -107,6 +108,8 @@ struct usb_request {
 	unsigned		short_not_ok:1;
 	unsigned		dma_mapped:1;
 
+	int			start_frame;		/* ISO ONLY */
+
 	void			(*complete)(struct usb_ep *ep,
 					struct usb_request *req);
 	void			*context;