Re: [PATCH] Input: mouse: synaptics - change msleep to usleep_range for small msecs

[Igor Grinberg <grinberg@compulab.co.il>]

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.