linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: musb: remove unused variable 'devctl'
@ 2020-11-17  8:21 min.guo
  2020-11-18 11:48 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 11+ messages in thread
From: min.guo @ 2020-11-17  8:21 UTC (permalink / raw)
  To: Bin Liu
  Cc: Greg Kroah-Hartman, Matthias Brugger, chunfeng.yun, linux-usb,
	linux-kernel, linux-arm-kernel, linux-mediatek, Min Guo

From: Min Guo <min.guo@mediatek.com>

Remove unused 'devctl' variable to fix compile warnings:

    drivers/usb/musb/musbhsdma.c: In function 'dma_controller_irq':
    drivers/usb/musb/musbhsdma.c:324:8: warning: variable 'devctl' set
    but not used [-Wunused-but-set-variable]

Signed-off-by: Min Guo <min.guo@mediatek.com>
---
 drivers/usb/musb/musbhsdma.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index 0aacfc8be5a1..7acd1635850d 100644
--- a/drivers/usb/musb/musbhsdma.c
+++ b/drivers/usb/musb/musbhsdma.c
@@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
 				musb_channel->channel.status =
 					MUSB_DMA_STATUS_BUS_ABORT;
 			} else {
-				u8 devctl;
-
 				addr = musb_read_hsdma_addr(mbase,
 						bchannel);
 				channel->actual_len = addr
@@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
 						< musb_channel->len) ?
 					"=> reconfig 0" : "=> complete");
 
-				devctl = musb_readb(mbase, MUSB_DEVCTL);
-
 				channel->status = MUSB_DMA_STATUS_FREE;
 
 				/* completed */
-- 
2.18.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: musb: remove unused variable 'devctl'
  2020-11-17  8:21 [PATCH] usb: musb: remove unused variable 'devctl' min.guo
@ 2020-11-18 11:48 ` Greg Kroah-Hartman
  2020-11-20  6:48   ` Min Guo
  0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-18 11:48 UTC (permalink / raw)
  To: min.guo
  Cc: Bin Liu, Matthias Brugger, chunfeng.yun, linux-usb, linux-kernel,
	linux-arm-kernel, linux-mediatek

On Tue, Nov 17, 2020 at 04:21:25PM +0800, min.guo@mediatek.com wrote:
> From: Min Guo <min.guo@mediatek.com>
> 
> Remove unused 'devctl' variable to fix compile warnings:
> 
>     drivers/usb/musb/musbhsdma.c: In function 'dma_controller_irq':
>     drivers/usb/musb/musbhsdma.c:324:8: warning: variable 'devctl' set
>     but not used [-Wunused-but-set-variable]
> 
> Signed-off-by: Min Guo <min.guo@mediatek.com>
> ---
>  drivers/usb/musb/musbhsdma.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> index 0aacfc8be5a1..7acd1635850d 100644
> --- a/drivers/usb/musb/musbhsdma.c
> +++ b/drivers/usb/musb/musbhsdma.c
> @@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
>  				musb_channel->channel.status =
>  					MUSB_DMA_STATUS_BUS_ABORT;
>  			} else {
> -				u8 devctl;
> -
>  				addr = musb_read_hsdma_addr(mbase,
>  						bchannel);
>  				channel->actual_len = addr
> @@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
>  						< musb_channel->len) ?
>  					"=> reconfig 0" : "=> complete");
>  
> -				devctl = musb_readb(mbase, MUSB_DEVCTL);

Are you sure that the hardware does not require this read to complete
the command?  Lots of hardware is that way, so be very careful about
this.  Did you test it?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: musb: remove unused variable 'devctl'
  2020-11-18 11:48 ` Greg Kroah-Hartman
@ 2020-11-20  6:48   ` Min Guo
  2020-11-20  6:54     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 11+ messages in thread
