[PATCH 0/6] Support device probe in non-zero ACPI D state

[PATCH 0/6] Support device probe in non-zero ACPI D state

[Bingbu Cao]

In many use cases where the chip is part of a camera module, and the camera
module is wired together with a privacy LED, powering on the device during
probe will cause the LED blink, it is undesireable as the privacy LED concern.

Sakari's change - 
https://lore.kernel.org/lkml/20211018121729.6357-1-sakari.ailus@linux.intel.com/
which add the support device probe in non-zero ACPI D state.

This following patch add support probe in non-zero ACPI D state for more camera
sensors: ov2740, ov5670, ov5675, ov8856, imx208 and hi556.

--

This patch series is based on linux-next branch of Rafael J. Wysocki's
linux-pm git repository:
https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git

--
--

Bingbu Cao (5):
  media: ov8856: support device probe in non-zero ACPI D state
  media: ov2740: support device probe in non-zero ACPI D state
  media: imx208: Support device probe in non-zero ACPI D state
  media: ov5675: Support device probe in non-zero ACPI D state
  media: hi556: Support device probe in non-zero ACPI D state

Sakari Ailus (1):
  media: ov5670: Support device probe in non-zero ACPI D state

 drivers/media/i2c/hi556.c  |  67 ++++++++++++-------
 drivers/media/i2c/imx208.c |  77 +++++++++++++--------
 drivers/media/i2c/ov2740.c |  69 ++++++++++++-------
 drivers/media/i2c/ov5670.c |  78 +++++++++++++---------
 drivers/media/i2c/ov5675.c |  71 +++++++++++++-------
 drivers/media/i2c/ov8856.c | 162 +++++++++++++++++++++++++--------------------
 6 files changed, 316 insertions(+), 208 deletions(-)

-- 
2.7.4

[PATCH 1/6] media: ov8856: support device probe in non-zero ACPI D state

[Bingbu Cao]

Tell ACPI device PM code that the driver supports the device being in
non-zero ACPI D state when the driver's probe function is entered.

Also do identification on the first access of the device, whether in probe
or when starting streaming.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/media/i2c/ov8856.c | 162 +++++++++++++++++++++++++--------------------
 1 file changed, 89 insertions(+), 73 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index c6c6050cda1a..8785764b7a74 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -1445,6 +1445,9 @@ struct ov8856 {
 
 	const struct ov8856_lane_cfg *priv_lane;
 	u8 modes_size;
+
+	/* True if the device has been identified */
+	bool identified;
 };
 
 struct ov8856_lane_cfg {
@@ -1685,6 +1688,71 @@ static int ov8856_write_reg_list(struct ov8856 *ov8856,
 	return 0;
 }
 
+static int ov8856_identify_module(struct ov8856 *ov8856)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
+	int ret;
+	u32 val;
+
+	if (ov8856->identified)
+		return 0;
+
+	ret = ov8856_read_reg(ov8856, OV8856_REG_CHIP_ID,
+			      OV8856_REG_VALUE_24BIT, &val);
+	if (ret)
+		return ret;
+
+	if (val != OV8856_CHIP_ID) {
+		dev_err(&client->dev, "chip id mismatch: %x!=%x",
+			OV8856_CHIP_ID, val);
+		return -ENXIO;
+	}
+
+	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
+			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
+	if (ret)
+		return ret;
+
+	ret = ov8856_write_reg(ov8856, OV8856_OTP_MODE_CTRL,
+			       OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
+	if (ret) {
+		dev_err(&client->dev, "failed to set otp mode");
+		return ret;
+	}
+
+	ret = ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
+			       OV8856_REG_VALUE_08BIT,
+			       OV8856_OTP_LOAD_CTRL_ENABLE);
+	if (ret) {
+		dev_err(&client->dev, "failed to enable load control");
+		return ret;
+	}
+
+	ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
+			      OV8856_REG_VALUE_08BIT, &val);
+	if (ret) {
+		dev_err(&client->dev, "failed to read module revision");
+		return ret;
+	}
+
+	dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
+		 val,
+		 val == OV8856_2A_MODULE ? "2A" :
+		 val == OV8856_1B_MODULE ? "1B" : "unknown revision",
+		 client->addr);
+
+	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
+			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STANDBY);
+	if (ret) {
+		dev_err(&client->dev, "failed to exit streaming mode");
+		return ret;
+	}
+
+	ov8856->identified = true;
+
+	return 0;
+}
+
 static int ov8856_update_digital_gain(struct ov8856 *ov8856, u32 d_gain)
 {
 	int ret;
@@ -1969,6 +2037,10 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
 	const struct ov8856_reg_list *reg_list;
 	int link_freq_index, ret;
 
+	ret = ov8856_identify_module(ov8856);
+	if (ret)
+		return ret;
+
 	link_freq_index = ov8856->cur_mode->link_freq_index;
 	reg_list = &ov8856->priv_lane->link_freq_configs[link_freq_index].reg_list;
 
@@ -2276,65 +2348,6 @@ static const struct v4l2_subdev_internal_ops ov8856_internal_ops = {
 	.open = ov8856_open,
 };
 
-static int ov8856_identify_module(struct ov8856 *ov8856)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
-	int ret;
-	u32 val;
-
-	ret = ov8856_read_reg(ov8856, OV8856_REG_CHIP_ID,
-			      OV8856_REG_VALUE_24BIT, &val);
-	if (ret)
-		return ret;
-
-	if (val != OV8856_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x",
-			OV8856_CHIP_ID, val);
-		return -ENXIO;
-	}
-
-	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
-			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
-	if (ret)
-		return ret;
-
-	ret = ov8856_write_reg(ov8856, OV8856_OTP_MODE_CTRL,
-			       OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
-	if (ret) {
-		dev_err(&client->dev, "failed to set otp mode");
-		return ret;
-	}
-
-	ret = ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
-			       OV8856_REG_VALUE_08BIT,
-			       OV8856_OTP_LOAD_CTRL_ENABLE);
-	if (ret) {
-		dev_err(&client->dev, "failed to enable load control");
-		return ret;
-	}
-
-	ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
-			      OV8856_REG_VALUE_08BIT, &val);
-	if (ret) {
-		dev_err(&client->dev, "failed to read module revision");
-		return ret;
-	}
-
-	dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
-		 val,
-		 val == OV8856_2A_MODULE ? "2A" :
-		 val == OV8856_1B_MODULE ? "1B" : "unknown revision",
-		 client->addr);
-
-	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
-			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STANDBY);
-	if (ret) {
-		dev_err(&client->dev, "failed to exit streaming mode");
-		return ret;
-	}
-
-	return 0;
-}
 
 static int ov8856_get_hwcfg(struct ov8856 *ov8856, struct device *dev)
 {
@@ -2458,6 +2471,7 @@ static int ov8856_probe(struct i2c_client *client)
 {
 	struct ov8856 *ov8856;
 	int ret;
+	bool full_power;
 
 	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
 	if (!ov8856)
@@ -2472,16 +2486,19 @@ static int ov8856_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
 
-	ret = __ov8856_power_on(ov8856);
-	if (ret) {
-		dev_err(&client->dev, "failed to power on\n");
-		return ret;
-	}
+	full_power = acpi_dev_state_d0(&client->dev);
+	if (full_power) {
+		ret = __ov8856_power_on(ov8856);
+		if (ret) {
+			dev_err(&client->dev, "failed to power on\n");
+			return ret;
+		}
 
-	ret = ov8856_identify_module(ov8856);
-	if (ret) {
-		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		goto probe_power_off;
+		ret = ov8856_identify_module(ov8856);
+		if (ret) {
+			dev_err(&client->dev, "failed to find sensor: %d", ret);
+			goto probe_power_off;
+		}
 	}
 
 	mutex_init(&ov8856->mutex);
@@ -2511,11 +2528,9 @@ static int ov8856_probe(struct i2c_client *client)
 		goto probe_error_media_entity_cleanup;
 	}
 
-	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
-	 */
-	pm_runtime_set_active(&client->dev);
+	/* 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);
 
@@ -2562,6 +2577,7 @@ static struct i2c_driver ov8856_i2c_driver = {
 	},
 	.probe_new = ov8856_probe,
 	.remove = ov8856_remove,
+	.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
 };
 
 module_i2c_driver(ov8856_i2c_driver);
-- 
2.7.4

[PATCH 2/6] media: ov5670: Support device probe in non-zero ACPI D state

[Bingbu Cao]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Tell ACPI device PM code that the driver supports the device being in
non-zero ACPI D state when the driver's probe function is entered.

Also do identification on the first access of the device, whether in probe
or when starting streaming.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ov5670.c | 78 +++++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 32 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 251f459ab484..5adf73cb88d4 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -1833,6 +1833,8 @@ struct ov5670 {
 
 	/* Streaming on/off */
 	bool streaming;
+	/* True if the device has been identified */
+	bool identified;
 };
 
 #define to_ov5670(_sd)	container_of(_sd, struct ov5670, sd)
@@ -2273,6 +2275,32 @@ static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
 	return 0;
 }
 
