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

[Joe Perches]

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.

> +{
> +	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.

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...