Re: [PATCH v5 3/6] ov5670: Support probe whilst the device is in a low power state
On 8/12/20 5:12 PM, Bingbu Cao wrote:
>
>
> On 8/10/20 10:27 PM, Sakari Ailus wrote:
>> Tell ACPI device PM code that the driver supports the device being in a
>> low power state when the driver's probe function is entered.
>>
>> Signed-off-by: Sakari Ailus
>> ---
>> drivers/media/i2c/ov5670.c | 23 ++-
>> 1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
>> index f26252e35e08d..1f75b888d2a18 100644
>> --- a/drivers/media/i2c/ov5670.c
>> +++ b/drivers/media/i2c/ov5670.c
>> @@ -2456,6 +2456,7 @@ static int ov5670_probe(struct i2c_client *client)
>> struct ov5670 *ov5670;
>> const char *err_msg;
>> u32 input_clk = 0;
>> +bool low_power;
>> int ret;
>>
>> device_property_read_u32(>dev, "clock-frequency", _clk);
>> @@ -2472,11 +2473,14 @@ static int ov5670_probe(struct i2c_client *client)
>> /* Initialize subdev */
>> v4l2_i2c_subdev_init(>sd, client, _subdev_ops);
>>
>> -/* Check module identity */
>> -ret = ov5670_identify_module(ov5670);
>> -if (ret) {
>> -err_msg = "ov5670_identify_module() error";
>> -goto error_print;
>> +low_power = acpi_dev_state_low_power(>dev);
>> +if (!low_power) {
>> +/* Check module identity */
>> +ret = ov5670_identify_module(ov5670);
>> +if (ret) {
>> +err_msg = "ov5670_identify_module() error";
>> +goto error_print;
>> +
>
> Sakari, thanks for your patch.
> one question - With this change, there will be no chance for driver to
> guarantee
> that the camera sensor plugged in is the camera that the matched driver
> actually
> can drive until try to streaming the camera, so is it necessary to return
> appropriate error in .s_stream ops to notify user it is not the hardware that
> current driver can drive? if no other better way.
Sakari, please ignore my previous comment, it is not related to this change. I
see the sub device open is caused by v4l_id program from udev.
>
--
Best regards,
Bingbu Cao
Re: [PATCH v5 3/6] ov5670: Support probe whilst the device is in a low power state
Hi Bingbu,
Thanks for the review.
On Wed, Aug 12, 2020 at 05:12:28PM +0800, Bingbu Cao wrote:
>
>
> On 8/10/20 10:27 PM, Sakari Ailus wrote:
> > Tell ACPI device PM code that the driver supports the device being in a
> > low power state when the driver's probe function is entered.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > drivers/media/i2c/ov5670.c | 23 ++-
> > 1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index f26252e35e08d..1f75b888d2a18 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -2456,6 +2456,7 @@ static int ov5670_probe(struct i2c_client *client)
> > struct ov5670 *ov5670;
> > const char *err_msg;
> > u32 input_clk = 0;
> > + bool low_power;
> > int ret;
> >
> > device_property_read_u32(>dev, "clock-frequency", _clk);
> > @@ -2472,11 +2473,14 @@ static int ov5670_probe(struct i2c_client *client)
> > /* Initialize subdev */
> > v4l2_i2c_subdev_init(>sd, client, _subdev_ops);
> >
> > - /* Check module identity */
> > - ret = ov5670_identify_module(ov5670);
> > - if (ret) {
> > - err_msg = "ov5670_identify_module() error";
> > - goto error_print;
> > + low_power = acpi_dev_state_low_power(>dev);
> > + if (!low_power) {
> > + /* Check module identity */
> > + ret = ov5670_identify_module(ov5670);
> > + if (ret) {
> > + err_msg = "ov5670_identify_module() error";
> > + goto error_print;
> > +
>
> Sakari, thanks for your patch.
> one question - With this change, there will be no chance for driver to
> guarantee
> that the camera sensor plugged in is the camera that the matched driver
> actually
> can drive until try to streaming the camera, so is it necessary to return
> appropriate error in .s_stream ops to notify user it is not the hardware that
> current driver can drive? if no other better way.
Indeed sensor identification is now skipped in probe. I'll add that for v6
--- and check other drivers, too.
--
Kind regards,
Sakari Ailus
Re: [PATCH v5 3/6] ov5670: Support probe whilst the device is in a low power state
On 8/10/20 10:27 PM, Sakari Ailus wrote:
> Tell ACPI device PM code that the driver supports the device being in a
> low power state when the driver's probe function is entered.
>
> Signed-off-by: Sakari Ailus
> ---
> drivers/media/i2c/ov5670.c | 23 ++-
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index f26252e35e08d..1f75b888d2a18 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2456,6 +2456,7 @@ static int ov5670_probe(struct i2c_client *client)
> struct ov5670 *ov5670;
> const char *err_msg;
> u32 input_clk = 0;
> + bool low_power;
> int ret;
>
> device_property_read_u32(>dev, "clock-frequency", _clk);
> @@ -2472,11 +2473,14 @@ static int ov5670_probe(struct i2c_client *client)
> /* Initialize subdev */
> v4l2_i2c_subdev_init(>sd, client, _subdev_ops);
>
> - /* Check module identity */
> - ret = ov5670_identify_module(ov5670);
> - if (ret) {
> - err_msg = "ov5670_identify_module() error";
> - goto error_print;
> + low_power = acpi_dev_state_low_power(>dev);
> + if (!low_power) {
> + /* Check module identity */
> + ret = ov5670_identify_module(ov5670);
> + if (ret) {
> + err_msg = "ov5670_identify_module() error";
> + goto error_print;
> +
Sakari, thanks for your patch.
one question - With this change, there will be no chance for driver to guarantee
that the camera sensor plugged in is the camera that the matched driver actually
can drive until try to streaming the camera, so is it necessary to return
appropriate error in .s_stream ops to notify user it is not the hardware that
current driver can drive? if no other better way.
--
Best regards,
Bingbu Cao
[PATCH v5 3/6] ov5670: Support probe whilst the device is in a low power state
Tell ACPI device PM code that the driver supports the device being in a
low power state when the driver's probe function is entered.
Signed-off-by: Sakari Ailus
---
drivers/media/i2c/ov5670.c | 23 ++-
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index f26252e35e08d..1f75b888d2a18 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2456,6 +2456,7 @@ static int ov5670_probe(struct i2c_client *client)
struct ov5670 *ov5670;
const char *err_msg;
u32 input_clk = 0;
+ bool low_power;
int ret;
device_property_read_u32(>dev, "clock-frequency", _clk);
@@ -2472,11 +2473,14 @@ static int ov5670_probe(struct i2c_client *client)
/* Initialize subdev */
v4l2_i2c_subdev_init(>sd, client, _subdev_ops);
- /* Check module identity */
- ret = ov5670_identify_module(ov5670);
- if (ret) {
- err_msg = "ov5670_identify_module() error";
- goto error_print;
+ low_power = acpi_dev_state_low_power(>dev);
+ if (!low_power) {
+ /* Check module identity */
+ ret = ov5670_identify_module(ov5670);
+ if (ret) {
+ err_msg = "ov5670_identify_module() error";
+ goto error_print;
+ }
}
mutex_init(>mutex);
@@ -2513,10 +2517,10 @@ 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.
+* Don't set the device's state to active if it's in a low power state.
*/
- pm_runtime_set_active(>dev);
+ if (!low_power)
+ pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
pm_runtime_idle(>dev);
@@ -2558,7 +2562,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 */ }
};
@@ -2573,6 +2577,7 @@ static struct i2c_driver ov5670_i2c_driver = {
},
.probe_new = ov5670_probe,
.remove = ov5670_remove,
+ .flags = I2C_DRV_FL_ALLOW_LOW_POWER_PROBE,
};
module_i2c_driver(ov5670_i2c_driver);
--
2.20.1
