Received-SPF: pass (google.com: domain of linux-kernel+bounces-97556-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 8 Mar 2024 15:34:04 -0500
From: Alan Stern <stern@rowland.harvard.edu>
To: Dingyan Li <18500469033@163.com>
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
  linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] USB: Use EHCI control transfer pid macros instead of
 constant values.
Message-ID: <37bdd932-07a4-4514-a5cc-b70d48c962a6@rowland.harvard.edu>
References: <1e7f57d6-a4c1-4a3d-8cff-f966c89a8140@rowland.harvard.edu>
 <20240308010859.81987-1-18500469033@163.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240308010859.81987-1-18500469033@163.com>

On Fri, Mar 08, 2024 at 09:08:59AM +0800, Dingyan Li wrote:
> Macros with good names offer better readability. Besides, also move
> the definition to ehci.h.
> 
> Signed-off-by: Dingyan Li <18500469033@163.com>
> ---

You found some things that I missed!  Good for you.  When you resubmit 
with a list of changes from the original version, you can add:

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

>  drivers/usb/host/ehci-dbg.c | 10 +++++-----
>  drivers/usb/host/ehci-q.c   | 20 ++++++++------------
>  drivers/usb/host/ehci.h     |  8 +++++++-
>  3 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
> index c063fb042926..435001128221 100644
> --- a/drivers/usb/host/ehci-dbg.c
> +++ b/drivers/usb/host/ehci-dbg.c
> @@ -430,13 +430,13 @@ static void qh_lines(struct ehci_hcd *ehci, struct ehci_qh *qh,
>  				mark = '/';
>  		}
>  		switch ((scratch >> 8) & 0x03) {
> -		case 0:
> +		case PID_CODE_OUT:
>  			type = "out";
>  			break;
> -		case 1:
> +		case PID_CODE_IN:
>  			type = "in";
>  			break;
> -		case 2:
> +		case PID_CODE_SETUP:
>  			type = "setup";
>  			break;
>  		default:
> @@ -602,10 +602,10 @@ static unsigned output_buf_tds_dir(char *buf, struct ehci_hcd *ehci,
>  	list_for_each_entry(qtd, &qh->qtd_list, qtd_list) {
>  		temp++;
>  		switch ((hc32_to_cpu(ehci, qtd->hw_token) >> 8)	& 0x03) {
> -		case 0:
> +		case PID_CODE_OUT:
>  			type = "out";
>  			continue;
> -		case 1:
> +		case PID_CODE_IN:
>  			type = "in";
>  			continue;
>  		}
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index 666f5c4db25a..ba37a9fcab92 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -27,10 +27,6 @@
>  
>  /*-------------------------------------------------------------------------*/
>  
> -/* PID Codes that are used here, from EHCI specification, Table 3-16. */
> -#define PID_CODE_IN    1
> -#define PID_CODE_SETUP 2
> -
>  /* fill a qtd, returning how much of the buffer we were able to queue up */
>  
>  static unsigned int
> @@ -230,7 +226,7 @@ static int qtd_copy_status (
>  			/* fs/ls interrupt xfer missed the complete-split */
>  			status = -EPROTO;
>  		} else if (token & QTD_STS_DBE) {
> -			status = (QTD_PID (token) == 1) /* IN ? */
> +			status = (QTD_PID(token) == PID_CODE_IN) /* IN ? */
>  				? -ENOSR  /* hc couldn't read data */
>  				: -ECOMM; /* hc couldn't write data */
>  		} else if (token & QTD_STS_XACT) {
> @@ -606,7 +602,7 @@ qh_urb_transaction (
>  		/* SETUP pid */
>  		qtd_fill(ehci, qtd, urb->setup_dma,
>  				sizeof (struct usb_ctrlrequest),
> -				token | (2 /* "setup" */ << 8), 8);
> +				token | (PID_CODE_SETUP << 8), 8);
>  
>  		/* ... and always at least one more pid */
>  		token ^= QTD_TOGGLE;
> @@ -620,7 +616,7 @@ qh_urb_transaction (
>  
>  		/* for zero length DATA stages, STATUS is always IN */
>  		if (len == 0)
> -			token |= (1 /* "in" */ << 8);
> +			token |= (PID_CODE_IN << 8);
>  	}
>  
>  	/*
> @@ -642,7 +638,7 @@ qh_urb_transaction (
>  	}
>  
>  	if (is_input)
> -		token |= (1 /* "in" */ << 8);
> +		token |= (PID_CODE_IN << 8);
>  	/* else it's already initted to "out" pid (0 << 8) */
>  
>  	maxpacket = usb_endpoint_maxp(&urb->ep->desc);
> @@ -709,7 +705,7 @@ qh_urb_transaction (
>  
>  		if (usb_pipecontrol (urb->pipe)) {
>  			one_more = 1;
> -			token ^= 0x0100;	/* "in" <--> "out"  */
> +			token ^= (PID_CODE_IN << 8);	/* "in" <--> "out"  */
>  			token |= QTD_TOGGLE;	/* force DATA1 */
>  		} else if (usb_pipeout(urb->pipe)
>  				&& (urb->transfer_flags & URB_ZERO_PACKET)
> @@ -1203,7 +1199,7 @@ static int ehci_submit_single_step_set_feature(
>  		/* SETUP pid, and interrupt after SETUP completion */
>  		qtd_fill(ehci, qtd, urb->setup_dma,
>  				sizeof(struct usb_ctrlrequest),
> -				QTD_IOC | token | (2 /* "setup" */ << 8), 8);
> +				QTD_IOC | token | (PID_CODE_SETUP << 8), 8);
>  
>  		submit_async(ehci, urb, &qtd_list, GFP_ATOMIC);
>  		return 0; /*Return now; we shall come back after 15 seconds*/
> @@ -1216,7 +1212,7 @@ static int ehci_submit_single_step_set_feature(
>  	token ^= QTD_TOGGLE;   /*We need to start IN with DATA-1 Pid-sequence*/
>  	buf = urb->transfer_dma;
>  
> -	token |= (1 /* "in" */ << 8);  /*This is IN stage*/
> +	token |= (PID_CODE_IN << 8);  /*This is IN stage*/
>  
>  	maxpacket = usb_endpoint_maxp(&urb->ep->desc);
>  
> @@ -1229,7 +1225,7 @@ static int ehci_submit_single_step_set_feature(
>  	qtd->hw_alt_next = EHCI_LIST_END(ehci);
>  
>  	/* STATUS stage for GetDesc control request */
> -	token ^= 0x0100;        /* "in" <--> "out"  */
> +	token ^= (PID_CODE_IN << 8);        /* "in" <--> "out"  */
>  	token |= QTD_TOGGLE;    /* force DATA1 */
>  
>  	qtd_prev = qtd;
> diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> index 1441e3400796..d7a3c8d13f6b 100644
> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -321,10 +321,16 @@ struct ehci_qtd {
>  	size_t			length;			/* length of buffer */
>  } __aligned(32);
>  
> +/* PID Codes that are used here, from EHCI specification, Table 3-16. */
> +#define PID_CODE_OUT   0
> +#define PID_CODE_IN    1
> +#define PID_CODE_SETUP 2
> +
>  /* mask NakCnt+T in qh->hw_alt_next */
>  #define QTD_MASK(ehci)	cpu_to_hc32(ehci, ~0x1f)
>  
> -#define IS_SHORT_READ(token) (QTD_LENGTH(token) != 0 && QTD_PID(token) == 1)
> +#define IS_SHORT_READ(token) (QTD_LENGTH(token) != 0 && \
> +						QTD_PID(token) == PID_CODE_IN)
>  
>  /*-------------------------------------------------------------------------*/
>  
> -- 
> 2.25.1
>