[RFC] usb: add usb_fill_iso_urb()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sebastian,

Thank you for the patch.

On Friday, 13 July 2018 01:35:27 EEST Sebastian Andrzej Siewior wrote:
> Provide usb_fill_iso_urb() for the initialisation of isochronous URBs.
> We already have one of this helpers for control, bulk and interruptible
> URB types. This helps to keep the initialisation of the URB members in
> one place.
> Update the documentation by adding this to the available init functions
> and remove the suggestion to use the `_int_' helper which might provide
> wrong encoding for the `interval' member.
> 
> This looks like it would cover most users nicely. The sound subsystem
> initialises the ->iso_frame_desc[].offset + length member (often) at a
> different location and I'm not sure ->interval will work always as
> expected. So we might need to overwrite those two in worst case.
> 
> Some users also initialise ->iso_frame_desc[].actual_length but I don't

s/I don't/I don't think/ ?

> this is required since it is the return value.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  Documentation/driver-api/usb/URB.rst | 12 +++----
>  include/linux/usb.h                  | 53 ++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/driver-api/usb/URB.rst
> b/Documentation/driver-api/usb/URB.rst index 61a54da9fce9..20030b781519
> 100644
> --- a/Documentation/driver-api/usb/URB.rst
> +++ b/Documentation/driver-api/usb/URB.rst
> @@ -116,11 +116,11 @@ What has to be filled in?
> 
>  Depending on the type of transaction, there are some inline functions
>  defined in ``linux/usb.h`` to simplify the initialization, such as
> -:c:func:`usb_fill_control_urb`, :c:func:`usb_fill_bulk_urb` and
> -:c:func:`usb_fill_int_urb`.  In general, they need the usb device pointer,
> -the pipe (usual format from usb.h), the transfer buffer, the desired
> transfer -length, the completion handler, and its context. Take a look at
> the some -existing drivers to see how they're used.
> +:c:func:`usb_fill_control_urb`, :c:func:`usb_fill_bulk_urb`,
> +:c:func:`usb_fill_int_urb` and :c:func:`usb_fill_iso_urb`.  In general,
> they +need the usb device pointer, the pipe (usual format from usb.h), the
> transfer +buffer, the desired transfer length, the completion handler, and
> its context. +Take a look at the some existing drivers to see how they're
> used.
> 
>  Flags:
> 
> @@ -243,7 +243,7 @@ Besides the fields present on a bulk transfer, for ISO,
> you also also have to set ``urb->interval`` to say how often to make
> transfers; it's often one per frame (which is once every microframe for
> highspeed devices). The actual interval used will be a power of two that's
> no bigger than what -you specify. You can use the
> :c:func:`usb_fill_int_urb` macro to fill +you specify. You can use the
> :c:func:`usb_fill_iso_urb` macro to fill most ISO transfer fields.
> 
>  For ISO transfers you also have to fill a
> :c:type:`usb_iso_packet_descriptor` diff --git a/include/linux/usb.h
> b/include/linux/usb.h
> index 4cdd515a4385..74a3339041d6 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1697,6 +1697,59 @@ static inline void usb_fill_int_urb(struct urb *urb,
>  	urb->start_frame = -1;
>  }
> 
> +/**
> + * usb_fill_iso_urb - macro to help initialize an isochronous urb

Strictly speaking this isn't a macro, so I'd write "initializes an isochronous 
urb"

> + * @urb: pointer to the urb to initialize.
> + * @dev: pointer to the struct usb_device for this urb.
> + * @pipe: the endpoint pipe
> + * @transfer_buffer: pointer to the transfer buffer
> + * @buffer_length: length of the transfer buffer
> + * @complete_fn: pointer to the usb_complete_t function
> + * @context: what to set the urb context to.
> + * @interval: what to set the urb interval to, encoded like
> + *	the endpoint descriptor's bInterval value.
> + * @packets: number of ISO packets.
> + * @packet_size: size of each ISO packet.
> + *
> + * Initializes an isochronous urb with the proper information needed to
> submit
> + * it to a device.
> + *
> + * Note that isochronous endpoints use a logarithmic encoding of the
> endpoint
> + * interval, and express polling intervals in microframes (eight per
> + * millisecond) rather than in frames (one per millisecond).
> + */
> +static inline void usb_fill_iso_urb(struct urb *urb,
> +				    struct usb_device *dev,
> +				    unsigned int pipe,
> +				    void *transfer_buffer,
> +				    int buffer_length,
> +				    usb_complete_t complete_fn,
> +				    void *context,
> +				    int interval,
> +				    unsigned int packets,
> +				    unsigned int packet_size)
> +{
> +	unsigned int i;
> +
> +	urb->dev = dev;
> +	urb->pipe = pipe;
> +	urb->transfer_buffer = transfer_buffer;
> +	urb->transfer_buffer_length = buffer_length;
> +	urb->complete = complete_fn;
> +	urb->context = context;
> +
> +	interval = clamp(interval, 1, 16);
> +	urb->interval = 1 << (interval - 1);
> +	urb->start_frame = -1;
> +
> +	urb->number_of_packets = packets;
> +
> +	for (i = 0; i < packets; i++) {
> +		urb->iso_frame_desc[i].offset = packet_size * i;
> +		urb->iso_frame_desc[i].length = packet_size;
> +	}
> +}

I think this should be moved to a .c file as the function is growing big, but 
that's true of the other URB helpers as well, so it can be done later in a 
separate patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  extern void usb_init_urb(struct urb *urb);
>  extern struct urb *usb_alloc_urb(int iso_packets, gfp_t mem_flags);
>  extern void usb_free_urb(struct urb *urb);