Re: [PATCH 2/2] media: i2c: ov7251: Introduce 1 ms delay between regulators and en GPIO

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Jan 23, 2025 at 07:33:48AM +0000, Sakari Ailus wrote:
> On Wed, Jan 22, 2025 at 06:36:52PM +0200, Andy Shevchenko wrote:
> > On Wed, Jan 22, 2025 at 11:54:11AM +0000, Sakari Ailus wrote:
> > > On Mon, Jan 20, 2025 at 07:23:07PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Jan 20, 2025 at 12:11:23PM +0200, Sakari Ailus wrote:
> > > > > Lift the xshutdown (enable) GPIO 1 ms after enabling the regulators, as
> > > > > required by the sensor's power-up sequence.

...

> > > > > +	usleep_range(1000, 1100);
> > > > 
> > > > Why not fsleep() ?
> > > 
> > > Could be, but fsleep() uses a range that is as large as the delay is.
> > 
> > fsleep() is recommended way as it knows much better the implementation details
> > of the delay APIs and which one to choose based on input. As recently stated by
> > the fix series to delay APIs the fsleep() will give up to 25% on top of the
> > asked delay, meaning in this case somewhat 250us. Taking it into account the
> > resulting values I do not think usleep_range() should be here. I.o.w. I do not
> > see enough justification for _not_ using fsleep().
> > 
> > Also note that fsleep() ranges try to keep balance between oversleep and power
> > consumption. Your delay is rather tough as sometimes 100us is almost the time
> > needed to go to the deep sleep for the CPU package and return from it.
> 
> Have you looked at what fsleep() does?

Definitely, and also read the (updated) documentation. So, let me repeat:
I do not see any justification for usleep_range() here and how 150us overhead
(on top of what you proposed) makes any difference.

> I agree most of the time it's the best choice when you need to sleep (at
> least) some time, but generally sensor power-up time is critical.

Why? I don't see this in the commit message. Please also answer the question
"what is the bad thing happen if we delay slightly more?" and the question of
how this will cope with the case when HRTIMER is off in the kernel.

> I'm fine using 1000 for both, too.

In accordance with the documentation one has to consider

fsleep(1000)
msleep(1)
usleep_range(1000, 1100)

(in this order), so I prefer that we use fsleep() if there is no good
justification for the other one(s).

Also just noticed that in the commit message you even used "ms" units and not
"us", which pretty much suggests that tough range for usleep_range() is not
justified.

-- 
With Best Regards,
Andy Shevchenko