| Message ID | 1614934975-15188-16-git-send-email-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/17] usb: xhci-mtk: remove or operator for setting schedule parameters | expand |
On Fri, Mar 05, 2021 at 05:02:54PM +0800, Chunfeng Yun wrote: > Add a new function to convert bInterval into the time expressed > in 1us unit. > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > --- > --- a/drivers/usb/common/common.c > +++ b/drivers/usb/common/common.c > @@ -165,6 +165,39 @@ enum usb_dr_mode usb_get_dr_mode(struct device *dev) > } > EXPORT_SYMBOL_GPL(usb_get_dr_mode); > > +unsigned int usb_decode_interval(const struct usb_endpoint_descriptor *epd, > + enum usb_device_speed speed) > +{ > + unsigned int interval = 0; > + > + switch (usb_endpoint_type(epd)) { > + case USB_ENDPOINT_XFER_CONTROL: > + /* uframes per NAK */ > + if (speed == USB_SPEED_HIGH) > + interval = epd->bInterval; > + break; > + case USB_ENDPOINT_XFER_ISOC: > + interval = 1 << (epd->bInterval - 1); > + break; > + case USB_ENDPOINT_XFER_BULK: > + /* uframes per NAK */ > + if (speed == USB_SPEED_HIGH && usb_endpoint_dir_out(epd)) > + interval = epd->bInterval; > + break; > + case USB_ENDPOINT_XFER_INT: > + if (speed >= USB_SPEED_HIGH) > + interval = 1 << (epd->bInterval - 1); > + else > + interval = epd->bInterval; > + break; > + } > + > + interval *= (speed >= USB_SPEED_HIGH) ? 125 : 1000; > + > + return interval; > +} > +EXPORT_SYMBOL_GPL(usb_decode_interval); > --- a/include/linux/usb/ch9.h > +++ b/include/linux/usb/ch9.h > @@ -90,6 +90,17 @@ extern enum usb_ssp_rate usb_get_maximum_ssp_rate(struct device *dev); > */ > extern const char *usb_state_string(enum usb_device_state state); > > +/** > + * usb_decode_interval - Decode bInterval into the time expressed in 1us unit > + * @epd: The descriptor of the endpoint > + * @speed: The speed that the endpoint works as > + * > + * Function returns the interval expressed in 1us unit for servicing > + * endpoint for data transfers. > + */ > +unsigned int usb_decode_interval(const struct usb_endpoint_descriptor *epd, > + enum usb_device_speed speed); As a general rule, I believe people expect to find the kerneldoc for a function next to the function's definition, not next to the declaration in a header file. Alan Stern
On Fri, Mar 05, 2021 at 10:33:12AM -0500, Alan Stern wrote: > On Fri, Mar 05, 2021 at 05:02:54PM +0800, Chunfeng Yun wrote: > > Add a new function to convert bInterval into the time expressed > > in 1us unit. > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > > --- a/drivers/usb/common/common.c > > +++ b/drivers/usb/common/common.c > > @@ -165,6 +165,39 @@ enum usb_dr_mode usb_get_dr_mode(struct device *dev) > > } > > EXPORT_SYMBOL_GPL(usb_get_dr_mode); > > > > +unsigned int usb_decode_interval(const struct usb_endpoint_descriptor *epd, > > + enum usb_device_speed speed) > > +{ > > + unsigned int interval = 0; > > + > > + switch (usb_endpoint_type(epd)) { > > + case USB_ENDPOINT_XFER_CONTROL: > > + /* uframes per NAK */ > > + if (speed == USB_SPEED_HIGH) > > + interval = epd->bInterval; > > + break; > > + case USB_ENDPOINT_XFER_ISOC: > > + interval = 1 << (epd->bInterval - 1); > > + break; > > + case USB_ENDPOINT_XFER_BULK: > > + /* uframes per NAK */ > > + if (speed == USB_SPEED_HIGH && usb_endpoint_dir_out(epd)) > > + interval = epd->bInterval; > > + break; > > + case USB_ENDPOINT_XFER_INT: > > + if (speed >= USB_SPEED_HIGH) > > + interval = 1 << (epd->bInterval - 1); > > + else > > + interval = epd->bInterval; > > + break; > > + } > > + > > + interval *= (speed >= USB_SPEED_HIGH) ? 125 : 1000; > > + > > + return interval; > > +} > > +EXPORT_SYMBOL_GPL(usb_decode_interval); > > > --- a/include/linux/usb/ch9.h > > +++ b/include/linux/usb/ch9.h > > @@ -90,6 +90,17 @@ extern enum usb_ssp_rate usb_get_maximum_ssp_rate(struct device *dev); > > */ > > extern const char *usb_state_string(enum usb_device_state state); > > > > +/** > > + * usb_decode_interval - Decode bInterval into the time expressed in 1us unit > > + * @epd: The descriptor of the endpoint > > + * @speed: The speed that the endpoint works as > > + * > > + * Function returns the interval expressed in 1us unit for servicing > > + * endpoint for data transfers. > > + */ > > +unsigned int usb_decode_interval(const struct usb_endpoint_descriptor *epd, > > + enum usb_device_speed speed); > > As a general rule, I believe people expect to find the kerneldoc for a > function next to the function's definition, not next to the declaration > in a header file. I was going to make the same review comment, but if you look above this in that file, there's other kernel doc information in the .h file, so this does match with the style of the file :( We can fix that all up later. thanks, greg k-h
On Fri, 2021-03-05 at 10:33 -0500, Alan Stern wrote: > On Fri, Mar 05, 2021 at 05:02:54PM +0800, Chunfeng Yun wrote: > > Add a new function to convert bInterval into the time expressed > > in 1us unit. > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > > --- a/drivers/usb/common/common.c > > +++ b/drivers/usb/common/common.c > > @@ -165,6 +165,39 @@ enum usb_dr_mode usb_get_dr_mode(struct device *dev) > > } > > EXPORT_SYMBOL_GPL(usb_get_dr_mode); > > > > +unsigned int usb_decode_interval(const struct usb_endpoint_descriptor *epd, > > + enum usb_device_speed speed) > > +{ > > + unsigned int interval = 0; > > + > > + switch (usb_endpoint_type(epd)) { > > + case USB_ENDPOINT_XFER_CONTROL: > > + /* uframes per NAK */ > > + if (speed == USB_SPEED_HIGH) > > + interval = epd->bInterval; > > + break; > > + case USB_ENDPOINT_XFER_ISOC: > > + interval = 1 << (epd->bInterval - 1); > > + break; > > + case USB_ENDPOINT_XFER_BULK: > > + /* uframes per NAK */ > > + if (speed == USB_SPEED_HIGH && usb_endpoint_dir_out(epd)) > > + interval = epd->bInterval; > > + break; > > + case USB_ENDPOINT_XFER_INT: > > + if (speed >= USB_SPEED_HIGH) > > + interval = 1 << (epd->bInterval - 1); > > + else > > + interval = epd->bInterval; > > + break; > > + } > > + > > + interval *= (speed >= USB_SPEED_HIGH) ? 125 : 1000; > > + > > + return interval; > > +} > > +EXPORT_SYMBOL_GPL(usb_decode_interval); > > > --- a/include/linux/usb/ch9.h > > +++ b/include/linux/usb/ch9.h > > @@ -90,6 +90,17 @@ extern enum usb_ssp_rate usb_get_maximum_ssp_rate(struct device *dev); > > */ > > extern const char *usb_state_string(enum usb_device_state state); > > > > +/** > > + * usb_decode_interval - Decode bInterval into the time expressed in 1us unit > > + * @epd: The descriptor of the endpoint > > + * @speed: The speed that the endpoint works as > > + * > > + * Function returns the interval expressed in 1us unit for servicing > > + * endpoint for data transfers. > > + */ > > +unsigned int usb_decode_interval(const struct usb_endpoint_descriptor *epd, > > + enum usb_device_speed speed); > > As a general rule, I believe people expect to find the kerneldoc for a > function next to the function's definition, not next to the declaration > in a header file. Got it, thanks > > Alan Stern
On Fri, 2021-03-05 at 16:47 +0100, Greg Kroah-Hartman wrote: > On Fri, Mar 05, 2021 at 10:33:12AM -0500, Alan Stern wrote: > > On Fri, Mar 05, 2021 at 05:02:54PM +0800, Chunfeng Yun wrote: > > > Add a new function to convert bInterval into the time expressed > > > in 1us unit. > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > > --- > > > > > --- a/drivers/usb/common/common.c > > > +++ b/drivers/usb/common/common.c > > > @@ -165,6 +165,39 @@ enum usb_dr_mode usb_get_dr_mode(struct device *dev) > > > } > > > EXPORT_SYMBOL_GPL(usb_get_dr_mode); > > > > > > +unsigned int usb_decode_interval(const struct usb_endpoint_descriptor *epd, > > > + enum usb_device_speed speed) > > > +{ > > > + unsigned int interval = 0; > > > + > > > + switch (usb_endpoint_type(epd)) { > > > + case USB_ENDPOINT_XFER_CONTROL: > > > + /* uframes per NAK */ > > > + if (speed == USB_SPEED_HIGH) > > > + interval = epd->bInterval; > > > + break; > > > + case USB_ENDPOINT_XFER_ISOC: > > > + interval = 1 << (epd->bInterval - 1); > > > + break; > > > + case USB_ENDPOINT_XFER_BULK: > > > + /* uframes per NAK */ > > > + if (speed == USB_SPEED_HIGH && usb_endpoint_dir_out(epd)) > > > + interval = epd->bInterval; > > > + break; > > > + case USB_ENDPOINT_XFER_INT: > > > + if (speed >= USB_SPEED_HIGH) > > > + interval = 1 << (epd->bInterval - 1); > > > + else > > > + interval = epd->bInterval; > > > + break; > > > + } > > > + > > > + interval *= (speed >= USB_SPEED_HIGH) ? 125 : 1000; > > > + > > > + return interval; > > > +} > > > +EXPORT_SYMBOL_GPL(usb_decode_interval); > > > > > --- a/include/linux/usb/ch9.h > > > +++ b/include/linux/usb/ch9.h > > > @@ -90,6 +90,17 @@ extern enum usb_ssp_rate usb_get_maximum_ssp_rate(struct device *dev); > > > */ > > > extern const char *usb_state_string(enum usb_device_state state); > > > > > > +/** > > > + * usb_decode_interval - Decode bInterval into the time expressed in 1us unit > > > + * @epd: The descriptor of the endpoint > > > + * @speed: The speed that the endpoint works as > > > + * > > > + * Function returns the interval expressed in 1us unit for servicing > > > + * endpoint for data transfers. > > > + */ > > > +unsigned int usb_decode_interval(const struct usb_endpoint_descriptor *epd, > > > + enum usb_device_speed speed); > > > > As a general rule, I believe people expect to find the kerneldoc for a > > function next to the function's definition, not next to the declaration > > in a header file. > > I was going to make the same review comment, but if you look above this > in that file, there's other kernel doc information in the .h file, so > this does match with the style of the file :( > > We can fix that all up later. I'll prepare a patch Thank you > > thanks, > > greg k-h
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c index fc21cf2d36f6..5dd7e657e369 100644 --- a/drivers/usb/common/common.c +++ b/drivers/usb/common/common.c @@ -165,6 +165,39 @@ enum usb_dr_mode usb_get_dr_mode(struct device *dev) } EXPORT_SYMBOL_GPL(usb_get_dr_mode); +unsigned int usb_decode_interval(const struct usb_endpoint_descriptor *epd, + enum usb_device_speed speed) +{ + unsigned int interval = 0; + + switch (usb_endpoint_type(epd)) { + case USB_ENDPOINT_XFER_CONTROL: + /* uframes per NAK */ + if (speed == USB_SPEED_HIGH) + interval = epd->bInterval; + break; + case USB_ENDPOINT_XFER_ISOC: + interval = 1 << (epd->bInterval - 1); + break; + case USB_ENDPOINT_XFER_BULK: + /* uframes per NAK */ + if (speed == USB_SPEED_HIGH && usb_endpoint_dir_out(epd)) + interval = epd->bInterval; + break; + case USB_ENDPOINT_XFER_INT: + if (speed >= USB_SPEED_HIGH) + interval = 1 << (epd->bInterval - 1); + else + interval = epd->bInterval; + break; + } + + interval *= (speed >= USB_SPEED_HIGH) ? 125 : 1000; + + return interval; +} +EXPORT_SYMBOL_GPL(usb_decode_interval); + #ifdef CONFIG_OF /** * of_usb_get_dr_mode_by_phy - Get dual role mode for the controller device diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c index 1ef2de6e375a..d8b0041de612 100644 --- a/drivers/usb/core/devices.c +++ b/drivers/usb/core/devices.c @@ -157,38 +157,25 @@ static char *usb_dump_endpoint_descriptor(int speed, char *start, char *end, switch (usb_endpoint_type(desc)) { case USB_ENDPOINT_XFER_CONTROL: type = "Ctrl"; - if (speed == USB_SPEED_HIGH) /* uframes per NAK */ - interval = desc->bInterval; - else - interval = 0; dir = 'B'; /* ctrl is bidirectional */ break; case USB_ENDPOINT_XFER_ISOC: type = "Isoc"; - interval = 1 << (desc->bInterval - 1); break; case USB_ENDPOINT_XFER_BULK: type = "Bulk"; - if (speed == USB_SPEED_HIGH && dir == 'O') /* uframes per NAK */ - interval = desc->bInterval; - else - interval = 0; break; case USB_ENDPOINT_XFER_INT: type = "Int."; - if (speed == USB_SPEED_HIGH || speed >= USB_SPEED_SUPER) - interval = 1 << (desc->bInterval - 1); - else - interval = desc->bInterval; break; default: /* "can't happen" */ return start; } - interval *= (speed == USB_SPEED_HIGH || - speed >= USB_SPEED_SUPER) ? 125 : 1000; - if (interval % 1000) + + interval = usb_decode_interval(desc, speed); + if (interval % 1000) { unit = 'u'; - else { + } else { unit = 'm'; interval /= 1000; } diff --git a/drivers/usb/core/endpoint.c b/drivers/usb/core/endpoint.c index 903426b6d305..a2530811cf7d 100644 --- a/drivers/usb/core/endpoint.c +++ b/drivers/usb/core/endpoint.c @@ -84,40 +84,13 @@ static ssize_t interval_show(struct device *dev, struct device_attribute *attr, char *buf) { struct ep_device *ep = to_ep_device(dev); + unsigned int interval; char unit; - unsigned interval = 0; - unsigned in; - in = (ep->desc->bEndpointAddress & USB_DIR_IN); - - switch (usb_endpoint_type(ep->desc)) { - case USB_ENDPOINT_XFER_CONTROL: - if (ep->udev->speed == USB_SPEED_HIGH) - /* uframes per NAK */ - interval = ep->desc->bInterval; - break; - - case USB_ENDPOINT_XFER_ISOC: - interval = 1 << (ep->desc->bInterval - 1); - break; - - case USB_ENDPOINT_XFER_BULK: - if (ep->udev->speed == USB_SPEED_HIGH && !in) - /* uframes per NAK */ - interval = ep->desc->bInterval; - break; - - case USB_ENDPOINT_XFER_INT: - if (ep->udev->speed == USB_SPEED_HIGH) - interval = 1 << (ep->desc->bInterval - 1); - else - interval = ep->desc->bInterval; - break; - } - interval *= (ep->udev->speed == USB_SPEED_HIGH) ? 125 : 1000; - if (interval % 1000) + interval = usb_decode_interval(ep->desc, ep->udev->speed); + if (interval % 1000) { unit = 'u'; - else { + } else { unit = 'm'; interval /= 1000; } diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h index abdd310c77f0..1a6d44ae707b 100644 --- a/include/linux/usb/ch9.h +++ b/include/linux/usb/ch9.h @@ -90,6 +90,17 @@ extern enum usb_ssp_rate usb_get_maximum_ssp_rate(struct device *dev); */ extern const char *usb_state_string(enum usb_device_state state); +/** + * usb_decode_interval - Decode bInterval into the time expressed in 1us unit + * @epd: The descriptor of the endpoint + * @speed: The speed that the endpoint works as + * + * Function returns the interval expressed in 1us unit for servicing + * endpoint for data transfers. + */ +unsigned int usb_decode_interval(const struct usb_endpoint_descriptor *epd, + enum usb_device_speed speed); + #ifdef CONFIG_TRACING /** * usb_decode_ctrl - Returns human readable representation of control request.
Add a new function to convert bInterval into the time expressed in 1us unit. Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> --- drivers/usb/common/common.c | 33 +++++++++++++++++++++++++++++++++ drivers/usb/core/devices.c | 21 ++++----------------- drivers/usb/core/endpoint.c | 35 ++++------------------------------- include/linux/usb/ch9.h | 11 +++++++++++ 4 files changed, 52 insertions(+), 48 deletions(-)