+/* Verify chip ID */
+static int ov5670_identify_module(struct ov5670 *ov5670)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
+	int ret;
+	u32 val;
+
+	if (ov5670->identified)
+		return 0;
+
+	ret = ov5670_read_reg(ov5670, OV5670_REG_CHIP_ID,
+			      OV5670_REG_VALUE_24BIT, &val);
+	if (ret)
+		return ret;
+
+	if (val != OV5670_CHIP_ID) {
+		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
+			OV5670_CHIP_ID, val);
+		return -ENXIO;
+	}
+
+	ov5670->identified = true;
+
+	return 0;
+}
+
 /* Prepare streaming by writing default values and customized values */
 static int ov5670_start_streaming(struct ov5670 *ov5670)
 {
@@ -2281,6 +2309,10 @@ static int ov5670_start_streaming(struct ov5670 *ov5670)
 	int link_freq_index;
 	int ret;
 
+	ret = ov5670_identify_module(ov5670);
+	if (ret)
+		return ret;
+
 	/* Get out of from software reset */
 	ret = ov5670_write_reg(ov5670, OV5670_REG_SOFTWARE_RST,
 			       OV5670_REG_VALUE_08BIT, OV5670_SOFTWARE_RST);
@@ -2400,27 +2432,6 @@ static int __maybe_unused ov5670_resume(struct device *dev)
 	return 0;
 }
 
-/* Verify chip ID */
-static int ov5670_identify_module(struct ov5670 *ov5670)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
-	int ret;
-	u32 val;
-
-	ret = ov5670_read_reg(ov5670, OV5670_REG_CHIP_ID,
-			      OV5670_REG_VALUE_24BIT, &val);
-	if (ret)
-		return ret;
-
-	if (val != OV5670_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
-			OV5670_CHIP_ID, val);
-		return -ENXIO;
-	}
-
-	return 0;
-}
-
 static const struct v4l2_subdev_core_ops ov5670_core_ops = {
 	.log_status = v4l2_ctrl_subdev_log_status,
 	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
@@ -2462,6 +2473,7 @@ static int ov5670_probe(struct i2c_client *client)
 	struct ov5670 *ov5670;
 	const char *err_msg;
 	u32 input_clk = 0;
+	bool full_power;
 	int ret;
 
 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
@@ -2478,11 +2490,14 @@ static int ov5670_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
-	/* Check module identity */
-	ret = ov5670_identify_module(ov5670);
-	if (ret) {
-		err_msg = "ov5670_identify_module() error";
-		goto error_print;
+	full_power = acpi_dev_state_d0(&client->dev);
+	if (full_power) {
+		/* Check module identity */
+		ret = ov5670_identify_module(ov5670);
+		if (ret) {
+			err_msg = "ov5670_identify_module() error";
+			goto error_print;
+		}
 	}
 
 	mutex_init(&ov5670->mutex);
@@ -2519,11 +2534,9 @@ static int ov5670_probe(struct i2c_client *client)
 
 	ov5670->streaming = false;
 
-	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
-	 */
-	pm_runtime_set_active(&client->dev);
+	/* 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);
 
@@ -2565,7 +2578,7 @@ static const struct dev_pm_ops ov5670_pm_ops = {
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id ov5670_acpi_ids[] = {
-	{"INT3479"},
+	{ "INT3479" },
 	{ /* sentinel */ }
 };
 
@@ -2580,6 +2593,7 @@ static struct i2c_driver ov5670_i2c_driver = {
 	},
 	.probe_new = ov5670_probe,
 	.remove = ov5670_remove,
+	.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
 };
 
 module_i2c_driver(ov5670_i2c_driver);
-- 
2.7.4

[PATCH 3/6] media: ov2740: support device probe in non-zero ACPI D state

[Bingbu Cao]

Tell ACPI device PM code that the driver supports the device being in
non-zero ACPI D state when the driver's probe function is entered.

Also do identification on the first access of the device, whether in probe
or when starting streaming.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/media/i2c/ov2740.c | 69 ++++++++++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 934c9d65cb09..bab720c7c1de 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -345,6 +345,9 @@ struct ov2740 {
 
 	/* NVM data inforamtion */
 	struct nvm_data *nvm;
+
+	/* True if the device has been identified */
+	bool identified;
 };
 
 static inline struct ov2740 *to_ov2740(struct v4l2_subdev *subdev)
@@ -440,6 +443,30 @@ static int ov2740_write_reg_list(struct ov2740 *ov2740,
 	return 0;
 }
 
+static int ov2740_identify_module(struct ov2740 *ov2740)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
+	int ret;
+	u32 val;
+
+	if (ov2740->identified)
+		return 0;
+
+	ret = ov2740_read_reg(ov2740, OV2740_REG_CHIP_ID, 3, &val);
+	if (ret)
+		return ret;
+
+	if (val != OV2740_CHIP_ID) {
+		dev_err(&client->dev, "chip id mismatch: %x!=%x",
+			OV2740_CHIP_ID, val);
+		return -ENXIO;
+	}
+
+	ov2740->identified = true;
+
+	return 0;
+}
+
 static int ov2740_update_digital_gain(struct ov2740 *ov2740, u32 d_gain)
 {
 	int ret = 0;
@@ -724,6 +751,10 @@ static int ov2740_start_streaming(struct ov2740 *ov2740)
 	int link_freq_index;
 	int ret = 0;
 
+	ret = ov2740_identify_module(ov2740);
+	if (ret)
+		return ret;
+
 	ov2740_load_otp_data(nvm);
 
 	link_freq_index = ov2740->cur_mode->link_freq_index;
@@ -956,25 +987,6 @@ static const struct v4l2_subdev_internal_ops ov2740_internal_ops = {
 	.open = ov2740_open,
 };
 
-static int ov2740_identify_module(struct ov2740 *ov2740)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
-	int ret;
-	u32 val;
-
-	ret = ov2740_read_reg(ov2740, OV2740_REG_CHIP_ID, 3, &val);
-	if (ret)
-		return ret;
-
-	if (val != OV2740_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x",
-			OV2740_CHIP_ID, val);
-		return -ENXIO;
-	}
-
-	return 0;
-}
-
 static int ov2740_check_hwcfg(struct device *dev)
 {
 	struct fwnode_handle *ep;
@@ -1137,6 +1149,7 @@ static int ov2740_probe(struct i2c_client *client)
 {
 	struct ov2740 *ov2740;
 	int ret = 0;
+	bool full_power;
 
 	ret = ov2740_check_hwcfg(&client->dev);
 	if (ret) {
@@ -1149,6 +1162,15 @@ static int ov2740_probe(struct i2c_client *client)
 	if (!ov2740)
 		return -ENOMEM;
 
+	full_power = acpi_dev_state_d0(&client->dev);
+	if (full_power) {
+		ret = ov2740_identify_module(ov2740);
+		if (ret) {
+			dev_err(&client->dev, "failed to find sensor: %d", ret);
+			return ret;
+		}
+	}
+
 	v4l2_i2c_subdev_init(&ov2740->sd, client, &ov2740_subdev_ops);
 	ret = ov2740_identify_module(ov2740);
 	if (ret) {
@@ -1186,11 +1208,9 @@ static int ov2740_probe(struct i2c_client *client)
 	if (ret)
 		dev_warn(&client->dev, "register nvmem failed, ret %d\n", ret);
 
-	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
-	 */
-	pm_runtime_set_active(&client->dev);
+	/* 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);
 
@@ -1225,6 +1245,7 @@ static struct i2c_driver ov2740_i2c_driver = {
 	},
 	.probe_new = ov2740_probe,
 	.remove = ov2740_remove,
+	.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
 };
 
 module_i2c_driver(ov2740_i2c_driver);
-- 
2.7.4

[PATCH 4/6] media: imx208: Support device probe in non-zero ACPI D state

[Bingbu Cao]

Tell ACPI device PM code that the driver supports the device being in
non-zero ACPI D state when the driver's probe function is entered.

Also do identification on the first access of the device, whether in probe
or when starting streaming.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/media/i2c/imx208.c | 77 +++++++++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c
index 6f3d9c1b5879..b9f6d5f33b58 100644
--- a/drivers/media/i2c/imx208.c
+++ b/drivers/media/i2c/imx208.c
@@ -296,6 +296,9 @@ struct imx208 {
 	/* OTP data */
 	bool otp_read;
 	char otp_data[IMX208_OTP_SIZE];
+
+	/* True if the device has been identified */
+	bool identified;
 };
 
 static inline struct imx208 *to_imx208(struct v4l2_subdev *_sd)
@@ -619,6 +622,34 @@ static int imx208_set_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int imx208_identify_module(struct imx208 *imx208)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd);
+	int ret;
+	u32 val;
+
+	if (imx208->identified)
+		return 0;
+
+	ret = imx208_read_reg(imx208, IMX208_REG_CHIP_ID,
+			      2, &val);
+	if (ret) {
+		dev_err(&client->dev, "failed to read chip id %x\n",
+			IMX208_CHIP_ID);
+		return ret;
+	}
+
+	if (val != IMX208_CHIP_ID) {
+		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
+			IMX208_CHIP_ID, val);
+		return -EIO;
+	}
+
+	imx208->identified = true;
+
+	return 0;
+}
+
 /* Start streaming */
 static int imx208_start_streaming(struct imx208 *imx208)
 {
@@ -626,6 +657,10 @@ static int imx208_start_streaming(struct imx208 *imx208)
 	const struct imx208_reg_list *reg_list;
 	int ret, link_freq_index;
 
+	ret = imx208_identify_module(imx208);
+	if (ret)
+		return ret;
+
 	/* Setup PLL */
 	link_freq_index = imx208->cur_mode->link_freq_index;
 	reg_list = &link_freq_configs[link_freq_index].reg_list;
@@ -752,29 +787,6 @@ static int __maybe_unused imx208_resume(struct device *dev)
 }
 
 /* Verify chip ID */