From: Min Guo @ 2020-11-20  6:48 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Bin Liu
  Cc: Matthias Brugger, chunfeng.yun, linux-usb, linux-kernel,
	linux-arm-kernel, linux-mediatek

Hi greg k-h:
On Wed, 2020-11-18 at 12:48 +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 17, 2020 at 04:21:25PM +0800, min.guo@mediatek.com wrote:
> > From: Min Guo <min.guo@mediatek.com>
> > 
> > Remove unused 'devctl' variable to fix compile warnings:
> > 
> >     drivers/usb/musb/musbhsdma.c: In function 'dma_controller_irq':
> >     drivers/usb/musb/musbhsdma.c:324:8: warning: variable 'devctl' set
> >     but not used [-Wunused-but-set-variable]
> > 
> > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > ---
> >  drivers/usb/musb/musbhsdma.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > index 0aacfc8be5a1..7acd1635850d 100644
> > --- a/drivers/usb/musb/musbhsdma.c
> > +++ b/drivers/usb/musb/musbhsdma.c
> > @@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> >  				musb_channel->channel.status =
> >  					MUSB_DMA_STATUS_BUS_ABORT;
> >  			} else {
> > -				u8 devctl;
> > -
> >  				addr = musb_read_hsdma_addr(mbase,
> >  						bchannel);
> >  				channel->actual_len = addr
> > @@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> >  						< musb_channel->len) ?
> >  					"=> reconfig 0" : "=> complete");
> >  
> > -				devctl = musb_readb(mbase, MUSB_DEVCTL);
> 
> Are you sure that the hardware does not require this read to complete
> the command?  Lots of hardware is that way, so be very careful about
> this.  Did you test it?

I have tested this patch on Mediatek's platform, and not sure if it
will affect other vendors' platforms.

Dear Bin:

Does this patch will affect other vendors' platforms?

Best Regards,
Min

> thanks,
> 
> greg k-h


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: musb: remove unused variable 'devctl'
  2020-11-20  6:48   ` Min Guo
@ 2020-11-20  6:54     ` Greg Kroah-Hartman
  2020-11-20  7:42       ` Min Guo
  0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-20  6:54 UTC (permalink / raw)
  To: Min Guo
  Cc: Bin Liu, Matthias Brugger, chunfeng.yun, linux-usb, linux-kernel,
	linux-arm-kernel, linux-mediatek

On Fri, Nov 20, 2020 at 02:48:50PM +0800, Min Guo wrote:
> Hi greg k-h:
> On Wed, 2020-11-18 at 12:48 +0100, Greg Kroah-Hartman wrote:
> > On Tue, Nov 17, 2020 at 04:21:25PM +0800, min.guo@mediatek.com wrote:
> > > From: Min Guo <min.guo@mediatek.com>
> > > 
> > > Remove unused 'devctl' variable to fix compile warnings:
> > > 
> > >     drivers/usb/musb/musbhsdma.c: In function 'dma_controller_irq':
> > >     drivers/usb/musb/musbhsdma.c:324:8: warning: variable 'devctl' set
> > >     but not used [-Wunused-but-set-variable]
> > > 
> > > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > > ---
> > >  drivers/usb/musb/musbhsdma.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > > index 0aacfc8be5a1..7acd1635850d 100644
> > > --- a/drivers/usb/musb/musbhsdma.c
> > > +++ b/drivers/usb/musb/musbhsdma.c
> > > @@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > >  				musb_channel->channel.status =
> > >  					MUSB_DMA_STATUS_BUS_ABORT;
> > >  			} else {
> > > -				u8 devctl;
> > > -
> > >  				addr = musb_read_hsdma_addr(mbase,
> > >  						bchannel);
> > >  				channel->actual_len = addr
> > > @@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > >  						< musb_channel->len) ?
> > >  					"=> reconfig 0" : "=> complete");
> > >  
> > > -				devctl = musb_readb(mbase, MUSB_DEVCTL);
> > 
> > Are you sure that the hardware does not require this read to complete
> > the command?  Lots of hardware is that way, so be very careful about
> > this.  Did you test it?
> 
> I have tested this patch on Mediatek's platform, and not sure if it
> will affect other vendors' platforms.
> 
> Dear Bin:
> 
> Does this patch will affect other vendors' platforms?

The hardware specs will answer this question, what do they say about
this read?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: musb: remove unused variable 'devctl'
  2020-11-20  6:54     ` Greg Kroah-Hartman
