diff mbox series

[v2] media: ov13b10: add PM control support based on power resources

Message ID 20230613050520.1580151-1-bingbu.cao@intel.com (mailing list archive)
State New, archived
Headers show
Series [v2] media: ov13b10: add PM control support based on power resources | expand

Commit Message

Bingbu Cao June 13, 2023, 5:05 a.m. UTC 
From: Bingbu Cao <bingbu.cao@intel.com>

On ACPI based platforms, the ov13b10 camera sensor need to be powered
up by acquire the PM resource from INT3472. INT3472 will register one
regulator 'avdd', one reset gpio and clock source for ov13b10.
Add 2 power interfaces that are registered as runtime PM callbacks.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
Signed-off-by: Hao Yao <hao.yao@intel.com>
---
 drivers/media/i2c/ov13b10.c | 100 +++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 2 deletions(-)

Comments

Hans de Goede June 13, 2023, 9:20 a.m. UTC | #1 
Hi,

Thank you for the patch.

On 6/13/23 07:05, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> On ACPI based platforms, the ov13b10 camera sensor need to be powered
> up by acquire the PM resource from INT3472. INT3472 will register one
> regulator 'avdd', one reset gpio and clock source for ov13b10.
> Add 2 power interfaces that are registered as runtime PM callbacks.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> Signed-off-by: Hao Yao <hao.yao@intel.com>
> ---
>  drivers/media/i2c/ov13b10.c | 100 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 98 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
> index 96d3bd6ab3bd..be2c7d8c83ac 100644
> --- a/drivers/media/i2c/ov13b10.c
> +++ b/drivers/media/i2c/ov13b10.c
> @@ -2,6 +2,9 @@
>  // Copyright (c) 2021 Intel Corporation.
>  
>  #include <linux/acpi.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> @@ -573,6 +576,11 @@ struct ov13b10 {
>  	struct media_pad pad;
>  
>  	struct v4l2_ctrl_handler ctrl_handler;
> +
> +	struct clk *img_clk;
> +	struct regulator *avdd;
> +	struct gpio_desc *reset;
> +
>  	/* V4L2 Controls */
>  	struct v4l2_ctrl *link_freq;
>  	struct v4l2_ctrl *pixel_rate;
> @@ -1051,6 +1059,52 @@ static int ov13b10_identify_module(struct ov13b10 *ov13b)
>  	return 0;
>  }
>  
> +static int ov13b10_power_off(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov13b10 *ov13b10 = to_ov13b10(sd);
> +
> +	if (ov13b10->reset)
> +		gpiod_set_value_cansleep(ov13b10->reset, 1);

Just like the clk APIs the gpiod APIs will happily take a NULL
pointer, so the "if (ov13b10->reset)" can be dropped
here and also in ov13b10_power_on().


> +	if (ov13b10->avdd)
> +		regulator_disable(ov13b10->avdd);
> +
> +	clk_disable_unprepare(ov13b10->img_clk);
> +
> +	return 0;
> +}
> +
> +static int ov13b10_power_on(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov13b10 *ov13b10 = to_ov13b10(sd);
> +	int ret;
> +
> +	ret = clk_prepare_enable(ov13b10->img_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable imaging clock: %d", ret);
> +		return ret;
> +	}
> +
> +	if (ov13b10->avdd) {
> +		ret = regulator_enable(ov13b10->avdd);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable avdd: %d", ret);
> +			clk_disable_unprepare(ov13b10->img_clk);
> +			return ret;
> +		}
> +	}
> +
> +	if (ov13b10->reset) {
> +		gpiod_set_value_cansleep(ov13b10->reset, 0);
> +		/* 5ms to wait ready after XSHUTDN assert */
> +		usleep_range(5000, 5500);
> +	}
> +
> +	return 0;
> +}
> +
>  static int ov13b10_start_streaming(struct ov13b10 *ov13b)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&ov13b->sd);
> @@ -1317,6 +1371,34 @@ static void ov13b10_free_controls(struct ov13b10 *ov13b)
>  	mutex_destroy(&ov13b->mutex);
>  }
>  
> +static int ov13b10_get_pm_resources(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov13b10 *ov13b = to_ov13b10(sd);
> +	int ret;
> +
> +	ov13b->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ov13b->reset))
> +		return dev_err_probe(dev, PTR_ERR(ov13b->reset),
> +				     "failed to get reset gpio\n");
> +
> +	ov13b->img_clk = devm_clk_get_optional(dev, NULL);
> +	if (IS_ERR(ov13b->img_clk))
> +		return dev_err_probe(dev, PTR_ERR(ov13b->img_clk),
> +				     "failed to get imaging clock\n");
> +
> +	ov13b->avdd = devm_regulator_get_optional(dev, "avdd");
> +	if (IS_ERR(ov13b->avdd)) {
> +		ret = PTR_ERR(ov13b->avdd);
> +		ov13b->avdd = NULL;
> +		if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "failed to get avdd regulator\n");
> +	}
> +
> +	return 0;
> +}
> +
>  static int ov13b10_check_hwcfg(struct device *dev)
>  {
>  	struct v4l2_fwnode_endpoint bus_cfg = {
> @@ -1407,13 +1489,23 @@ static int ov13b10_probe(struct i2c_client *client)
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops);
>  
> +	ret = ov13b10_get_pm_resources(&client->dev);
> +	if (ret)
> +		return ret;
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
> +		ov13b10_power_on(&client->dev);
> +		if (ret) {
> +			dev_err(&client->dev, "failed to power on\n");
> +			return ret;
> +		}
> +
>  		/* Check module identity */
>  		ret = ov13b10_identify_module(ov13b);
>  		if (ret) {
>  			dev_err(&client->dev, "failed to find sensor: %d\n", ret);
> -			return ret;
> +			goto error_power_off;
>  		}
>  	}
>  
> @@ -1422,7 +1514,7 @@ static int ov13b10_probe(struct i2c_client *client)
>  
>  	ret = ov13b10_init_controls(ov13b);
>  	if (ret)
> -		return ret;
> +		goto error_power_off;
>  
>  	/* Initialize subdev */
>  	ov13b->sd.internal_ops = &ov13b10_internal_ops;
> @@ -1462,6 +1554,9 @@ static int ov13b10_probe(struct i2c_client *client)
>  	ov13b10_free_controls(ov13b);
>  	dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
>  
> +error_power_off:
> +	ov13b10_power_off(&client->dev);
> +
>  	return ret;
>  }
>  
> @@ -1479,6 +1574,7 @@ static void ov13b10_remove(struct i2c_client *client)
>  
>  static const struct dev_pm_ops ov13b10_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(ov13b10_suspend, ov13b10_resume)
> +	SET_RUNTIME_PM_OPS(ov13b10_power_off, ov13b10_power_on, NULL)

