[PATCH 1/2] media: i2c: ov7251: Set enable GPIO low in probe

[PATCH 1/2] media: i2c: ov7251: Set enable GPIO low in probe

[Sakari Ailus]

Set the enable GPIO low when acquiring it.

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, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 30f61e04ecaf..f3e2d26bb840 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1696,7 +1696,7 @@ static int ov7251_probe(struct i2c_client *client)
 		return PTR_ERR(ov7251->analog_regulator);
 	}
 
-	ov7251->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
+	ov7251->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
 	if (IS_ERR(ov7251->enable_gpio)) {
 		dev_err(dev, "cannot get enable gpio\n");
 		return PTR_ERR(ov7251->enable_gpio);

base-commit: 5de275c450c9496031006e0fb3f7c96a8fcaaa55
-- 
2.39.5

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

[Sakari Ailus]

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(+)

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

Re: [PATCH 1/2] media: i2c: ov7251: Set enable GPIO low in probe

[Dave Stevenson]

Hi Sakari

On Mon, 20 Jan 2025 at 10:11, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Set the enable GPIO low when acquiring it.
>
> 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>

Agreed that the datasheet says that as the regulators aren't on at
this point, the enable GPIO must be low.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/media/i2c/ov7251.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
> index 30f61e04ecaf..f3e2d26bb840 100644
> --- a/drivers/media/i2c/ov7251.c
> +++ b/drivers/media/i2c/ov7251.c
> @@ -1696,7 +1696,7 @@ static int ov7251_probe(struct i2c_client *client)
>                 return PTR_ERR(ov7251->analog_regulator);
>         }
>
> -       ov7251->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
> +       ov7251->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
>         if (IS_ERR(ov7251->enable_gpio)) {
>                 dev_err(dev, "cannot get enable gpio\n");
>                 return PTR_ERR(ov7251->enable_gpio);
>
> base-commit: 5de275c450c9496031006e0fb3f7c96a8fcaaa55
> --
> 2.39.5
>
>

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

[Dave Stevenson]

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
>
>

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

[Andy Shevchenko]

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() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] media: i2c: ov7251: Set enable GPIO low in probe

[Andy Shevchenko]

On Mon, Jan 20, 2025 at 12:11:22PM +0200, Sakari Ailus wrote:
> Set the enable GPIO low when acquiring it.

...

> -	ov7251->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
> +	ov7251->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);

Fine with me, but it's not helpful for MS Surface family of the devices anyway.

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

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.

-- 
Sakari Ailus

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

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.

-- 
Regards,

Sakari Ailus

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

[Andy Shevchenko]

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