@ 2020-11-20  7:42       ` Min Guo
  2020-11-20  8:36         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 11+ messages in thread
From: Min Guo @ 2020-11-20  7:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Bin Liu, Matthias Brugger, chunfeng.yun, linux-usb, linux-kernel,
	linux-arm-kernel, linux-mediatek

On Fri, 2020-11-20 at 07:54 +0100, Greg Kroah-Hartman wrote:
> On Fri, Nov 20, 2020 at 02:48:50PM +0800, Min Guo wrote:
> > Hi greg k-h:
> > On Wed, 2020-11-18 at 12:48 +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Nov 17, 2020 at 04:21:25PM +0800, min.guo@mediatek.com wrote:
> > > > From: Min Guo <min.guo@mediatek.com>
> > > > 
> > > > Remove unused 'devctl' variable to fix compile warnings:
> > > > 
> > > >     drivers/usb/musb/musbhsdma.c: In function 'dma_controller_irq':
> > > >     drivers/usb/musb/musbhsdma.c:324:8: warning: variable 'devctl' set
> > > >     but not used [-Wunused-but-set-variable]
> > > > 
> > > > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > > > ---
> > > >  drivers/usb/musb/musbhsdma.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > > > index 0aacfc8be5a1..7acd1635850d 100644
> > > > --- a/drivers/usb/musb/musbhsdma.c
> > > > +++ b/drivers/usb/musb/musbhsdma.c
> > > > @@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > >  				musb_channel->channel.status =
> > > >  					MUSB_DMA_STATUS_BUS_ABORT;
> > > >  			} else {
> > > > -				u8 devctl;
> > > > -
> > > >  				addr = musb_read_hsdma_addr(mbase,
> > > >  						bchannel);
> > > >  				channel->actual_len = addr
> > > > @@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > >  						< musb_channel->len) ?
> > > >  					"=> reconfig 0" : "=> complete");
> > > >  
> > > > -				devctl = musb_readb(mbase, MUSB_DEVCTL);
> > > 
> > > Are you sure that the hardware does not require this read to complete
> > > the command?  Lots of hardware is that way, so be very careful about
> > > this.  Did you test it?
> > 
> > I have tested this patch on Mediatek's platform, and not sure if it
> > will affect other vendors' platforms.
> > 
> > Dear Bin:
> > 
> > Does this patch will affect other vendors' platforms?
> 
> The hardware specs will answer this question, what do they say about
> this read?

Sorry, I didn't seen the comment on the hardware specs indicate that
devctl register needs to read once to take effect.

> thanks,
> 
> greg k-h


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: musb: remove unused variable 'devctl'
  2020-11-20  7:42       ` Min Guo
