RE: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs

From: "Aniroop Mathur" <a.mathur@samsung.com>
To: "vojtech@ucw.cz" <vojtech@ucw.cz>,
	"dmitry.torokhov@gmail.com" <dmitry.torokhov@gmail.com>,
	"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "SAMUEL SEQUEIRA" <s.samuel@samsung.com>,
	"Rahul Mahale" <r.mahale@samsung.com>,
	"aniroop.mathur@gmail.com" <aniroop.mathur@gmail.com>
Subject: RE: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs
Date: Mon, 28 Nov 2016 13:49:31 +0000	[thread overview]
Message-ID: <20161128134931epcms5p66207a434afe573b432d28fdb69eb7de5@epcms5p6> (raw)
In-Reply-To: 20161128114356epcms5p836ef0c36a1cbb8b68638ed046afbb777@epcms5p8

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

Hello Mr. Vojtech Pavlik,

On 28 Nov 2016 17:23, "vojtech@ucw.cz" <vojtech@ucw.cz> wrote:
 >
 > Hi.
 >
 > ADI_INIT_DELAY/ADI_DATA_DELAY doesn't have to be exact, and a longer
 > sleep doesn't matter. In the initilization sequence - first chunk of
 > your patch - a way too long delay could in theory make the device fail
 > to initialize. What's critical is that the mdelay() calls are precise.
 >
 > One day I'll open my box of old joystick and re-test these drivers to
 > see if they survived the years of kernel infrastructure updates ...
 >
 > Vojtech
 >

 Well, it seems to me that there is some kind of confusion, so I'd like to
 clarify things about this patch.
 As you have mentioned that in the initialization sequence, long delay could
 in theory make the device fail to initialize - This patch actually solves
 this problem.
 msleep is built on jiffies / legacy timers and usleep_range is built on top
 of hrtimers so the wakeup will be precise.
 Source - https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

 For example in initialization sequence, if we use msleep(4), then the process
 could sleep for much more than 4 ms, say 6 ms or 10 ms or more depending on
 machine architecture. Like on a machine with tick rate / HZ is defined to be
 100 so msleep(4) will make the process to sleep for minimum 10 ms. 
 Whereas usleep_range(4000, 4100) will make sure that the process do not sleep
 for more than 4100 us or 4.1 ms. So usleep_range is precise but msleep is not.

 Originally, I added you in this patch to request you if you could help to
 test this patch or provide contact points of individuals who could help
 to test this patch as we do not have the hardware available with us?
 Like this driver, we also need to test other joystick drivers as well like
 gf2k.c, analog.c, sidewinder.c and gameport driver ns558.c as all have
 similar problem.
 Original patch link - https://patchwork.kernel.org/patch/9446201/
 As we do not have hardware available, so we decided to split patch into
 individual drivers and request to person who could support to test this patch

 I have also applied the same patch in our driver whose hardware is available
 with me and I found that wake up time became precise indeed and so I
 decided to apply the same fix in other input subsystem drivers as well.

 Thank you!

 BR,
 Aniroop Mathur

 > On Mon, Nov 28, 2016 at 11:43:56AM +0000, Aniroop Mathur wrote:
 > > msleep(1~20) may not do what the caller intends, and will often sleep longer.
 > > (~20 ms actual sleep for any value given in the 1~20ms range)
 > > This is not the desired behaviour for many cases like device resume time,
 > > device suspend time, device enable time, data reading time, etc.
 > > Thus, change msleep to usleep_range for precise wakeups.
 > >
 > > Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
 > > ---
 > >  joystick/adi.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
 > >
 > > diff --git a/joystick/adi.c b/joystick/adi.c
 > > index d09cefa..6799bd9 100644
 > > --- a/joystick/adi.c
 > > +++ b/joystick/adi.c
 > > @@ -47,8 +47,8 @@ MODULE_LICENSE("GPL");
 > >
 > >  #define ADI_MAX_START                200     /* Trigger to packet timeout [200us] */
 > >  #define ADI_MAX_STROBE               40      /* Single bit timeout [40us] */
 > > -#define ADI_INIT_DELAY               10      /* Delay after init packet [10ms] */
 > > -#define ADI_DATA_DELAY               4       /* Delay after data packet [4ms] */
 > > +#define ADI_INIT_DELAY               10000   /* Delay after init packet [10ms] */
 > > +#define ADI_DATA_DELAY               4000    /* Delay after data packet [4000us] */
 > >
 > >  #define ADI_MAX_LENGTH               256
 > >  #define ADI_MIN_LENGTH               8
 > > @@ -319,7 +319,7 @@ static void adi_init_digital(struct gameport *gameport)
 > >       for (i = 0; seq[i]; i++) {
 > >               gameport_trigger(gameport);
 > >               if (seq[i] > 0)
 > > -                     msleep(seq[i]);
 > > +                     usleep_range(seq[i] * 1000, (seq[i] * 1000) + 100);
 > >               if (seq[i] < 0) {
 > >                       mdelay(-seq[i]);
 > >                       udelay(-seq[i]*14);     /* It looks like mdelay() is off by approx 1.4% */
 > > @@ -512,9 +512,9 @@ static int adi_connect(struct gameport *gameport, struct gameport_driver *drv)
 > >       gameport_set_poll_handler(gameport, adi_poll);
 > >       gameport_set_poll_interval(gameport, 20);
 > >
 > > -     msleep(ADI_INIT_DELAY);
 > > +     usleep_range(ADI_INIT_DELAY, ADI_INIT_DELAY + 100);
 > >       if (adi_read(port)) {
 > > -             msleep(ADI_DATA_DELAY);
 > > +             usleep_range(ADI_DATA_DELAY, ADI_DATA_DELAY + 100);
 > >               adi_read(port);
 > >       }
 > >
 > > --
 > > 2.6.4.windows.1
 >
 >
 > --
 > Vojtech Pavlik