Re: [PATCH v3 3/4] USB: Add a function to obtain USB version independent maximum bpi value

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Alan,

Thanks for the review.

On Thu, Aug 07, 2025 at 11:19:15AM -0400, Alan Stern wrote:
> On Thu, Aug 07, 2025 at 08:53:54AM +0300, Sakari Ailus wrote:
> > From: "Rai, Amardeep" <amardeep.rai@intel.com>
> > 
> > Add usb_endpoint_max_isoc_bpi() to obtain maximum bytes per interval for
> > isochronous endpoints in a USB version independent way.
> 
> Is "bpi" really a commonly recognized acronym?  Offhand, I wouldn't 
> guess that it stands for "bytes per interval".  Can you come up with a 
> more explicit name?

There's not a single canonical name for this as the information is derived
from different sources depending on the USB version. But yes, that's the
abbreviation. I used it because "bytes_per_interval" would be quite long.
"bpi" has also been used by the UVC driver (see the 4th patch).

> 
> > Signed-off-by: Rai, Amardeep <amardeep.rai@intel.com>
> > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > Co-developed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reviewed-by: Hans de Goede <hansg@kernel.org>
> > ---
> >  include/linux/usb.h | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > index 535ac37198a1..da0f51dfe15f 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -2049,6 +2049,37 @@ static inline int usb_translate_errors(int error_code)
> >  	}
> >  }
> >  
> > +/**
> > + * usb_endpoint_max_isoc_bpi - Get maximum isochronous transfer bytes per interval
> > + * @dev: The USB device
> > + * @ep: The endpoint
> > + *
> > + * Returns: the maximum number of bytes isochronous endpoint @endpoint can
> > + * transfer in during a service interval, or 0 for non-isochronous endpoints.
> > + */
> > +static inline u32 usb_endpoint_max_isoc_bpi(struct usb_device *dev,
> > +					    const struct usb_host_endpoint *ep)
> > +{
> > +	if (usb_endpoint_type(&ep->desc) != USB_ENDPOINT_XFER_ISOC)
> > +		return 0;
> > +
> > +	switch (dev->speed) {
> > +	case USB_SPEED_SUPER_PLUS:
> > +		if (USB_SS_SSP_ISOC_COMP(ep->ss_ep_comp.bmAttributes))
> > +			return le32_to_cpu(ep->ssp_isoc_ep_comp.dwBytesPerInterval);
> > +		fallthrough;
> > +	case USB_SPEED_SUPER:
> > +		return le16_to_cpu(ep->ss_ep_comp.wBytesPerInterval);
> > +	case USB_SPEED_HIGH:
> > +		if (ep->eusb2_isoc_ep_comp.bDescriptorType &&
> > +		    !usb_endpoint_maxp(&ep->desc) && usb_endpoint_dir_in(&ep->desc))
> > +			return le32_to_cpu(ep->eusb2_isoc_ep_comp.dwBytesPerInterval);
> > +		fallthrough;
> > +	default:
> > +		return usb_endpoint_maxp(&ep->desc) * usb_endpoint_maxp_mult(&ep->desc);
> > +	}
> > +}
> 
> This function is complicated enough that it probably should not be an 
> inline routine.  Not unless it's used in only one place (in which case 
> why define it in a .h file?).

The single user currently (and probably will be for some time at least) is
the UVC driver but this code is better located in the USB tree. I'd keep it
this way until we have more users, unless Mathias can suggest where the
function should be located.

-- 
Kind regards,

Sakari Ailus