@ 2020-11-20  8:36         ` Greg Kroah-Hartman
  2020-11-20  9:02           ` Min Guo
  2020-11-20 16:15           ` Alan Stern
  0 siblings, 2 replies; 11+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-20  8:36 UTC (permalink / raw)
  To: Min Guo
  Cc: Bin Liu, Matthias Brugger, chunfeng.yun, linux-usb, linux-kernel,
	linux-arm-kernel, linux-mediatek

On Fri, Nov 20, 2020 at 03:42:06PM +0800, Min Guo wrote:
> On Fri, 2020-11-20 at 07:54 +0100, Greg Kroah-Hartman wrote:
> > On Fri, Nov 20, 2020 at 02:48:50PM +0800, Min Guo wrote:
> > > Hi greg k-h:
> > > On Wed, 2020-11-18 at 12:48 +0100, Greg Kroah-Hartman wrote:
> > > > On Tue, Nov 17, 2020 at 04:21:25PM +0800, min.guo@mediatek.com wrote:
> > > > > From: Min Guo <min.guo@mediatek.com>
> > > > > 
> > > > > Remove unused 'devctl' variable to fix compile warnings:
> > > > > 
> > > > >     drivers/usb/musb/musbhsdma.c: In function 'dma_controller_irq':
> > > > >     drivers/usb/musb/musbhsdma.c:324:8: warning: variable 'devctl' set
> > > > >     but not used [-Wunused-but-set-variable]
> > > > > 
> > > > > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > > > > ---
> > > > >  drivers/usb/musb/musbhsdma.c | 4 ----
> > > > >  1 file changed, 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > > > > index 0aacfc8be5a1..7acd1635850d 100644
> > > > > --- a/drivers/usb/musb/musbhsdma.c
> > > > > +++ b/drivers/usb/musb/musbhsdma.c
> > > > > @@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > > >  				musb_channel->channel.status =
> > > > >  					MUSB_DMA_STATUS_BUS_ABORT;
> > > > >  			} else {
> > > > > -				u8 devctl;
> > > > > -
> > > > >  				addr = musb_read_hsdma_addr(mbase,
> > > > >  						bchannel);
> > > > >  				channel->actual_len = addr
> > > > > @@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > > >  						< musb_channel->len) ?
> > > > >  					"=> reconfig 0" : "=> complete");
> > > > >  
> > > > > -				devctl = musb_readb(mbase, MUSB_DEVCTL);
> > > > 
> > > > Are you sure that the hardware does not require this read to complete
> > > > the command?  Lots of hardware is that way, so be very careful about
> > > > this.  Did you test it?
> > > 
> > > I have tested this patch on Mediatek's platform, and not sure if it
> > > will affect other vendors' platforms.
> > > 
> > > Dear Bin:
> > > 
> > > Does this patch will affect other vendors' platforms?
> > 
> > The hardware specs will answer this question, what do they say about
> > this read?
> 
> Sorry, I didn't seen the comment on the hardware specs indicate that
> devctl register needs to read once to take effect.

Perhaps you might want to add a comment here so that people will not
keep making this same mistake when they run auto-checkers on the
codebase?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: musb: remove unused variable 'devctl'
  2020-11-20  8:36         ` Greg Kroah-Hartman