-static int imx208_identify_module(struct imx208 *imx208)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd);
-	int ret;
-	u32 val;
-
-	ret = imx208_read_reg(imx208, IMX208_REG_CHIP_ID,
-			      2, &val);
-	if (ret) {
-		dev_err(&client->dev, "failed to read chip id %x\n",
-			IMX208_CHIP_ID);
-		return ret;
-	}
-
-	if (val != IMX208_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
-			IMX208_CHIP_ID, val);
-		return -EIO;
-	}
-
-	return 0;
-}
-
 static const struct v4l2_subdev_video_ops imx208_video_ops = {
 	.s_stream = imx208_set_stream,
 };
@@ -961,6 +973,7 @@ static int imx208_probe(struct i2c_client *client)
 {
 	struct imx208 *imx208;
 	int ret;
+	bool full_power;
 	u32 val = 0;
 
 	device_property_read_u32(&client->dev, "clock-frequency", &val);
@@ -978,11 +991,14 @@ static int imx208_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&imx208->sd, client, &imx208_subdev_ops);
 
-	/* Check module identity */
-	ret = imx208_identify_module(imx208);
-	if (ret) {
-		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		goto error_probe;
+	full_power = acpi_dev_state_d0(&client->dev);
+	if (full_power) {
+		/* Check module identity */
+		ret = imx208_identify_module(imx208);
+		if (ret) {
+			dev_err(&client->dev, "failed to find sensor: %d", ret);
+			goto error_probe;
+		}
 	}
 
 	/* Set default mode to max resolution */
@@ -1017,7 +1033,9 @@ static int imx208_probe(struct i2c_client *client)
 		goto error_async_subdev;
 	}
 
-	pm_runtime_set_active(&client->dev);
+	/* 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);
 
@@ -1077,6 +1095,7 @@ static struct i2c_driver imx208_i2c_driver = {
 	},
 	.probe_new = imx208_probe,
 	.remove = imx208_remove,
+	.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
 };
 
 module_i2c_driver(imx208_i2c_driver);
-- 
2.7.4

[PATCH 5/6] media: ov5675: Support device probe in non-zero ACPI D state

[Bingbu Cao]

Tell ACPI device PM code that the driver supports the device being in
non-zero ACPI D state when the driver's probe function is entered.

Also do identification on the first access of the device, whether in probe
or when starting streaming.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/media/i2c/ov5675.c | 71 ++++++++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 25 deletions(-)

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index da5850b7ad07..00925850fa7c 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -493,6 +493,9 @@ struct ov5675 {
 
 	/* Streaming on/off */
 	bool streaming;
+
+	/* True if the device has been identified */
+	bool identified;
 };
 
 static u64 to_pixel_rate(u32 f_index)
@@ -808,12 +811,41 @@ static void ov5675_update_pad_format(const struct ov5675_mode *mode,
 	fmt->field = V4L2_FIELD_NONE;
 }
 
+static int ov5675_identify_module(struct ov5675 *ov5675)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
+	int ret;
+	u32 val;
+
+	if (ov5675->identified)
+		return 0;
+
+	ret = ov5675_read_reg(ov5675, OV5675_REG_CHIP_ID,
+			      OV5675_REG_VALUE_24BIT, &val);
+	if (ret)
+		return ret;
+
+	if (val != OV5675_CHIP_ID) {
+		dev_err(&client->dev, "chip id mismatch: %x!=%x",
+			OV5675_CHIP_ID, val);
+		return -ENXIO;
+	}
+
+	ov5675->identified = true;
+
+	return 0;
+}
+
 static int ov5675_start_streaming(struct ov5675 *ov5675)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
 	const struct ov5675_reg_list *reg_list;
 	int link_freq_index, ret;
 
+	ret = ov5675_identify_module(ov5675);
+	if (ret)
+		return ret;
+
 	link_freq_index = ov5675->cur_mode->link_freq_index;
 	reg_list = &link_freq_configs[link_freq_index].reg_list;
 	ret = ov5675_write_reg_list(ov5675, reg_list);
@@ -1048,26 +1080,6 @@ static const struct v4l2_subdev_internal_ops ov5675_internal_ops = {
 	.open = ov5675_open,
 };
 
