Hi, Pawel Laszczak <pawell@xxxxxxxxxxx> writes: >>On Thu, Jan 31, 2019 at 11:52:29AM +0000, Pawel Laszczak wrote: >>> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver >>> to driver/usb/common/debug.c file. These moved functions include: >>> dwc3_decode_get_status >>> dwc3_decode_set_clear_feature >>> dwc3_decode_set_address >>> dwc3_decode_get_set_descriptor >>> dwc3_decode_get_configuration >>> dwc3_decode_set_configuration >>> dwc3_decode_get_intf >>> dwc3_decode_set_intf >>> dwc3_decode_synch_frame >>> dwc3_decode_set_sel >>> dwc3_decode_set_isoch_delay >>> dwc3_decode_ctrl >>> >>> These functions are used also in inroduced cdns3 driver. >>> >>> All functions prefixes were changed from dwc3 to usb. >> >>Ick, why? > > Because CDNS3 driver in one of the previous version had implemented very similar function as dwc3 and Felipe suggested that we should > make common file with these functions. He also suggested that this function also could be used on host side. right, host controllers can make use of Control transfer decoding.a >>> Also, function's parameters has been extended according to the name >>> of fields in standard SETUP packet. >>> Additionally, patch adds usb_decode_ctrl function to >>> include/linux/usb/ch9.h file. >> >>Why ch9.h? It's not something that is specified in the spec, it's a >>usb-specific thing :) > Why not ? > > Similar as usb_state_string function from include/linux/usb/ch9.h which > " Returns human readable name for the state", the usb_decode_ctrl function > make the same but for standard USB request. right, I would say usb_state_string() should be moved elsewhere. ch9.h is, really, just what's described in chapter 9 of the usb specification. >>Also, the api for that function is not ok. If you are going to make >>this something that the whole kernel can call, you have to fix it up: >> >>> +/** >>> + * usb_decode_ctrl - Returns human readable representation of control request. >>> + * @str: buffer to return a human-readable representation of control request. >>> + * This buffer should have about 200 bytes. >> >>"about 200 bytes" is not very specific. >> >>Pass in the length so we know we don't overflow it. > > I didn't want to change to much this code, because it's not my code. > I'm not sure if I should ? I have a patch for that, then you can start on top of it. I'll send it soon after testing. -- balbi
Attachment:
signature.asc
Description: PGP signature