@ 2020-11-20  9:02           ` Min Guo
  2020-11-20  9:20             ` Greg Kroah-Hartman
  2020-11-20 16:15           ` Alan Stern
  1 sibling, 1 reply; 11+ messages in thread
From: Min Guo @ 2020-11-20  9:02 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Bin Liu, Matthias Brugger, chunfeng.yun, linux-usb, linux-kernel,
	linux-arm-kernel, linux-mediatek

On Fri, 2020-11-20 at 09:36 +0100, Greg Kroah-Hartman wrote:
> On Fri, Nov 20, 2020 at 03:42:06PM +0800, Min Guo wrote:
> > On Fri, 2020-11-20 at 07:54 +0100, Greg Kroah-Hartman wrote:
> > > On Fri, Nov 20, 2020 at 02:48:50PM +0800, Min Guo wrote:
> > > > Hi greg k-h:
> > > > On Wed, 2020-11-18 at 12:48 +0100, Greg Kroah-Hartman wrote:
> > > > > On Tue, Nov 17, 2020 at 04:21:25PM +0800, min.guo@mediatek.com wrote:
> > > > > > From: Min Guo <min.guo@mediatek.com>
> > > > > > 
> > > > > > Remove unused 'devctl' variable to fix compile warnings:
> > > > > > 
> > > > > >     drivers/usb/musb/musbhsdma.c: In function 'dma_controller_irq':
> > > > > >     drivers/usb/musb/musbhsdma.c:324:8: warning: variable 'devctl' set
> > > > > >     but not used [-Wunused-but-set-variable]
> > > > > > 
> > > > > > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > > > > > ---
> > > > > >  drivers/usb/musb/musbhsdma.c | 4 ----
> > > > > >  1 file changed, 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > > > > > index 0aacfc8be5a1..7acd1635850d 100644
> > > > > > --- a/drivers/usb/musb/musbhsdma.c
> > > > > > +++ b/drivers/usb/musb/musbhsdma.c
> > > > > > @@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > > > >  				musb_channel->channel.status =
> > > > > >  					MUSB_DMA_STATUS_BUS_ABORT;
> > > > > >  			} else {
> > > > > > -				u8 devctl;
> > > > > > -
> > > > > >  				addr = musb_read_hsdma_addr(mbase,
> > > > > >  						bchannel);
> > > > > >  				channel->actual_len = addr
> > > > > > @@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > > > >  						< musb_channel->len) ?
> > > > > >  					"=> reconfig 0" : "=> complete");
> > > > > >  
> > > > > > -				devctl = musb_readb(mbase, MUSB_DEVCTL);
> > > > > 
> > > > > Are you sure that the hardware does not require this read to complete
> > > > > the command?  Lots of hardware is that way, so be very careful about
> > > > > this.  Did you test it?
> > > > 
> > > > I have tested this patch on Mediatek's platform, and not sure if it
> > > > will affect other vendors' platforms.
> > > > 
> > > > Dear Bin:
> > > > 
> > > > Does this patch will affect other vendors' platforms?
> > > 
> > > The hardware specs will answer this question, what do they say about
> > > this read?
> > 
> > Sorry, I didn't seen the comment on the hardware specs indicate that
> > devctl register needs to read once to take effect.
> 
> Perhaps you might want to add a comment here so that people will not
> keep making this same mistake when they run auto-checkers on the
> codebase?

Sorry for confused you, I searched the hardware specs, but did not find
the special description of the register devctl indicating that it needs
to be read out before the hardware can work.

> thanks,
> 
> greg k-h


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: musb: remove unused variable 'devctl'
  2020-11-20  9:02           ` Min Guo
