Macros with good names offer better readability. Besides, also move
the definition to ehci.h.
Signed-off-by: Dingyan Li <[email protected]>
---
drivers/usb/host/ehci-q.c | 10 +++-------
drivers/usb/host/ehci.h | 8 +++++++-
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index 666f5c4db25a..51b46001e344 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;
@@ -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);
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 1441e3400796..dafa6628e134 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
On Fri, Mar 08, 2024 at 01:31: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 <[email protected]>
> ---
Good idea, but you missed a few spots.
> drivers/usb/host/ehci-q.c | 10 +++-------
> drivers/usb/host/ehci.h | 8 +++++++-
> 2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index 666f5c4db25a..51b46001e344 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;
There is an occurrence on line 623.
> @@ -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);
You could use PID_CODE_IN on lines 712 and 1232.
There are occurrences on lines 1206, 1219.
Also, there's a bunch of "switch" cases near line 433 in ehci-dbg.c.
Alan Stern
Macros with good names offer better readability. Besides, also move
the definition to ehci.h.
Signed-off-by: Dingyan Li <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/process/submitting-patches.rst for what
needs to be done here to properly describe this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
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 <[email protected]>
> ---
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 <[email protected]>
> 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
>
Macros with good names offer better readability. Besides, also move
the definition to ehci.h.
Signed-off-by: Dingyan Li <[email protected]>
Reviewed-by: Alan Stern <[email protected]>
---
V1 -> V2: Replacement in more places where Alan pointed out.
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
On Sat, Mar 09, 2024 at 11:37:09AM +0800, Dingyan Li wrote:
> Macros with good names offer better readability. Besides, also move
> the definition to ehci.h.
>
> Signed-off-by: Dingyan Li <[email protected]>
> Reviewed-by: Alan Stern <[email protected]>
> ---
> V1 -> V2: Replacement in more places where Alan pointed out.
>
This should be v3, as you have added Alan's reviewed-by, and added the
proper versioning information. Please resend it as such as our tools
can't dig it out from the end of the thread like this :(
thanks,
greg k-h
Macros with good names offer better readability. Besides, also move
the definition to ehci.h.
Signed-off-by: Dingyan Li <[email protected]>
Reviewed-by: Alan Stern <[email protected]>
---
V1 -> V2: Replacement in more places where Alan pointed out.
V2 -> V3: Add Reviewed-by for Alan
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
At 2024-03-09 15:14:48, "Greg KH" <[email protected]> wrote:
>On Sat, Mar 09, 2024 at 11:37:09AM +0800, Dingyan Li wrote:
>> Macros with good names offer better readability. Besides, also move
>> the definition to ehci.h.
>>
>> Signed-off-by: Dingyan Li <[email protected]>
>> Reviewed-by: Alan Stern <[email protected]>
>> ---
>> V1 -> V2: Replacement in more places where Alan pointed out.
>>
>
>This should be v3, as you have added Alan's reviewed-by, and added the
>proper versioning information. Please resend it as such as our tools
>can't dig it out from the end of the thread like this :(
>
>thanks,
>
>greg k-h
Hi Greg,
Sorry that I'm not quite familiar with the process. Since I haven't
got any feedback for a while after sending the v3 patch. Do that mean
the patch is accepted? Or anything else I still need to improve?
Regards,
Dingyan