-static int ov5675_identify_module(struct ov5675 *ov5675)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
-	int ret;
-	u32 val;
-
-	ret = ov5675_read_reg(ov5675, OV5675_REG_CHIP_ID,
-			      OV5675_REG_VALUE_24BIT, &val);
-	if (ret)
-		return ret;
-
-	if (val != OV5675_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x",
-			OV5675_CHIP_ID, val);
-		return -ENXIO;
-	}
-
-	return 0;
-}
-
 static int ov5675_check_hwcfg(struct device *dev)
 {
 	struct fwnode_handle *ep;
@@ -1154,6 +1166,7 @@ static int ov5675_remove(struct i2c_client *client)
 static int ov5675_probe(struct i2c_client *client)
 {
 	struct ov5675 *ov5675;
+	bool full_power;
 	int ret;
 
 	ret = ov5675_check_hwcfg(&client->dev);
@@ -1168,10 +1181,14 @@ static int ov5675_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	v4l2_i2c_subdev_init(&ov5675->sd, client, &ov5675_subdev_ops);
-	ret = ov5675_identify_module(ov5675);
-	if (ret) {
-		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		return ret;
+
+	full_power = acpi_dev_state_d0(&client->dev);
+	if (full_power) {
+		ret = ov5675_identify_module(ov5675);
+		if (ret) {
+			dev_err(&client->dev, "failed to find sensor: %d", ret);
+			return ret;
+		}
 	}
 
 	mutex_init(&ov5675->mutex);
@@ -1204,7 +1221,10 @@ static int ov5675_probe(struct i2c_client *client)
 	 * Device is already turned on by i2c-core with ACPI domain PM.
 	 * Enable runtime PM and turn off the device.
 	 */
-	pm_runtime_set_active(&client->dev);
+
+	/* 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);
 
@@ -1241,6 +1261,7 @@ static struct i2c_driver ov5675_i2c_driver = {
 	},
 	.probe_new = ov5675_probe,
 	.remove = ov5675_remove,
+	.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
 };
 
 module_i2c_driver(ov5675_i2c_driver);
-- 
2.7.4

[PATCH 6/6] media: hi556: Support device probe in non-zero ACPI D state

[Bingbu Cao]

Tell ACPI device PM code that the driver supports the device being in
non-zero ACPI D state when the driver's probe function is entered.

Also do identification on the first access of the device, whether in probe
or when starting streaming.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
Signed-off-by: Kao, Arec <arec.kao@intel.com>
---
 drivers/media/i2c/hi556.c | 67 +++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 25 deletions(-)

diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
index 8db1cbedc1fd..c8011467d1a4 100644
--- a/drivers/media/i2c/hi556.c
+++ b/drivers/media/i2c/hi556.c
@@ -495,6 +495,9 @@ struct hi556 {
 
 	/* Streaming on/off */
 	bool streaming;
+
+	/* True if the device has been identified */
+	bool identified;
 };
 
 static u64 to_pixel_rate(u32 f_index)
@@ -757,12 +760,38 @@ static void hi556_assign_pad_format(const struct hi556_mode *mode,
 	fmt->field = V4L2_FIELD_NONE;
 }
 
+static int hi556_identify_module(struct hi556 *hi556)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&hi556->sd);
+	int ret;
+	u32 val;
+
+	ret = hi556_read_reg(hi556, HI556_REG_CHIP_ID,
+			     HI556_REG_VALUE_16BIT, &val);
+	if (ret)
+		return ret;
+
+	if (val != HI556_CHIP_ID) {
+		dev_err(&client->dev, "chip id mismatch: %x!=%x",
+			HI556_CHIP_ID, val);
+		return -ENXIO;
+	}
+
+	hi556->identified = true;
+
+	return 0;
+}
+
 static int hi556_start_streaming(struct hi556 *hi556)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&hi556->sd);
 	const struct hi556_reg_list *reg_list;
 	int link_freq_index, ret;
 
+	ret = hi556_identify_module(hi556);
+	if (ret)
+		return ret;
+
 	link_freq_index = hi556->cur_mode->link_freq_index;
 	reg_list = &link_freq_configs[link_freq_index].reg_list;
 	ret = hi556_write_reg_list(hi556, reg_list);
@@ -1001,26 +1030,6 @@ static const struct v4l2_subdev_internal_ops hi556_internal_ops = {
 	.open = hi556_open,
 };
 
-static int hi556_identify_module(struct hi556 *hi556)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&hi556->sd);
-	int ret;
-	u32 val;
-
-	ret = hi556_read_reg(hi556, HI556_REG_CHIP_ID,
-			     HI556_REG_VALUE_16BIT, &val);
-	if (ret)
-		return ret;
-
-	if (val != HI556_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x",
-			HI556_CHIP_ID, val);
-		return -ENXIO;
-	}
-
-	return 0;
-}
-
 static int hi556_check_hwcfg(struct device *dev)
 {
 	struct fwnode_handle *ep;
@@ -1106,6 +1115,7 @@ static int hi556_remove(struct i2c_client *client)
 static int hi556_probe(struct i2c_client *client)
 {
 	struct hi556 *hi556;
+	bool full_power;
 	int ret;
 
 	ret = hi556_check_hwcfg(&client->dev);
@@ -1120,10 +1130,14 @@ static int hi556_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	v4l2_i2c_subdev_init(&hi556->sd, client, &hi556_subdev_ops);
-	ret = hi556_identify_module(hi556);
-	if (ret) {
-		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		return ret;
+
+	full_power = acpi_dev_state_d0(&client->dev);
+	if (full_power) {
+		ret = hi556_identify_module(hi556);
+		if (ret) {
+			dev_err(&client->dev, "failed to find sensor: %d", ret);
+			return ret;
+		}
 	}
 
 	mutex_init(&hi556->mutex);
@@ -1152,7 +1166,9 @@ static int hi556_probe(struct i2c_client *client)
 		goto probe_error_media_entity_cleanup;
 	}
 
-	pm_runtime_set_active(&client->dev);
+	/* 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);
 
@@ -1189,6 +1205,7 @@ static struct i2c_driver hi556_i2c_driver = {
 	},
 	.probe_new = hi556_probe,
 	.remove = hi556_remove,
+	.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
 };
 
 module_i2c_driver(hi556_i2c_driver);
-- 
2.7.4

Re: [PATCH 4/6] media: imx208: Support device probe in non-zero ACPI D state

[Sakari Ailus]

Hi Bingbu,

On Tue, Nov 09, 2021 at 04:48:33PM +0800, Bingbu Cao wrote:
> Tell ACPI device PM code that the driver supports the device being in
> non-zero ACPI D state when the driver's probe function is entered.
> 
> Also do identification on the first access of the device, whether in probe
> or when starting streaming.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> ---
>  drivers/media/i2c/imx208.c | 77 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 48 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c
> index 6f3d9c1b5879..b9f6d5f33b58 100644
> --- a/drivers/media/i2c/imx208.c
> +++ b/drivers/media/i2c/imx208.c
> @@ -296,6 +296,9 @@ struct imx208 {
>  	/* OTP data */
>  	bool otp_read;
>  	char otp_data[IMX208_OTP_SIZE];
> +
> +	/* True if the device has been identified */
> +	bool identified;
>  };
>  
>  static inline struct imx208 *to_imx208(struct v4l2_subdev *_sd)
> @@ -619,6 +622,34 @@ static int imx208_set_pad_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int imx208_identify_module(struct imx208 *imx208)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd);
> +	int ret;
> +	u32 val;
> +
> +	if (imx208->identified)
> +		return 0;

Otp access requires this as well.

How about adding a runtime PM resume callback for this?

I guess it'd be better for the rest, too. Up to you.

> +
> +	ret = imx208_read_reg(imx208, IMX208_REG_CHIP_ID,
> +			      2, &val);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read chip id %x\n",
> +			IMX208_CHIP_ID);
> +		return ret;
> +	}
> +
> +	if (val != IMX208_CHIP_ID) {
> +		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
> +			IMX208_CHIP_ID, val);
> +		return -EIO;
> +	}
> +
> +	imx208->identified = true;
> +
> +	return 0;
> +}
> +
>  /* Start streaming */
>  static int imx208_start_streaming(struct imx208 *imx208)
>  {
> @@ -626,6 +657,10 @@ static int imx208_start_streaming(struct imx208 *imx208)
>  	const struct imx208_reg_list *reg_list;
>  	int ret, link_freq_index;
>  
> +	ret = imx208_identify_module(imx208);
> +	if (ret)
> +		return ret;
> +
>  	/* Setup PLL */
>  	link_freq_index = imx208->cur_mode->link_freq_index;
>  	reg_list = &link_freq_configs[link_freq_index].reg_list;
> @@ -752,29 +787,6 @@ static int __maybe_unused imx208_resume(struct device *dev)
>  }
>  
>  /* Verify chip ID */
> -static int imx208_identify_module(struct imx208 *imx208)
> -{
> -	struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd);
> -	int ret;
> -	u32 val;
> -
> -	ret = imx208_read_reg(imx208, IMX208_REG_CHIP_ID,
> -			      2, &val);
> -	if (ret) {
> -		dev_err(&client->dev, "failed to read chip id %x\n",
> -			IMX208_CHIP_ID);
> -		return ret;
> -	}
> -
> -	if (val != IMX208_CHIP_ID) {
> -		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
> -			IMX208_CHIP_ID, val);
> -		return -EIO;
> -	}
> -
> -	return 0;
> -}
> -
>  static const struct v4l2_subdev_video_ops imx208_video_ops = {
>  	.s_stream = imx208_set_stream,
>  };
> @@ -961,6 +973,7 @@ static int imx208_probe(struct i2c_client *client)
>  {
>  	struct imx208 *imx208;
>  	int ret;
> +	bool full_power;
>  	u32 val = 0;
>  
>  	device_property_read_u32(&client->dev, "clock-frequency", &val);
> @@ -978,11 +991,14 @@ static int imx208_probe(struct i2c_client *client)
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&imx208->sd, client, &imx208_subdev_ops);
>  
> -	/* Check module identity */
> -	ret = imx208_identify_module(imx208);
> -	if (ret) {
> -		dev_err(&client->dev, "failed to find sensor: %d", ret);
> -		goto error_probe;
> +	full_power = acpi_dev_state_d0(&client->dev);
> +	if (full_power) {
> +		/* Check module identity */
> +		ret = imx208_identify_module(imx208);
> +		if (ret) {
> +			dev_err(&client->dev, "failed to find sensor: %d", ret);
> +			goto error_probe;
> +		}
>  	}
>  
>  	/* Set default mode to max resolution */
> @@ -1017,7 +1033,9 @@ static int imx208_probe(struct i2c_client *client)
>  		goto error_async_subdev;
>  	}
>  
> -	pm_runtime_set_active(&client->dev);
> +	/* 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);
>  
> @@ -1077,6 +1095,7 @@ static struct i2c_driver imx208_i2c_driver = {
>  	},
>  	.probe_new = imx208_probe,
>  	.remove = imx208_remove,
> +	.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
>  };
>  
>  module_i2c_driver(imx208_i2c_driver);

-- 
Regards,

Sakari Ailus

Re: [PATCH 6/6] media: hi556: Support device probe in non-zero ACPI D state

[Sakari Ailus]

Hi Bingbu,

On Tue, Nov 09, 2021 at 04:48:35PM +0800, Bingbu Cao wrote:
> Tell ACPI device PM code that the driver supports the device being in
> non-zero ACPI D state when the driver's probe function is entered.
> 
> Also do identification on the first access of the device, whether in probe
> or when starting streaming.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> Signed-off-by: Kao, Arec <arec.kao@intel.com>
> ---
>  drivers/media/i2c/hi556.c | 67 +++++++++++++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
> index 8db1cbedc1fd..c8011467d1a4 100644
> --- a/drivers/media/i2c/hi556.c
> +++ b/drivers/media/i2c/hi556.c
> @@ -495,6 +495,9 @@ struct hi556 {
>  
>  	/* Streaming on/off */
>  	bool streaming;
> +
> +	/* True if the device has been identified */
> +	bool identified;
>  };
>  
>  static u64 to_pixel_rate(u32 f_index)
> @@ -757,12 +760,38 @@ static void hi556_assign_pad_format(const struct hi556_mode *mode,
>  	fmt->field = V4L2_FIELD_NONE;
>  }
>  
> +static int hi556_identify_module(struct hi556 *hi556)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&hi556->sd);
> +	int ret;
> +	u32 val;

If the sensor's already identified, you can return 0 here.

> +
> +	ret = hi556_read_reg(hi556, HI556_REG_CHIP_ID,
> +			     HI556_REG_VALUE_16BIT, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != HI556_CHIP_ID) {
> +		dev_err(&client->dev, "chip id mismatch: %x!=%x",
> +			HI556_CHIP_ID, val);
> +		return -ENXIO;
> +	}
> +
> +	hi556->identified = true;
> +
> +	return 0;
> +}

-- 
Sakari Ailus

Re: [PATCH 6/6] media: hi556: Support device probe in non-zero ACPI D state

[Bingbu Cao]

On 12/15/21 12:02 AM, Sakari Ailus wrote:
> Hi Bingbu,
> 
> On Tue, Nov 09, 2021 at 04:48:35PM +0800, Bingbu Cao wrote:
>> Tell ACPI device PM code that the driver supports the device being in
>> non-zero ACPI D state when the driver's probe function is entered.
>>
>> Also do identification on the first access of the device, whether in probe
>> or when starting streaming.
>>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> Signed-off-by: Kao, Arec <arec.kao@intel.com>
>> ---
>>  drivers/media/i2c/hi556.c | 67 +++++++++++++++++++++++++++++------------------
>>  1 file changed, 42 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
>> index 8db1cbedc1fd..c8011467d1a4 100644
>> --- a/drivers/media/i2c/hi556.c
>> +++ b/drivers/media/i2c/hi556.c
>> @@ -495,6 +495,9 @@ struct hi556 {
>>  
>>  	/* Streaming on/off */
>>  	bool streaming;
>> +
>> +	/* True if the device has been identified */
>> +	bool identified;
>>  };
>>  
>>  static u64 to_pixel_rate(u32 f_index)
>> @@ -757,12 +760,38 @@ static void hi556_assign_pad_format(const struct hi556_mode *mode,
>>  	fmt->field = V4L2_FIELD_NONE;
>>  }
>>  
>> +static int hi556_identify_module(struct hi556 *hi556)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(&hi556->sd);
>> +	int ret;
>> +	u32 val;
> 
> If the sensor's already identified, you can return 0 here.

Yes, I will add in v2. Thanks!

> 
>> +
>> +	ret = hi556_read_reg(hi556, HI556_REG_CHIP_ID,
>> +			     HI556_REG_VALUE_16BIT, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (val != HI556_CHIP_ID) {
>> +		dev_err(&client->dev, "chip id mismatch: %x!=%x",
>> +			HI556_CHIP_ID, val);
>> +		return -ENXIO;
>> +	}
>> +
>> +	hi556->identified = true;
>> +
>> +	return 0;
>> +}
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH 4/6] media: imx208: Support device probe in non-zero ACPI D state

[Bingbu Cao]

On 12/14/21 11:59 PM, Sakari Ailus wrote:
> Hi Bingbu,
> 
> On Tue, Nov 09, 2021 at 04:48:33PM +0800, Bingbu Cao wrote:
>> Tell ACPI device PM code that the driver supports the device being in
>> non-zero ACPI D state when the driver's probe function is entered.
>>
>> Also do identification on the first access of the device, whether in probe
>> or when starting streaming.
>>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> ---
>>  drivers/media/i2c/imx208.c | 77 +++++++++++++++++++++++++++++-----------------
>>  1 file changed, 48 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c
>> index 6f3d9c1b5879..b9f6d5f33b58 100644
>> --- a/drivers/media/i2c/imx208.c
>> +++ b/drivers/media/i2c/imx208.c
>> @@ -296,6 +296,9 @@ struct imx208 {
>>  	/* OTP data */
>>  	bool otp_read;
>>  	char otp_data[IMX208_OTP_SIZE];
>> +
>> +	/* True if the device has been identified */
>> +	bool identified;
>>  };
>>  
>>  static inline struct imx208 *to_imx208(struct v4l2_subdev *_sd)
>> @@ -619,6 +622,34 @@ static int imx208_set_pad_format(struct v4l2_subdev *sd,
>>  	return 0;
>>  }
>>  
>> +static int imx208_identify_module(struct imx208 *imx208)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd);
>> +	int ret;
>> +	u32 val;
>> +
>> +	if (imx208->identified)
>> +		return 0;
> 
> Otp access requires this as well.

Sakari, thanks for your review.

Yes, OTP read will trigger the LED blink, but I am not sure it makes sense that camera
framework try to read the OTP data without streaming, and it would complain once fail to
access the OTP data.
> 
> How about adding a runtime PM resume callback for this?

For the runtime PM callback, what is the benefit adding a callback here as will call try
pm_runtime_get_sync() each stream on?

> 
> I guess it'd be better for the rest, too. Up to you.
> 
>> +
>> +	ret = imx208_read_reg(imx208, IMX208_REG_CHIP_ID,
>> +			      2, &val);
>> +	if (ret) {
>> +		dev_err(&client->dev, "failed to read chip id %x\n",
>> +			IMX208_CHIP_ID);
>> +		return ret;
>> +	}
>> +
>> +	if (val != IMX208_CHIP_ID) {
>> +		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
>> +			IMX208_CHIP_ID, val);
>> +		return -EIO;
>> +	}
>> +
>> +	imx208->identified = true;
>> +
>> +	return 0;
>> +}
>> +
>>  /* Start streaming */
>>  static int imx208_start_streaming(struct imx208 *imx208)
>>  {
>> @@ -626,6 +657,10 @@ static int imx208_start_streaming(struct imx208 *imx208)
>>  	const struct imx208_reg_list *reg_list;
>>  	int ret, link_freq_index;
>>  
>> +	ret = imx208_identify_module(imx208);
>> +	if (ret)
>> +		return ret;
>> +
>>  	/* Setup PLL */
>>  	link_freq_index = imx208->cur_mode->link_freq_index;
>>  	reg_list = &link_freq_configs[link_freq_index].reg_list;
>> @@ -752,29 +787,6 @@ static int __maybe_unused imx208_resume(struct device *dev)
>>  }
>>  
>>  /* Verify chip ID */
>> -static int imx208_identify_module(struct imx208 *imx208)
>> -{
>> -	struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd);
>> -	int ret;
>> -	u32 val;
>> -
>> -	ret = imx208_read_reg(imx208, IMX208_REG_CHIP_ID,
>> -			      2, &val);
>> -	if (ret) {
>> -		dev_err(&client->dev, "failed to read chip id %x\n",
>> -			IMX208_CHIP_ID);
>> -		return ret;
>> -	}
>> -
>> -	if (val != IMX208_CHIP_ID) {
>> -		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
>> -			IMX208_CHIP_ID, val);
>> -		return -EIO;
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>  static const struct v4l2_subdev_video_ops imx208_video_ops = {
>>  	.s_stream = imx208_set_stream,
>>  };
>> @@ -961,6 +973,7 @@ static int imx208_probe(struct i2c_client *client)
>>  {
>>  	struct imx208 *imx208;
>>  	int ret;
>> +	bool full_power;
>>  	u32 val = 0;
>>  
>>  	device_property_read_u32(&client->dev, "clock-frequency", &val);
>> @@ -978,11 +991,14 @@ static int imx208_probe(struct i2c_client *client)
>>  	/* Initialize subdev */
>>  	v4l2_i2c_subdev_init(&imx208->sd, client, &imx208_subdev_ops);
>>  
>> -	/* Check module identity */
>> -	ret = imx208_identify_module(imx208);
>> -	if (ret) {
>> -		dev_err(&client->dev, "failed to find sensor: %d", ret);
>> -		goto error_probe;
>> +	full_power = acpi_dev_state_d0(&client->dev);
>> +	if (full_power) {
>> +		/* Check module identity */
>> +		ret = imx208_identify_module(imx208);
>> +		if (ret) {
>> +			dev_err(&client->dev, "failed to find sensor: %d", ret);
>> +			goto error_probe;
>> +		}
>>  	}
>>  
>>  	/* Set default mode to max resolution */
>> @@ -1017,7 +1033,9 @@ static int imx208_probe(struct i2c_client *client)
>>  		goto error_async_subdev;
>>  	}
>>  
>> -	pm_runtime_set_active(&client->dev);
>> +	/* 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);
>>  
>> @@ -1077,6 +1095,7 @@ static struct i2c_driver imx208_i2c_driver = {
>>  	},
>>  	.probe_new = imx208_probe,
>>  	.remove = imx208_remove,
>> +	.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
>>  };
>>  
>>  module_i2c_driver(imx208_i2c_driver);
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH 4/6] media: imx208: Support device probe in non-zero ACPI D state

[Sakari Ailus]

Hi Bingbu,

On Wed, Dec 15, 2021 at 11:13:03AM +0800, Bingbu Cao wrote:
> 
> 
> On 12/14/21 11:59 PM, Sakari Ailus wrote:
> > Hi Bingbu,
> > 
> > On Tue, Nov 09, 2021 at 04:48:33PM +0800, Bingbu Cao wrote:
> >> Tell ACPI device PM code that the driver supports the device being in
> >> non-zero ACPI D state when the driver's probe function is entered.
> >>
> >> Also do identification on the first access of the device, whether in probe
> >> or when starting streaming.
> >>
> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> >> ---
> >>  drivers/media/i2c/imx208.c | 77 +++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 48 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c
> >> index 6f3d9c1b5879..b9f6d5f33b58 100644
> >> --- a/drivers/media/i2c/imx208.c
> >> +++ b/drivers/media/i2c/imx208.c
> >> @@ -296,6 +296,9 @@ struct imx208 {
> >>  	/* OTP data */
> >>  	bool otp_read;
> >>  	char otp_data[IMX208_OTP_SIZE];
> >> +
> >> +	/* True if the device has been identified */
> >> +	bool identified;
> >>  };
> >>  
> >>  static inline struct imx208 *to_imx208(struct v4l2_subdev *_sd)
> >> @@ -619,6 +622,34 @@ static int imx208_set_pad_format(struct v4l2_subdev *sd,
> >>  	return 0;
> >>  }
> >>  
> >> +static int imx208_identify_module(struct imx208 *imx208)
> >> +{
> >> +	struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd);
> >> +	int ret;
> >> +	u32 val;
> >> +
> >> +	if (imx208->identified)
> >> +		return 0;
> > 
> > Otp access requires this as well.
> 
> Sakari, thanks for your review.
> 
> Yes, OTP read will trigger the LED blink, but I am not sure it makes sense that camera
> framework try to read the OTP data without streaming, and it would complain once fail to
> access the OTP data.

That might be the case, but the interface this driver provides is
accessible in the user space before streaming is enabled. Accessing
sensor's other registers shouldn't be done before the sensor is identified.

> > 
> > How about adding a runtime PM resume callback for this?
> 
> For the runtime PM callback, what is the benefit adding a callback here as will call try
> pm_runtime_get_sync() each stream on?

My suggestion is to identify the sensor from the runtime PM resume callback
(which this driver doesn't have yet), i.e. not here.

-- 
Kind regards,

Sakari Ailus