[RFC,media] s5k6aa: set usleep_range greater 0

  As this is not in atomic context and it does not seem like a critical 
timing setting a range of 1ms allows the timer subsystem to optimize 
the hrtimer here.

Fixes: commit bfa8dd3a0524 ("[media] v4l: Add v4l2 subdev driver for S5K6AAFX sensor")
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

problem was located by coccinelle spatch

The problem is that usleep_range is calculating the delay by
     exp = ktime_add_us(ktime_get(), min)
     delta = (u64)(max - min) * NSEC_PER_USEC
so delta is set to 0
and then calls
  schedule_hrtimeout_range(exp, 0,...)
effectively this means that the clock subsystem has no room to
optimize which makes little sense as this is not atomic context
anyway so there is not guaratee of precision here.

As this is not a critical delay it is set to a range of 4 to 5
milliseconds - this change needs a review by someone that knows
the details of the device though and preferably would increase
that range.

Patch was only compile tested with: x86_64_defconfig + MEDIA_SUPPORT=m
MEDIA_CAMERA_SUPPORT=y (implies VIDEO_V4L2=m)
MEDIA_DIGITAL_TV_SUPPORT=y, CONFIG_MEDIA_CONTROLLER=y,
CONFIG_VIDEO_V4L2_SUBDEV_API=y

Patch is against 4.9.0 (localversion-next is next-20161212)

 drivers/media/i2c/s5k6aa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Hi Nicholas,

On Tue, Dec 13, 2016 at 02:58:02AM +0100, Nicholas Mc Guire wrote:
> As this is not in atomic context and it does not seem like a critical 
> timing setting a range of 1ms allows the timer subsystem to optimize 
> the hrtimer here.

I'd suggest not to. These delays are often directly visible to the user in
use cases where attention is indeed paid to milliseconds.

The same applies to register accesses. An delay of 0 to 100 µs isn't much as
such, but when you multiply that with the number of accesses it begins to
add up.
  

On 13/12/16 09:43, Sakari Ailus wrote:
> Hi Nicholas,
>
> On Tue, Dec 13, 2016 at 02:58:02AM +0100, Nicholas Mc Guire wrote:
>> As this is not in atomic context and it does not seem like a critical
>> timing setting a range of 1ms allows the timer subsystem to optimize
>> the hrtimer here.
> I'd suggest not to. These delays are often directly visible to the user in
> use cases where attention is indeed paid to milliseconds.
>
> The same applies to register accesses. An delay of 0 to 100 µs isn't much as
> such, but when you multiply that with the number of accesses it begins to
> add up.
>
Data sheet for this device [1] says STBYN deassertion to RSTN 
deassertion should be >50us, though this is actually referenced to MCLK 
startup. See Figure 36, Power-Up Sequence, page 42.

I think the usleep range here could be greatly reduced and opened up to 
allow hr timer tweaks if desired.

[1] http://www.bdtic.com/DataSheet/SAMSUNG/S5K6AAFX13.pdf

Regards,
Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Tue, Dec 13, 2016 at 10:10:51AM +0000, Ian Arkver wrote:
> On 13/12/16 09:43, Sakari Ailus wrote:
> >Hi Nicholas,
> >
> >On Tue, Dec 13, 2016 at 02:58:02AM +0100, Nicholas Mc Guire wrote:
> >>As this is not in atomic context and it does not seem like a critical
> >>timing setting a range of 1ms allows the timer subsystem to optimize
> >>the hrtimer here.
> >I'd suggest not to. These delays are often directly visible to the user in
> >use cases where attention is indeed paid to milliseconds.
> >
> >The same applies to register accesses. An delay of 0 to 100 µs isn't much as
> >such, but when you multiply that with the number of accesses it begins to
> >add up.
> >
> Data sheet for this device [1] says STBYN deassertion to RSTN deassertion
> should be >50us, though this is actually referenced to MCLK startup. See
> Figure 36, Power-Up Sequence, page 42.
> 
> I think the usleep range here could be greatly reduced and opened up to
> allow hr timer tweaks if desired.
> 
> [1] http://www.bdtic.com/DataSheet/SAMSUNG/S5K6AAFX13.pdf

Good point. Datasheets do not always tell everything though; it'd be good to
get a comment from the original driver authors on why they've used the value
which can now be found in the driver.
  

