Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <4d4a8a02e4ec7fad9213302d17a6acd17851d8da.camel@perches.com>
Subject: Re: [PATCH v10 2/6] usb:common Separated decoding functions from
 dwc3 driver.
From:   Joe Perches <joe@perches.com>
To:     Pawel Laszczak <pawell@cadence.com>, felipe.balbi@linux.intel.com
Cc:     gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
        rogerq@ti.com, linux-kernel@vger.kernel.org, jbergsagel@ti.com,
        nsekhar@ti.com, nm@ti.com, sureshp@cadence.com, jpawar@cadence.com,
        kurahul@cadence.com, aniljoy@cadence.com
Date:   Sun, 21 Jul 2019 12:05:37 -0700
In-Reply-To: <1563733939-21214-3-git-send-email-pawell@cadence.com>
References: <1563733939-21214-1-git-send-email-pawell@cadence.com>
         <1563733939-21214-3-git-send-email-pawell@cadence.com>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.30.5-0ubuntu0.18.10.1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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