diff mbox series

[1/5] usb: host: xhci: use ffs() in xhci_mem_init()

Message ID 1644994755-12975-2-git-send-email-quic_linyyuan@quicinc.com (mailing list archive)
State Superseded
Headers show
Series usb: xhci: fix of some code/comment | expand

Commit Message

Linyu Yuan Feb. 16, 2022, 6:59 a.m. UTC
The for loop to find page size bit can be replaced with ffs().

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 drivers/usb/host/xhci-mem.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Sergey Shtylyov Feb. 16, 2022, 9:41 a.m. UTC | #1
Hello!

On 2/16/22 9:59 AM, Linyu Yuan wrote:

> The for loop to find page size bit can be replaced with ffs().
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
>  drivers/usb/host/xhci-mem.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 0e31206..3cbc7f2 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -2395,12 +2395,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>  	page_size = readl(&xhci->op_regs->page_size);
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>  			"Supported page size register = 0x%x", page_size);
> -	for (i = 0; i < 16; i++) {
> -		if ((0x1 & page_size) != 0)
> -			break;
> -		page_size = page_size >> 1;
> -	}
> -	if (i < 16)
> +	if ((i = ffs(page_size)) < 16)

   Always run your patches thru scripts/checkpatch.pl -- in this case it will complain
of an assignment in the *if* expression...

[...]

MNR, Sergey
Linyu Yuan Feb. 16, 2022, 9:47 a.m. UTC | #2
Thanks,
that's correct,  from my view, one line is good,

What's your suggestion ? two lines ?

-----Original Message-----
From: Sergei Shtylyov <sergei.shtylyov@gmail.com> 
Sent: Wednesday, February 16, 2022 5:42 PM
To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>; Mathias Nyman <mathias.nyman@intel.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
Subject: Re: [PATCH 1/5] usb: host: xhci: use ffs() in xhci_mem_init()

Hello!

On 2/16/22 9:59 AM, Linyu Yuan wrote:

> The for loop to find page size bit can be replaced with ffs().
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
>  drivers/usb/host/xhci-mem.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 0e31206..3cbc7f2 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -2395,12 +2395,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>  	page_size = readl(&xhci->op_regs->page_size);
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>  			"Supported page size register = 0x%x", page_size);
> -	for (i = 0; i < 16; i++) {
> -		if ((0x1 & page_size) != 0)
> -			break;
> -		page_size = page_size >> 1;
> -	}
> -	if (i < 16)
> +	if ((i = ffs(page_size)) < 16)

   Always run your patches thru scripts/checkpatch.pl -- in this case it will complain
of an assignment in the *if* expression...

[...]

MNR, Sergey
Sergey Shtylyov Feb. 16, 2022, 9:53 a.m. UTC | #3
On 2/16/22 12:47 PM, Linyu Yuan (QUIC) wrote:

> that's correct,  from my view, one line is good,
> 
> What's your suggestion ? two lines ?

   Yes, and it is not just my suggestion -- it's the kernel coding style.

[...]

MBR, Sergey
Linyu Yuan Feb. 16, 2022, 9:57 a.m. UTC | #4
> From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
> Sent: Wednesday, February 16, 2022 5:53 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>; Mathias Nyman
> <mathias.nyman@intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> Subject: Re: [PATCH 1/5] usb: host: xhci: use ffs() in xhci_mem_init()
> 
> On 2/16/22 12:47 PM, Linyu Yuan (QUIC) wrote:
> 
> > that's correct,  from my view, one line is good,
> >
> > What's your suggestion ? two lines ?
> 
>    Yes, and it is not just my suggestion -- it's the kernel coding style.
I think in linux, there are many code like this,

arch/m68k/sun3x/dvma.c:         if((pmd = pmd_alloc(&init_mm, pud, vaddr)) == NULL) {
...

Two lines here is not good.
> 
> [...]
> 
> MBR, Sergey
Sergey Shtylyov Feb. 16, 2022, 10:11 a.m. UTC | #5
On 2/16/22 12:57 PM, Linyu Yuan (QUIC) wrote:

[...]

>>> that's correct,  from my view, one line is good,
>>>
>>> What's your suggestion ? two lines ?
>>
>>    Yes, and it is not just my suggestion -- it's the kernel coding style.
> I think in linux, there are many code like this,
> 
> arch/m68k/sun3x/dvma.c:         if((pmd = pmd_alloc(&init_mm, pud, vaddr)) == NULL) {
> ...
> 
> Two lines here is not good.

   Why?! 8-)

>> [...]

MBR, Sergey
Greg Kroah-Hartman Feb. 16, 2022, 10:34 a.m. UTC | #6
On Wed, Feb 16, 2022 at 09:57:04AM +0000, Linyu Yuan (QUIC) wrote:
> > From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
> > Sent: Wednesday, February 16, 2022 5:53 PM
> > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>; Mathias Nyman
> > <mathias.nyman@intel.com>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>
> > Cc: linux-usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> > Subject: Re: [PATCH 1/5] usb: host: xhci: use ffs() in xhci_mem_init()
> > 
> > On 2/16/22 12:47 PM, Linyu Yuan (QUIC) wrote:
> > 
> > > that's correct,  from my view, one line is good,
> > >
> > > What's your suggestion ? two lines ?
> > 
> >    Yes, and it is not just my suggestion -- it's the kernel coding style.
> I think in linux, there are many code like this,
> 
> arch/m68k/sun3x/dvma.c:         if((pmd = pmd_alloc(&init_mm, pud, vaddr)) == NULL) {
> ...

That file pre-dates the use of the kernel coding style.  Please look at
modern code (i.e. written in the last 15 years.)

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 0e31206..3cbc7f2 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2395,12 +2395,7 @@  int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 	page_size = readl(&xhci->op_regs->page_size);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
 			"Supported page size register = 0x%x", page_size);
-	for (i = 0; i < 16; i++) {
-		if ((0x1 & page_size) != 0)
-			break;
-		page_size = page_size >> 1;
-	}
-	if (i < 16)
+	if ((i = ffs(page_size)) < 16)
 		xhci_dbg_trace(xhci, trace_xhci_dbg_init,
 			"Supported page size of %iK", (1 << (i+12)) / 1024);
 	else