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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx wrote:
>> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Signed-off-by: Hao Yao <hao.yao@xxxxxxxxx>
>> ---
>>  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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux