| Message ID | 20190510100930.14641-4-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Support running driver's probe for a device powered off | expand |
Hi Sakari, On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote: > Tell ACPI device PM code that the driver supports the device being powered > off when the driver's probe function is entered. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/i2c/ov5670.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > index 041fcbb4eebdf..57e8b92f90e09 100644 > --- a/drivers/media/i2c/ov5670.c > +++ b/drivers/media/i2c/ov5670.c > @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client) > struct ov5670 *ov5670; > const char *err_msg; > u32 input_clk = 0; > + bool powered_off; > int ret; > > device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > @@ -2460,11 +2461,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; > + powered_off = acpi_dev_powered_off_for_probe(&client->dev); > + if (!powered_off) { > + /* Check module identity */ > + ret = ov5670_identify_module(ov5670); > + if (ret) { > + err_msg = "ov5670_identify_module() error"; > + goto error_print; > + } > } I don't like the fact that we can't detect any hardware connection issue here anymore and we would actually get some obscure failure when we actually start streaming. Wouldn't it be possible to still keep this behavior of not powering on the device at boot-up if no driver is bound and then have this driver built as a module and loaded later when the camera is to be used for the first time after the system boots? Best regards, Tomasz
Hi Tomasz, On Wed, Jun 05, 2019 at 04:07:52PM +0900, Tomasz Figa wrote: > Hi Sakari, > > On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote: > > Tell ACPI device PM code that the driver supports the device being powered > > off when the driver's probe function is entered. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/i2c/ov5670.c | 25 ++++++++++++++----------- > > 1 file changed, 14 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > > index 041fcbb4eebdf..57e8b92f90e09 100644 > > --- a/drivers/media/i2c/ov5670.c > > +++ b/drivers/media/i2c/ov5670.c > > @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client) > > struct ov5670 *ov5670; > > const char *err_msg; > > u32 input_clk = 0; > > + bool powered_off; > > int ret; > > > > device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > > @@ -2460,11 +2461,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; > > + powered_off = acpi_dev_powered_off_for_probe(&client->dev); > > + if (!powered_off) { > > + /* Check module identity */ > > + ret = ov5670_identify_module(ov5670); > > + if (ret) { > > + err_msg = "ov5670_identify_module() error"; > > + goto error_print; > > + } > > } > > I don't like the fact that we can't detect any hardware connection issue > here anymore and we would actually get some obscure failure when we > actually start streaming. > > Wouldn't it be possible to still keep this behavior of not powering on > the device at boot-up if no driver is bound and then have this driver > built as a module and loaded later when the camera is to be used for the > first time after the system boots? That'd be a way to work around this, but the downside would be that the user space would need to know not only which drivers to load, but also which drivers _not_ to load. The user space could obtain the former from the kernel but not the latter, it'd be system specific configuration. Moving the responsibility of loading the driver to user space would also not address figuring out whether the sensor is accessible through its control bus: you have to power it on to do that. In fact, if you want to be sure that the hardware is all right, you have to start streaming on the device first and that is not a part of a typical driver initialisation sequence. Just checking the sensor is accessible over I²C is not enough. The proposed solution addresses the problem without user space changes.
On Wed, Jun 5, 2019 at 7:15 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Tomasz, > > On Wed, Jun 05, 2019 at 04:07:52PM +0900, Tomasz Figa wrote: > > Hi Sakari, > > > > On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote: > > > Tell ACPI device PM code that the driver supports the device being powered > > > off when the driver's probe function is entered. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/media/i2c/ov5670.c | 25 ++++++++++++++----------- > > > 1 file changed, 14 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > > > index 041fcbb4eebdf..57e8b92f90e09 100644 > > > --- a/drivers/media/i2c/ov5670.c > > > +++ b/drivers/media/i2c/ov5670.c > > > @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client) > > > struct ov5670 *ov5670; > > > const char *err_msg; > > > u32 input_clk = 0; > > > + bool powered_off; > > > int ret; > > > > > > device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > > > @@ -2460,11 +2461,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; > > > + powered_off = acpi_dev_powered_off_for_probe(&client->dev); > > > + if (!powered_off) { > > > + /* Check module identity */ > > > + ret = ov5670_identify_module(ov5670); > > > + if (ret) { > > > + err_msg = "ov5670_identify_module() error"; > > > + goto error_print; > > > + } > > > } > > > > I don't like the fact that we can't detect any hardware connection issue > > here anymore and we would actually get some obscure failure when we > > actually start streaming. > > > > Wouldn't it be possible to still keep this behavior of not powering on > > the device at boot-up if no driver is bound and then have this driver > > built as a module and loaded later when the camera is to be used for the > > first time after the system boots? > > That'd be a way to work around this, but the downside would be that the > user space would need to know not only which drivers to load, but also > which drivers _not_ to load. The user space could obtain the former from > the kernel but not the latter, it'd be system specific configuration. > > Moving the responsibility of loading the driver to user space would also > not address figuring out whether the sensor is accessible through its > control bus: you have to power it on to do that. In fact, if you want to be > sure that the hardware is all right, you have to start streaming on the > device first and that is not a part of a typical driver initialisation > sequence. Just checking the sensor is accessible over I涎 is not enough. > > The proposed solution addresses the problem without user space changes. I guess that makes sense indeed. If going this way, why not just move all the hardware access from probe to streamon and avoid any conditional checks at all? Best regards, Tomasz
Hi Tomasz, On Fri, Jun 07, 2019 at 04:54:06PM +0900, Tomasz Figa wrote: > On Wed, Jun 5, 2019 at 7:15 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Tomasz, > > > > On Wed, Jun 05, 2019 at 04:07:52PM +0900, Tomasz Figa wrote: > > > Hi Sakari, > > > > > > On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote: > > > > Tell ACPI device PM code that the driver supports the device being powered > > > > off when the driver's probe function is entered. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > drivers/media/i2c/ov5670.c | 25 ++++++++++++++----------- > > > > 1 file changed, 14 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > > > > index 041fcbb4eebdf..57e8b92f90e09 100644 > > > > --- a/drivers/media/i2c/ov5670.c > > > > +++ b/drivers/media/i2c/ov5670.c > > > > @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client) > > > > struct ov5670 *ov5670; > > > > const char *err_msg; > > > > u32 input_clk = 0; > > > > + bool powered_off; > > > > int ret; > > > > > > > > device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > > > > @@ -2460,11 +2461,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; > > > > + powered_off = acpi_dev_powered_off_for_probe(&client->dev); > > > > + if (!powered_off) { > > > > + /* Check module identity */ > > > > + ret = ov5670_identify_module(ov5670); > > > > + if (ret) { > > > > + err_msg = "ov5670_identify_module() error"; > > > > + goto error_print; > > > > + } > > > > } > > > > > > I don't like the fact that we can't detect any hardware connection issue > > > here anymore and we would actually get some obscure failure when we > > > actually start streaming. > > > > > > Wouldn't it be possible to still keep this behavior of not powering on > > > the device at boot-up if no driver is bound and then have this driver > > > built as a module and loaded later when the camera is to be used for the > > > first time after the system boots? > > > > That'd be a way to work around this, but the downside would be that the > > user space would need to know not only which drivers to load, but also > > which drivers _not_ to load. The user space could obtain the former from > > the kernel but not the latter, it'd be system specific configuration. > > > > Moving the responsibility of loading the driver to user space would also > > not address figuring out whether the sensor is accessible through its > > control bus: you have to power it on to do that. In fact, if you want to be > > sure that the hardware is all right, you have to start streaming on the > > device first and that is not a part of a typical driver initialisation > > sequence. Just checking the sensor is accessible over I涎 is not enough. > > > > The proposed solution addresses the problem without user space changes. > > I guess that makes sense indeed. If going this way, why not just move > all the hardware access from probe to streamon and avoid any > conditional checks at all? My apologies for the late answer. In that case there would be no way to verify the hardware actually is there, even on systems where there is no adverse effect from doing that. For a sensor driver this could be just fine, but I have doubts it'd be appropriate for e.g. the at24 driver.
On Mon, Aug 26, 2019 at 5:38 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Tomasz, > > On Fri, Jun 07, 2019 at 04:54:06PM +0900, Tomasz Figa wrote: > > On Wed, Jun 5, 2019 at 7:15 PM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Tomasz, > > > > > > On Wed, Jun 05, 2019 at 04:07:52PM +0900, Tomasz Figa wrote: > > > > Hi Sakari, > > > > > > > > On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote: > > > > > Tell ACPI device PM code that the driver supports the device being powered > > > > > off when the driver's probe function is entered. > > > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > > --- > > > > > drivers/media/i2c/ov5670.c | 25 ++++++++++++++----------- > > > > > 1 file changed, 14 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > > > > > index 041fcbb4eebdf..57e8b92f90e09 100644 > > > > > --- a/drivers/media/i2c/ov5670.c > > > > > +++ b/drivers/media/i2c/ov5670.c > > > > > @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client) > > > > > struct ov5670 *ov5670; > > > > > const char *err_msg; > > > > > u32 input_clk = 0; > > > > > + bool powered_off; > > > > > int ret; > > > > > > > > > > device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > > > > > @@ -2460,11 +2461,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; > > > > > + powered_off = acpi_dev_powered_off_for_probe(&client->dev); > > > > > + if (!powered_off) { > > > > > + /* Check module identity */ > > > > > + ret = ov5670_identify_module(ov5670); > > > > > + if (ret) { > > > > > + err_msg = "ov5670_identify_module() error"; > > > > > + goto error_print; > > > > > + } > > > > > } > > > > > > > > I don't like the fact that we can't detect any hardware connection issue > > > > here anymore and we would actually get some obscure failure when we > > > > actually start streaming. > > > > > > > > Wouldn't it be possible to still keep this behavior of not powering on > > > > the device at boot-up if no driver is bound and then have this driver > > > > built as a module and loaded later when the camera is to be used for the > > > > first time after the system boots? > > > > > > That'd be a way to work around this, but the downside would be that the > > > user space would need to know not only which drivers to load, but also > > > which drivers _not_ to load. The user space could obtain the former from > > > the kernel but not the latter, it'd be system specific configuration. > > > > > > Moving the responsibility of loading the driver to user space would also > > > not address figuring out whether the sensor is accessible through its > > > control bus: you have to power it on to do that. In fact, if you want to be > > > sure that the hardware is all right, you have to start streaming on the > > > device first and that is not a part of a typical driver initialisation > > > sequence. Just checking the sensor is accessible over I涎 is not enough. > > > > > > The proposed solution addresses the problem without user space changes. > > > > I guess that makes sense indeed. If going this way, why not just move > > all the hardware access from probe to streamon and avoid any > > conditional checks at all? > > My apologies for the late answer. > > In that case there would be no way to verify the hardware actually is > there, even on systems where there is no adverse effect from doing that. > For a sensor driver this could be just fine, but I have doubts it'd be > appropriate for e.g. the at24 driver. For an eeprom, the first read could fail. That said, I agree that for systems on which there is no such concern it would still be desirable to check if the device is accessible in probe. Anyway, I think you convinced me. :) Reviewed-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz
diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c index 041fcbb4eebdf..57e8b92f90e09 100644 --- a/drivers/media/i2c/ov5670.c +++ b/drivers/media/i2c/ov5670.c @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client) struct ov5670 *ov5670; const char *err_msg; u32 input_clk = 0; + bool powered_off; int ret; device_property_read_u32(&client->dev, "clock-frequency", &input_clk); @@ -2460,11 +2461,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; + powered_off = acpi_dev_powered_off_for_probe(&client->dev); + if (!powered_off) { + /* Check module identity */ + ret = ov5670_identify_module(ov5670); + if (ret) { + err_msg = "ov5670_identify_module() error"; + goto error_print; + } } mutex_init(&ov5670->mutex); @@ -2500,11 +2504,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); + /* Don't set the device's state to active if it's powered off. */ + if (!powered_off) + pm_runtime_set_active(&client->dev); pm_runtime_enable(&client->dev); pm_runtime_idle(&client->dev); @@ -2546,7 +2548,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 */ } }; @@ -2556,6 +2558,7 @@ MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids); static struct i2c_driver ov5670_i2c_driver = { .driver = { .name = "ov5670", + .probe_powered_off = true, .pm = &ov5670_pm_ops, .acpi_match_table = ACPI_PTR(ov5670_acpi_ids), },
Tell ACPI device PM code that the driver supports the device being powered off when the driver's probe function is entered. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/i2c/ov5670.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)