@ 2020-11-20  9:20             ` Greg Kroah-Hartman
  0 siblings, 0 replies; 11+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-20  9:20 UTC (permalink / raw)
  To: Min Guo
  Cc: Bin Liu, Matthias Brugger, chunfeng.yun, linux-usb, linux-kernel,
	linux-arm-kernel, linux-mediatek

On Fri, Nov 20, 2020 at 05:02:15PM +0800, Min Guo wrote:
> On Fri, 2020-11-20 at 09:36 +0100, Greg Kroah-Hartman wrote:
> > On Fri, Nov 20, 2020 at 03:42:06PM +0800, Min Guo wrote:
> > > On Fri, 2020-11-20 at 07:54 +0100, Greg Kroah-Hartman wrote:
> > > > On Fri, Nov 20, 2020 at 02:48:50PM +0800, Min Guo wrote:
> > > > > Hi greg k-h:
> > > > > On Wed, 2020-11-18 at 12:48 +0100, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Nov 17, 2020 at 04:21:25PM +0800, min.guo@mediatek.com wrote:
> > > > > > > From: Min Guo <min.guo@mediatek.com>
> > > > > > > 
> > > > > > > Remove unused 'devctl' variable to fix compile warnings:
> > > > > > > 
> > > > > > >     drivers/usb/musb/musbhsdma.c: In function 'dma_controller_irq':
> > > > > > >     drivers/usb/musb/musbhsdma.c:324:8: warning: variable 'devctl' set
> > > > > > >     but not used [-Wunused-but-set-variable]
> > > > > > > 
> > > > > > > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > > > > > > ---
> > > > > > >  drivers/usb/musb/musbhsdma.c | 4 ----
> > > > > > >  1 file changed, 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > > > > > > index 0aacfc8be5a1..7acd1635850d 100644
> > > > > > > --- a/drivers/usb/musb/musbhsdma.c
> > > > > > > +++ b/drivers/usb/musb/musbhsdma.c
> > > > > > > @@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > > > > >  				musb_channel->channel.status =
> > > > > > >  					MUSB_DMA_STATUS_BUS_ABORT;
> > > > > > >  			} else {
> > > > > > > -				u8 devctl;
> > > > > > > -
> > > > > > >  				addr = musb_read_hsdma_addr(mbase,
> > > > > > >  						bchannel);
> > > > > > >  				channel->actual_len = addr
> > > > > > > @@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > > > > >  						< musb_channel->len) ?
> > > > > > >  					"=> reconfig 0" : "=> complete");
> > > > > > >  
> > > > > > > -				devctl = musb_readb(mbase, MUSB_DEVCTL);
> > > > > > 
> > > > > > Are you sure that the hardware does not require this read to complete
> > > > > > the command?  Lots of hardware is that way, so be very careful about
> > > > > > this.  Did you test it?
> > > > > 
> > > > > I have tested this patch on Mediatek's platform, and not sure if it
> > > > > will affect other vendors' platforms.
> > > > > 
> > > > > Dear Bin:
> > > > > 
> > > > > Does this patch will affect other vendors' platforms?
> > > > 
> > > > The hardware specs will answer this question, what do they say about
> > > > this read?
> > > 
> > > Sorry, I didn't seen the comment on the hardware specs indicate that
> > > devctl register needs to read once to take effect.
> > 
> > Perhaps you might want to add a comment here so that people will not
> > keep making this same mistake when they run auto-checkers on the
> > codebase?
> 
> Sorry for confused you, I searched the hardware specs, but did not find
> the special description of the register devctl indicating that it needs
> to be read out before the hardware can work.

If this is a PCI device, that is implied as that is how all PCI devices
work, right?

What is the problem that is fixed by removing this read?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: musb: remove unused variable 'devctl'
  2020-11-20  8:36         ` Greg Kroah-Hartman
  2020-11-20  9:02           ` Min Guo
@ 2020-11-20 16:15           ` Alan Stern
  2020-11-20 16:32             ` Greg Kroah-Hartman
  1 sibling, 1 reply; 11+ messages in thread
From: Alan Stern @ 2020-11-20 16:15 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Min Guo, Bin Liu, Matthias Brugger, chunfeng.yun, linux-usb,
	linux-kernel, linux-arm-kernel, linux-mediatek

On Fri, Nov 20, 2020 at 09:36:33AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Nov 20, 2020 at 03:42:06PM +0800, Min Guo wrote:
> > On Fri, 2020-11-20 at 07:54 +0100, Greg Kroah-Hartman wrote:
> > > On Fri, Nov 20, 2020 at 02:48:50PM +0800, Min Guo wrote:
> > > > Hi greg k-h:
> > > > On Wed, 2020-11-18 at 12:48 +0100, Greg Kroah-Hartman wrote:
> > > > > On Tue, Nov 17, 2020 at 04:21:25PM +0800, min.guo@mediatek.com wrote:
> > > > > > From: Min Guo <min.guo@mediatek.com>
> > > > > > 
> > > > > > Remove unused 'devctl' variable to fix compile warnings:
> > > > > > 
> > > > > >     drivers/usb/musb/musbhsdma.c: In function 'dma_controller_irq':
> > > > > >     drivers/usb/musb/musbhsdma.c:324:8: warning: variable 'devctl' set
> > > > > >     but not used [-Wunused-but-set-variable]
> > > > > > 
> > > > > > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > > > > > ---
> > > > > >  drivers/usb/musb/musbhsdma.c | 4 ----
> > > > > >  1 file changed, 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > > > > > index 0aacfc8be5a1..7acd1635850d 100644
> > > > > > --- a/drivers/usb/musb/musbhsdma.c
> > > > > > +++ b/drivers/usb/musb/musbhsdma.c
> > > > > > @@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > > > >  				musb_channel->channel.status =
> > > > > >  					MUSB_DMA_STATUS_BUS_ABORT;
> > > > > >  			} else {
> > > > > > -				u8 devctl;
> > > > > > -
> > > > > >  				addr = musb_read_hsdma_addr(mbase,
> > > > > >  						bchannel);
> > > > > >  				channel->actual_len = addr
> > > > > > @@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > > > >  						< musb_channel->len) ?
> > > > > >  					"=> reconfig 0" : "=> complete");
> > > > > >  
> > > > > > -				devctl = musb_readb(mbase, MUSB_DEVCTL);
> > > > > 
> > > > > Are you sure that the hardware does not require this read to complete
> > > > > the command?  Lots of hardware is that way, so be very careful about
> > > > > this.  Did you test it?
> > > > 
> > > > I have tested this patch on Mediatek's platform, and not sure if it
> > > > will affect other vendors' platforms.
> > > > 
> > > > Dear Bin:
> > > > 
> > > > Does this patch will affect other vendors' platforms?
> > > 
> > > The hardware specs will answer this question, what do they say about
> > > this read?
> > 
> > Sorry, I didn't seen the comment on the hardware specs indicate that
> > devctl register needs to read once to take effect.
> 
> Perhaps you might want to add a comment here so that people will not
> keep making this same mistake when they run auto-checkers on the
> codebase?

A better change would be

-			devctl = musb_readb(mbase, MUSB_DEVCTL);
+			(void) musb_readb(mbase, MUSB_DEVCTL);

and eliminate the unused variable.  Then there wouldn't be any compiler 
warning.

Alan Stern

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: musb: remove unused variable 'devctl'
  2020-11-20 16:15           ` Alan Stern
