From mboxrd@z Thu Jan 1 00:00:00 1970 X-GM-THRID: 6392907056098050048 Date: Thu, 2 Mar 2017 07:17:07 -0800 (PST) From: SIMRAN SINGHAL To: outreachy-kernel Cc: singhalsimran0@gmail.com, marvin24@gmx.de, gregkh@linuxfoundation.org, ac100@lists.launchpad.net, linux-tegra@vger.kernel.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Message-Id: <4958c8a8-4b50-4567-91d8-9554e9bdf7f6@googlegroups.com> In-Reply-To: References: <20170302142418.GA16773@singhal-Inspiron-5558> Subject: Re: [Outreachy kernel] [PATCH] staging: nvec: cleanup USLEEP_RANGE checkpatch checks MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----=_Part_970_585911967.1488467827387" X-Google-Token: EPPu4MUFh-n8rPTgdAw0 X-Google-IP: 14.139.82.6 ------=_Part_970_585911967.1488467827387 Content-Type: multipart/alternative; boundary="----=_Part_971_825758182.1488467827387" ------=_Part_971_825758182.1488467827387 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On Thursday, March 2, 2017 at 8:31:23 PM UTC+5:30, Julia Lawall wrote: > > > > On Thu, 2 Mar 2017, SIMRAN SINGHAL wrote: > > > > > > > On Thursday, March 2, 2017 at 8:06:40 PM UTC+5:30, Julia Lawall wrote: > > > > > > On Thu, 2 Mar 2017, simran singhal wrote: > > > > > Resolve strict checkpatch USLEEP_RANGE checks by converting > > delays and > > > sleeps as described in > > ./Documentation/timers/timers-howto.txt. > > > > > > CHECK: usleep_range is preferred over udelay; see > > Documentation/ > > > timers/timers-howto.txt > > > > > > Signed-off-by: simran singhal > > > --- > > > drivers/staging/nvec/nvec.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/staging/nvec/nvec.c > > b/drivers/staging/nvec/nvec.c > > > index c1feccf..cd35e64 100644 > > > --- a/drivers/staging/nvec/nvec.c > > > +++ b/drivers/staging/nvec/nvec.c > > > @@ -631,7 +631,7 @@ static irqreturn_t nvec_interrupt(int irq, > > void *dev) > > > break; > > > case 2: /* first byte after command */ > > > if (status == (I2C_SL_IRQ | RNW | RCVD)) { > > > - udelay(33); > > > + usleep_range(33, 100); > > > > How did you choose the upper limit. > > > > I believe that Greg previously suggested not to make these > > changes if you > > have no way to test them. > > > > Julia, After going through the reply given by Nicholas Mc Guire > > > https://www.mail-archive.com/kernelnewbies@kernelnewbies.org/msg16464.html > > in this reply he has mentioned that even the range of 10 microsecond is > > enough, > > so I prefer to take 100 as upper limit. > > Than you for the link. > > It looks like he suggests to change 33 to 30-40, not to 33-100. In any > case, you have three choices for this kind of issue: > 1. Don't make the change, because you can't test the result > 2. Make the change, and explain the commit log what your rationale is > 3. Make the change, and explain below the --- that you have no idea what > you are doing, and you are just proposing the patch as something concrete > to start a discussion. > > But your preference is not a suitable justification. The hardware does > something, and the choice can only really be made by the person who knows > what it does. > > Thanks, Julia I'll keep this in mind from next time. I choose the range from 33 to 100 for being on more safer side. Should I make it 30-40 and send v2. julia > > > > > Simran > > > > julia > > > > > > > if (nvec->rx->data[0] != 0x01) { > > > dev_err(nvec->dev, > > > "Read without prior > > read command\n"); > > > @@ -718,7 +718,7 @@ static irqreturn_t nvec_interrupt(int irq, > > void *dev) > > > * We experience less incomplete messages with this > > delay than without > > > * it, but we don't know why. Help is appreciated. > > > */ > > > - udelay(100); > > > + usleep_range(100, 200); > > > > > > return IRQ_HANDLED; > > > } > > > -- > > > 2.7.4 > > > > > > -- > > > 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-kern...@googlegroups.com. > > > To post to this group, send email to > > outreach...@googlegroups.com. > > > To view this discussion on the web visithttps:// > groups.google.com/d/msgid/outreachy-kernel/20170302142418.GA16773%4 > > 0singhal-Inspiron-5558. > > > For more options, visit https://groups.google.com/d/optout. > > > > > > > -- > > 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-kern...@googlegroups.com . > > To post to this group, send email to outreach...@googlegroups.com > . > > To view this discussion on the web visithttps:// > groups.google.com/d/msgid/outreachy-kernel/b90bc602-cf06-4abb-bea2- > > 6386d4976864%40googlegroups.com. > > For more options, visit https://groups.google.com/d/optout. > > > > ------=_Part_971_825758182.1488467827387 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable


On Thursday, March 2, 2017 at 8:31:23 PM UTC+5:30,= Julia Lawall wrote:


On Thu, 2 Mar 2017, SIMRAN SINGHAL wrote:

>
>
> On Thursday, March 2, 2017 at 8:06:40 PM UTC+5:30, Julia Lawall wr= ote:
>
>
> =C2=A0 =C2=A0 =C2=A0 On Thu, 2 Mar 2017, simran singhal wrote:
>
> =C2=A0 =C2=A0 =C2=A0 > Resolve strict checkpatch USLEEP_RANGE c= hecks by converting
> =C2=A0 =C2=A0 =C2=A0 delays and
> =C2=A0 =C2=A0 =C2=A0 > sleeps as described in
> =C2=A0 =C2=A0 =C2=A0 ./Documentation/timers/timers-howto.txt.
> =C2=A0 =C2=A0 =C2=A0 >
> =C2=A0 =C2=A0 =C2=A0 > CHECK: usleep_range is preferred over ud= elay; see
> =C2=A0 =C2=A0 =C2=A0 Documentation/
> =C2=A0 =C2=A0 =C2=A0 > timers/timers-howto.txt
> =C2=A0 =C2=A0 =C2=A0 >
> =C2=A0 =C2=A0 =C2=A0 > Signed-off-by: simran singhal <sin= ghal...@gmail.com>
> =C2=A0 =C2=A0 =C2=A0 > ---
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0drivers/staging/nvec/nvec.c | 4 ++= --
> =C2=A0 =C2=A0 =C2=A0 > =C2=A01 file changed, 2 insertions(+), 2= deletions(-)
> =C2=A0 =C2=A0 =C2=A0 >
> =C2=A0 =C2=A0 =C2=A0 > diff --git a/drivers/staging/nvec/nvec.c
> =C2=A0 =C2=A0 =C2=A0 b/drivers/staging/nvec/nvec.c
> =C2=A0 =C2=A0 =C2=A0 > index c1feccf..cd35e64 100644
> =C2=A0 =C2=A0 =C2=A0 > --- a/drivers/staging/nvec/nvec.c
> =C2=A0 =C2=A0 =C2=A0 > +++ b/drivers/staging/nvec/nvec.c
> =C2=A0 =C2=A0 =C2=A0 > @@ -631,7 +631,7 @@ static irqreturn_t n= vec_interrupt(int irq,
> =C2=A0 =C2=A0 =C2=A0 void *dev)
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0break;
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0case 2:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* first byte after command */
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (status = =3D=3D (I2C_SL_IRQ | RNW | RCVD)) {
> =C2=A0 =C2=A0 =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0udelay(33);
> =C2=A0 =C2=A0 =C2=A0 > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0usleep_range(33, 100);
>
> =C2=A0 =C2=A0 =C2=A0 How did you choose the upper limit.
>
> =C2=A0 =C2=A0 =C2=A0 I believe that Greg previously suggested not = to make these
> =C2=A0 =C2=A0 =C2=A0 changes if you
> =C2=A0 =C2=A0 =C2=A0 have no way to test them.
>
> Julia, After going through the reply given by=C2=A0Nicholas Mc Gui= re=C2=A0
> https://www.mail-archive.com/kernelnewbies@kernelnewbies.org= /msg16464.html
> in this reply he has mentioned that even the range of 10 microseco= nd is
> enough,
> so I prefer to take 100 as upper limit. =C2=A0

Than you for the link.

It looks like he suggests to change 33 to 30-40, not to 33-100. =C2=A0I= n any
case, you have three choices for this kind of issue:
1. Don't make the change, because you can't test the result
2. Make the change, and explain the commit log what your rationale is
3. Make the change, and explain below the --- that you have no idea wha= t
you are doing, and you are just proposing the patch as something concre= te
to start a discussion.

But your preference is not a suitable justification. =C2=A0The hardware= does
something, and the choice can only really be made by the person who kno= ws
what it does.

=C2=A0
Thanks,=C2=A0
Julia I'= ll keep this in mind from next time.

I choose the = range from 33 to 100 for being on more safer side.
Should I make = it 30-40 and send v2.

julia

> =C2=A0
> Simran
>
> =C2=A0 =C2=A0 =C2=A0 julia
>
>
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (nvec->rx->data[0] !=3D 0x01) = {
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0dev_err(nvec->dev,
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"Read w= ithout prior
> =C2=A0 =C2=A0 =C2=A0 read command\n");
> =C2=A0 =C2=A0 =C2=A0 > @@ -718,7 +718,7 @@ static irqreturn_t n= vec_interrupt(int irq,
> =C2=A0 =C2=A0 =C2=A0 void *dev)
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 * We experience less incomplete messages with this
> =C2=A0 =C2=A0 =C2=A0 delay than without
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 * it, but we don't know why. Help is appreciated.
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 */
> =C2=A0 =C2=A0 =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0udelay(100);
> =C2=A0 =C2=A0 =C2=A0 > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0usleep_range(100, 200);
> =C2=A0 =C2=A0 =C2=A0 >
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0return IRQ_HANDLED;
> =C2=A0 =C2=A0 =C2=A0 > =C2=A0}
> =C2=A0 =C2=A0 =C2=A0 > --
> =C2=A0 =C2=A0 =C2=A0 > 2.7.4
> =C2=A0 =C2=A0 =C2=A0 >
> =C2=A0 =C2=A0 =C2=A0 > --
> =C2=A0 =C2=A0 =C2=A0 > You received this message because you ar= e subscribed to the
> =C2=A0 =C2=A0 =C2=A0 Google Groups "outreachy-kernel" gr= oup.
> =C2=A0 =C2=A0 =C2=A0 > To unsubscribe from this group and stop = receiving emails from
> =C2=A0 =C2=A0 =C2=A0 it, send an email to outreachy-kern...@googlegroups.com.
> =C2=A0 =C2=A0 =C2=A0 > To post to this group, send email to
> =C2=A0 =C2=A0 =C2=A0 outreach...@googlegroups.com.
> =C2=A0 =C2=A0 =C2=A0 > To view this discussion on the web visit= https://groups.google.com/d/msgid/outreachy-kernel/20170302142418.GA16= 773%4
> =C2=A0 =C2=A0 =C2=A0 0singhal-Inspiron-5558.
> =C2=A0 =C2=A0 =C2=A0 > For more options, visit https://groups.google.com/d/optout.
> =C2=A0 =C2=A0 =C2=A0 >
>
> --
> 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-kern...@googlegroups.com.
> To post to this group, send email to outreach...@googlegroups.com<= /a>.
> To view this discussion on the web visithttps://groups.google.com/= d/msgid/outreachy-kernel/b90bc602-cf06-4abb-bea2-
> 6386d4976864%40googlegroups.com.
> For more options, visit https://groups.go= ogle.com/d/optout.
>
>
------=_Part_971_825758182.1488467827387-- ------=_Part_970_585911967.1488467827387--