On 12/13/2016 02:58 AM, Nicholas Mc Guire wrote:
> As this is not in atomic context and it does not seem like a critical 
> timing setting a range of 1ms allows the timer subsystem to optimize 
> the hrtimer here.
> 
> Fixes: commit bfa8dd3a0524 ("[media] v4l: Add v4l2 subdev driver for S5K6AAFX sensor")
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---

Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

I'm not sure the "Fixes" tag is needed here.

> Patch is against 4.9.0 (localversion-next is next-20161212)

Ideally patches for the media subsystem should be normally based on
master branch of the media tree (git://linuxtv.org/media_tree.git).
  

Hi Sylwester,

On Tuesday 13 Dec 2016 13:38:52 Sylwester Nawrocki wrote:
> On 12/13/2016 02:58 AM, Nicholas Mc Guire wrote:
> > As this is not in atomic context and it does not seem like a critical
> > timing setting a range of 1ms allows the timer subsystem to optimize
> > the hrtimer here.
> > 
> > Fixes: commit bfa8dd3a0524 ("[media] v4l: Add v4l2 subdev driver for
> > S5K6AAFX sensor") Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> 
> Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> I'm not sure the "Fixes" tag is needed here.
> 
> > Patch is against 4.9.0 (localversion-next is next-20161212)
> 
> Ideally patches for the media subsystem should be normally based on
> master branch of the media tree (git://linuxtv.org/media_tree.git).

As pointed out by Ian Arkver, the datasheet states the delay should be >50µs. 
Would it make sense to reduce the sleep duration to (3000, 4000) for instance 
(or possibly even lower), instead of increasing it ?
  

Hi Laurent,

On 12/13/2016 03:10 PM, Laurent Pinchart wrote:
> As pointed out by Ian Arkver, the datasheet states the delay should be >50µs. 
> Would it make sense to reduce the sleep duration to (3000, 4000) for instance 
> (or possibly even lower), instead of increasing it ?

Theoretically it would make sense, I believe the delay call should really
be part of the set_power callback.  I think it is safe to decrease the
delay value now, the boards using that driver have been dropped with commit

commit ca9143501c30a2ce5886757961408488fac2bb4c
ARM: EXYNOS: Remove unused board files

As far as I am concerned you can do whatever you want with that delay
call, remove it or decrease value, whatever helps to stop triggering
warnings from the static analysis tools.

--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Tue, Dec 13, 2016 at 03:53:47PM +0100, Sylwester Nawrocki wrote:
> Hi Laurent,
> 
> On 12/13/2016 03:10 PM, Laurent Pinchart wrote:
> > As pointed out by Ian Arkver, the datasheet states the delay should be >50µs. 
> > Would it make sense to reduce the sleep duration to (3000, 4000) for instance 
> > (or possibly even lower), instead of increasing it ?
> 
> Theoretically it would make sense, I believe the delay call should really
> be part of the set_power callback.  I think it is safe to decrease the
> delay value now, the boards using that driver have been dropped with commit
> 
> commit ca9143501c30a2ce5886757961408488fac2bb4c
> ARM: EXYNOS: Remove unused board files
> 
> As far as I am concerned you can do whatever you want with that delay
> call, remove it or decrease value, whatever helps to stop triggering
> warnings from the static analysis tools.
>
if its actually unused then it might be best to completely drop the code
raher than fixing up dead-code. Is the EXYNOS the only system that had
this device in use ? If it shold stay in then setting it to the above
proposed 3000, 4000 would seem the most resonable to me as I asume this
change would stay untested.

thx!
hofrat
 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 12/15/2016 02:14 AM, Nicholas Mc Guire wrote:
> if its actually unused then it might be best to completely drop the code
> raher than fixing up dead-code. Is the EXYNOS the only system that had
> this device in use ? If it shold stay in then setting it to the above
> proposed 3000, 4000 would seem the most resonable to me as I asume this
> change would stay untested.

I agree, there little sense in modifying unused code which cannot be
tested anyway. The whole driver is a candidate for removal as it has
no users in mainline. AFAIK it had only been used on Exynos platforms.
I'd suggest to just drop the delay call, there are already usleep_range()
calls after the GPIO state change. IIRC the delay was needed to ensure
proper I2C bus operation after enabling the voltage level translator,
but I'm not 100% sure.