[v1] media: ov08x40: Avoid sensor probing in D0 state

  From: Jason Chen <jason.z.chen@intel.com>

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@intel.com>
---
 drivers/media/i2c/ov08x40.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
  

Hi Jason,

On Fri, Jan 05, 2024 at 06:20:08PM +0800, Chen, Jason Z wrote:
> From: Jason Chen <jason.z.chen@intel.com>
> 
> 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@intel.com>
> ---
>  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);
  

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@linux.intel.com> 
Sent: Monday, January 8, 2024 4:36 PM
To: Chen, Jason Z <jason.z.chen@intel.com>
Cc: linux-media@vger.kernel.org; Yeh, Andy <andy.yeh@intel.com>; Zhang, Qingwu <qingwu.zhang@intel.com>; bingbu.cao@linux.intel.com
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@intel.com>
> 
> 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@intel.com>
> ---
>  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