| Message ID | 20240105102008.2879073-1-jason.z.chen@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] media: ov08x40: Avoid sensor probing in D0 state | expand |
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
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); + 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. */ - 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);