Hi, > >On Sun, 2019-07-21 at 19:32 +0100, 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: >[] >> diff --git a/drivers/usb/common/debug.c b/drivers/usb/common/debug.c >[] >> +static void usb_decode_set_clear_feature(__u8 bRequestType, __u8 bRequest, >> + __u16 wValue, __u16 wIndex, >> + char *str, size_t size) > >It's probably not necessary to use Hungarian >when moving these functions into generic code. In my opinion it's ok in this place. It's consistence with USB specification ch9 and with https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/usb/ch9.h. (look at usb_ctrlrequest). > >> +{ >> + switch (bRequestType & USB_RECIP_MASK) { >> + case USB_RECIP_DEVICE: >> + snprintf(str, size, "%s Device Feature(%s%s)", >> + bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set", >> + ({char *s; >> + switch (wValue) { >> + case USB_DEVICE_SELF_POWERED: >> + s = "Self Powered"; >> + break; >> + case USB_DEVICE_REMOTE_WAKEUP: >> + s = "Remote Wakeup"; >> + break; >> + case USB_DEVICE_TEST_MODE: >> + s = "Test Mode"; >> + break; >> + case USB_DEVICE_U1_ENABLE: >> + s = "U1 Enable"; >> + break; >> + case USB_DEVICE_U2_ENABLE: >> + s = "U2 Enable"; >> + break; >> + case USB_DEVICE_LTM_ENABLE: >> + s = "LTM Enable"; >> + break; >> + default: >> + s = "UNKNOWN"; >> + } s; }), >> + wValue == USB_DEVICE_TEST_MODE ? >> + ({ char *s; >> + switch (wIndex) { >> + case TEST_J: >> + s = ": TEST_J"; >> + break; >> + case TEST_K: >> + s = ": TEST_K"; >> + break; >> + case TEST_SE0_NAK: >> + s = ": TEST_SE0_NAK"; >> + break; >> + case TEST_PACKET: >> + s = ": TEST_PACKET"; >> + break; >> + case TEST_FORCE_EN: >> + s = ": TEST_FORCE_EN"; >> + break; >> + default: >> + s = ": UNKNOWN"; >> + } s; }) : ""); > >I always find this sort of embedded switch/case char * >statement expressions difficult to read and think it's >better to use separate lookup functions. > It has been changed in next patch in the series. >I would much prefer something like: > >static const char *usb_device_mode_desc(u16 mode) >{ > switch (mode) { > case USB_DEVICE_SELF_POWERED: > return "Self Powered"; > case USB_DEVICE_REMOTE_WAKEUP: > return "Remote Wakeup"; > case USB_DEVICE_TEST_MODE: > return "Test Mode"; > case USB_DEVICE_U1_ENABLE: > return "U1 Enable"; > case USB_DEVICE_U2_ENABLE: > return "U2 Enable"; > case USB_DEVICE_LTM_ENABLE: > return "LTM Enable"; > } > return "UNKNOWN"; >} > > snprintf(str, size, "%s Device Feature(%s%s)", >> bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set", > usb_device_mode_desc(wValue), > etc...); > > >etc... > Cheers, Pawell