@ 2020-11-20 16:32             ` Greg Kroah-Hartman
  2020-11-20 16:37               ` Alan Stern
  0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-20 16:32 UTC (permalink / raw)
  To: Alan Stern
  Cc: Min Guo, Bin Liu, Matthias Brugger, chunfeng.yun, linux-usb,
	linux-kernel, linux-arm-kernel, linux-mediatek

On Fri, Nov 20, 2020 at 11:15:19AM -0500, Alan Stern wrote:
> On Fri, Nov 20, 2020 at 09:36:33AM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Nov 20, 2020 at 03:42:06PM +0800, Min Guo wrote:
> > > On Fri, 2020-11-20 at 07:54 +0100, Greg Kroah-Hartman wrote:
> > > > On Fri, Nov 20, 2020 at 02:48:50PM +0800, Min Guo wrote:
> > > > > Hi greg k-h:
> > > > > On Wed, 2020-11-18 at 12:48 +0100, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Nov 17, 2020 at 04:21:25PM +0800, min.guo@mediatek.com wrote:
> > > > > > > From: Min Guo <min.guo@mediatek.com>
> > > > > > > 
> > > > > > > Remove unused 'devctl' variable to fix compile warnings:
> > > > > > > 
> > > > > > >     drivers/usb/musb/musbhsdma.c: In function 'dma_controller_irq':
> > > > > > >     drivers/usb/musb/musbhsdma.c:324:8: warning: variable 'devctl' set
> > > > > > >     but not used [-Wunused-but-set-variable]
> > > > > > > 
> > > > > > > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > > > > > > ---
> > > > > > >  drivers/usb/musb/musbhsdma.c | 4 ----
> > > > > > >  1 file changed, 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > > > > > > index 0aacfc8be5a1..7acd1635850d 100644
> > > > > > > --- a/drivers/usb/musb/musbhsdma.c
> > > > > > > +++ b/drivers/usb/musb/musbhsdma.c
> > > > > > > @@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > > > > >  				musb_channel->channel.status =
> > > > > > >  					MUSB_DMA_STATUS_BUS_ABORT;
> > > > > > >  			} else {
> > > > > > > -				u8 devctl;
> > > > > > > -
> > > > > > >  				addr = musb_read_hsdma_addr(mbase,
> > > > > > >  						bchannel);
> > > > > > >  				channel->actual_len = addr
> > > > > > > @@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
> > > > > > >  						< musb_channel->len) ?
> > > > > > >  					"=> reconfig 0" : "=> complete");
> > > > > > >  
> > > > > > > -				devctl = musb_readb(mbase, MUSB_DEVCTL);
> > > > > > 
> > > > > > Are you sure that the hardware does not require this read to complete
> > > > > > the command?  Lots of hardware is that way, so be very careful about
> > > > > > this.  Did you test it?
> > > > > 
> > > > > I have tested this patch on Mediatek's platform, and not sure if it
> > > > > will affect other vendors' platforms.
> > > > > 
> > > > > Dear Bin:
> > > > > 
> > > > > Does this patch will affect other vendors' platforms?
> > > > 
> > > > The hardware specs will answer this question, what do they say about
> > > > this read?
> > > 
> > > Sorry, I didn't seen the comment on the hardware specs indicate that
> > > devctl register needs to read once to take effect.
> > 
> > Perhaps you might want to add a comment here so that people will not
> > keep making this same mistake when they run auto-checkers on the
> > codebase?
> 
> A better change would be
> 
> -			devctl = musb_readb(mbase, MUSB_DEVCTL);
> +			(void) musb_readb(mbase, MUSB_DEVCTL);
> 
> and eliminate the unused variable.  Then there wouldn't be any compiler 
> warning.

