RE: [PATCH v3 2/6] usb:common Separated decoding functions from dwc3 driver.

From: Felipe Balbi
Date: Tue Feb 05 2019 - 03:25:22 EST


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.


Attachment: signature.asc
Description: PGP signature