[PATCH 1/4] media: ov13b10: support new ACPI HID 'OVTI13B1'

[PATCH 1/4] media: ov13b10: support new ACPI HID 'OVTI13B1'

[bingbu.cao]

From: Bingbu Cao <bingbu.cao@intel.com>

On ACPI systems, the HID of ov13b10 is 'OVTI13B1', add this new
HID in acpi IDs table to make driver support it.

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

diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
index c1430044fb1e..ede33899248c 100644
--- a/drivers/media/i2c/ov13b10.c
+++ b/drivers/media/i2c/ov13b10.c
@@ -1484,6 +1484,7 @@ static const struct dev_pm_ops ov13b10_pm_ops = {
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id ov13b10_acpi_ids[] = {
 	{"OVTIDB10"},
+	{"OVTI13B1"},
 	{ /* sentinel */ }
 };
 
-- 
2.40.1

[PATCH 2/4] media: ov13b10: Defer probe if no endpoint found

[bingbu.cao]

From: Bingbu Cao <bingbu.cao@intel.com>

The ov13b10 need be connected to a CIO2 or IPU device by bridge, sometimes
the bridge driver was not probed before ov13b10 driver, then the absence
of the fwnode endpoint for this device is expected, so driver return
-EPROBE_DEFER in this case to let the probe occur after bridge driver.

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

diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
index ede33899248c..2d48c94659a4 100644
--- a/drivers/media/i2c/ov13b10.c
+++ b/drivers/media/i2c/ov13b10.c
@@ -1331,6 +1331,10 @@ static int ov13b10_check_hwcfg(struct device *dev)
 	if (!fwnode)
 		return -ENXIO;
 
+	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
+	if (!ep)
+		return -EPROBE_DEFER;
+
 	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
 				       &ext_clk);
 	if (ret) {
@@ -1344,10 +1348,6 @@ static int ov13b10_check_hwcfg(struct device *dev)
 		return -EINVAL;
 	}
 
-	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
-	if (!ep)
-		return -ENXIO;
-
 	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
 	fwnode_handle_put(ep);
 	if (ret)
-- 
2.40.1

[PATCH 3/4] media: ov13b10: add PM control support based on power resources

[bingbu.cao]

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. On such platform, 1 GPIO
can be used to enable AVDD and DOVDD, 1 GPIO to reset, we just have
one power supply 'vdd' for camera. Add 2 power interfaces and also
registered as runtime PM callbacks to support that.

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

diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
index 2d48c94659a4..b1faa89a3571 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 *vdd;
+	struct gpio_desc *reset;
+
 	/* V4L2 Controls */
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
@@ -1051,6 +1059,50 @@ 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->vdd || !ov13b10->reset || !ov13b10->img_clk)
+		return 0;
+
+	gpiod_set_value_cansleep(ov13b10->reset, 1);
+	regulator_disable(ov13b10->vdd);
+	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;
+
+	if (!ov13b10->vdd || !ov13b10->reset || !ov13b10->img_clk)
+		return 0;
+
+	ret = clk_prepare_enable(ov13b10->img_clk);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable imaging clock: %d", ret);
+		return ret;
+	}
+
+	ret = regulator_enable(ov13b10->vdd);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable vdd: %d", ret);
+		return ret;
+	}
+
+	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 +1369,37 @@ static void ov13b10_free_controls(struct ov13b10 *ov13b)
 	mutex_destroy(&ov13b->mutex);
 }
 
