[PATCH] staging: rts5208: Replace udelay() with usleep_range()

[PATCH] staging: rts5208: Replace udelay() with usleep_range()

[Mamta Shukla]

    udelay(unsigned long usec) replaced with usleep_range(unsigned long
    min, unsigned long max) to prevent trigger by interrupt in specified
    duration.Also provides better scheduling,power management for drivers.

    Issue found with checkpatch.pl

Signed-off-by: Mamta Shukla <mamtashukla555@gmail.com>
---
 drivers/staging/rts5208/ms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rts5208/ms.c b/drivers/staging/rts5208/ms.c
index f53adf1..6654cdd 100644
--- a/drivers/staging/rts5208/ms.c
+++ b/drivers/staging/rts5208/ms.c
@@ -3245,7 +3245,7 @@ static int ms_write_multiple_pages(struct rtsx_chip *chip, u16 old_blk,
 			return STATUS_FAIL;
 		}
 
-		udelay(30);
+		usleep_range(30, 60);
 
 		rtsx_init_cmd(chip);
 
@@ -4167,7 +4167,7 @@ int mg_set_ICV(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 
 #ifdef MG_SET_ICV_SLOW
 	for (i = 0; i < 2; i++) {
-		udelay(50);
+		usleep_range(50, 100);
 
 		rtsx_init_cmd(chip);
 
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] staging: rts5208: Replace udelay() with usleep_range()

[Julia Lawall]

On Tue, 2 Oct 2018, Mamta Shukla wrote:

>     udelay(unsigned long usec) replaced with usleep_range(unsigned long
>     min, unsigned long max) to prevent trigger by interrupt in specified
>     duration.Also provides better scheduling,power management for drivers.

Thanks for the patch.  A few picky comments.  In English, it is more
readable to put some space after punctuation.  It is also not necessary to
indent the log message.

Also, it would be good to explain how you chose the upper limit.

julia


>
>     Issue found with checkpatch.pl
>
> Signed-off-by: Mamta Shukla <mamtashukla555@gmail.com>
> ---
>  drivers/staging/rts5208/ms.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rts5208/ms.c b/drivers/staging/rts5208/ms.c
> index f53adf1..6654cdd 100644
> --- a/drivers/staging/rts5208/ms.c
> +++ b/drivers/staging/rts5208/ms.c
> @@ -3245,7 +3245,7 @@ static int ms_write_multiple_pages(struct rtsx_chip *chip, u16 old_blk,
>  			return STATUS_FAIL;
>  		}
>
> -		udelay(30);
> +		usleep_range(30, 60);
>
>  		rtsx_init_cmd(chip);
>
> @@ -4167,7 +4167,7 @@ int mg_set_ICV(struct scsi_cmnd *srb, struct rtsx_chip *chip)
>
>  #ifdef MG_SET_ICV_SLOW
>  	for (i = 0; i < 2; i++) {
> -		udelay(50);
> +		usleep_range(50, 100);
>
>  		rtsx_init_cmd(chip);
>
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20181002130124.GA27163%40armorer.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] staging: rts5208: Replace udelay() with usleep_range()

[Greg KH]

On Tue, Oct 02, 2018 at 03:07:14PM +0200, Julia Lawall wrote:
> 
> 
> On Tue, 2 Oct 2018, Mamta Shukla wrote:
> 
> >     udelay(unsigned long usec) replaced with usleep_range(unsigned long
> >     min, unsigned long max) to prevent trigger by interrupt in specified
> >     duration.Also provides better scheduling,power management for drivers.
> 
> Thanks for the patch.  A few picky comments.  In English, it is more
> readable to put some space after punctuation.  It is also not necessary to
> indent the log message.
> 
> Also, it would be good to explain how you chose the upper limit.

It is _required_ to explain this.  This change comes up every so often,
and I keep rejecting it as no one knows what value to put here that will
correctly work.  So I recommend just leaving it alone unless you hav the
hardware and can test this type of change properly.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] staging: rts5208: Replace udelay() with usleep_range()

[Mamta Shukla]