You also need to add ov13b10_power_off / ov13b10_power_on calls
to ov13b10_suspend and ov13b10_resume so that the sensor gets
properly powered-off if it was not already runtime-suspend during
system suspend.

Regards,

Hans




>  };
>  
>  #ifdef CONFIG_ACPI
Hans de Goede June 13, 2023, 9:24 a.m. UTC | #2 
Hi again,

On 6/13/23 11:20, Hans de Goede wrote:
> Hi,
> 
> Thank you for the patch.
> 
> On 6/13/23 07:05, bingbu.cao@intel.com wrote:
>> From: Bingbu Cao <bingbu.cao@intel.com>
>>
>> On ACPI based platforms, the ov13b10 camera sensor need to be powered
>> up by acquire the PM resource from INT3472. INT3472 will register one
>> regulator 'avdd', one reset gpio and clock source for ov13b10.
>> Add 2 power interfaces that are registered as runtime PM callbacks.
>>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>> ---
>>  drivers/media/i2c/ov13b10.c | 100 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 98 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
>> index 96d3bd6ab3bd..be2c7d8c83ac 100644
>> --- a/drivers/media/i2c/ov13b10.c
>> +++ b/drivers/media/i2c/ov13b10.c
>> @@ -2,6 +2,9 @@
>>  // Copyright (c) 2021 Intel Corporation.
>>  
>>  #include <linux/acpi.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/i2c.h>
>>  #include <linux/module.h>
>>  #include <linux/pm_runtime.h>
>> @@ -573,6 +576,11 @@ struct ov13b10 {
>>  	struct media_pad pad;
>>  
>>  	struct v4l2_ctrl_handler ctrl_handler;
>> +
>> +	struct clk *img_clk;
>> +	struct regulator *avdd;
>> +	struct gpio_desc *reset;
>> +
>>  	/* V4L2 Controls */
>>  	struct v4l2_ctrl *link_freq;
>>  	struct v4l2_ctrl *pixel_rate;
>> @@ -1051,6 +1059,52 @@ static int ov13b10_identify_module(struct ov13b10 *ov13b)
>>  	return 0;
>>  }
>>  
>> +static int ov13b10_power_off(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ov13b10 *ov13b10 = to_ov13b10(sd);
>> +
>> +	if (ov13b10->reset)
>> +		gpiod_set_value_cansleep(ov13b10->reset, 1);
> 
> Just like the clk APIs the gpiod APIs will happily take a NULL
> pointer, so the "if (ov13b10->reset)" can be dropped
> here and also in ov13b10_power_on().
> 
> 
>> +	if (ov13b10->avdd)
>> +		regulator_disable(ov13b10->avdd);
>> +
>> +	clk_disable_unprepare(ov13b10->img_clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ov13b10_power_on(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ov13b10 *ov13b10 = to_ov13b10(sd);
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(ov13b10->img_clk);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to enable imaging clock: %d", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (ov13b10->avdd) {
>> +		ret = regulator_enable(ov13b10->avdd);
>> +		if (ret < 0) {
>> +			dev_err(dev, "failed to enable avdd: %d", ret);
>> +			clk_disable_unprepare(ov13b10->img_clk);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	if (ov13b10->reset) {
>> +		gpiod_set_value_cansleep(ov13b10->reset, 0);
>> +		/* 5ms to wait ready after XSHUTDN assert */
>> +		usleep_range(5000, 5500);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int ov13b10_start_streaming(struct ov13b10 *ov13b)
>>  {
>>  	struct i2c_client *client = v4l2_get_subdevdata(&ov13b->sd);
>> @@ -1317,6 +1371,34 @@ static void ov13b10_free_controls(struct ov13b10 *ov13b)
>>  	mutex_destroy(&ov13b->mutex);
>>  }
>>  
>> +static int ov13b10_get_pm_resources(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ov13b10 *ov13b = to_ov13b10(sd);
>> +	int ret;
>> +
>> +	ov13b->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ov13b->reset))
>> +		return dev_err_probe(dev, PTR_ERR(ov13b->reset),
>> +				     "failed to get reset gpio\n");
>> +
>> +	ov13b->img_clk = devm_clk_get_optional(dev, NULL);
>> +	if (IS_ERR(ov13b->img_clk))
>> +		return dev_err_probe(dev, PTR_ERR(ov13b->img_clk),
>> +				     "failed to get imaging clock\n");
>> +
>> +	ov13b->avdd = devm_regulator_get_optional(dev, "avdd");
>> +	if (IS_ERR(ov13b->avdd)) {
>> +		ret = PTR_ERR(ov13b->avdd);
>> +		ov13b->avdd = NULL;
>> +		if (ret != -ENODEV)
>> +			return dev_err_probe(dev, ret,
>> +					     "failed to get avdd regulator\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int ov13b10_check_hwcfg(struct device *dev)
>>  {
>>  	struct v4l2_fwnode_endpoint bus_cfg = {
>> @@ -1407,13 +1489,23 @@ static int ov13b10_probe(struct i2c_client *client)
>>  	/* Initialize subdev */
>>  	v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops);
>>  
>> +	ret = ov13b10_get_pm_resources(&client->dev);
>> +	if (ret)
>> +		return ret;
>> +
>>  	full_power = acpi_dev_state_d0(&client->dev);
>>  	if (full_power) {
>> +		ov13b10_power_on(&client->dev);
>> +		if (ret) {
>> +			dev_err(&client->dev, "failed to power on\n");
>> +			return ret;
>> +		}
>> +
>>  		/* Check module identity */
>>  		ret = ov13b10_identify_module(ov13b);
>>  		if (ret) {
>>  			dev_err(&client->dev, "failed to find sensor: %d\n", ret);
>> -			return ret;
>> +			goto error_power_off;
>>  		}
>>  	}
>>  
>> @@ -1422,7 +1514,7 @@ static int ov13b10_probe(struct i2c_client *client)
>>  
>>  	ret = ov13b10_init_controls(ov13b);
>>  	if (ret)
>> -		return ret;
>> +		goto error_power_off;
>>  
>>  	/* Initialize subdev */
>>  	ov13b->sd.internal_ops = &ov13b10_internal_ops;
>> @@ -1462,6 +1554,9 @@ static int ov13b10_probe(struct i2c_client *client)
>>  	ov13b10_free_controls(ov13b);
>>  	dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
>>  
>> +error_power_off:
>> +	ov13b10_power_off(&client->dev);
>> +
>>  	return ret;
>>  }
>>  
>> @@ -1479,6 +1574,7 @@ static void ov13b10_remove(struct i2c_client *client)
>>  
>>  static const struct dev_pm_ops ov13b10_pm_ops = {
>>  	SET_SYSTEM_SLEEP_PM_OPS(ov13b10_suspend, ov13b10_resume)
>> +	SET_RUNTIME_PM_OPS(ov13b10_power_off, ov13b10_power_on, NULL)
> 
> You also need to add ov13b10_power_off / ov13b10_power_on calls
> to ov13b10_suspend and ov13b10_resume so that the sensor gets
> properly powered-off if it was not already runtime-suspend during
> system suspend.

Or you can simply use a single set of suspend/resume ops for both
system and runtime suspend, since is_streaming will always be
false during runtime suspend, e.g. :

static int ov2680_suspend(struct device *dev)
{
	struct v4l2_subdev *sd = dev_get_drvdata(dev);
	struct ov2680_dev *sensor = to_ov2680_dev(sd);

	if (sensor->is_streaming)
		ov2680_stream_disable(sensor);

	return ov2680_power_off(sensor);
}

static int ov2680_resume(struct device *dev)
{
	struct v4l2_subdev *sd = dev_get_drvdata(dev);
	struct ov2680_dev *sensor = to_ov2680_dev(sd);
	int ret;

	ret = ov2680_power_on(sensor);
	if (ret < 0)
		goto stream_disable;

	if (sensor->is_streaming) {
		ret = ov2680_stream_enable(sensor);
		if (ret < 0)
			goto stream_disable;
	}

	return 0;

stream_disable:
	ov2680_stream_disable(sensor);
	sensor->is_streaming = false;

	return ret;
}

static DEFINE_RUNTIME_DEV_PM_OPS(ov2680_pm_ops, ov2680_suspend, ov2680_resume, NULL);

static struct i2c_driver ov2680_i2c_driver = {
        .driver = {
                .pm = pm_sleep_ptr(&ov2680_pm_ops),
...

and by switching to the new DEFINE_RUNTIME_DEV_PM_OPS() helper you can also drop
the __maybe_unused from ov13b10_resume() and ov13b10_suspend().

Regards,
 
Hans
Bingbu Cao June 13, 2023, 10:51 a.m. UTC | #3 
Hans, 

Thanks for your review.

On 6/13/23 5:20 PM, Hans de Goede wrote:
> Hi,
> 
> Thank you for the patch.
> 
> On 6/13/23 07:05, bingbu.cao@intel.com wrote:
>> From: Bingbu Cao <bingbu.cao@intel.com>
>>
>> On ACPI based platforms, the ov13b10 camera sensor need to be powered
>> up by acquire the PM resource from INT3472. INT3472 will register one
>> regulator 'avdd', one reset gpio and clock source for ov13b10.
>> Add 2 power interfaces that are registered as runtime PM callbacks.
>>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>> ---
>>  drivers/media/i2c/ov13b10.c | 100 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 98 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
>> index 96d3bd6ab3bd..be2c7d8c83ac 100644
>> --- a/drivers/media/i2c/ov13b10.c
>> +++ b/drivers/media/i2c/ov13b10.c
>> @@ -2,6 +2,9 @@
>>  // Copyright (c) 2021 Intel Corporation.
>>  
>>  #include <linux/acpi.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/i2c.h>
>>  #include <linux/module.h>
>>  #include <linux/pm_runtime.h>
>> @@ -573,6 +576,11 @@ struct ov13b10 {
>>  	struct media_pad pad;
>>  
>>  	struct v4l2_ctrl_handler ctrl_handler;
>> +
>> +	struct clk *img_clk;
>> +	struct regulator *avdd;
>> +	struct gpio_desc *reset;
>> +
>>  	/* V4L2 Controls */
>>  	struct v4l2_ctrl *link_freq;
>>  	struct v4l2_ctrl *pixel_rate;
>> @@ -1051,6 +1059,52 @@ static int ov13b10_identify_module(struct ov13b10 *ov13b)
>>  	return 0;
>>  }
>>  
>> +static int ov13b10_power_off(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ov13b10 *ov13b10 = to_ov13b10(sd);
>> +
>> +	if (ov13b10->reset)
>> +		gpiod_set_value_cansleep(ov13b10->reset, 1);
> 
> Just like the clk APIs the gpiod APIs will happily take a NULL
> pointer, so the "if (ov13b10->reset)" can be dropped
> here and also in ov13b10_power_on().

Correct, I ignored the VALIDATE_DESC_VOID(desc) by mistake.
Will fix in v3.

> 
> 
>> +	if (ov13b10->avdd)
>> +		regulator_disable(ov13b10->avdd);
>> +
>> +	clk_disable_unprepare(ov13b10->img_clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ov13b10_power_on(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ov13b10 *ov13b10 = to_ov13b10(sd);
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(ov13b10->img_clk);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to enable imaging clock: %d", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (ov13b10->avdd) {
>> +		ret = regulator_enable(ov13b10->avdd);
>> +		if (ret < 0) {
>> +			dev_err(dev, "failed to enable avdd: %d", ret);
>> +			clk_disable_unprepare(ov13b10->img_clk);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	if (ov13b10->reset) {
>> +		gpiod_set_value_cansleep(ov13b10->reset, 0);
>> +		/* 5ms to wait ready after XSHUTDN assert */
>> +		usleep_range(5000, 5500);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int ov13b10_start_streaming(struct ov13b10 *ov13b)
>>  {
>>  	struct i2c_client *client = v4l2_get_subdevdata(&ov13b->sd);
>> @@ -1317,6 +1371,34 @@ static void ov13b10_free_controls(struct ov13b10 *ov13b)
>>  	mutex_destroy(&ov13b->mutex);
>>  }
>>  
>> +static int ov13b10_get_pm_resources(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ov13b10 *ov13b = to_ov13b10(sd);
>> +	int ret;
>> +
>> +	ov13b->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ov13b->reset))
>> +		return dev_err_probe(dev, PTR_ERR(ov13b->reset),
>> +				     "failed to get reset gpio\n");
>> +
>> +	ov13b->img_clk = devm_clk_get_optional(dev, NULL);
>> +	if (IS_ERR(ov13b->img_clk))
>> +		return dev_err_probe(dev, PTR_ERR(ov13b->img_clk),
>> +				     "failed to get imaging clock\n");
>> +
>> +	ov13b->avdd = devm_regulator_get_optional(dev, "avdd");
>> +	if (IS_ERR(ov13b->avdd)) {
>> +		ret = PTR_ERR(ov13b->avdd);
>> +		ov13b->avdd = NULL;
>> +		if (ret != -ENODEV)
>> +			return dev_err_probe(dev, ret,
>> +					     "failed to get avdd regulator\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int ov13b10_check_hwcfg(struct device *dev)
>>  {
>>  	struct v4l2_fwnode_endpoint bus_cfg = {
>> @@ -1407,13 +1489,23 @@ static int ov13b10_probe(struct i2c_client *client)
>>  	/* Initialize subdev */
>>  	v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops);
>>  
>> +	ret = ov13b10_get_pm_resources(&client->dev);
>> +	if (ret)
>> +		return ret;
>> +
>>  	full_power = acpi_dev_state_d0(&client->dev);
>>  	if (full_power) {
>> +		ov13b10_power_on(&client->dev);
>> +		if (ret) {
>> +			dev_err(&client->dev, "failed to power on\n");
>> +			return ret;
>> +		}
>> +
>>  		/* Check module identity */
>>  		ret = ov13b10_identify_module(ov13b);
>>  		if (ret) {
>>  			dev_err(&client->dev, "failed to find sensor: %d\n", ret);
>> -			return ret;
>> +			goto error_power_off;
>>  		}
>>  	}
>>  
>> @@ -1422,7 +1514,7 @@ static int ov13b10_probe(struct i2c_client *client)
>>  
>>  	ret = ov13b10_init_controls(ov13b);
>>  	if (ret)
>> -		return ret;
>> +		goto error_power_off;
>>  
>>  	/* Initialize subdev */
>>  	ov13b->sd.internal_ops = &ov13b10_internal_ops;
>> @@ -1462,6 +1554,9 @@ static int ov13b10_probe(struct i2c_client *client)
>>  	ov13b10_free_controls(ov13b);
>>  	dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
>>  
>> +error_power_off:
>> +	ov13b10_power_off(&client->dev);
>> +
>>  	return ret;
>>  }
>>  
>> @@ -1479,6 +1574,7 @@ static void ov13b10_remove(struct i2c_client *client)
>>  
>>  static const struct dev_pm_ops ov13b10_pm_ops = {
>>  	SET_SYSTEM_SLEEP_PM_OPS(ov13b10_suspend, ov13b10_resume)
>> +	SET_RUNTIME_PM_OPS(ov13b10_power_off, ov13b10_power_on, NULL)
> 
> You also need to add ov13b10_power_off / ov13b10_power_on calls
> to ov13b10_suspend and ov13b10_resume so that the sensor gets
> properly powered-off if it was not already runtime-suspend during
> system suspend.

Ack.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>>  };
>>  
>>  #ifdef CONFIG_ACPI
>
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
index 96d3bd6ab3bd..be2c7d8c83ac 100644
--- a/drivers/media/i2c/ov13b10.c
+++ b/drivers/media/i2c/ov13b10.c
@@ -2,6 +2,9 @@ 
 // Copyright (c) 2021 Intel Corporation.
 
 #include <linux/acpi.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
@@ -573,6 +576,11 @@  struct ov13b10 {
 	struct media_pad pad;
 
 	struct v4l2_ctrl_handler ctrl_handler;
+
+	struct clk *img_clk;
+	struct regulator *avdd;
+	struct gpio_desc *reset;
+
 	/* V4L2 Controls */
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
@@ -1051,6 +1059,52 @@  static int ov13b10_identify_module(struct ov13b10 *ov13b)
 	return 0;
 }
 
+static int ov13b10_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov13b10 *ov13b10 = to_ov13b10(sd);
+
+	if (ov13b10->reset)
+		gpiod_set_value_cansleep(ov13b10->reset, 1);
+
+	if (ov13b10->avdd)
+		regulator_disable(ov13b10->avdd);
+
+	clk_disable_unprepare(ov13b10->img_clk);
+
+	return 0;
+}
+
+static int ov13b10_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov13b10 *ov13b10 = to_ov13b10(sd);
+	int ret;
+
+	ret = clk_prepare_enable(ov13b10->img_clk);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable imaging clock: %d", ret);
+		return ret;
+	}
+
+	if (ov13b10->avdd) {
+		ret = regulator_enable(ov13b10->avdd);
+		if (ret < 0) {
+			dev_err(dev, "failed to enable avdd: %d", ret);
+			clk_disable_unprepare(ov13b10->img_clk);
+			return ret;
+		}
+	}
+
+	if (ov13b10->reset) {
+		gpiod_set_value_cansleep(ov13b10->reset, 0);
+		/* 5ms to wait ready after XSHUTDN assert */
+		usleep_range(5000, 5500);
+	}
+
+	return 0;
+}
+
 static int ov13b10_start_streaming(struct ov13b10 *ov13b)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&ov13b->sd);
@@ -1317,6 +1371,34 @@  static void ov13b10_free_controls(struct ov13b10 *ov13b)
 	mutex_destroy(&ov13b->mutex);
 }
 
+static int ov13b10_get_pm_resources(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov13b10 *ov13b = to_ov13b10(sd);
+	int ret;
+
+	ov13b->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ov13b->reset))
+		return dev_err_probe(dev, PTR_ERR(ov13b->reset),
+				     "failed to get reset gpio\n");
+
+	ov13b->img_clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(ov13b->img_clk))
+		return dev_err_probe(dev, PTR_ERR(ov13b->img_clk),
+				     "failed to get imaging clock\n");
+
+	ov13b->avdd = devm_regulator_get_optional(dev, "avdd");
+	if (IS_ERR(ov13b->avdd)) {
+		ret = PTR_ERR(ov13b->avdd);
+		ov13b->avdd = NULL;
+		if (ret != -ENODEV)
+			return dev_err_probe(dev, ret,
+					     "failed to get avdd regulator\n");
+	}
+
+	return 0;
+}
+
 static int ov13b10_check_hwcfg(struct device *dev)
 {
 	struct v4l2_fwnode_endpoint bus_cfg = {
@@ -1407,13 +1489,23 @@  static int ov13b10_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops);
 
+	ret = ov13b10_get_pm_resources(&client->dev);
+	if (ret)
+		return ret;
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
+		ov13b10_power_on(&client->dev);
+		if (ret) {
+			dev_err(&client->dev, "failed to power on\n");
+			return ret;
+		}
+
 		/* Check module identity */
 		ret = ov13b10_identify_module(ov13b);
 		if (ret) {
 			dev_err(&client->dev, "failed to find sensor: %d\n", ret);
-			return ret;
+			goto error_power_off;
 		}
 	}
 
@@ -1422,7 +1514,7 @@  static int ov13b10_probe(struct i2c_client *client)
 
 	ret = ov13b10_init_controls(ov13b);
 	if (ret)
-		return ret;
+		goto error_power_off;
 
 	/* Initialize subdev */
 	ov13b->sd.internal_ops = &ov13b10_internal_ops;
@@ -1462,6 +1554,9 @@  static int ov13b10_probe(struct i2c_client *client)
 	ov13b10_free_controls(ov13b);
 	dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
 
+error_power_off:
+	ov13b10_power_off(&client->dev);
+
 	return ret;
 }
 
@@ -1479,6 +1574,7 @@  static void ov13b10_remove(struct i2c_client *client)
 
 static const struct dev_pm_ops ov13b10_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(ov13b10_suspend, ov13b10_resume)
+	SET_RUNTIME_PM_OPS(ov13b10_power_off, ov13b10_power_on, NULL)
 };
 
 #ifdef CONFIG_ACPI