| Message ID | 20250120101123.148482-2-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] media: i2c: ov7251: Set enable GPIO low in probe | expand |
On Mon, 20 Jan 2025 at 10:11, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Lift the xshutdown (enable) GPIO 1 ms after enabling the regulators, as > required by the sensor's power-up sequence. > > Fixes: d30bb512da3d ("media: Add a driver for the ov7251 camera sensor") > Cc: stable@vger.kernel.org > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/media/i2c/ov7251.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c > index f3e2d26bb840..3226888d77e9 100644 > --- a/drivers/media/i2c/ov7251.c > +++ b/drivers/media/i2c/ov7251.c > @@ -922,6 +922,8 @@ static int ov7251_set_power_on(struct device *dev) > return ret; > } > > + usleep_range(1000, 1100); > + > gpiod_set_value_cansleep(ov7251->enable_gpio, 1); > > /* wait at least 65536 external clock cycles */ > -- > 2.39.5 > >
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() ?
Hi Andy, 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.
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.
Hi Andy, 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? 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. I'm fine using 1000 for both, too.
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.
diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c index f3e2d26bb840..3226888d77e9 100644 --- a/drivers/media/i2c/ov7251.c +++ b/drivers/media/i2c/ov7251.c @@ -922,6 +922,8 @@ static int ov7251_set_power_on(struct device *dev) return ret; } + usleep_range(1000, 1100); + gpiod_set_value_cansleep(ov7251->enable_gpio, 1); /* wait at least 65536 external clock cycles */
Lift the xshutdown (enable) GPIO 1 ms after enabling the regulators, as required by the sensor's power-up sequence. Fixes: d30bb512da3d ("media: Add a driver for the ov7251 camera sensor") Cc: stable@vger.kernel.org Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/i2c/ov7251.c | 2 ++ 1 file changed, 2 insertions(+)