+static void ov13b10_get_pm_resources(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov13b10 *ov13b = to_ov13b10(sd);
+
+	if (!is_acpi_node(dev_fwnode(dev)))
+		return;
+
+	ov13b->reset = devm_gpiod_get_optional(dev, "reset",
+					       GPIOD_OUT_LOW);
+	if (IS_ERR(ov13b->reset)) {
+		dev_dbg(dev, "failed to get reset gpio: %ld",
+			PTR_ERR(ov13b->reset));
+		ov13b->reset = NULL;
+	}
+
+	ov13b->img_clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(ov13b->img_clk)) {
+		dev_dbg(dev, "failed to get imaging clock: %ld",
+			PTR_ERR(ov13b->img_clk));
+		ov13b->img_clk = NULL;
+	}
+
+	ov13b->vdd = devm_regulator_get_optional(dev, "vdd");
+	if (IS_ERR(ov13b->vdd)) {
+		dev_dbg(dev, "failed to get vdd regulator: %ld",
+			PTR_ERR(ov13b->vdd));
+		ov13b->vdd = NULL;
+	}
+}
+
 static int ov13b10_check_hwcfg(struct device *dev)
 {
 	struct v4l2_fwnode_endpoint bus_cfg = {
@@ -1407,13 +1490,21 @@ static int ov13b10_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops);
 
+	ov13b10_get_pm_resources(&client->dev);
+
 	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");
+			goto error_power_off;
+		}
+
 		/* 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 +1513,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;
@@ -1461,6 +1552,8 @@ static int ov13b10_probe(struct i2c_client *client)
 error_handler_free:
 	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 +1572,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
-- 
2.40.1

Re: [PATCH 3/4] media: ov13b10: add PM control support based on power resources

[Sakari Ailus]

Hi Bingbu,

Thanks for the patch.

On Fri, May 26, 2023 at 05:58:39PM +0800, 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. On such platform, 1 GPIO
> can be used to enable AVDD and DOVDD, 1 GPIO to reset, we just have
> one power supply 'vdd' for camera. Add 2 power interfaces and also
> registered as runtime PM callbacks to support that.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> Signed-off-by: Hao Yao <hao.yao@intel.com>
> ---
>  drivers/media/i2c/ov13b10.c | 98 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 96 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
> index 2d48c94659a4..b1faa89a3571 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 *vdd;
> +	struct gpio_desc *reset;
> +
>  	/* V4L2 Controls */
>  	struct v4l2_ctrl *link_freq;
>  	struct v4l2_ctrl *pixel_rate;
> @@ -1051,6 +1059,50 @@ 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->vdd || !ov13b10->reset || !ov13b10->img_clk)
> +		return 0;

This can be dropped.

> +
> +	gpiod_set_value_cansleep(ov13b10->reset, 1);
> +	regulator_disable(ov13b10->vdd);
> +	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;
> +
> +	if (!ov13b10->vdd || !ov13b10->reset || !ov13b10->img_clk)
> +		return 0;

Instead I'd execute the sleep below if any of these is non-NULL. No need
to return here.

> +
> +	ret = clk_prepare_enable(ov13b10->img_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable imaging clock: %d", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(ov13b10->vdd);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable vdd: %d", ret);
> +		return ret;
> +	}
> +
> +	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 +1369,37 @@ static void ov13b10_free_controls(struct ov13b10 *ov13b)
>  	mutex_destroy(&ov13b->mutex);
>  }
>  
> +static void ov13b10_get_pm_resources(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov13b10 *ov13b = to_ov13b10(sd);
> +
> +	if (!is_acpi_node(dev_fwnode(dev)))
> +		return;

I'd omit this check. This would work on DT although bindings should still
be written. That may be left for a future excercise I think.

> +
> +	ov13b->reset = devm_gpiod_get_optional(dev, "reset",
> +					       GPIOD_OUT_LOW);
> +	if (IS_ERR(ov13b->reset)) {
> +		dev_dbg(dev, "failed to get reset gpio: %ld",
> +			PTR_ERR(ov13b->reset));
> +		ov13b->reset = NULL;
> +	}
> +
> +	ov13b->img_clk = devm_clk_get_optional(dev, NULL);
> +	if (IS_ERR(ov13b->img_clk)) {
> +		dev_dbg(dev, "failed to get imaging clock: %ld",
> +			PTR_ERR(ov13b->img_clk));
> +		ov13b->img_clk = NULL;
> +	}
> +
> +	ov13b->vdd = devm_regulator_get_optional(dev, "vdd");
> +	if (IS_ERR(ov13b->vdd)) {
> +		dev_dbg(dev, "failed to get vdd regulator: %ld",
> +			PTR_ERR(ov13b->vdd));
> +		ov13b->vdd = NULL;

You should return the error code instead, in all three cases.

> +	}
> +}
> +
>  static int ov13b10_check_hwcfg(struct device *dev)
>  {
>  	struct v4l2_fwnode_endpoint bus_cfg = {
> @@ -1407,13 +1490,21 @@ static int ov13b10_probe(struct i2c_client *client)
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops);
>  
> +	ov13b10_get_pm_resources(&client->dev);
> +
>  	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");
> +			goto error_power_off;

return ret;

See also below...

> +		}
> +
>  		/* 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 +1513,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;
> @@ -1461,6 +1552,8 @@ static int ov13b10_probe(struct i2c_client *client)
>  error_handler_free:
>  	ov13b10_free_controls(ov13b);
>  	dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
> +error_power_off:
> +	ov13b10_power_off(&client->dev);

This may only be called if ov13b10_power_on() was successfully called
first.

>  
>  	return ret;
>  }
> @@ -1479,6 +1572,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
> 

-- 
Kind regards,

Sakari Ailus

RE: [PATCH 3/4] media: ov13b10: add PM control support based on power resources

[Cao, Bingbu]

Sakari, 

Thanks for your review.

------------------------------------------------------------------------
BRs,  
Bingbu Cao 

>-----Original Message-----
>From: Sakari Ailus <sakari.ailus@linux.intel.com>
>Sent: Friday, May 26, 2023 18:02
>To: Cao, Bingbu <bingbu.cao@intel.com>
>Cc: linux-media@vger.kernel.org; Kao, Arec <arec.kao@intel.com>; Yao, Hao
><hao.yao@intel.com>; bingbu.cao@linux.intel.com
>Subject: Re: [PATCH 3/4] media: ov13b10: add PM control support based on
>power resources
>
>Hi Bingbu,
>
>Thanks for the patch.
>
>On Fri, May 26, 2023 at 05:58:39PM +0800, 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. On such platform, 1 GPIO
>> can be used to enable AVDD and DOVDD, 1 GPIO to reset, we just have
>> one power supply 'vdd' for camera. Add 2 power interfaces and also
>> registered as runtime PM callbacks to support that.
>>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>> ---
>>  drivers/media/i2c/ov13b10.c | 98
>> ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 96 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
>> index 2d48c94659a4..b1faa89a3571 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 *vdd;
>> +	struct gpio_desc *reset;
>> +
>>  	/* V4L2 Controls */
>>  	struct v4l2_ctrl *link_freq;
>>  	struct v4l2_ctrl *pixel_rate;
>> @@ -1051,6 +1059,50 @@ 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->vdd || !ov13b10->reset || !ov13b10->img_clk)
>> +		return 0;
>
>This can be dropped.
>
>> +
>> +	gpiod_set_value_cansleep(ov13b10->reset, 1);
>> +	regulator_disable(ov13b10->vdd);
>> +	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;
>> +
>> +	if (!ov13b10->vdd || !ov13b10->reset || !ov13b10->img_clk)
>> +		return 0;
>
>Instead I'd execute the sleep below if any of these is non-NULL. No need to
>return here.
>
>> +
>> +	ret = clk_prepare_enable(ov13b10->img_clk);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to enable imaging clock: %d", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = regulator_enable(ov13b10->vdd);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to enable vdd: %d", ret);
>> +		return ret;
>> +	}
>> +
>> +	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 +1369,37 @@ static void ov13b10_free_controls(struct ov13b10
>*ov13b)
>>  	mutex_destroy(&ov13b->mutex);
>>  }
>>
>> +static void ov13b10_get_pm_resources(struct device *dev) {
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ov13b10 *ov13b = to_ov13b10(sd);
>> +
>> +	if (!is_acpi_node(dev_fwnode(dev)))
>> +		return;
>
>I'd omit this check. This would work on DT although bindings should still
>be written. That may be left for a future excercise I think.
>
>> +
>> +	ov13b->reset = devm_gpiod_get_optional(dev, "reset",
>> +					       GPIOD_OUT_LOW);
>> +	if (IS_ERR(ov13b->reset)) {
>> +		dev_dbg(dev, "failed to get reset gpio: %ld",
>> +			PTR_ERR(ov13b->reset));
>> +		ov13b->reset = NULL;
>> +	}
>> +
>> +	ov13b->img_clk = devm_clk_get_optional(dev, NULL);
>> +	if (IS_ERR(ov13b->img_clk)) {
>> +		dev_dbg(dev, "failed to get imaging clock: %ld",
>> +			PTR_ERR(ov13b->img_clk));
>> +		ov13b->img_clk = NULL;
>> +	}
>> +
>> +	ov13b->vdd = devm_regulator_get_optional(dev, "vdd");
>> +	if (IS_ERR(ov13b->vdd)) {
>> +		dev_dbg(dev, "failed to get vdd regulator: %ld",
>> +			PTR_ERR(ov13b->vdd));
>> +		ov13b->vdd = NULL;
>
>You should return the error code instead, in all three cases.

On some platforms such as Chromebook, there is no INT3472 requirement.
Here it ignores that case and return without errors, thus the power on
and off callback would be NULL functions then.


>
>> +	}
>> +}
>> +
>>  static int ov13b10_check_hwcfg(struct device *dev)  {
>>  	struct v4l2_fwnode_endpoint bus_cfg = { @@ -1407,13 +1490,21 @@
>> static int ov13b10_probe(struct i2c_client *client)
>>  	/* Initialize subdev */
>>  	v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops);
>>
>> +	ov13b10_get_pm_resources(&client->dev);
>> +
>>  	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");
>> +			goto error_power_off;
>
>return ret;

After power on, the caller has no idea which PM resource was
set, so here call power off as a rollback.

>
>See also below...
>
>> +		}
>> +
>>  		/* 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 +1513,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; @@ -1461,6 +1552,8
>> @@ static int ov13b10_probe(struct i2c_client *client)
>>  error_handler_free:
>>  	ov13b10_free_controls(ov13b);
>>  	dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
>> +error_power_off:
>> +	ov13b10_power_off(&client->dev);
>
>This may only be called if ov13b10_power_on() was successfully called first.
>
>>
>>  	return ret;
>>  }
>> @@ -1479,6 +1572,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
>>
>
>--
>Kind regards,
>
>Sakari Ailus

Re: [PATCH 3/4] media: ov13b10: add PM control support based on power resources

[Sakari Ailus]

Hi Bingbu,

On Fri, May 26, 2023 at 11:21:20AM +0000, Cao, Bingbu wrote:
> Sakari, 
> 
> Thanks for your review.
> 
> ------------------------------------------------------------------------
> BRs,  
> Bingbu Cao 
> 
> >-----Original Message-----
> >From: Sakari Ailus <sakari.ailus@linux.intel.com>
> >Sent: Friday, May 26, 2023 18:02
> >To: Cao, Bingbu <bingbu.cao@intel.com>
> >Cc: linux-media@vger.kernel.org; Kao, Arec <arec.kao@intel.com>; Yao, Hao
> ><hao.yao@intel.com>; bingbu.cao@linux.intel.com
> >Subject: Re: [PATCH 3/4] media: ov13b10: add PM control support based on
> >power resources
> >
> >Hi Bingbu,
> >
> >Thanks for the patch.
> >
> >On Fri, May 26, 2023 at 05:58:39PM +0800, 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. On such platform, 1 GPIO
> >> can be used to enable AVDD and DOVDD, 1 GPIO to reset, we just have
> >> one power supply 'vdd' for camera. Add 2 power interfaces and also
> >> registered as runtime PM callbacks to support that.
> >>
> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> >> Signed-off-by: Hao Yao <hao.yao@intel.com>
> >> ---
> >>  drivers/media/i2c/ov13b10.c | 98
> >> ++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 96 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
> >> index 2d48c94659a4..b1faa89a3571 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 *vdd;
> >> +	struct gpio_desc *reset;
> >> +
> >>  	/* V4L2 Controls */
> >>  	struct v4l2_ctrl *link_freq;
> >>  	struct v4l2_ctrl *pixel_rate;
> >> @@ -1051,6 +1059,50 @@ 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->vdd || !ov13b10->reset || !ov13b10->img_clk)
> >> +		return 0;
> >
> >This can be dropped.
> >
> >> +
> >> +	gpiod_set_value_cansleep(ov13b10->reset, 1);
> >> +	regulator_disable(ov13b10->vdd);
> >> +	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;
> >> +
> >> +	if (!ov13b10->vdd || !ov13b10->reset || !ov13b10->img_clk)
> >> +		return 0;
> >
> >Instead I'd execute the sleep below if any of these is non-NULL. No need to
> >return here.
> >
> >> +
> >> +	ret = clk_prepare_enable(ov13b10->img_clk);
> >> +	if (ret < 0) {
> >> +		dev_err(dev, "failed to enable imaging clock: %d", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = regulator_enable(ov13b10->vdd);
> >> +	if (ret < 0) {
> >> +		dev_err(dev, "failed to enable vdd: %d", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	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 +1369,37 @@ static void ov13b10_free_controls(struct ov13b10
> >*ov13b)
> >>  	mutex_destroy(&ov13b->mutex);
> >>  }
> >>
> >> +static void ov13b10_get_pm_resources(struct device *dev) {
> >> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >> +	struct ov13b10 *ov13b = to_ov13b10(sd);
> >> +
> >> +	if (!is_acpi_node(dev_fwnode(dev)))
> >> +		return;
> >
> >I'd omit this check. This would work on DT although bindings should still
> >be written. That may be left for a future excercise I think.
> >
> >> +
> >> +	ov13b->reset = devm_gpiod_get_optional(dev, "reset",
> >> +					       GPIOD_OUT_LOW);
> >> +	if (IS_ERR(ov13b->reset)) {
> >> +		dev_dbg(dev, "failed to get reset gpio: %ld",
> >> +			PTR_ERR(ov13b->reset));
> >> +		ov13b->reset = NULL;
> >> +	}
> >> +
> >> +	ov13b->img_clk = devm_clk_get_optional(dev, NULL);
> >> +	if (IS_ERR(ov13b->img_clk)) {
> >> +		dev_dbg(dev, "failed to get imaging clock: %ld",
> >> +			PTR_ERR(ov13b->img_clk));
> >> +		ov13b->img_clk = NULL;
> >> +	}
> >> +
> >> +	ov13b->vdd = devm_regulator_get_optional(dev, "vdd");
> >> +	if (IS_ERR(ov13b->vdd)) {
> >> +		dev_dbg(dev, "failed to get vdd regulator: %ld",
> >> +			PTR_ERR(ov13b->vdd));
> >> +		ov13b->vdd = NULL;
> >
> >You should return the error code instead, in all three cases.
> 
> On some platforms such as Chromebook, there is no INT3472 requirement.
> Here it ignores that case and return without errors, thus the power on
> and off callback would be NULL functions then.

Don't the _optional() variants of these functions return NULL if there's no
such thing for the device?

> 
> 
> >
> >> +	}
> >> +}
> >> +
> >>  static int ov13b10_check_hwcfg(struct device *dev)  {
> >>  	struct v4l2_fwnode_endpoint bus_cfg = { @@ -1407,13 +1490,21 @@
> >> static int ov13b10_probe(struct i2c_client *client)
> >>  	/* Initialize subdev */
> >>  	v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops);
> >>
> >> +	ov13b10_get_pm_resources(&client->dev);
> >> +
> >>  	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");
> >> +			goto error_power_off;
> >
> >return ret;
> 
> After power on, the caller has no idea which PM resource was
> set, so here call power off as a rollback.

Sure, but it needs to be done in ov13b10_power_on() instead. If there's an
error, some of the resources weren't enabled so you can't undo that here.

> 
> >
> >See also below...
> >
> >> +		}
> >> +
> >>  		/* 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 +1513,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; @@ -1461,6 +1552,8
> >> @@ static int ov13b10_probe(struct i2c_client *client)
> >>  error_handler_free:
> >>  	ov13b10_free_controls(ov13b);
> >>  	dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
> >> +error_power_off:
> >> +	ov13b10_power_off(&client->dev);
> >
> >This may only be called if ov13b10_power_on() was successfully called first.
> >
> >>
> >>  	return ret;
> >>  }
> >> @@ -1479,6 +1572,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
> >>

-- 
Kind regards,

Sakari Ailus