* [PATCH 1/5] ACPI: Enable driver and firmware hints to control power at probe time
2019-05-10 10:09 [PATCH 0/5] Support running driver's probe for a device powered off Sakari Ailus
@ 2019-05-10 10:09 ` Sakari Ailus
2019-05-31 9:17 ` Rafael J. Wysocki
2019-05-10 10:09 ` [PATCH 2/5] ACPI: Add a convenience function to tell a device is suspended in probe Sakari Ailus
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2019-05-10 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: rajmohan.mani, linux-media
Allow drivers and firmware tell ACPI that there's no need to power on a
device for probe. This requires both a hint from the firmware as well as
an indication from a driver to leave the device off.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/device_pm.c | 9 +++++++--
include/linux/device.h | 6 ++++++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index b859d75eaf9f6..ca2409a30d26d 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1225,7 +1225,9 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
if (adev && dev->pm_domain == &acpi_general_pm_domain) {
dev_pm_domain_set(dev, NULL);
acpi_remove_pm_notifier(adev);
- if (power_off) {
+ if (power_off &&
+ !(dev->driver->probe_powered_off &&
+ device_property_present(dev, "avoid-power-probe"))) {
/*
* If the device's PM QoS resume latency limit or flags
* have been exposed to user space, they have to be
@@ -1273,7 +1275,10 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
acpi_add_pm_notifier(adev, dev, acpi_pm_notify_work_func);
dev_pm_domain_set(dev, &acpi_general_pm_domain);
- if (power_on) {
+
+ if (power_on &&
+ !(dev->driver->probe_powered_off &&
+ device_property_present(dev, "avoid-power-probe"))) {
acpi_dev_pm_full_power(adev);
acpi_device_wakeup_disable(adev);
}
diff --git a/include/linux/device.h b/include/linux/device.h
index e85264fb66161..2a459fd5b954a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -245,6 +245,11 @@ enum probe_type {
* @owner: The module owner.
* @mod_name: Used for built-in modules.
* @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @probe_powered_off: The driver supports its probe function being called while
+ * the device is powered off, independently of the expected
+ * behaviour on combination of a given bus and firmware
+ * interface etc. The driver is responsible for powering the
+ * device on using runtime PM in such case.
* @probe_type: Type of the probe (synchronous or asynchronous) to use.
* @of_match_table: The open firmware table.
* @acpi_match_table: The ACPI match table.
@@ -282,6 +287,7 @@ struct device_driver {
const char *mod_name; /* used for built-in modules */
bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
+ bool probe_powered_off;
enum probe_type probe_type;
const struct of_device_id *of_match_table;
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] ACPI: Enable driver and firmware hints to control power at probe time
2019-05-10 10:09 ` [PATCH 1/5] ACPI: Enable driver and firmware hints to control power at probe time Sakari Ailus
@ 2019-05-31 9:17 ` Rafael J. Wysocki
2019-06-05 12:07 ` Sakari Ailus
0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2019-05-31 9:17 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-acpi, rajmohan.mani, linux-media, Mika Westerberg
On Friday, May 10, 2019 12:09:26 PM CEST Sakari Ailus wrote:
> Allow drivers and firmware tell ACPI that there's no need to power on a
> device for probe. This requires both a hint from the firmware as well as
> an indication from a driver to leave the device off.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/acpi/device_pm.c | 9 +++++++--
> include/linux/device.h | 6 ++++++
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index b859d75eaf9f6..ca2409a30d26d 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1225,7 +1225,9 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
> if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> dev_pm_domain_set(dev, NULL);
> acpi_remove_pm_notifier(adev);
> - if (power_off) {
> + if (power_off &&
> + !(dev->driver->probe_powered_off &&
> + device_property_present(dev, "avoid-power-probe"))) {
> /*
> * If the device's PM QoS resume latency limit or flags
> * have been exposed to user space, they have to be
> @@ -1273,7 +1275,10 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
>
> acpi_add_pm_notifier(adev, dev, acpi_pm_notify_work_func);
> dev_pm_domain_set(dev, &acpi_general_pm_domain);
> - if (power_on) {
> +
> + if (power_on &&
> + !(dev->driver->probe_powered_off &&
> + device_property_present(dev, "avoid-power-probe"))) {
> acpi_dev_pm_full_power(adev);
> acpi_device_wakeup_disable(adev);
> }
> diff --git a/include/linux/device.h b/include/linux/device.h
> index e85264fb66161..2a459fd5b954a 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -245,6 +245,11 @@ enum probe_type {
> * @owner: The module owner.
> * @mod_name: Used for built-in modules.
> * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> + * @probe_powered_off: The driver supports its probe function being called while
> + * the device is powered off, independently of the expected
> + * behaviour on combination of a given bus and firmware
> + * interface etc. The driver is responsible for powering the
> + * device on using runtime PM in such case.
> * @probe_type: Type of the probe (synchronous or asynchronous) to use.
> * @of_match_table: The open firmware table.
> * @acpi_match_table: The ACPI match table.
> @@ -282,6 +287,7 @@ struct device_driver {
> const char *mod_name; /* used for built-in modules */
>
> bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
> + bool probe_powered_off;
This is a bit of a misnomer IMO, because it is not just about devices that are completely off.
From the ACPI perspective that is about all devices not in D0, which may mean gated clocks
etc.
I would call it probe_low_power or similar and analogously in patch [2/5], and apart from this
I have no objections against this series, but I would suggest to CC the next iteration of it
to Greg K-H and the LKML as it touches the driver core.
> enum probe_type probe_type;
>
> const struct of_device_id *of_match_table;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] ACPI: Enable driver and firmware hints to control power at probe time
2019-05-31 9:17 ` Rafael J. Wysocki
@ 2019-06-05 12:07 ` Sakari Ailus
0 siblings, 0 replies; 13+ messages in thread
From: Sakari Ailus @ 2019-06-05 12:07 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, rajmohan.mani, linux-media, Mika Westerberg
Hi Rafael,
On Fri, May 31, 2019 at 11:17:10AM +0200, Rafael J. Wysocki wrote:
> On Friday, May 10, 2019 12:09:26 PM CEST Sakari Ailus wrote:
...
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index e85264fb66161..2a459fd5b954a 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -245,6 +245,11 @@ enum probe_type {
> > * @owner: The module owner.
> > * @mod_name: Used for built-in modules.
> > * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> > + * @probe_powered_off: The driver supports its probe function being called while
> > + * the device is powered off, independently of the expected
> > + * behaviour on combination of a given bus and firmware
> > + * interface etc. The driver is responsible for powering the
> > + * device on using runtime PM in such case.
> > * @probe_type: Type of the probe (synchronous or asynchronous) to use.
> > * @of_match_table: The open firmware table.
> > * @acpi_match_table: The ACPI match table.
> > @@ -282,6 +287,7 @@ struct device_driver {
> > const char *mod_name; /* used for built-in modules */
> >
> > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
> > + bool probe_powered_off;
>
> This is a bit of a misnomer IMO, because it is not just about devices that are completely off.
> From the ACPI perspective that is about all devices not in D0, which may mean gated clocks
> etc.
>
> I would call it probe_low_power or similar and analogously in patch [2/5], and apart from this
> I have no objections against this series, but I would suggest to CC the next iteration of it
> to Greg K-H and the LKML as it touches the driver core.
Ack. I'll do that for v2.
Thanks for the review!
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/5] ACPI: Add a convenience function to tell a device is suspended in probe
2019-05-10 10:09 [PATCH 0/5] Support running driver's probe for a device powered off Sakari Ailus
2019-05-10 10:09 ` [PATCH 1/5] ACPI: Enable driver and firmware hints to control power at probe time Sakari Ailus
@ 2019-05-10 10:09 ` Sakari Ailus
2019-05-10 10:09 ` [PATCH 3/5] ov5670: Support probe whilst the device is off Sakari Ailus
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Sakari Ailus @ 2019-05-10 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: rajmohan.mani, linux-media
Add a convenience function to tell whether a device is suspended for probe
or remove, for busses where the custom is that drivers don't need to
resume devices in probe, or suspend them in their remove handlers.
Returns false on non-ACPI systems.
Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/device_pm.c | 33 +++++++++++++++++++++++++++++++++
include/linux/acpi.h | 5 +++++
2 files changed, 38 insertions(+)
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index ca2409a30d26d..774bf4380afd3 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1287,4 +1287,37 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
return 1;
}
EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
+
+/**
+ * acpi_dev_powered_off_for_probe - Tell if a device is powered off for probe
+ * @dev: The device
+ *
+ * Tell whether a given device has not been powered on for probe or remove.
+ *
+ * Drivers of devices on certain busses such as I²C can generally assume (on
+ * ACPI based systems) that the devices they control are powered on without
+ * driver having to do anything about it. Using struct device_driver.probe_off
+ * and "avoid-power-probe" property, this can be negated and the driver has full
+ * control of the device power management. Always returns false on non-ACPI
+ * based systems. True is returned on ACPI based systems iff the device is
+ * powered off.
+ */
+bool acpi_dev_powered_off_for_probe(struct device *dev)
+{
+ int power_state;
+ int ret;
+
+ if (!is_acpi_device_node(dev_fwnode(dev)))
+ return false;
+
+ ret = acpi_device_get_power(ACPI_COMPANION(dev), &power_state);
+ if (ret) {
+ dev_warn(dev, "Cannot obtain power state (%d)\n", ret);
+ return false;
+ }
+
+ return power_state != ACPI_STATE_D0;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_powered_off_for_probe);
+
#endif /* CONFIG_PM */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index e22c237be46ae..fc6b97aadcf17 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -913,6 +913,7 @@ int acpi_dev_resume(struct device *dev);
int acpi_subsys_runtime_suspend(struct device *dev);
int acpi_subsys_runtime_resume(struct device *dev);
int acpi_dev_pm_attach(struct device *dev, bool power_on);
+bool acpi_dev_powered_off_for_probe(struct device *dev);
#else
static inline int acpi_dev_runtime_suspend(struct device *dev) { return 0; }
static inline int acpi_dev_runtime_resume(struct device *dev) { return 0; }
@@ -922,6 +923,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
{
return 0;
}
+static inline bool acpi_dev_powered_off_for_probe(struct device *dev)
+{
+ return false;
+}
#endif
#if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/5] ov5670: Support probe whilst the device is off
2019-05-10 10:09 [PATCH 0/5] Support running driver's probe for a device powered off Sakari Ailus
2019-05-10 10:09 ` [PATCH 1/5] ACPI: Enable driver and firmware hints to control power at probe time Sakari Ailus
2019-05-10 10:09 ` [PATCH 2/5] ACPI: Add a convenience function to tell a device is suspended in probe Sakari Ailus
@ 2019-05-10 10:09 ` Sakari Ailus
2019-06-05 7:07 ` Tomasz Figa
2019-05-10 10:09 ` [PATCH 4/5] media: i2c: imx319: Support probe while " Sakari Ailus
2019-05-10 10:09 ` [PATCH 5/5] at24: Support probing while off Sakari Ailus
4 siblings, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2019-05-10 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: rajmohan.mani, linux-media
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;
+ }
}
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),
},
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] ov5670: Support probe whilst the device is off
2019-05-10 10:09 ` [PATCH 3/5] ov5670: Support probe whilst the device is off Sakari Ailus
@ 2019-06-05 7:07 ` Tomasz Figa
2019-06-05 10:15 ` Sakari Ailus
0 siblings, 1 reply; 13+ messages in thread
From: Tomasz Figa @ 2019-06-05 7:07 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-acpi, rajmohan.mani, linux-media
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] ov5670: Support probe whilst the device is off
2019-06-05 7:07 ` Tomasz Figa
@ 2019-06-05 10:15 ` Sakari Ailus
2019-06-07 7:54 ` Tomasz Figa
0 siblings, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2019-06-05 10:15 UTC (permalink / raw)
To: Tomasz Figa; +Cc: linux-acpi, rajmohan.mani, linux-media
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.
--
Kind regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] ov5670: Support probe whilst the device is off
2019-06-05 10:15 ` Sakari Ailus
@ 2019-06-07 7:54 ` Tomasz Figa
2019-08-26 8:38 ` Sakari Ailus
0 siblings, 1 reply; 13+ messages in thread
From: Tomasz Figa @ 2019-06-07 7:54 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-acpi, Mani, Rajmohan, Linux Media Mailing List
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] ov5670: Support probe whilst the device is off
2019-06-07 7:54 ` Tomasz Figa
@ 2019-08-26 8:38 ` Sakari Ailus
2019-08-26 9:21 ` Tomasz Figa
0 siblings, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2019-08-26 8:38 UTC (permalink / raw)
To: Tomasz Figa; +Cc: linux-acpi, Mani, Rajmohan, Linux Media Mailing List
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.
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] ov5670: Support probe whilst the device is off
2019-08-26 8:38 ` Sakari Ailus
@ 2019-08-26 9:21 ` Tomasz Figa
0 siblings, 0 replies; 13+ messages in thread
From: Tomasz Figa @ 2019-08-26 9:21 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-acpi, Mani, Rajmohan, Linux Media Mailing List
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/5] media: i2c: imx319: Support probe while the device is off
2019-05-10 10:09 [PATCH 0/5] Support running driver's probe for a device powered off Sakari Ailus
` (2 preceding siblings ...)
2019-05-10 10:09 ` [PATCH 3/5] ov5670: Support probe whilst the device is off Sakari Ailus
@ 2019-05-10 10:09 ` Sakari Ailus
2019-05-10 10:09 ` [PATCH 5/5] at24: Support probing while off Sakari Ailus
4 siblings, 0 replies; 13+ messages in thread
From: Sakari Ailus @ 2019-05-10 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: rajmohan.mani, linux-media
From: Rajmohan Mani <rajmohan.mani@intel.com>
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: Rajmohan Mani <rajmohan.mani@intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/i2c/imx319.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221e..e929de86a8e22 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -2424,6 +2424,7 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
static int imx319_probe(struct i2c_client *client)
{
struct imx319 *imx319;
+ bool powered_off;
int ret;
u32 i;
@@ -2436,11 +2437,14 @@ static int imx319_probe(struct i2c_client *client)
/* Initialize subdev */
v4l2_i2c_subdev_init(&imx319->sd, client, &imx319_subdev_ops);
- /* Check module identity */
- ret = imx319_identify_module(imx319);
- if (ret) {
- dev_err(&client->dev, "failed to find sensor: %d", ret);
- goto error_probe;
+ powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+ if (!powered_off) {
+ /* Check module identity */
+ ret = imx319_identify_module(imx319);
+ if (ret) {
+ dev_err(&client->dev, "failed to find sensor: %d", ret);
+ goto error_probe;
+ }
}
imx319->hwcfg = imx319_get_hwcfg(&client->dev);
@@ -2492,11 +2496,9 @@ static int imx319_probe(struct i2c_client *client)
if (ret < 0)
goto error_media_entity;
- /*
- * 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);
@@ -2536,7 +2538,7 @@ static const struct dev_pm_ops imx319_pm_ops = {
};
static const struct acpi_device_id imx319_acpi_ids[] = {
- { "SONY319A" },
+ { "SONY319A", },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(acpi, imx319_acpi_ids);
@@ -2544,6 +2546,7 @@ MODULE_DEVICE_TABLE(acpi, imx319_acpi_ids);
static struct i2c_driver imx319_i2c_driver = {
.driver = {
.name = "imx319",
+ .probe_powered_off = true,
.pm = &imx319_pm_ops,
.acpi_match_table = ACPI_PTR(imx319_acpi_ids),
},
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/5] at24: Support probing while off
2019-05-10 10:09 [PATCH 0/5] Support running driver's probe for a device powered off Sakari Ailus
` (3 preceding siblings ...)
2019-05-10 10:09 ` [PATCH 4/5] media: i2c: imx319: Support probe while " Sakari Ailus
@ 2019-05-10 10:09 ` Sakari Ailus
4 siblings, 0 replies; 13+ messages in thread
From: Sakari Ailus @ 2019-05-10 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: rajmohan.mani, linux-media
In certain use cases (where the chip is part of a camera module, and the
camera module is wired together with a camera privacy LED), powering on
the device during probe is undesirable. Add support for the at24 to
execute probe while being powered off. For this to happen, a hint in form
of a device property is required from the firmware.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/misc/eeprom/at24.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 63aa541c96088..b9dbe5b6a97be 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -579,6 +579,7 @@ static int at24_probe(struct i2c_client *client)
bool i2c_fn_i2c, i2c_fn_block;
unsigned int i, num_addresses;
struct at24_data *at24;
+ bool powered_off;
struct regmap *regmap;
size_t at24_size;
bool writable;
@@ -701,19 +702,24 @@ static int at24_probe(struct i2c_client *client)
i2c_set_clientdata(client, at24);
- /* enable runtime pm */
- pm_runtime_set_active(dev);
+ powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+ if (!powered_off)
+ pm_runtime_set_active(dev);
+
pm_runtime_enable(dev);
/*
- * Perform a one-byte test read to verify that the
- * chip is functional.
+ * Perform a one-byte test read to verify that the chip is functional,
+ * unless powering on the device is to be avoided during probe (i.e.
+ * it's powered off right now).
*/
- err = at24_read(at24, 0, &test_byte, 1);
- pm_runtime_idle(dev);
- if (err) {
- err = -ENODEV;
- goto err_clients;
+ if (!powered_off) {
+ err = at24_read(at24, 0, &test_byte, 1);
+ pm_runtime_idle(dev);
+ if (err) {
+ err = -ENODEV;
+ goto err_clients;
+ }
}
nvmem_config.name = dev_name(dev);
@@ -752,12 +758,15 @@ static int at24_probe(struct i2c_client *client)
static int at24_remove(struct i2c_client *client)
{
struct at24_data *at24;
+ bool powered_off;
at24 = i2c_get_clientdata(client);
at24_remove_dummy_clients(at24);
pm_runtime_disable(&client->dev);
- pm_runtime_set_suspended(&client->dev);
+ powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+ if (!powered_off)
+ pm_runtime_set_suspended(&client->dev);
return 0;
}
@@ -765,6 +774,7 @@ static int at24_remove(struct i2c_client *client)
static struct i2c_driver at24_driver = {
.driver = {
.name = "at24",
+ .probe_powered_off = true,
.of_match_table = at24_of_match,
.acpi_match_table = ACPI_PTR(at24_acpi_ids),
},
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread