diff mbox series

[v3,6/8] media: i2c: ov5670: Add runtime_pm operations

Message ID 20220329090133.338073-7-jacopo@jmondi.org (mailing list archive)
State Changes Requested
Delegated to: Sakari Ailus
Headers
Series media: i2c: ov5670: OF support, runtime_pm, regulators |

Commit Message

Jacopo Mondi March 29, 2022, 9:01 a.m. UTC 
  Implement the power up and power down routines and install them as
runtime_pm handler for runtime_suspend and runtime_resume operations.

Rework the runtime_pm enablement and the chip power handling during
probe, as calling pm_runtime_idle() in a driver that registers no
idle callback is a nop.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5670.c | 58 ++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 6 deletions(-)

Comments

Sakari Ailus March 31, 2022, 10:08 a.m. UTC | #1 
Hi Jacopo,

On Tue, Mar 29, 2022 at 11:01:31AM +0200, Jacopo Mondi wrote:
> Implement the power up and power down routines and install them as
> runtime_pm handler for runtime_suspend and runtime_resume operations.
> 
> Rework the runtime_pm enablement and the chip power handling during
> probe, as calling pm_runtime_idle() in a driver that registers no
> idle callback is a nop.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 58 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 9e69b4008917..b63b07d8ca2f 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -4,6 +4,7 @@
>  #include <linux/acpi.h>
>  #include <linux/clk.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -2424,6 +2425,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>  
> +static int __maybe_unused ov5670_runtime_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5670 *ov5670 = to_ov5670(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
> +	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
> +
> +	/* 8192 * 2 clock pulses before the first SCCB transaction. */
> +	usleep_range(1000, 1500);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ov5670_runtime_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5670 *ov5670 = to_ov5670(sd);
> +
> +	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
> +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
> +	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused ov5670_suspend(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> @@ -2564,14 +2598,25 @@ static int ov5670_probe(struct i2c_client *client)
>  		goto error_print;
>  	}
>  
> +	pm_runtime_enable(&client->dev);
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
> +		ret = pm_runtime_resume_and_get(&client->dev);

This will power the device on, but only on OF systems.

Could you instead power the device on on OF systems explicitly? That's what
other drivers do, too. The changes would be probably more simple to
implement, too.

> +		if (ret) {
> +			err_msg = "Failed to power on";
> +			goto error_print;
> +		}
> +
>  		/* Check module identity */
>  		ret = ov5670_identify_module(ov5670);
>  		if (ret) {
>  			err_msg = "ov5670_identify_module() error";
> -			goto error_print;
> +			goto error_power_off;
>  		}
> +
> +		/* Set the device's state to active if it's in D0 state. */
> +		pm_runtime_set_active(&client->dev);
>  	}
>  
>  	mutex_init(&ov5670->mutex);
> @@ -2608,11 +2653,7 @@ static int ov5670_probe(struct i2c_client *client)
>  
>  	ov5670->streaming = false;
>  
> -	/* Set the device's state to active if it's in D0 state. */
> -	if (full_power)
> -		pm_runtime_set_active(&client->dev);
> -	pm_runtime_enable(&client->dev);
> -	pm_runtime_idle(&client->dev);
> +	pm_runtime_suspend(&client->dev);
>  
>  	return 0;
>  
> @@ -2625,6 +2666,9 @@ static int ov5670_probe(struct i2c_client *client)
>  error_mutex_destroy:
>  	mutex_destroy(&ov5670->mutex);
>  
> +error_power_off:
> +	pm_runtime_put(&client->dev);
> +
>  error_print:
>  	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
>  
> @@ -2641,6 +2685,7 @@ static int ov5670_remove(struct i2c_client *client)
>  	v4l2_ctrl_handler_free(sd->ctrl_handler);
>  	mutex_destroy(&ov5670->mutex);
>  
> +	pm_runtime_put(&client->dev);
>  	pm_runtime_disable(&client->dev);
>  
>  	return 0;
> @@ -2648,6 +2693,7 @@ static int ov5670_remove(struct i2c_client *client)
>  
>  static const struct dev_pm_ops ov5670_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume)
> +	SET_RUNTIME_PM_OPS(ov5670_runtime_suspend, ov5670_runtime_resume, NULL)
>  };
>  
>  #ifdef CONFIG_ACPI
Laurent Pinchart March 31, 2022, 10:33 a.m. UTC | #2 
Hi Sakari,

On Thu, Mar 31, 2022 at 01:08:17PM +0300, Sakari Ailus wrote:
> On Tue, Mar 29, 2022 at 11:01:31AM +0200, Jacopo Mondi wrote:
> > Implement the power up and power down routines and install them as
> > runtime_pm handler for runtime_suspend and runtime_resume operations.
> > 
> > Rework the runtime_pm enablement and the chip power handling during
> > probe, as calling pm_runtime_idle() in a driver that registers no
> > idle callback is a nop.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ov5670.c | 58 ++++++++++++++++++++++++++++++++++----
> >  1 file changed, 52 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index 9e69b4008917..b63b07d8ca2f 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -4,6 +4,7 @@
> >  #include <linux/acpi.h>
> >  #include <linux/clk.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/delay.h>
> >  #include <linux/i2c.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> > @@ -2424,6 +2425,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
> >  	return ret;
> >  }
> >  
> > +static int __maybe_unused ov5670_runtime_resume(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct ov5670 *ov5670 = to_ov5670(sd);
> > +	int ret;
> > +
> > +	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> > +	if (ret)
> > +		return ret;
> > +
> > +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
> > +	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
> > +
> > +	/* 8192 * 2 clock pulses before the first SCCB transaction. */
> > +	usleep_range(1000, 1500);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused ov5670_runtime_suspend(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct ov5670 *ov5670 = to_ov5670(sd);
> > +
> > +	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
> > +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
> > +	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> > +
> > +	return 0;
> > +}
> > +
> >  static int __maybe_unused ov5670_suspend(struct device *dev)
> >  {
> >  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > @@ -2564,14 +2598,25 @@ static int ov5670_probe(struct i2c_client *client)
> >  		goto error_print;
> >  	}
> >  
> > +	pm_runtime_enable(&client->dev);
> > +
> >  	full_power = acpi_dev_state_d0(&client->dev);
> >  	if (full_power) {
> > +		ret = pm_runtime_resume_and_get(&client->dev);
> 
> This will power the device on, but only on OF systems.
> 
> Could you instead power the device on on OF systems explicitly? That's what
> other drivers do, too. The changes would be probably more simple to
> implement, too.

Can we fix ACPI to do something more sane instead ? :-) I don't want to
see those complicated patterns replicated in all drivers.

> > +		if (ret) {
> > +			err_msg = "Failed to power on";
> > +			goto error_print;
> > +		}
> > +
> >  		/* Check module identity */
> >  		ret = ov5670_identify_module(ov5670);
> >  		if (ret) {
> >  			err_msg = "ov5670_identify_module() error";
> > -			goto error_print;
> > +			goto error_power_off;
> >  		}
> > +
> > +		/* Set the device's state to active if it's in D0 state. */
> > +		pm_runtime_set_active(&client->dev);
> >  	}
> >  
> >  	mutex_init(&ov5670->mutex);
> > @@ -2608,11 +2653,7 @@ static int ov5670_probe(struct i2c_client *client)
> >  
> >  	ov5670->streaming = false;
> >  
> > -	/* Set the device's state to active if it's in D0 state. */
> > -	if (full_power)
> > -		pm_runtime_set_active(&client->dev);
> > -	pm_runtime_enable(&client->dev);
> > -	pm_runtime_idle(&client->dev);
> > +	pm_runtime_suspend(&client->dev);
> >  
> >  	return 0;
> >  
> > @@ -2625,6 +2666,9 @@ static int ov5670_probe(struct i2c_client *client)
> >  error_mutex_destroy:
> >  	mutex_destroy(&ov5670->mutex);
> >  
> > +error_power_off:
> > +	pm_runtime_put(&client->dev);
> > +
> >  error_print:
> >  	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
> >  
> > @@ -2641,6 +2685,7 @@ static int ov5670_remove(struct i2c_client *client)
> >  	v4l2_ctrl_handler_free(sd->ctrl_handler);
> >  	mutex_destroy(&ov5670->mutex);
> >  
> > +	pm_runtime_put(&client->dev);
> >  	pm_runtime_disable(&client->dev);
> >  
> >  	return 0;
> > @@ -2648,6 +2693,7 @@ static int ov5670_remove(struct i2c_client *client)
> >  
> >  static const struct dev_pm_ops ov5670_pm_ops = {
> >  	SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume)
> > +	SET_RUNTIME_PM_OPS(ov5670_runtime_suspend, ov5670_runtime_resume, NULL)
> >  };
> >  
> >  #ifdef CONFIG_ACPI
Sakari Ailus March 31, 2022, 12:34 p.m. UTC | #3 
Hi Laurent,

On Thu, Mar 31, 2022 at 01:33:27PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thu, Mar 31, 2022 at 01:08:17PM +0300, Sakari Ailus wrote:
> > On Tue, Mar 29, 2022 at 11:01:31AM +0200, Jacopo Mondi wrote:
> > > Implement the power up and power down routines and install them as
> > > runtime_pm handler for runtime_suspend and runtime_resume operations.
> > > 
> > > Rework the runtime_pm enablement and the chip power handling during
> > > probe, as calling pm_runtime_idle() in a driver that registers no
> > > idle callback is a nop.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/i2c/ov5670.c | 58 ++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 52 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > index 9e69b4008917..b63b07d8ca2f 100644
> > > --- a/drivers/media/i2c/ov5670.c
> > > +++ b/drivers/media/i2c/ov5670.c
> > > @@ -4,6 +4,7 @@
> > >  #include <linux/acpi.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/gpio/consumer.h>
> > > +#include <linux/delay.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/mod_devicetable.h>
> > >  #include <linux/module.h>
> > > @@ -2424,6 +2425,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
> > >  	return ret;
> > >  }
> > >  
> > > +static int __maybe_unused ov5670_runtime_resume(struct device *dev)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +	struct ov5670 *ov5670 = to_ov5670(sd);
> > > +	int ret;
> > > +
> > > +	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
> > > +	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
> > > +
> > > +	/* 8192 * 2 clock pulses before the first SCCB transaction. */
> > > +	usleep_range(1000, 1500);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __maybe_unused ov5670_runtime_suspend(struct device *dev)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +	struct ov5670 *ov5670 = to_ov5670(sd);
> > > +
> > > +	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
> > > +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
> > > +	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int __maybe_unused ov5670_suspend(struct device *dev)
> > >  {
> > >  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > @@ -2564,14 +2598,25 @@ static int ov5670_probe(struct i2c_client *client)
> > >  		goto error_print;
> > >  	}
> > >  
> > > +	pm_runtime_enable(&client->dev);
> > > +
> > >  	full_power = acpi_dev_state_d0(&client->dev);
> > >  	if (full_power) {
> > > +		ret = pm_runtime_resume_and_get(&client->dev);
> > 
> > This will power the device on, but only on OF systems.
> > 
> > Could you instead power the device on on OF systems explicitly? That's what
> > other drivers do, too. The changes would be probably more simple to
> > implement, too.
> 
> Can we fix ACPI to do something more sane instead ? :-) I don't want to
> see those complicated patterns replicated in all drivers.

I guess it could be changed. But it will take time.

This patch does not quite represent what it takes to implement runtime PM
support without ACPI.

Also, sensor drivers shouldn't be somehow special when it comes to power
management so depending on CONFIG_PM isn't all that nice either. Of course,
removing that Kconfig option would simplify quite a few things.
Sakari Ailus April 27, 2022, 9:57 a.m. UTC | #4 
Hi Jacopo,

On Tue, Mar 29, 2022 at 11:01:31AM +0200, Jacopo Mondi wrote:
> Implement the power up and power down routines and install them as
> runtime_pm handler for runtime_suspend and runtime_resume operations.
> 
> Rework the runtime_pm enablement and the chip power handling during
> probe, as calling pm_runtime_idle() in a driver that registers no
> idle callback is a nop.

The suspend callback is called by rpm_idle() in the absence of the
idle callback.

> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 58 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 9e69b4008917..b63b07d8ca2f 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -4,6 +4,7 @@
>  #include <linux/acpi.h>
>  #include <linux/clk.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -2424,6 +2425,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>  
> +static int __maybe_unused ov5670_runtime_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5670 *ov5670 = to_ov5670(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
> +	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
> +
> +	/* 8192 * 2 clock pulses before the first SCCB transaction. */
> +	usleep_range(1000, 1500);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ov5670_runtime_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5670 *ov5670 = to_ov5670(sd);
> +
> +	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
> +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
> +	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused ov5670_suspend(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> @@ -2564,14 +2598,25 @@ static int ov5670_probe(struct i2c_client *client)
>  		goto error_print;
>  	}
>  
> +	pm_runtime_enable(&client->dev);
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
> +		ret = pm_runtime_resume_and_get(&client->dev);

Please see how e.g. the CCS driver does this (no need for autosuspend
though). E.g. don't use runtime PM to power the sensor on in probe, or off
in remove.

> +		if (ret) {
> +			err_msg = "Failed to power on";
> +			goto error_print;
> +		}
> +
>  		/* Check module identity */
>  		ret = ov5670_identify_module(ov5670);
>  		if (ret) {
>  			err_msg = "ov5670_identify_module() error";
> -			goto error_print;
> +			goto error_power_off;
>  		}
> +
> +		/* Set the device's state to active if it's in D0 state. */
> +		pm_runtime_set_active(&client->dev);
>  	}
>  
>  	mutex_init(&ov5670->mutex);
> @@ -2608,11 +2653,7 @@ static int ov5670_probe(struct i2c_client *client)
>  
>  	ov5670->streaming = false;
>  
> -	/* Set the device's state to active if it's in D0 state. */
> -	if (full_power)
> -		pm_runtime_set_active(&client->dev);
> -	pm_runtime_enable(&client->dev);
> -	pm_runtime_idle(&client->dev);
> +	pm_runtime_suspend(&client->dev);
>  
>  	return 0;
>  
> @@ -2625,6 +2666,9 @@ static int ov5670_probe(struct i2c_client *client)
>  error_mutex_destroy:
>  	mutex_destroy(&ov5670->mutex);
>  
> +error_power_off:
> +	pm_runtime_put(&client->dev);
> +
>  error_print:
>  	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
>  
> @@ -2641,6 +2685,7 @@ static int ov5670_remove(struct i2c_client *client)
>  	v4l2_ctrl_handler_free(sd->ctrl_handler);
>  	mutex_destroy(&ov5670->mutex);
>  
> +	pm_runtime_put(&client->dev);
>  	pm_runtime_disable(&client->dev);
>  
>  	return 0;
> @@ -2648,6 +2693,7 @@ static int ov5670_remove(struct i2c_client *client)
>  
>  static const struct dev_pm_ops ov5670_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume)
> +	SET_RUNTIME_PM_OPS(ov5670_runtime_suspend, ov5670_runtime_resume, NULL)
>  };
>  
>  #ifdef CONFIG_ACPI
Sakari Ailus April 27, 2022, 9:59 a.m. UTC | #5 
On Wed, Apr 27, 2022 at 12:57:16PM +0300, Sakari Ailus wrote:
> > +		ret = pm_runtime_resume_and_get(&client->dev);
> 
> Please see how e.g. the CCS driver does this (no need for autosuspend
> though). E.g. don't use runtime PM to power the sensor on in probe, or off
> in remove.

What's actually needed in this driver would be explicit use of the resume
and suspend callbacks for !CONFIG_PM as well as setting the callbacks in
ov5670_pm_ops. No other changes should be needed.
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 9e69b4008917..b63b07d8ca2f 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -4,6 +4,7 @@ 
 #include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/gpio/consumer.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -2424,6 +2425,39 @@  static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static int __maybe_unused ov5670_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov5670 *ov5670 = to_ov5670(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
+	if (ret)
+		return ret;
+
+	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
+	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
+
+	/* 8192 * 2 clock pulses before the first SCCB transaction. */
+	usleep_range(1000, 1500);
+
+	return 0;
+}
+
+static int __maybe_unused ov5670_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov5670 *ov5670 = to_ov5670(sd);
+
+	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
+	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
+	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
+
+	return 0;
+}
+
 static int __maybe_unused ov5670_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
@@ -2564,14 +2598,25 @@  static int ov5670_probe(struct i2c_client *client)
 		goto error_print;
 	}
 
+	pm_runtime_enable(&client->dev);
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
+		ret = pm_runtime_resume_and_get(&client->dev);
+		if (ret) {
+			err_msg = "Failed to power on";
+			goto error_print;
+		}
+
 		/* Check module identity */
 		ret = ov5670_identify_module(ov5670);
 		if (ret) {
 			err_msg = "ov5670_identify_module() error";
-			goto error_print;
+			goto error_power_off;
 		}
+
+		/* Set the device's state to active if it's in D0 state. */
+		pm_runtime_set_active(&client->dev);
 	}
 
 	mutex_init(&ov5670->mutex);
@@ -2608,11 +2653,7 @@  static int ov5670_probe(struct i2c_client *client)
 
 	ov5670->streaming = false;
 
-	/* Set the device's state to active if it's in D0 state. */
-	if (full_power)
-		pm_runtime_set_active(&client->dev);
-	pm_runtime_enable(&client->dev);
-	pm_runtime_idle(&client->dev);
+	pm_runtime_suspend(&client->dev);
 
 	return 0;
 
@@ -2625,6 +2666,9 @@  static int ov5670_probe(struct i2c_client *client)
 error_mutex_destroy:
 	mutex_destroy(&ov5670->mutex);
 
+error_power_off:
+	pm_runtime_put(&client->dev);
+
 error_print:
 	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
 
@@ -2641,6 +2685,7 @@  static int ov5670_remove(struct i2c_client *client)
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 	mutex_destroy(&ov5670->mutex);
 
+	pm_runtime_put(&client->dev);
 	pm_runtime_disable(&client->dev);
 
 	return 0;
@@ -2648,6 +2693,7 @@  static int ov5670_remove(struct i2c_client *client)
 
 static const struct dev_pm_ops ov5670_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume)
+	SET_RUNTIME_PM_OPS(ov5670_runtime_suspend, ov5670_runtime_resume, NULL)
 };
 
 #ifdef CONFIG_ACPI