Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1544100 > unrolled thread
| Started by | Igor Grinberg <grinberg@compulab.co.il> |
|---|---|
| First post | 2016-12-18 08:40 +0100 |
| Last post | 2016-12-19 00:40 +0100 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] Input: mouse: synaptics - change msleep to usleep_range for small msecs Igor Grinberg <grinberg@compulab.co.il> - 2016-12-18 08:40 +0100
Re: [PATCH] Input: mouse: synaptics - change msleep to usleep_range for small msecs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-12-19 00:40 +0100
| From | Igor Grinberg <grinberg@compulab.co.il> |
|---|---|
| Date | 2016-12-18 08:40 +0100 |
| Subject | Re: [PATCH] Input: mouse: synaptics - change msleep to usleep_range for small msecs |
| Message-ID | <sPEeB-4CJ-3@gated-at.bofh.it> |
Hi Aniroop Mathur,
On 12/04/16 15:05, Aniroop Mathur wrote:
> Hello Mr. Igor Grinberg
>
> On Sun, Dec 4, 2016 at 1:32 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> Hi Aniroop Mathur,
>>
>> On 11/29/16 23:02, Aniroop Mathur wrote:
>>> Dear Mike Rapoport, Igor Grinberg,
>>> Greetings!
>>>
>>> I am Aniroop Mathur from Samsung R&D Institute, India.
>>>
>>> I have submitted one patch as below for review to Linux Open Source.
>>> The problem is that we do not have the hardware available with us to
>>> test it and we would like to test it before actually applying it.
>>> As you are the author of this driver, I am contacting you to request you
>>> provide your feedback upon this patch.
>>>
>>> Also if you have the hardware available, could you please help to
>>> test this patch on your hardware? or could you provide contact points
>>> of individuals who could support to test it?
>>
>> This touchpad and the driver was used on an old PXA270 based PDA.
>> I currently don't have those at hand to test the patch.
>>
>>>
>>> Thank you!
>>>
>>> BR,
>>> Aniroop Mathur
>>>
>>> On Tue, Nov 29, 2016 at 1:25 AM, Aniroop Mathur <a.mathur@samsung.com> 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)
>>
>> Well, it should be at least 1ms as stated in my comment just before the define.
>> So, from the correctness perspective larger values will also do the job.
>> Additionally, since I've taken a spare 2ms, and you are making it even more
>> precise (3000us + 100us) - it will still do the job and stay correct.
>> So, there should be no issue from correctness POV.
>>
>
> Alright, Thanks!
>
>>>> This is not the desired behaviour for many cases like device resume time,
>>>> device suspend time, device enable time, retry logic, etc.
>>>> Thus, change msleep to usleep_range for precise wakeups.
>>
>> This is a human interface touchpad device, even having 20ms soft reset
>> sleep time will not impact the responsiveness for humans.
>> IMHO, there is no need for precise wakeups for this device, so I wouldn't
>> bother.
>>
>
> Well, from the point of view of device working and responsiveness for "humans",
> I agree that it is okay to sleep for 20 / 40 ms or even 100 ms. However, this
> patch is not trying to solve any such issues. This patch is only trying to make
> the process sleep for appropriate time as mentioned in the parameter and does
> not cause any harm here. I could see that this function is called during device
> resume and device probe time. So this change will improve the resume and probe
> time for this device and doing the same change in other drivers will
> contribute to
> improvement in overall kernel resume and boot time a little bit so response time
> increases a little bit. Plus, it is recommended and mentioned in kernel
> documentation to use usleep_range for delays between 1-10 ms.
> So usleep_range should serve better here.
> Explained originally here to why not use msleep for 1 - 20 ms:
> http://lkml.org/lkml/2007/8/3/250
I have absolutely 0 objections to this.
So, as I already have said, it will be hard for me to test it currently, but
if you want it and Dmitry wants to apply it, my ack is below.
>>>>
>>>> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
Acked-by: Igor Grinberg <grinberg@compulab.co.il>
>>>> ---
>>>> drivers/input/mouse/synaptics_i2c.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/input/mouse/synaptics_i2c.c b/drivers/input/mouse/synaptics_i2c.c
>>>> index aa7c5da..4d688a6 100644
>>>> --- a/drivers/input/mouse/synaptics_i2c.c
>>>> +++ b/drivers/input/mouse/synaptics_i2c.c
>>>> @@ -29,7 +29,7 @@
>>>> * after soft reset, we should wait for 1 ms
>>>> * before the device becomes operational
>>>> */
>>>> -#define SOFT_RESET_DELAY_MS 3
>>>> +#define SOFT_RESET_DELAY_US 3000
>>>> /* and after hard reset, we should wait for max 500ms */
>>>> #define HARD_RESET_DELAY_MS 500
>>>>
>>>> @@ -311,7 +311,7 @@ static int synaptics_i2c_reset_config(struct i2c_client *client)
>>>> if (ret) {
>>>> dev_err(&client->dev, "Unable to reset device\n");
>>>> } else {
>>>> - msleep(SOFT_RESET_DELAY_MS);
>>>> + usleep_range(SOFT_RESET_DELAY_US, SOFT_RESET_DELAY_US + 100);
>>>> ret = synaptics_i2c_config(client);
>>>> if (ret)
>>>> dev_err(&client->dev, "Unable to config device\n");
>>>> --
>>>> 2.6.2
>>>>
>>>
>>
>> --
>> Regards,
>> Igor.
>
--
Regards,
Igor.
[toc] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2016-12-19 00:40 +0100 |
| Message-ID | <sPTdE-8rg-31@gated-at.bofh.it> |
| In reply to | #1544100 |
On Sun, Dec 18, 2016 at 09:35:48AM +0200, Igor Grinberg wrote: > Hi Aniroop Mathur, > > > On 12/04/16 15:05, Aniroop Mathur wrote: > > Hello Mr. Igor Grinberg > > > > On Sun, Dec 4, 2016 at 1:32 PM, Igor Grinberg <grinberg@compulab.co.il> wrote: > >> Hi Aniroop Mathur, > >> > >> On 11/29/16 23:02, Aniroop Mathur wrote: > >>> Dear Mike Rapoport, Igor Grinberg, > >>> Greetings! > >>> > >>> I am Aniroop Mathur from Samsung R&D Institute, India. > >>> > >>> I have submitted one patch as below for review to Linux Open Source. > >>> The problem is that we do not have the hardware available with us to > >>> test it and we would like to test it before actually applying it. > >>> As you are the author of this driver, I am contacting you to request you > >>> provide your feedback upon this patch. > >>> > >>> Also if you have the hardware available, could you please help to > >>> test this patch on your hardware? or could you provide contact points > >>> of individuals who could support to test it? > >> > >> This touchpad and the driver was used on an old PXA270 based PDA. > >> I currently don't have those at hand to test the patch. > >> > >>> > >>> Thank you! > >>> > >>> BR, > >>> Aniroop Mathur > >>> > >>> On Tue, Nov 29, 2016 at 1:25 AM, Aniroop Mathur <a.mathur@samsung.com> 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) > >> > >> Well, it should be at least 1ms as stated in my comment just before the define. > >> So, from the correctness perspective larger values will also do the job. > >> Additionally, since I've taken a spare 2ms, and you are making it even more > >> precise (3000us + 100us) - it will still do the job and stay correct. > >> So, there should be no issue from correctness POV. > >> > > > > Alright, Thanks! > > > >>>> This is not the desired behaviour for many cases like device resume time, > >>>> device suspend time, device enable time, retry logic, etc. > >>>> Thus, change msleep to usleep_range for precise wakeups. > >> > >> This is a human interface touchpad device, even having 20ms soft reset > >> sleep time will not impact the responsiveness for humans. > >> IMHO, there is no need for precise wakeups for this device, so I wouldn't > >> bother. > >> > > > > Well, from the point of view of device working and responsiveness for "humans", > > I agree that it is okay to sleep for 20 / 40 ms or even 100 ms. However, this > > patch is not trying to solve any such issues. This patch is only trying to make > > the process sleep for appropriate time as mentioned in the parameter and does > > not cause any harm here. I could see that this function is called during device > > resume and device probe time. So this change will improve the resume and probe > > time for this device and doing the same change in other drivers will > > contribute to > > improvement in overall kernel resume and boot time a little bit so response time > > increases a little bit. Plus, it is recommended and mentioned in kernel > > documentation to use usleep_range for delays between 1-10 ms. > > So usleep_range should serve better here. > > Explained originally here to why not use msleep for 1 - 20 ms: > > http://lkml.org/lkml/2007/8/3/250 > > I have absolutely 0 objections to this. > So, as I already have said, it will be hard for me to test it currently, but > if you want it and Dmitry wants to apply it, my ack is below. OK, it does not matter much either way, but let's apply it. > > >>>> > >>>> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com> > > Acked-by: Igor Grinberg <grinberg@compulab.co.il> Thanks. -- Dmitry
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web