[-- Attachment #1: Type: text/plain, Size: 1667 bytes --]

On Tue, Oct 2, 2018 at 6:43 PM Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Oct 02, 2018 at 03:07:14PM +0200, Julia Lawall wrote:
> >
> >
> > On Tue, 2 Oct 2018, Mamta Shukla wrote:
> >
> > >     udelay(unsigned long usec) replaced with usleep_range(unsigned long
> > >     min, unsigned long max) to prevent trigger by interrupt in
> specified
> > >     duration.Also provides better scheduling,power management for
> drivers.
> >
> > Thanks for the patch.  A few picky comments.  In English, it is more
> > readable to put some space after punctuation.  It is also not necessary
> to
> > indent the log message.
> >
> > Also, it would be good to explain how you chose the upper limit.
>
  Thank you for feedback Julia.I will keep in mind all these  while writing
next patch.
  I replaced udelay() with usleep_range to improve the responsiveness. I
considered these values
  to be equivalent for values by udelay(). But as mentioned by Greg , since
this depends on response
  time  of hardware and no exact value can be decided.

>
> It is _required_ to explain this.  This change comes up every so often,
> and I keep rejecting it as no one knows what value to put here that will
> correctly work.  So I recommend just leaving it alone unless you hav the
> hardware and can test this type of change properly.
>
> thanks,
>
> greg k-h
>
 Thank you for feedback Greg. I understood the hardware dependency for this
driver.
 Is it possible to remove this delay ?  This driver needs DMA accessibility
in Linux.
 So if one has to develop PCIe driver for any processor(intel, altera),
there should be
 different driver for each platform.


Thanks,

Mamta Shukla

[-- Attachment #2: Type: text/html, Size: 2574 bytes --]

Re: [Outreachy kernel] [PATCH] staging: rts5208: Replace udelay() with usleep_range()

[Greg KH]

On Tue, Oct 02, 2018 at 08:24:55PM +0530, Mamta Shukla wrote:
> > It is _required_ to explain this.  This change comes up every so often,
> > and I keep rejecting it as no one knows what value to put here that will
> > correctly work.  So I recommend just leaving it alone unless you hav the
> > hardware and can test this type of change properly.
> >
> >
>  Thank you for feedback Greg. I understood the hardware dependency for this
> driver.
>  Is it possible to remove this delay ?

It was probably added on purpose, why do you think it can be removed?

> This driver needs DMA accessibility in Linux.

What do you mean by this?  What does DMA have to do with this driver?

>  So if one has to develop PCIe driver for any processor(intel, altera),
> there should be different driver for each platform.

No, that is not how Linux drivers are written.  They should work on all
hardware platforms that the hardware can be plugged into.  Any
platform-specific issues are handled in bus-specific or architecture
specific drivers.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] staging: rts5208: Replace udelay() with usleep_range()

[Himanshu Jha]

On Tue, Oct 02, 2018 at 08:24:55PM +0530, Mamta Shukla wrote:
> On Tue, Oct 2, 2018 at 6:43 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Oct 02, 2018 at 03:07:14PM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Tue, 2 Oct 2018, Mamta Shukla wrote:
> > >
> > > >     udelay(unsigned long usec) replaced with usleep_range(unsigned long
> > > >     min, unsigned long max) to prevent trigger by interrupt in
> > specified
> > > >     duration.Also provides better scheduling,power management for
> > drivers.
> > >
> > > Thanks for the patch.  A few picky comments.  In English, it is more
> > > readable to put some space after punctuation.  It is also not necessary
> > to
> > > indent the log message.
> > >
> > > Also, it would be good to explain how you chose the upper limit.
> >
>   Thank you for feedback Julia.I will keep in mind all these  while writing
> next patch.
>   I replaced udelay() with usleep_range to improve the responsiveness. I
> considered these values
>   to be equivalent for values by udelay(). But as mentioned by Greg , since
> this depends on response
>   time  of hardware and no exact value can be decided.

The selection b/w udelay() Vs usleep_range() always gets fuzzy. And it
is often advised to only play with these if you have the hardware and
can the test/verfiy the change.

Selections also depends on Atomic Vs Non-atomic context ...


-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology