Thanks for the review, Sakari.
Due to project duties, I am running late in updating the changes.
Let me update an updated version for this.
-----Original Message-----
From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
Sent: Monday, January 8, 2024 4:36 PM
To: Chen, Jason Z <jason.z.chen@xxxxxxxxx>
Cc: linux-media@xxxxxxxxxxxxxxx; Yeh, Andy <andy.yeh@xxxxxxxxx>; Zhang, Qingwu <qingwu.zhang@xxxxxxxxx>; bingbu.cao@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v1] media: ov08x40: Avoid sensor probing in D0 state
Hi Jason,
On Fri, Jan 05, 2024 at 06:20:08PM +0800, Chen, Jason Z wrote:
> From: Jason Chen <jason.z.chen@xxxxxxxxx>
>
> When the system enters the D0 state and attempt to probe the device,
> another component, such as LED, will also be pulled high due to the
> hardware design. It's advisable to keep the device being probed in a
> different D state.
>
> Signed-off-by: Jason Chen <jason.z.chen@xxxxxxxxx>
> ---
> drivers/media/i2c/ov08x40.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
> index 8f24be08c7b..f46cf0eb6c1 100644
> --- a/drivers/media/i2c/ov08x40.c
> +++ b/drivers/media/i2c/ov08x40.c
> @@ -3226,6 +3226,7 @@ static int ov08x40_probe(struct i2c_client
> *client) {
> struct ov08x40 *ov08x;
> int ret;
> + bool full_power;
>
> /* Check HW config */
> ret = ov08x40_check_hwcfg(&client->dev);
> @@ -3241,11 +3242,14 @@ static int ov08x40_probe(struct i2c_client *client)
> /* Initialize subdev */
> v4l2_i2c_subdev_init(&ov08x->sd, client, &ov08x40_subdev_ops);
>
> - /* Check module identity */
> - ret = ov08x40_identify_module(ov08x);
> - if (ret) {
> - dev_err(&client->dev, "failed to find sensor: %d\n", ret);
> - return ret;
> + full_power = acpi_dev_state_d0(&client->dev);
> + if (full_power) {
> + /* Check module identity */
> + ret = ov08x40_identify_module(ov08x);
This way the sensor identification will be missed if the device wasn't powered on im probe. See e.g. commit d1d2ed5925c370ac09fa6efd39f5f569f562e5b7
for an example.
> + if (ret) {
> + dev_err(&client->dev, "failed to find sensor: %d\n", ret);
> + return ret;
> + }
> }
>
> /* Set default mode to max resolution */ @@ -3277,7 +3281,8 @@
> static int ov08x40_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.
> */
Please remove this comment, too.
> - pm_runtime_set_active(&client->dev);
> + if (full_power)
> + pm_runtime_set_active(&client->dev);
> pm_runtime_enable(&client->dev);
> pm_runtime_idle(&client->dev);
>
> @@ -3321,6 +3326,7 @@ static struct i2c_driver ov08x40_i2c_driver = {
> },
> .probe = ov08x40_probe,
> .remove = ov08x40_remove,
> + .flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
> };
>
> module_i2c_driver(ov08x40_i2c_driver);
--
Regards,
Sakari Ailus