No need for the (void), the compiler shouldn't warn about that, right?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: musb: remove unused variable 'devctl'
  2020-11-20 16:32             ` Greg Kroah-Hartman
@ 2020-11-20 16:37               ` Alan Stern
  0 siblings, 0 replies; 11+ messages in thread
From: Alan Stern @ 2020-11-20 16:37 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Min Guo, Bin Liu, Matthias Brugger, chunfeng.yun, linux-usb,
	linux-kernel, linux-arm-kernel, linux-mediatek

On Fri, Nov 20, 2020 at 05:32:44PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Nov 20, 2020 at 11:15:19AM -0500, Alan Stern wrote:
> > > Perhaps you might want to add a comment here so that people will not
> > > keep making this same mistake when they run auto-checkers on the
> > > codebase?
> > 
> > A better change would be
> > 
> > -			devctl = musb_readb(mbase, MUSB_DEVCTL);
> > +			(void) musb_readb(mbase, MUSB_DEVCTL);
> > 
> > and eliminate the unused variable.  Then there wouldn't be any compiler 
> > warning.
> 
> No need for the (void), the compiler shouldn't warn about that, right?

True, but it clearly indicates to a human reader that the value was 
intended to be read and thrown away.  Alternatively, the (void) cast 
could be left out and a comment added.

Alan Stern

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-11-20 16:37 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-17  8:21 [PATCH] usb: musb: remove unused variable 'devctl' min.guo
2020-11-18 11:48 ` Greg Kroah-Hartman
2020-11-20  6:48   ` Min Guo
2020-11-20  6:54     ` Greg Kroah-Hartman
2020-11-20  7:42       ` Min Guo
2020-11-20  8:36         ` Greg Kroah-Hartman
2020-11-20  9:02           ` Min Guo
2020-11-20  9:20             ` Greg Kroah-Hartman
2020-11-20 16:15           ` Alan Stern
2020-11-20 16:32             ` Greg Kroah-Hartman
2020-11-20 16:37               ` Alan Stern

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).