Hi all,
These patches enable calling (and finishing) a driver's probe function
without powering on the respective device on busses where the practice is
to power on the device for probe. While it generally is a driver's job to
check the that the device is there, there are cases where it might be
undesirable. (In this case it stems from a combination of hardware design
and user expectations; see below.) The downside with this change is that
if there is something wrong with the device, it will only be found at the
time the device is used. In this case (the camera sensors + EEPROM in a
sensor) I don't see any tangible harm from that though.
An indication both from the driver and the firmware is required to allow
the device's power state to remain off during probe (see the first patch).
The use case is such that there is a privacy LED next to an integrated
user-facing laptop camera, and this LED is there to signal the user that
the camera is recording a video or capturing images. That LED also happens
to be wired to one of the power supplies of the camera, so whenever you
power on the camera, the LED will be lit, whether images are captured from
the camera --- or not. There's no way to implement this differently
without additional software control (allowing of which is itself a
hardware design decision) on most CSI-2-connected camera sensors as they
simply have no pin to signal the camera streaming state.
This is also what happens during driver probe: the camera will be powered
on by the I²C subsystem calling dev_pm_domain_attach() and the device is
already powered on when the driver's own probe function is called. To the
user this visible during the boot process as a blink of the privacy LED,
suggesting that the camera is recording without the user having used an
application to do that. From the end user's point of view the behaviour is
not expected and for someone unfamiliar with internal workings of a
computer surely seems quite suspicious --- even if images are not being
actually captured.
I've tested these on linux-next master. They also apply to Wolfram's
i2c/for-next branch, there's a patch that affects the I²C core changes
here (see below). The patches apart from that apply to Bartosz's
at24/for-next as well as Mauro's linux-media master branch.
since v7 <URL:https://lore.kernel.org/linux-acpi/[email protected]/>:
- Reorder documentation patch right after the implemenation in the I²C
framework.
- Rename allow-low-power-probe property as i2c-allow-low-power-probe.
- Remove extra "property" from the description of the
i2c-allow-low-power-probe property and mention it's a device property.
- Add an example to the documentation and refer to the _DSD property spec.
since v6 <URL:https://lore.kernel.org/linux-acpi/[email protected]/>:
- Use u32 for the flags field in struct i2c_driver.
- Use acpi_dev_get_property to read the allow-low-power-probe property.
since v5 <URL:https://lore.kernel.org/linux-acpi/[email protected]/>:
- Identify sensors when they're first powered on. In previous versions, if
this wasn't in probe, it was not done at all.
- Return allow_low_power_probe() only for ACPI devices, i.e. OF systems
are not affected by these changes.
- Document that I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag only applies to ACPI
drivers.
- Fix extra regulator_disable in at24 driver's remove function when the
device was already in low power state.
since v4 <URL:https://lore.kernel.org/linux-acpi/[email protected]/>:
- Rename "probe-low-power" property as "allow-low-power-probe". This is
taken into account in function and file naming, too.
- Turn probe_low_power field in struct i2c_driver into flags field.
- Rebase on Wolfram's i2c/for-next branch that contains the removal of the
support for disabling I²C core IRQ mappings (commit
0c2a34937f7e4c4776bb261114c475392da2355c).
- Change wording for "allow-low-power-probe" property in ACPI
documentation.
since v3 <URL:https://lore.kernel.org/linux-acpi/[email protected]/T/#t>:
- Rework the 2nd patch based on Rafael's comments
- Rework description of the ACPI low power state helper function,
according to Rafael's text.
- Rename and rework the same function as
acpi_dev_state_low_power().
- Reflect the changes in commit message as well.
- Added a patch to document the probe-low-power _DSD property.
since v2 <URL:https://patchwork.kernel.org/cover/11114255/>:
- Remove extra CONFIG_PM ifdefs; these are not needed.
- Move the checks for power state hints from drivers/base/dd.c to
drivers/i2c/i2c-base-core.c; these are I²C devices anyway.
- Move the probe_low_power field from struct device_driver to struct
i2c_driver.
since v1:
- Rename probe_powered_off struct device field as probe_low_power and
reflect the similar naming to the patches overall.
- Work with CONFIG_PM disabled, too.
Rajmohan Mani (1):
media: i2c: imx319: Support probe while the device is off
Sakari Ailus (5):
i2c: Allow an ACPI driver to manage the device's power state during
probe
ACPI: Add a convenience function to tell a device is in low power
state
ov5670: Support probe whilst the device is in a low power state
at24: Support probing while off
Documentation: ACPI: Document allow-low-power-probe _DSD property
.../acpi/dsd/allow-low-power-probe.rst | 28 +++++++
Documentation/firmware-guide/acpi/index.rst | 1 +
drivers/acpi/device_pm.c | 31 ++++++++
drivers/i2c/i2c-core-base.c | 19 ++++-
drivers/media/i2c/imx319.c | 74 +++++++++++-------
drivers/media/i2c/ov5670.c | 76 +++++++++++--------
drivers/misc/eeprom/at24.c | 43 ++++++-----
include/linux/acpi.h | 5 ++
include/linux/i2c.h | 14 ++++
9 files changed, 212 insertions(+), 79 deletions(-)
create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst
--
2.20.1
Rajmohan Mani (1):
media: i2c: imx319: Support probe while the device is off
Sakari Ailus (5):
i2c: Allow an ACPI driver to manage the device's power state during
probe
Documentation: ACPI: Document i2c-allow-low-power-probe _DSD property
ACPI: Add a convenience function to tell a device is in low power
state
ov5670: Support probe whilst the device is in a low power state
at24: Support probing while off
.../acpi/dsd/i2c-allow-low-power-probe.rst | 60 +++++++++++++++
Documentation/firmware-guide/acpi/index.rst | 1 +
drivers/acpi/device_pm.c | 31 ++++++++
drivers/i2c/i2c-core-base.c | 19 ++++-
drivers/media/i2c/imx319.c | 74 +++++++++++-------
drivers/media/i2c/ov5670.c | 76 +++++++++++--------
drivers/misc/eeprom/at24.c | 43 ++++++-----
include/linux/acpi.h | 5 ++
include/linux/i2c.h | 14 ++++
9 files changed, 244 insertions(+), 79 deletions(-)
create mode 100644 Documentation/firmware-guide/acpi/dsd/i2c-allow-low-power-probe.rst
--
2.20.1
Add a convenience function to tell whether a device is in low power state,
primarily for use in drivers' probe or remove functions on busses where
the custom is to power on the device for the duration of both.
Returns false on non-ACPI systems.
Suggested-by: Mika Westerberg <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/device_pm.c | 31 +++++++++++++++++++++++++++++++
include/linux/acpi.h | 5 +++++
2 files changed, 36 insertions(+)
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 94d91c67aeaeb..e3c488d4af0d4 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1344,4 +1344,35 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
return 1;
}
EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
+
+/**
+ * acpi_dev_state_low_power - Check the current ACPI power state of a device.
+ * @dev: Physical device the ACPI power state of which to check
+ *
+ * On a system without ACPI, return false. On a system with ACPI, return true if
+ * the current ACPI power state of the device is not D0, or false otherwise.
+ *
+ * Note that the power state of a device is not well-defined after it has been
+ * passed to acpi_device_set_power() and before that function returns, so it is
+ * not valid to ask for the ACPI power state of the device in that time frame.
+ */
+bool acpi_dev_state_low_power(struct device *dev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+ int power_state;
+ int ret;
+
+ if (!adev)
+ return false;
+
+ ret = acpi_device_get_power(adev, &power_state);
+ if (ret) {
+ dev_dbg(dev, "Cannot obtain power state (%d)\n", ret);
+ return false;
+ }
+
+ return power_state != ACPI_STATE_D0;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_state_low_power);
+
#endif /* CONFIG_PM */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 17e80e182802e..d76d851259a49 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -978,6 +978,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_state_low_power(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; }
@@ -987,6 +988,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
{
return 0;
}
+static inline bool acpi_dev_state_low_power(struct device *dev)
+{
+ return false;
+}
#endif
#if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
--
2.20.1
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.
Also do identification on the first access of the device, whether in probe
or when starting streaming.
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/media/i2c/ov5670.c | 76 +++++++++++++++++++++++---------------
1 file changed, 46 insertions(+), 30 deletions(-)
diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index f26252e35e08d..74b0325c22565 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -1832,6 +1832,8 @@ struct ov5670 {
/* Streaming on/off */
bool streaming;
+ /* True if the device has been identified */
+ bool identified;
};
#define to_ov5670(_sd) container_of(_sd, struct ov5670, sd)
@@ -2270,6 +2272,32 @@ static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
return 0;
}
+/* Verify chip ID */
+static int ov5670_identify_module(struct ov5670 *ov5670)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
+ int ret;
+ u32 val;
+
+ if (ov5670->identified)
+ return 0;
+
+ ret = ov5670_read_reg(ov5670, OV5670_REG_CHIP_ID,
+ OV5670_REG_VALUE_24BIT, &val);
+ if (ret)
+ return ret;
+
+ if (val != OV5670_CHIP_ID) {
+ dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
+ OV5670_CHIP_ID, val);
+ return -ENXIO;
+ }
+
+ ov5670->identified = true;
+
+ return 0;
+}
+
/* Prepare streaming by writing default values and customized values */
static int ov5670_start_streaming(struct ov5670 *ov5670)
{
@@ -2278,6 +2306,10 @@ static int ov5670_start_streaming(struct ov5670 *ov5670)
int link_freq_index;
int ret;
+ ret = ov5670_identify_module(ov5670);
+ if (ret)
+ return ret;
+
/* Get out of from software reset */
ret = ov5670_write_reg(ov5670, OV5670_REG_SOFTWARE_RST,
OV5670_REG_VALUE_08BIT, OV5670_SOFTWARE_RST);
@@ -2401,27 +2433,6 @@ static int __maybe_unused ov5670_resume(struct device *dev)
return 0;
}
-/* Verify chip ID */
-static int ov5670_identify_module(struct ov5670 *ov5670)
-{
- struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
- int ret;
- u32 val;
-
- ret = ov5670_read_reg(ov5670, OV5670_REG_CHIP_ID,
- OV5670_REG_VALUE_24BIT, &val);
- if (ret)
- return ret;
-
- if (val != OV5670_CHIP_ID) {
- dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
- OV5670_CHIP_ID, val);
- return -ENXIO;
- }
-
- return 0;
-}
-
static const struct v4l2_subdev_video_ops ov5670_video_ops = {
.s_stream = ov5670_set_stream,
};
@@ -2456,6 +2467,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(&client->dev, "clock-frequency", &input_clk);
@@ -2472,11 +2484,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;
+ low_power = acpi_dev_state_low_power(&client->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(&ov5670->mutex);
@@ -2513,10 +2528,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(&client->dev);
+ if (!low_power)
+ pm_runtime_set_active(&client->dev);
pm_runtime_enable(&client->dev);
pm_runtime_idle(&client->dev);
@@ -2558,7 +2573,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 +2588,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
On Thu, Sep 10, 2020 at 08:06:06PM +0800, Bingbu Cao wrote:
...
> > @@ -987,6 +988,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
> > {
> > return 0;
> > }
>
> Need a blank line here, ;).
I'll add that for v9.
--
Sakari Ailus
Hi Luca,
On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote:
> Hi Sakari,
>
> On 03/09/20 10:15, Sakari Ailus wrote:
> >
> > Hi all,
> >
> > These patches enable calling (and finishing) a driver's probe function
> > without powering on the respective device on busses where the practice is
> > to power on the device for probe. While it generally is a driver's job to
> > check the that the device is there, there are cases where it might be
> > undesirable. (In this case it stems from a combination of hardware design
> > and user expectations; see below.) The downside with this change is that
> > if there is something wrong with the device, it will only be found at the
> > time the device is used. In this case (the camera sensors + EEPROM in a
> > sensor) I don't see any tangible harm from that though.
> >
> > An indication both from the driver and the firmware is required to allow
> > the device's power state to remain off during probe (see the first patch).
> >
> >
> > The use case is such that there is a privacy LED next to an integrated
> > user-facing laptop camera, and this LED is there to signal the user that
> > the camera is recording a video or capturing images. That LED also happens
> > to be wired to one of the power supplies of the camera, so whenever you
> > power on the camera, the LED will be lit, whether images are captured from
> > the camera --- or not. There's no way to implement this differently
> > without additional software control (allowing of which is itself a
> > hardware design decision) on most CSI-2-connected camera sensors as they
> > simply have no pin to signal the camera streaming state.
> >
> > This is also what happens during driver probe: the camera will be powered
> > on by the I?C subsystem calling dev_pm_domain_attach() and the device is
> > already powered on when the driver's own probe function is called. To the
> > user this visible during the boot process as a blink of the privacy LED,
> > suggesting that the camera is recording without the user having used an
> > application to do that. From the end user's point of view the behaviour is
> > not expected and for someone unfamiliar with internal workings of a
> > computer surely seems quite suspicious --- even if images are not being
> > actually captured.
> >
> > I've tested these on linux-next master. They also apply to Wolfram's
> > i2c/for-next branch, there's a patch that affects the I?C core changes
> > here (see below). The patches apart from that apply to Bartosz's
> > at24/for-next as well as Mauro's linux-media master branch.
>
> Apologies for having joined this discussion this late.
No worries. But thanks for the comments.
>
> This patchset seems a good base to cover a different use case, where I
> also cannot access the physical device at probe time.
>
> I'm going to try these patches, but in my case there are a few
> differences that need a better understanding.
>
> First, I'm using device tree, not ACPI. In addition to adding OF support
> similar to the work you've done for ACPI, I think instead of
> acpi_dev_state_low_power() we should have a function that works for both
> ACPI and DT.
acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI
power state of the device during probe or remove. It is not needed on DT
since the power state of the device is controlled directly by the driver.
On I?C ACPI devices, it's the framework that powers them on for probe.
You could have a helper function on DT to tell a driver what to do in
probe, but the functionality in that case is unrelated.
I'll answer to the second point later on.
--
Kind regards,
Sakari Ailus
Hi Sakari,
On 03/09/20 10:15, Sakari Ailus wrote:
>
> Hi all,
>
> These patches enable calling (and finishing) a driver's probe function
> without powering on the respective device on busses where the practice is
> to power on the device for probe. While it generally is a driver's job to
> check the that the device is there, there are cases where it might be
> undesirable. (In this case it stems from a combination of hardware design
> and user expectations; see below.) The downside with this change is that
> if there is something wrong with the device, it will only be found at the
> time the device is used. In this case (the camera sensors + EEPROM in a
> sensor) I don't see any tangible harm from that though.
>
> An indication both from the driver and the firmware is required to allow
> the device's power state to remain off during probe (see the first patch).
>
>
> The use case is such that there is a privacy LED next to an integrated
> user-facing laptop camera, and this LED is there to signal the user that
> the camera is recording a video or capturing images. That LED also happens
> to be wired to one of the power supplies of the camera, so whenever you
> power on the camera, the LED will be lit, whether images are captured from
> the camera --- or not. There's no way to implement this differently
> without additional software control (allowing of which is itself a
> hardware design decision) on most CSI-2-connected camera sensors as they
> simply have no pin to signal the camera streaming state.
>
> This is also what happens during driver probe: the camera will be powered
> on by the I²C subsystem calling dev_pm_domain_attach() and the device is
> already powered on when the driver's own probe function is called. To the
> user this visible during the boot process as a blink of the privacy LED,
> suggesting that the camera is recording without the user having used an
> application to do that. From the end user's point of view the behaviour is
> not expected and for someone unfamiliar with internal workings of a
> computer surely seems quite suspicious --- even if images are not being
> actually captured.
>
> I've tested these on linux-next master. They also apply to Wolfram's
> i2c/for-next branch, there's a patch that affects the I²C core changes
> here (see below). The patches apart from that apply to Bartosz's
> at24/for-next as well as Mauro's linux-media master branch.
Apologies for having joined this discussion this late.
This patchset seems a good base to cover a different use case, where I
also cannot access the physical device at probe time.
I'm going to try these patches, but in my case there are a few
differences that need a better understanding.
First, I'm using device tree, not ACPI. In addition to adding OF support
similar to the work you've done for ACPI, I think instead of
acpi_dev_state_low_power() we should have a function that works for both
ACPI and DT.
Second, even though all the chips I'm interested in are connected via
I2C, some of them (IIO sensors) can alternatively be connected via SPI
and it would make perfectly sense to use SPI instead of I2C. The "i2c-"
prefix added in v8 on the ACPI property looks like a limitation in that
respect. The same reasoning applies to the implementation in the I2C
core, but implementation could be generalized later.
I'd love to know your opinion on the above points.
--
Luca
Hi Sakari,
On 11/09/20 15:01, Sakari Ailus wrote:
> Hi Luca,
>
> On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote:
>> Hi Sakari,
>>
>> On 03/09/20 10:15, Sakari Ailus wrote:
>>>
>>> Hi all,
>>>
>>> These patches enable calling (and finishing) a driver's probe function
>>> without powering on the respective device on busses where the practice is
>>> to power on the device for probe. While it generally is a driver's job to
>>> check the that the device is there, there are cases where it might be
>>> undesirable. (In this case it stems from a combination of hardware design
>>> and user expectations; see below.) The downside with this change is that
>>> if there is something wrong with the device, it will only be found at the
>>> time the device is used. In this case (the camera sensors + EEPROM in a
>>> sensor) I don't see any tangible harm from that though.
>>>
>>> An indication both from the driver and the firmware is required to allow
>>> the device's power state to remain off during probe (see the first patch).
>>>
>>>
>>> The use case is such that there is a privacy LED next to an integrated
>>> user-facing laptop camera, and this LED is there to signal the user that
>>> the camera is recording a video or capturing images. That LED also happens
>>> to be wired to one of the power supplies of the camera, so whenever you
>>> power on the camera, the LED will be lit, whether images are captured from
>>> the camera --- or not. There's no way to implement this differently
>>> without additional software control (allowing of which is itself a
>>> hardware design decision) on most CSI-2-connected camera sensors as they
>>> simply have no pin to signal the camera streaming state.
>>>
>>> This is also what happens during driver probe: the camera will be powered
>>> on by the I²C subsystem calling dev_pm_domain_attach() and the device is
>>> already powered on when the driver's own probe function is called. To the
>>> user this visible during the boot process as a blink of the privacy LED,
>>> suggesting that the camera is recording without the user having used an
>>> application to do that. From the end user's point of view the behaviour is
>>> not expected and for someone unfamiliar with internal workings of a
>>> computer surely seems quite suspicious --- even if images are not being
>>> actually captured.
>>>
>>> I've tested these on linux-next master. They also apply to Wolfram's
>>> i2c/for-next branch, there's a patch that affects the I²C core changes
>>> here (see below). The patches apart from that apply to Bartosz's
>>> at24/for-next as well as Mauro's linux-media master branch.
>>
>> Apologies for having joined this discussion this late.
>
> No worries. But thanks for the comments.
>
>>
>> This patchset seems a good base to cover a different use case, where I
>> also cannot access the physical device at probe time.
>>
>> I'm going to try these patches, but in my case there are a few
>> differences that need a better understanding.
>>
>> First, I'm using device tree, not ACPI. In addition to adding OF support
>> similar to the work you've done for ACPI, I think instead of
>> acpi_dev_state_low_power() we should have a function that works for both
>> ACPI and DT.
>
> acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI
> power state of the device during probe or remove. It is not needed on DT
> since the power state of the device is controlled directly by the driver.
> On I²C ACPI devices, it's the framework that powers them on for probe.
I see, thanks for clarifying. I'm not used to ACPI so I didn't get that.
> You could have a helper function on DT to tell a driver what to do in
> probe, but the functionality in that case is unrelated.
So in case of DT we might think of a function that just tells whether
the device is marked to allow low-power probe, but it's just an info
from DT:
int mydriver_probe(struct i2c_client *client)
{
...
low_power = of_dev_state_low_power(&client->dev);
if (!low_power) {
mydriver_initialize(); /* power+clocks, write regs */
}
...
}
...and, if (low_power), call mydriver_initialize() at first usage.
I'm wondering whether this might make sense in mainline.
--
Luca
Hi Sakari,
On 14/09/20 11:47, Sakari Ailus wrote:
> Hi Luca,
>
> On Mon, Sep 14, 2020 at 09:58:24AM +0200, Luca Ceresoli wrote:
>> Hi Sakari,
>>
>> On 11/09/20 15:01, Sakari Ailus wrote:
>>> Hi Luca,
>>>
>>> On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote:
>>>> Hi Sakari,
>>>>
>>>> On 03/09/20 10:15, Sakari Ailus wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> These patches enable calling (and finishing) a driver's probe function
>>>>> without powering on the respective device on busses where the practice is
>>>>> to power on the device for probe. While it generally is a driver's job to
>>>>> check the that the device is there, there are cases where it might be
>>>>> undesirable. (In this case it stems from a combination of hardware design
>>>>> and user expectations; see below.) The downside with this change is that
>>>>> if there is something wrong with the device, it will only be found at the
>>>>> time the device is used. In this case (the camera sensors + EEPROM in a
>>>>> sensor) I don't see any tangible harm from that though.
>>>>>
>>>>> An indication both from the driver and the firmware is required to allow
>>>>> the device's power state to remain off during probe (see the first patch).
>>>>>
>>>>>
>>>>> The use case is such that there is a privacy LED next to an integrated
>>>>> user-facing laptop camera, and this LED is there to signal the user that
>>>>> the camera is recording a video or capturing images. That LED also happens
>>>>> to be wired to one of the power supplies of the camera, so whenever you
>>>>> power on the camera, the LED will be lit, whether images are captured from
>>>>> the camera --- or not. There's no way to implement this differently
>>>>> without additional software control (allowing of which is itself a
>>>>> hardware design decision) on most CSI-2-connected camera sensors as they
>>>>> simply have no pin to signal the camera streaming state.
>>>>>
>>>>> This is also what happens during driver probe: the camera will be powered
>>>>> on by the I²C subsystem calling dev_pm_domain_attach() and the device is
>>>>> already powered on when the driver's own probe function is called. To the
>>>>> user this visible during the boot process as a blink of the privacy LED,
>>>>> suggesting that the camera is recording without the user having used an
>>>>> application to do that. From the end user's point of view the behaviour is
>>>>> not expected and for someone unfamiliar with internal workings of a
>>>>> computer surely seems quite suspicious --- even if images are not being
>>>>> actually captured.
>>>>>
>>>>> I've tested these on linux-next master. They also apply to Wolfram's
>>>>> i2c/for-next branch, there's a patch that affects the I²C core changes
>>>>> here (see below). The patches apart from that apply to Bartosz's
>>>>> at24/for-next as well as Mauro's linux-media master branch.
>>>>
>>>> Apologies for having joined this discussion this late.
>>>
>>> No worries. But thanks for the comments.
>>>
>>>>
>>>> This patchset seems a good base to cover a different use case, where I
>>>> also cannot access the physical device at probe time.
>>>>
>>>> I'm going to try these patches, but in my case there are a few
>>>> differences that need a better understanding.
>>>>
>>>> First, I'm using device tree, not ACPI. In addition to adding OF support
>>>> similar to the work you've done for ACPI, I think instead of
>>>> acpi_dev_state_low_power() we should have a function that works for both
>>>> ACPI and DT.
>>>
>>> acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI
>>> power state of the device during probe or remove. It is not needed on DT
>>> since the power state of the device is controlled directly by the driver.
>>> On I²C ACPI devices, it's the framework that powers them on for probe.
>>
>> I see, thanks for clarifying. I'm not used to ACPI so I didn't get that.
>>
>>> You could have a helper function on DT to tell a driver what to do in
>>> probe, but the functionality in that case is unrelated.
>>
>> So in case of DT we might think of a function that just tells whether
>> the device is marked to allow low-power probe, but it's just an info
>> from DT:
>>
>> int mydriver_probe(struct i2c_client *client)
>> {
>> ...
>> low_power = of_dev_state_low_power(&client->dev);
>> if (!low_power) {
>> mydriver_initialize(); /* power+clocks, write regs */
>> }
>> ...
>> }
>>
>> ...and, if (low_power), call mydriver_initialize() at first usage.
>>
>> I'm wondering whether this might make sense in mainline.
>
> Quite possibly, if there are drivers that would need it.
>
> The function should probably be called differently though as what it does
> is quite different after all.
>
> Unless... we did the following:
>
> - Redefine the I²C driver flag added by this patchset into what tells the
> I²C framework whether the driver does its own power management
> independently of the I²C framework. It could be called e.g.
> I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power
> management of the device, and the I²C framework would not power on the
> device for probe or remove.
>
> - Add a firmware function to tell whether the device identification should
> take place during probe or not. For this is what we're really doing here
> from driver's point of view: lazy device probing.
Indeed my needs have nothing to do with power management. What I need is
lazy device probing as the I2C bus may need time before it can be used.
From the driver code point of view it looks similar (there's an if()
around initializations in probe() and init is done later if needed), but
the usage is different.
Another approach would be to add a new I2C driver operation [say
init_hw()], then move code for lazy init out of probe() into init_hw().
probe() would still allocate resources. init_hw() would be called by the
framework (or the controller driver?) when it knows eveything is ready.
Just wild thoughts while I'm trying to focus the problem...
> There are no dependencies between the two but they can be used together to
> implement the same functionality as this patchset currently does. This way
> also the differences between driver implementations for ACPI and DT can be
> reduced as the logic is the same.
>
> Further on, with this approach, if other busses happen to need this
> functionality in the future, it would be straightforward to add support ---
> it only takes to query whether it's indicated by the DT or ACPI property.
>
> Thoughts, opinions?
>
--
Luca
Hi Luca,
On Mon, Sep 14, 2020 at 06:49:29PM +0200, Luca Ceresoli wrote:
> Hi Sakari,
>
> On 14/09/20 11:47, Sakari Ailus wrote:
> > Hi Luca,
> >
> > On Mon, Sep 14, 2020 at 09:58:24AM +0200, Luca Ceresoli wrote:
> >> Hi Sakari,
> >>
> >> On 11/09/20 15:01, Sakari Ailus wrote:
> >>> Hi Luca,
> >>>
> >>> On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote:
> >>>> Hi Sakari,
> >>>>
> >>>> On 03/09/20 10:15, Sakari Ailus wrote:
> >>>>>
> >>>>> Hi all,
> >>>>>
> >>>>> These patches enable calling (and finishing) a driver's probe function
> >>>>> without powering on the respective device on busses where the practice is
> >>>>> to power on the device for probe. While it generally is a driver's job to
> >>>>> check the that the device is there, there are cases where it might be
> >>>>> undesirable. (In this case it stems from a combination of hardware design
> >>>>> and user expectations; see below.) The downside with this change is that
> >>>>> if there is something wrong with the device, it will only be found at the
> >>>>> time the device is used. In this case (the camera sensors + EEPROM in a
> >>>>> sensor) I don't see any tangible harm from that though.
> >>>>>
> >>>>> An indication both from the driver and the firmware is required to allow
> >>>>> the device's power state to remain off during probe (see the first patch).
> >>>>>
> >>>>>
> >>>>> The use case is such that there is a privacy LED next to an integrated
> >>>>> user-facing laptop camera, and this LED is there to signal the user that
> >>>>> the camera is recording a video or capturing images. That LED also happens
> >>>>> to be wired to one of the power supplies of the camera, so whenever you
> >>>>> power on the camera, the LED will be lit, whether images are captured from
> >>>>> the camera --- or not. There's no way to implement this differently
> >>>>> without additional software control (allowing of which is itself a
> >>>>> hardware design decision) on most CSI-2-connected camera sensors as they
> >>>>> simply have no pin to signal the camera streaming state.
> >>>>>
> >>>>> This is also what happens during driver probe: the camera will be powered
> >>>>> on by the I?C subsystem calling dev_pm_domain_attach() and the device is
> >>>>> already powered on when the driver's own probe function is called. To the
> >>>>> user this visible during the boot process as a blink of the privacy LED,
> >>>>> suggesting that the camera is recording without the user having used an
> >>>>> application to do that. From the end user's point of view the behaviour is
> >>>>> not expected and for someone unfamiliar with internal workings of a
> >>>>> computer surely seems quite suspicious --- even if images are not being
> >>>>> actually captured.
> >>>>>
> >>>>> I've tested these on linux-next master. They also apply to Wolfram's
> >>>>> i2c/for-next branch, there's a patch that affects the I?C core changes
> >>>>> here (see below). The patches apart from that apply to Bartosz's
> >>>>> at24/for-next as well as Mauro's linux-media master branch.
> >>>>
> >>>> Apologies for having joined this discussion this late.
> >>>
> >>> No worries. But thanks for the comments.
> >>>
> >>>>
> >>>> This patchset seems a good base to cover a different use case, where I
> >>>> also cannot access the physical device at probe time.
> >>>>
> >>>> I'm going to try these patches, but in my case there are a few
> >>>> differences that need a better understanding.
> >>>>
> >>>> First, I'm using device tree, not ACPI. In addition to adding OF support
> >>>> similar to the work you've done for ACPI, I think instead of
> >>>> acpi_dev_state_low_power() we should have a function that works for both
> >>>> ACPI and DT.
> >>>
> >>> acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI
> >>> power state of the device during probe or remove. It is not needed on DT
> >>> since the power state of the device is controlled directly by the driver.
> >>> On I?C ACPI devices, it's the framework that powers them on for probe.
> >>
> >> I see, thanks for clarifying. I'm not used to ACPI so I didn't get that.
> >>
> >>> You could have a helper function on DT to tell a driver what to do in
> >>> probe, but the functionality in that case is unrelated.
> >>
> >> So in case of DT we might think of a function that just tells whether
> >> the device is marked to allow low-power probe, but it's just an info
> >> from DT:
> >>
> >> int mydriver_probe(struct i2c_client *client)
> >> {
> >> ...
> >> low_power = of_dev_state_low_power(&client->dev);
> >> if (!low_power) {
> >> mydriver_initialize(); /* power+clocks, write regs */
> >> }
> >> ...
> >> }
> >>
> >> ...and, if (low_power), call mydriver_initialize() at first usage.
> >>
> >> I'm wondering whether this might make sense in mainline.
> >
> > Quite possibly, if there are drivers that would need it.
> >
> > The function should probably be called differently though as what it does
> > is quite different after all.
> >
> > Unless... we did the following:
> >
> > - Redefine the I?C driver flag added by this patchset into what tells the
> > I?C framework whether the driver does its own power management
> > independently of the I?C framework. It could be called e.g.
> > I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power
> > management of the device, and the I?C framework would not power on the
> > device for probe or remove.
> >
> > - Add a firmware function to tell whether the device identification should
> > take place during probe or not. For this is what we're really doing here
> > from driver's point of view: lazy device probing.
>
> Indeed my needs have nothing to do with power management. What I need is
> lazy device probing as the I2C bus may need time before it can be used.
> From the driver code point of view it looks similar (there's an if()
> around initializations in probe() and init is done later if needed), but
> the usage is different.
>
> Another approach would be to add a new I2C driver operation [say
> init_hw()], then move code for lazy init out of probe() into init_hw().
> probe() would still allocate resources. init_hw() would be called by the
> framework (or the controller driver?) when it knows eveything is ready.
> Just wild thoughts while I'm trying to focus the problem...
What makes the controller driver not ready to operate the controller when
the client devices are probed?
--
Sakari Ailus
Hi Sakari,
On 23/09/20 13:08, Sakari Ailus wrote:
> Hi Luca,
>
> On Mon, Sep 14, 2020 at 06:49:29PM +0200, Luca Ceresoli wrote:
>> Hi Sakari,
>>
>> On 14/09/20 11:47, Sakari Ailus wrote:
>>> Hi Luca,
>>>
>>> On Mon, Sep 14, 2020 at 09:58:24AM +0200, Luca Ceresoli wrote:
>>>> Hi Sakari,
>>>>
>>>> On 11/09/20 15:01, Sakari Ailus wrote:
>>>>> Hi Luca,
>>>>>
>>>>> On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote:
>>>>>> Hi Sakari,
>>>>>>
>>>>>> On 03/09/20 10:15, Sakari Ailus wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> These patches enable calling (and finishing) a driver's probe function
>>>>>>> without powering on the respective device on busses where the practice is
>>>>>>> to power on the device for probe. While it generally is a driver's job to
>>>>>>> check the that the device is there, there are cases where it might be
>>>>>>> undesirable. (In this case it stems from a combination of hardware design
>>>>>>> and user expectations; see below.) The downside with this change is that
>>>>>>> if there is something wrong with the device, it will only be found at the
>>>>>>> time the device is used. In this case (the camera sensors + EEPROM in a
>>>>>>> sensor) I don't see any tangible harm from that though.
>>>>>>>
>>>>>>> An indication both from the driver and the firmware is required to allow
>>>>>>> the device's power state to remain off during probe (see the first patch).
>>>>>>>
>>>>>>>
>>>>>>> The use case is such that there is a privacy LED next to an integrated
>>>>>>> user-facing laptop camera, and this LED is there to signal the user that
>>>>>>> the camera is recording a video or capturing images. That LED also happens
>>>>>>> to be wired to one of the power supplies of the camera, so whenever you
>>>>>>> power on the camera, the LED will be lit, whether images are captured from
>>>>>>> the camera --- or not. There's no way to implement this differently
>>>>>>> without additional software control (allowing of which is itself a
>>>>>>> hardware design decision) on most CSI-2-connected camera sensors as they
>>>>>>> simply have no pin to signal the camera streaming state.
>>>>>>>
>>>>>>> This is also what happens during driver probe: the camera will be powered
>>>>>>> on by the I²C subsystem calling dev_pm_domain_attach() and the device is
>>>>>>> already powered on when the driver's own probe function is called. To the
>>>>>>> user this visible during the boot process as a blink of the privacy LED,
>>>>>>> suggesting that the camera is recording without the user having used an
>>>>>>> application to do that. From the end user's point of view the behaviour is
>>>>>>> not expected and for someone unfamiliar with internal workings of a
>>>>>>> computer surely seems quite suspicious --- even if images are not being
>>>>>>> actually captured.
>>>>>>>
>>>>>>> I've tested these on linux-next master. They also apply to Wolfram's
>>>>>>> i2c/for-next branch, there's a patch that affects the I²C core changes
>>>>>>> here (see below). The patches apart from that apply to Bartosz's
>>>>>>> at24/for-next as well as Mauro's linux-media master branch.
>>>>>>
>>>>>> Apologies for having joined this discussion this late.
>>>>>
>>>>> No worries. But thanks for the comments.
>>>>>
>>>>>>
>>>>>> This patchset seems a good base to cover a different use case, where I
>>>>>> also cannot access the physical device at probe time.
>>>>>>
>>>>>> I'm going to try these patches, but in my case there are a few
>>>>>> differences that need a better understanding.
>>>>>>
>>>>>> First, I'm using device tree, not ACPI. In addition to adding OF support
>>>>>> similar to the work you've done for ACPI, I think instead of
>>>>>> acpi_dev_state_low_power() we should have a function that works for both
>>>>>> ACPI and DT.
>>>>>
>>>>> acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI
>>>>> power state of the device during probe or remove. It is not needed on DT
>>>>> since the power state of the device is controlled directly by the driver.
>>>>> On I²C ACPI devices, it's the framework that powers them on for probe.
>>>>
>>>> I see, thanks for clarifying. I'm not used to ACPI so I didn't get that.
>>>>
>>>>> You could have a helper function on DT to tell a driver what to do in
>>>>> probe, but the functionality in that case is unrelated.
>>>>
>>>> So in case of DT we might think of a function that just tells whether
>>>> the device is marked to allow low-power probe, but it's just an info
>>>> from DT:
>>>>
>>>> int mydriver_probe(struct i2c_client *client)
>>>> {
>>>> ...
>>>> low_power = of_dev_state_low_power(&client->dev);
>>>> if (!low_power) {
>>>> mydriver_initialize(); /* power+clocks, write regs */
>>>> }
>>>> ...
>>>> }
>>>>
>>>> ...and, if (low_power), call mydriver_initialize() at first usage.
>>>>
>>>> I'm wondering whether this might make sense in mainline.
>>>
>>> Quite possibly, if there are drivers that would need it.
>>>
>>> The function should probably be called differently though as what it does
>>> is quite different after all.
>>>
>>> Unless... we did the following:
>>>
>>> - Redefine the I²C driver flag added by this patchset into what tells the
>>> I²C framework whether the driver does its own power management
>>> independently of the I²C framework. It could be called e.g.
>>> I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power
>>> management of the device, and the I²C framework would not power on the
>>> device for probe or remove.
>>>
>>> - Add a firmware function to tell whether the device identification should
>>> take place during probe or not. For this is what we're really doing here
>>> from driver's point of view: lazy device probing.
>>
>> Indeed my needs have nothing to do with power management. What I need is
>> lazy device probing as the I2C bus may need time before it can be used.
>> From the driver code point of view it looks similar (there's an if()
>> around initializations in probe() and init is done later if needed), but
>> the usage is different.
>>
>> Another approach would be to add a new I2C driver operation [say
>> init_hw()], then move code for lazy init out of probe() into init_hw().
>> probe() would still allocate resources. init_hw() would be called by the
>> framework (or the controller driver?) when it knows eveything is ready.
>> Just wild thoughts while I'm trying to focus the problem...
>
> What makes the controller driver not ready to operate the controller when
> the client devices are probed?
I'm working with a camera module connected via a serializer/deserializer
chip pair, and the link can go away and get back at any moment for
various reasons: mechanical, electromagnetic, cable not connected etc.
This scenario is not well handled with current kernel structures, so I'm
handling it partially with some local hacks but I'm always curious about
any possible kernel improvement that can improve the situation.
See these links for some info about my troubles:
https://lucaceresoli.net/plumbers-i2c-bof/
https://elinux.org/images/f/fc/Ceresoli-elce2019-video-serdes-linux.pdf
https://youtu.be/7hLv6fYAW-E?list=PLbzoR-pLrL6pamOj4UifcMJf560Ph6mJp
Your patchset looks very similar to something I need: not communicating
with the device during probe. Another piece would be to trigger a device
configuration "in some way" when the link is known to be back.
--
Luca
On Mon, Sep 14, 2020 at 12:47:27PM +0300, Sakari Ailus wrote:
> Hi Luca,
>
> On Mon, Sep 14, 2020 at 09:58:24AM +0200, Luca Ceresoli wrote:
> > Hi Sakari,
> >
> > On 11/09/20 15:01, Sakari Ailus wrote:
> > > Hi Luca,
> > >
> > > On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote:
> > >> Hi Sakari,
> > >>
> > >> On 03/09/20 10:15, Sakari Ailus wrote:
> > >>>
> > >>> Hi all,
> > >>>
> > >>> These patches enable calling (and finishing) a driver's probe function
> > >>> without powering on the respective device on busses where the practice is
> > >>> to power on the device for probe. While it generally is a driver's job to
> > >>> check the that the device is there, there are cases where it might be
> > >>> undesirable. (In this case it stems from a combination of hardware design
> > >>> and user expectations; see below.) The downside with this change is that
> > >>> if there is something wrong with the device, it will only be found at the
> > >>> time the device is used. In this case (the camera sensors + EEPROM in a
> > >>> sensor) I don't see any tangible harm from that though.
> > >>>
> > >>> An indication both from the driver and the firmware is required to allow
> > >>> the device's power state to remain off during probe (see the first patch).
> > >>>
> > >>>
> > >>> The use case is such that there is a privacy LED next to an integrated
> > >>> user-facing laptop camera, and this LED is there to signal the user that
> > >>> the camera is recording a video or capturing images. That LED also happens
> > >>> to be wired to one of the power supplies of the camera, so whenever you
> > >>> power on the camera, the LED will be lit, whether images are captured from
> > >>> the camera --- or not. There's no way to implement this differently
> > >>> without additional software control (allowing of which is itself a
> > >>> hardware design decision) on most CSI-2-connected camera sensors as they
> > >>> simply have no pin to signal the camera streaming state.
> > >>>
> > >>> This is also what happens during driver probe: the camera will be powered
> > >>> on by the I?C subsystem calling dev_pm_domain_attach() and the device is
> > >>> already powered on when the driver's own probe function is called. To the
> > >>> user this visible during the boot process as a blink of the privacy LED,
> > >>> suggesting that the camera is recording without the user having used an
> > >>> application to do that. From the end user's point of view the behaviour is
> > >>> not expected and for someone unfamiliar with internal workings of a
> > >>> computer surely seems quite suspicious --- even if images are not being
> > >>> actually captured.
> > >>>
> > >>> I've tested these on linux-next master. They also apply to Wolfram's
> > >>> i2c/for-next branch, there's a patch that affects the I?C core changes
> > >>> here (see below). The patches apart from that apply to Bartosz's
> > >>> at24/for-next as well as Mauro's linux-media master branch.
> > >>
> > >> Apologies for having joined this discussion this late.
> > >
> > > No worries. But thanks for the comments.
> > >
> > >>
> > >> This patchset seems a good base to cover a different use case, where I
> > >> also cannot access the physical device at probe time.
> > >>
> > >> I'm going to try these patches, but in my case there are a few
> > >> differences that need a better understanding.
> > >>
> > >> First, I'm using device tree, not ACPI. In addition to adding OF support
> > >> similar to the work you've done for ACPI, I think instead of
> > >> acpi_dev_state_low_power() we should have a function that works for both
> > >> ACPI and DT.
> > >
> > > acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI
> > > power state of the device during probe or remove. It is not needed on DT
> > > since the power state of the device is controlled directly by the driver.
> > > On I?C ACPI devices, it's the framework that powers them on for probe.
> >
> > I see, thanks for clarifying. I'm not used to ACPI so I didn't get that.
> >
> > > You could have a helper function on DT to tell a driver what to do in
> > > probe, but the functionality in that case is unrelated.
> >
> > So in case of DT we might think of a function that just tells whether
> > the device is marked to allow low-power probe, but it's just an info
> > from DT:
> >
> > int mydriver_probe(struct i2c_client *client)
> > {
> > ...
> > low_power = of_dev_state_low_power(&client->dev);
> > if (!low_power) {
> > mydriver_initialize(); /* power+clocks, write regs */
> > }
> > ...
> > }
> >
> > ...and, if (low_power), call mydriver_initialize() at first usage.
> >
> > I'm wondering whether this might make sense in mainline.
>
> Quite possibly, if there are drivers that would need it.
>
> The function should probably be called differently though as what it does
> is quite different after all.
>
> Unless... we did the following:
>
> - Redefine the I?C driver flag added by this patchset into what tells the
> I?C framework whether the driver does its own power management
> independently of the I?C framework. It could be called e.g.
> I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power
> management of the device, and the I?C framework would not power on the
> device for probe or remove.
>
> - Add a firmware function to tell whether the device identification should
> take place during probe or not. For this is what we're really doing here
> from driver's point of view: lazy device probing.
>
> There are no dependencies between the two but they can be used together to
> implement the same functionality as this patchset currently does. This way
> also the differences between driver implementations for ACPI and DT can be
> reduced as the logic is the same.
>
> Further on, with this approach, if other busses happen to need this
> functionality in the future, it would be straightforward to add support ---
> it only takes to query whether it's indicated by the DT or ACPI property.
>
> Thoughts, opinions?
I think we might be overly complicating things. IMHO the series as is
with the "i2c_" prefix removed from the flags introduced would be
reusable as is for any other subsystem that needs it. Of course, for
now, the handling of the flag would remain implemented only in the I2C
subsystem.
Best regards,
Tomasz
Hi Sakari,
On Thu, Sep 03, 2020 at 11:15:44AM +0300, Sakari Ailus wrote:
>
> Hi all,
>
> These patches enable calling (and finishing) a driver's probe function
> without powering on the respective device on busses where the practice is
> to power on the device for probe. While it generally is a driver's job to
> check the that the device is there, there are cases where it might be
> undesirable. (In this case it stems from a combination of hardware design
> and user expectations; see below.) The downside with this change is that
> if there is something wrong with the device, it will only be found at the
> time the device is used. In this case (the camera sensors + EEPROM in a
> sensor) I don't see any tangible harm from that though.
>
> An indication both from the driver and the firmware is required to allow
> the device's power state to remain off during probe (see the first patch).
>
>
> The use case is such that there is a privacy LED next to an integrated
> user-facing laptop camera, and this LED is there to signal the user that
> the camera is recording a video or capturing images. That LED also happens
> to be wired to one of the power supplies of the camera, so whenever you
> power on the camera, the LED will be lit, whether images are captured from
> the camera --- or not. There's no way to implement this differently
> without additional software control (allowing of which is itself a
> hardware design decision) on most CSI-2-connected camera sensors as they
> simply have no pin to signal the camera streaming state.
>
> This is also what happens during driver probe: the camera will be powered
> on by the I?C subsystem calling dev_pm_domain_attach() and the device is
> already powered on when the driver's own probe function is called. To the
> user this visible during the boot process as a blink of the privacy LED,
> suggesting that the camera is recording without the user having used an
> application to do that. From the end user's point of view the behaviour is
> not expected and for someone unfamiliar with internal workings of a
> computer surely seems quite suspicious --- even if images are not being
> actually captured.
>
> I've tested these on linux-next master. They also apply to Wolfram's
> i2c/for-next branch, there's a patch that affects the I?C core changes
> here (see below). The patches apart from that apply to Bartosz's
> at24/for-next as well as Mauro's linux-media master branch.
Besides the suggestion to make the defintions added less specific to i2c
(but still keeping the implementation so for now), feel free to add:
Reviewed-by: Tomasz Figa <[email protected]>
Best regards,
Tomasz
> I think we might be overly complicating things. IMHO the series as is
> with the "i2c_" prefix removed from the flags introduced would be
> reusable as is for any other subsystem that needs it. Of course, for
> now, the handling of the flag would remain implemented only in the I2C
> subsystem.
Just to be clear: you are suggesting to remove "i2c" from the DSD
binding "i2c-allow-low-power-probe". And you are not talking about
moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I
recall the latter has been NACKed by gkh so far.
On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang <[email protected]> wrote:
>
>
> > I think we might be overly complicating things. IMHO the series as is
> > with the "i2c_" prefix removed from the flags introduced would be
> > reusable as is for any other subsystem that needs it. Of course, for
> > now, the handling of the flag would remain implemented only in the I2C
> > subsystem.
>
> Just to be clear: you are suggesting to remove "i2c" from the DSD
> binding "i2c-allow-low-power-probe". And you are not talking about
> moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I
> recall the latter has been NACKed by gkh so far.
>
I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all
the implementation would remain where it is in the code. IOW, I'm just
suggesting a naming change to avoid proliferating duplicate flags of
the same meaning across subsystems.
On Sun, Sep 27, 2020 at 9:44 PM Tomasz Figa <[email protected]> wrote:
>
> On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang <[email protected]> wrote:
> >
> >
> > > I think we might be overly complicating things. IMHO the series as is
> > > with the "i2c_" prefix removed from the flags introduced would be
> > > reusable as is for any other subsystem that needs it. Of course, for
> > > now, the handling of the flag would remain implemented only in the I2C
> > > subsystem.
> >
> > Just to be clear: you are suggesting to remove "i2c" from the DSD
> > binding "i2c-allow-low-power-probe". And you are not talking about
> > moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I
> > recall the latter has been NACKed by gkh so far.
> >
>
> I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all
> the implementation would remain where it is in the code. IOW, I'm just
> suggesting a naming change to avoid proliferating duplicate flags of
> the same meaning across subsystems.
But that would indicate that the property was recognized by other
subsystems which wouldn't be the case, so it would be confusing.
That's why it cannot be documented as a general property ATM too.
On Mon, Sep 28, 2020 at 4:18 PM Rafael J. Wysocki <[email protected]> wrote:
>
> On Sun, Sep 27, 2020 at 9:44 PM Tomasz Figa <[email protected]> wrote:
> >
> > On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang <[email protected]> wrote:
> > >
> > >
> > > > I think we might be overly complicating things. IMHO the series as is
> > > > with the "i2c_" prefix removed from the flags introduced would be
> > > > reusable as is for any other subsystem that needs it. Of course, for
> > > > now, the handling of the flag would remain implemented only in the I2C
> > > > subsystem.
> > >
> > > Just to be clear: you are suggesting to remove "i2c" from the DSD
> > > binding "i2c-allow-low-power-probe". And you are not talking about
> > > moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I
> > > recall the latter has been NACKed by gkh so far.
> > >
> >
> > I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all
> > the implementation would remain where it is in the code. IOW, I'm just
> > suggesting a naming change to avoid proliferating duplicate flags of
> > the same meaning across subsystems.
>
> But that would indicate that the property was recognized by other
> subsystems which wouldn't be the case, so it would be confusing.
>
> That's why it cannot be documented as a general property ATM too.
I guess that's true. Well, this is kAPI in the end, so if we have more
subsystems, it could be always renamed. So feel free to ignore my
previous comment.
Best regards,
Tomasz
Hi Tomasz,
On Mon, Sep 28, 2020 at 06:49:22PM +0200, Tomasz Figa wrote:
> On Mon, Sep 28, 2020 at 4:18 PM Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Sun, Sep 27, 2020 at 9:44 PM Tomasz Figa <[email protected]> wrote:
> > >
> > > On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang <[email protected]> wrote:
> > > >
> > > >
> > > > > I think we might be overly complicating things. IMHO the series as is
> > > > > with the "i2c_" prefix removed from the flags introduced would be
> > > > > reusable as is for any other subsystem that needs it. Of course, for
> > > > > now, the handling of the flag would remain implemented only in the I2C
> > > > > subsystem.
> > > >
> > > > Just to be clear: you are suggesting to remove "i2c" from the DSD
> > > > binding "i2c-allow-low-power-probe". And you are not talking about
> > > > moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I
> > > > recall the latter has been NACKed by gkh so far.
> > > >
> > >
> > > I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all
> > > the implementation would remain where it is in the code. IOW, I'm just
> > > suggesting a naming change to avoid proliferating duplicate flags of
> > > the same meaning across subsystems.
> >
> > But that would indicate that the property was recognized by other
> > subsystems which wouldn't be the case, so it would be confusing.
> >
> > That's why it cannot be documented as a general property ATM too.
>
> I guess that's true. Well, this is kAPI in the end, so if we have more
> subsystems, it could be always renamed. So feel free to ignore my
> previous comment.
I wouldn't expect this flag to be needed outside I?C since the other
potential use case (I3C) appears to be entirely free of power management,
so it's up to the drivers on ACPI, too.
The property itself, though, might be.
--
Regards,
Sakari Ailus
Add a convenience function to tell whether a device is in low power state,
primarily for use in drivers' probe or remove functions on busses where
the custom is to power on the device for the duration of both.
Returns false on non-ACPI systems.
Suggested-by: Mika Westerberg <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/device_pm.c | 31 +++++++++++++++++++++++++++++++
include/linux/acpi.h | 6 ++++++
2 files changed, 37 insertions(+)
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 096153761ebc3..1b5336af0b4cd 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1336,4 +1336,35 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
return 1;
}
EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
+
+/**
+ * acpi_dev_state_low_power - Check the current ACPI power state of a device.
+ * @dev: Physical device the ACPI power state of which to check
+ *
+ * On a system without ACPI, return false. On a system with ACPI, return true if
+ * the current ACPI power state of the device is not D0, or false otherwise.
+ *
+ * Note that the power state of a device is not well-defined after it has been
+ * passed to acpi_device_set_power() and before that function returns, so it is
+ * not valid to ask for the ACPI power state of the device in that time frame.
+ */
+bool acpi_dev_state_low_power(struct device *dev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+ int power_state;
+ int ret;
+
+ if (!adev)
+ return false;
+
+ ret = acpi_device_get_power(adev, &power_state);
+ if (ret) {
+ dev_dbg(dev, "Cannot obtain power state (%d)\n", ret);
+ return false;
+ }
+
+ return power_state != ACPI_STATE_D0;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_state_low_power);
+
#endif /* CONFIG_PM */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 4703daafcce9e..864786adb567c 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -982,6 +982,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_state_low_power(struct device *dev);
#else
static inline int acpi_subsys_runtime_suspend(struct device *dev) { return 0; }
static inline int acpi_subsys_runtime_resume(struct device *dev) { return 0; }
@@ -989,6 +990,11 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
{
return 0;
}
+
+static inline bool acpi_dev_state_low_power(struct device *dev)
+{
+ return false;
+}
#endif
#if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
--
2.20.1
Document the use of the _PRE object for setting desirable power state
during probe.
Signed-off-by: Sakari Ailus <[email protected]>
---
Documentation/firmware-guide/acpi/index.rst | 1 +
.../firmware-guide/acpi/low-power-probe.rst | 51 +++++++++++++++++++
2 files changed, 52 insertions(+)
create mode 100644 Documentation/firmware-guide/acpi/low-power-probe.rst
diff --git a/Documentation/firmware-guide/acpi/index.rst b/Documentation/firmware-guide/acpi/index.rst
index f72b5f1769fb2..d02712acccbc0 100644
--- a/Documentation/firmware-guide/acpi/index.rst
+++ b/Documentation/firmware-guide/acpi/index.rst
@@ -25,5 +25,6 @@ ACPI Support
acpi-lid
lpit
video_extension
+ low-power-probe
extcon-intel-int3496
intel-pmc-mux
diff --git a/Documentation/firmware-guide/acpi/low-power-probe.rst b/Documentation/firmware-guide/acpi/low-power-probe.rst
new file mode 100644
index 0000000000000..3437f8c8f233a
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/low-power-probe.rst
@@ -0,0 +1,51 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+======================================
+Probing I²C devices in low power state
+======================================
+
+Introduction
+============
+
+In some cases it may be preferred to leave certain devices powered off for the
+entire system bootup if powering on these devices has adverse side effects,
+beyond just powering on the said device.
+
+How it works
+============
+
+The _PRE object that evaluates to integer 0 may be used to tell Linux that the
+device should remain in the low power state during probe. If the driver
+indicates its support for this by setting the I2C_DRV_FL_ALLOW_LOW_POWER_PROBE
+flag in struct i2c_driver.flags field and the _PRE object evaluates to integer
+0, the device will not be powered on for probe.
+
+The downside is that as the device is not powered on, even if there's a problem
+with the device, the driver likely probes just fine but the first user will
+find out the device doesn't work, instead of a failure at probe time. This
+feature should thus be used sparingly.
+
+Example
+=======
+
+An ASL example describing an ACPI device using this property looks like
+this. Some objects not relevant from the example point of view have been
+omitted.
+
+.. code-block:: text
+
+ Device (CAM0)
+ {
+ Name (_HID, "SONY319A")
+ Name (_UID, Zero)
+ Name (_CRS, ResourceTemplate ()
+ {
+ I2cSerialBus(0x0020, ControllerInitiated, 0x00061A80,
+ AddressingMode7Bit, "\\_SB.PCI0.I2C0",
+ 0x00, ResourceConsumer)
+ })
+ Name (_PRE, 0, NotSerialized)
+ {
+ Return (0x0)
+ }
+ }
--
2.20.1
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 <[email protected]>
---
drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 926408b41270c..dd0b3f24e3808 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -595,6 +595,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 low_power;
struct regmap *regmap;
bool writable;
u8 test_byte;
@@ -750,14 +751,16 @@ static int at24_probe(struct i2c_client *client)
i2c_set_clientdata(client, at24);
- err = regulator_enable(at24->vcc_reg);
- if (err) {
- dev_err(dev, "Failed to enable vcc regulator\n");
- return err;
- }
+ low_power = acpi_dev_state_low_power(&client->dev);
+ if (!low_power) {
+ err = regulator_enable(at24->vcc_reg);
+ if (err) {
+ dev_err(dev, "Failed to enable vcc regulator\n");
+ return err;
+ }
- /* enable runtime pm */
- pm_runtime_set_active(dev);
+ pm_runtime_set_active(dev);
+ }
pm_runtime_enable(dev);
at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
@@ -768,14 +771,17 @@ static int at24_probe(struct i2c_client *client)
}
/*
- * 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);
- if (err) {
- pm_runtime_disable(dev);
- regulator_disable(at24->vcc_reg);
- return -ENODEV;
+ if (!low_power) {
+ err = at24_read(at24, 0, &test_byte, 1);
+ if (err) {
+ pm_runtime_disable(dev);
+ regulator_disable(at24->vcc_reg);
+ return -ENODEV;
+ }
}
pm_runtime_idle(dev);
@@ -795,9 +801,11 @@ static int at24_remove(struct i2c_client *client)
struct at24_data *at24 = i2c_get_clientdata(client);
pm_runtime_disable(&client->dev);
- if (!pm_runtime_status_suspended(&client->dev))
- regulator_disable(at24->vcc_reg);
- pm_runtime_set_suspended(&client->dev);
+ if (!acpi_dev_state_low_power(&client->dev)) {
+ if (!pm_runtime_status_suspended(&client->dev))
+ regulator_disable(at24->vcc_reg);
+ pm_runtime_set_suspended(&client->dev);
+ }
return 0;
}
@@ -834,6 +842,7 @@ static struct i2c_driver at24_driver = {
.probe_new = at24_probe,
.remove = at24_remove,
.id_table = at24_ids,
+ .flags = I2C_DRV_FL_ALLOW_LOW_POWER_PROBE,
};
static int __init at24_init(void)
--
2.20.1
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.
Also do identification on the first access of the device, whether in probe
or when starting streaming.
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/media/i2c/ov5670.c | 76 +++++++++++++++++++++++---------------
1 file changed, 46 insertions(+), 30 deletions(-)
diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 866c8c2e8f59a..eb0d599f3318b 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -1832,6 +1832,8 @@ struct ov5670 {
/* Streaming on/off */
bool streaming;
+ /* True if the device has been identified */
+ bool identified;
};
#define to_ov5670(_sd) container_of(_sd, struct ov5670, sd)
@@ -2271,6 +2273,32 @@ static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
return 0;
}
+/* Verify chip ID */
+static int ov5670_identify_module(struct ov5670 *ov5670)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
+ int ret;
+ u32 val;
+
+ if (ov5670->identified)
+ return 0;
+
+ ret = ov5670_read_reg(ov5670, OV5670_REG_CHIP_ID,
+ OV5670_REG_VALUE_24BIT, &val);
+ if (ret)
+ return ret;
+
+ if (val != OV5670_CHIP_ID) {
+ dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
+ OV5670_CHIP_ID, val);
+ return -ENXIO;
+ }
+
+ ov5670->identified = true;
+
+ return 0;
+}
+
/* Prepare streaming by writing default values and customized values */
static int ov5670_start_streaming(struct ov5670 *ov5670)
{
@@ -2279,6 +2307,10 @@ static int ov5670_start_streaming(struct ov5670 *ov5670)
int link_freq_index;
int ret;
+ ret = ov5670_identify_module(ov5670);
+ if (ret)
+ return ret;
+
/* Get out of from software reset */
ret = ov5670_write_reg(ov5670, OV5670_REG_SOFTWARE_RST,
OV5670_REG_VALUE_08BIT, OV5670_SOFTWARE_RST);
@@ -2400,27 +2432,6 @@ static int __maybe_unused ov5670_resume(struct device *dev)
return 0;
}
-/* Verify chip ID */
-static int ov5670_identify_module(struct ov5670 *ov5670)
-{
- struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
- int ret;
- u32 val;
-
- ret = ov5670_read_reg(ov5670, OV5670_REG_CHIP_ID,
- OV5670_REG_VALUE_24BIT, &val);
- if (ret)
- return ret;
-
- if (val != OV5670_CHIP_ID) {
- dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
- OV5670_CHIP_ID, val);
- return -ENXIO;
- }
-
- return 0;
-}
-
static const struct v4l2_subdev_video_ops ov5670_video_ops = {
.s_stream = ov5670_set_stream,
};
@@ -2455,6 +2466,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(&client->dev, "clock-frequency", &input_clk);
@@ -2471,11 +2483,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;
+ low_power = acpi_dev_state_low_power(&client->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(&ov5670->mutex);
@@ -2512,10 +2527,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(&client->dev);
+ if (!low_power)
+ pm_runtime_set_active(&client->dev);
pm_runtime_enable(&client->dev);
pm_runtime_idle(&client->dev);
@@ -2557,7 +2572,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 */ }
};
@@ -2572,6 +2587,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
From: Rajmohan Mani <[email protected]>
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 <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/media/i2c/imx319.c | 72 +++++++++++++++++++++++---------------
1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 8473c0bbb35d6..e0b22e9318fed 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -140,6 +140,8 @@ struct imx319 {
/* Streaming on/off */
bool streaming;
+ /* True if the device has been identified */
+ bool identified;
};
static const struct imx319_reg imx319_global_regs[] = {
@@ -2084,6 +2086,31 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
return 0;
}
+/* Verify chip ID */
+static int imx319_identify_module(struct imx319 *imx319)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
+ int ret;
+ u32 val;
+
+ if (imx319->identified)
+ return 0;
+
+ ret = imx319_read_reg(imx319, IMX319_REG_CHIP_ID, 2, &val);
+ if (ret)
+ return ret;
+
+ if (val != IMX319_CHIP_ID) {
+ dev_err(&client->dev, "chip id mismatch: %x!=%x",
+ IMX319_CHIP_ID, val);
+ return -EIO;
+ }
+
+ imx319->identified = true;
+
+ return 0;
+}
+
/* Start streaming */
static int imx319_start_streaming(struct imx319 *imx319)
{
@@ -2091,6 +2118,10 @@ static int imx319_start_streaming(struct imx319 *imx319)
const struct imx319_reg_list *reg_list;
int ret;
+ ret = imx319_identify_module(imx319);
+ if (ret)
+ return ret;
+
/* Global Setting */
reg_list = &imx319_global_setting;
ret = imx319_write_regs(imx319, reg_list->regs, reg_list->num_of_regs);
@@ -2208,26 +2239,6 @@ static int __maybe_unused imx319_resume(struct device *dev)
return ret;
}
-/* Verify chip ID */
-static int imx319_identify_module(struct imx319 *imx319)
-{
- struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
- int ret;
- u32 val;
-
- ret = imx319_read_reg(imx319, IMX319_REG_CHIP_ID, 2, &val);
- if (ret)
- return ret;
-
- if (val != IMX319_CHIP_ID) {
- dev_err(&client->dev, "chip id mismatch: %x!=%x",
- IMX319_CHIP_ID, val);
- return -EIO;
- }
-
- return 0;
-}
-
static const struct v4l2_subdev_core_ops imx319_subdev_core_ops = {
.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
.unsubscribe_event = v4l2_event_subdev_unsubscribe,
@@ -2422,6 +2433,7 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
static int imx319_probe(struct i2c_client *client)
{
struct imx319 *imx319;
+ bool low_power;
int ret;
u32 i;
@@ -2434,11 +2446,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;
+ low_power = acpi_dev_state_low_power(&client->dev);
+ if (!low_power) {
+ /* 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);
@@ -2491,10 +2506,10 @@ static int imx319_probe(struct i2c_client *client)
goto error_media_entity;
/*
- * 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(&client->dev);
+ if (!low_power)
+ pm_runtime_set_active(&client->dev);
pm_runtime_enable(&client->dev);
pm_runtime_idle(&client->dev);
@@ -2547,6 +2562,7 @@ static struct i2c_driver imx319_i2c_driver = {
},
.probe_new = imx319_probe,
.remove = imx319_remove,
+ .flags = I2C_DRV_FL_ALLOW_LOW_POWER_PROBE,
};
module_i2c_driver(imx319_i2c_driver);
--
2.20.1
Store a device's desired enumeration power state in struct
acpi_device_power_flags during acpi_device object's initialisation.
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/acpi/scan.c | 6 ++++++
include/acpi/acpi_bus.h | 3 ++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1d7a02ee45e05..b077c645c9845 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
static void acpi_bus_get_power_flags(struct acpi_device *device)
{
+ unsigned long long pre;
+ acpi_status status;
u32 i;
/* Presence of _PS0|_PR0 indicates 'power manageable' */
@@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
if (acpi_has_method(device->handle, "_DSW"))
device->power.flags.dsw_present = 1;
+ status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre);
+ if (ACPI_SUCCESS(status) && !pre)
+ device->power.flags.allow_low_power_probe = 1;
+
/*
* Enumerate supported power management states
*/
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 02a716a0af5d4..020b850120883 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -258,7 +258,8 @@ struct acpi_device_power_flags {
u32 power_removed:1; /* Optimize Dx->D0 */
u32 ignore_parent:1; /* Power is independent of parent power state */
u32 dsw_present:1; /* _DSW present? */
- u32 reserved:26;
+ u32 allow_low_power_probe:1; /* Allow low power state in device probe */
+ u32 reserved:25;
};
struct acpi_device_power_state {
--
2.20.1
Enable drivers to tell ACPI that there's no need to power on a device for
probe. Drivers should still perform this by themselves if there's a need
to. In some cases powering on the device during probe is undesirable, and
this change enables a driver to choose what fits best for it.
Add a field called "flags" into struct i2c_driver for driver flags, and a
flag I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to tell a driver supports probe in
low power state.
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/i2c/i2c-core-acpi.c | 10 ++++++++++
drivers/i2c/i2c-core-base.c | 9 ++++++---
include/linux/i2c.h | 19 +++++++++++++++++++
3 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 8ceaa88dd78fb..045d344dd0cb3 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -493,6 +493,16 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
}
EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
+bool i2c_acpi_allow_low_power_probe(struct device *dev)
+{
+ struct i2c_driver *driver = to_i2c_driver(dev->driver);
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+
+ return driver->flags & I2C_DRV_FL_ALLOW_LOW_POWER_PROBE &&
+ adev && adev->power.flags.allow_low_power_probe;
+}
+EXPORT_SYMBOL_GPL(i2c_acpi_allow_low_power_probe);
+
#ifdef CONFIG_ACPI_I2C_OPREGION
static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
u8 cmd, u8 *data, u8 data_len)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 63ebf722a4248..87b84eee01da6 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -514,7 +514,8 @@ static int i2c_device_probe(struct device *dev)
if (status < 0)
goto err_clear_wakeup_irq;
- status = dev_pm_domain_attach(&client->dev, true);
+ status = dev_pm_domain_attach(&client->dev,
+ !i2c_acpi_allow_low_power_probe(dev));
if (status)
goto err_clear_wakeup_irq;
@@ -536,7 +537,8 @@ static int i2c_device_probe(struct device *dev)
return 0;
err_detach_pm_domain:
- dev_pm_domain_detach(&client->dev, true);
+ dev_pm_domain_detach(&client->dev,
+ !i2c_acpi_allow_low_power_probe(dev));
err_clear_wakeup_irq:
dev_pm_clear_wake_irq(&client->dev);
device_init_wakeup(&client->dev, false);
@@ -563,7 +565,8 @@ static int i2c_device_remove(struct device *dev)
dev_warn(dev, "remove failed (%pe), will be ignored\n", ERR_PTR(status));
}
- dev_pm_domain_detach(&client->dev, true);
+ dev_pm_domain_detach(&client->dev,
+ !i2c_acpi_allow_low_power_probe(dev));
dev_pm_clear_wake_irq(&client->dev);
device_init_wakeup(&client->dev, false);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 56622658b2158..1a103c5933d2f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -11,6 +11,7 @@
#define _LINUX_I2C_H
#include <linux/acpi.h> /* for acpi_handle */
+#include <linux/bits.h>
#include <linux/mod_devicetable.h>
#include <linux/device.h> /* for struct device */
#include <linux/sched.h> /* for completion */
@@ -217,6 +218,16 @@ enum i2c_alert_protocol {
I2C_PROTOCOL_SMBUS_HOST_NOTIFY,
};
+/**
+ * enum i2c_driver_flags - Flags for an I2C device driver
+ *
+ * @I2C_DRV_FL_ALLOW_LOW_POWER_PROBE: Let the ACPI driver manage the device's
+ * power state during probe and remove
+ */
+enum i2c_driver_flags {
+ I2C_DRV_FL_ALLOW_LOW_POWER_PROBE = BIT(0),
+};
+
/**
* struct i2c_driver - represent an I2C device driver
* @class: What kind of i2c device we instantiate (for detect)
@@ -231,6 +242,7 @@ enum i2c_alert_protocol {
* @detect: Callback for device detection
* @address_list: The I2C addresses to probe (for detect)
* @clients: List of detected clients we created (for i2c-core use only)
+ * @flags: A bitmask of flags defined in &enum i2c_driver_flags
*
* The driver.owner field should be set to the module owner of this driver.
* The driver.name field should be set to the name of this driver.
@@ -289,6 +301,8 @@ struct i2c_driver {
int (*detect)(struct i2c_client *client, struct i2c_board_info *info);
const unsigned short *address_list;
struct list_head clients;
+
+ u32 flags;
};
#define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
@@ -996,6 +1010,7 @@ u32 i2c_acpi_find_bus_speed(struct device *dev);
struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
struct i2c_board_info *info);
struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
+bool i2c_acpi_allow_low_power_probe(struct device *dev);
#else
static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
struct acpi_resource_i2c_serialbus **i2c)
@@ -1015,6 +1030,10 @@ static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle ha
{
return NULL;
}
+static inline bool i2c_acpi_allow_low_power_probe(struct device *dev)
+{
+ return false;
+}
#endif /* CONFIG_ACPI */
#endif /* _LINUX_I2C_H */
--
2.20.1
Reviewed-by: Bingbu Cao <[email protected]>
On 1/29/21 7:27 AM, Sakari Ailus wrote:
> From: Rajmohan Mani <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Sakari Ailus <[email protected]>
> ---
> drivers/media/i2c/imx319.c | 72 +++++++++++++++++++++++---------------
> 1 file changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> index 8473c0bbb35d6..e0b22e9318fed 100644
> --- a/drivers/media/i2c/imx319.c
> +++ b/drivers/media/i2c/imx319.c
> @@ -140,6 +140,8 @@ struct imx319 {
>
> /* Streaming on/off */
> bool streaming;
> + /* True if the device has been identified */
> + bool identified;
> };
>
> static const struct imx319_reg imx319_global_regs[] = {
> @@ -2084,6 +2086,31 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
> return 0;
> }
>
> +/* Verify chip ID */
> +static int imx319_identify_module(struct imx319 *imx319)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
> + int ret;
> + u32 val;
> +
> + if (imx319->identified)
> + return 0;
> +
> + ret = imx319_read_reg(imx319, IMX319_REG_CHIP_ID, 2, &val);
> + if (ret)
> + return ret;
> +
> + if (val != IMX319_CHIP_ID) {
> + dev_err(&client->dev, "chip id mismatch: %x!=%x",
> + IMX319_CHIP_ID, val);
> + return -EIO;
> + }
> +
> + imx319->identified = true;
> +
> + return 0;
> +}
> +
> /* Start streaming */
> static int imx319_start_streaming(struct imx319 *imx319)
> {
> @@ -2091,6 +2118,10 @@ static int imx319_start_streaming(struct imx319 *imx319)
> const struct imx319_reg_list *reg_list;
> int ret;
>
> + ret = imx319_identify_module(imx319);
> + if (ret)
> + return ret;
> +
> /* Global Setting */
> reg_list = &imx319_global_setting;
> ret = imx319_write_regs(imx319, reg_list->regs, reg_list->num_of_regs);
> @@ -2208,26 +2239,6 @@ static int __maybe_unused imx319_resume(struct device *dev)
> return ret;
> }
>
> -/* Verify chip ID */
> -static int imx319_identify_module(struct imx319 *imx319)
> -{
> - struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
> - int ret;
> - u32 val;
> -
> - ret = imx319_read_reg(imx319, IMX319_REG_CHIP_ID, 2, &val);
> - if (ret)
> - return ret;
> -
> - if (val != IMX319_CHIP_ID) {
> - dev_err(&client->dev, "chip id mismatch: %x!=%x",
> - IMX319_CHIP_ID, val);
> - return -EIO;
> - }
> -
> - return 0;
> -}
> -
> static const struct v4l2_subdev_core_ops imx319_subdev_core_ops = {
> .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> @@ -2422,6 +2433,7 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
> static int imx319_probe(struct i2c_client *client)
> {
> struct imx319 *imx319;
> + bool low_power;
> int ret;
> u32 i;
>
> @@ -2434,11 +2446,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;
> + low_power = acpi_dev_state_low_power(&client->dev);
> + if (!low_power) {
> + /* 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);
> @@ -2491,10 +2506,10 @@ static int imx319_probe(struct i2c_client *client)
> goto error_media_entity;
>
> /*
> - * 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(&client->dev);
> + if (!low_power)
> + pm_runtime_set_active(&client->dev);
> pm_runtime_enable(&client->dev);
> pm_runtime_idle(&client->dev);
>
> @@ -2547,6 +2562,7 @@ static struct i2c_driver imx319_i2c_driver = {
> },
> .probe_new = imx319_probe,
> .remove = imx319_remove,
> + .flags = I2C_DRV_FL_ALLOW_LOW_POWER_PROBE,
> };
> module_i2c_driver(imx319_i2c_driver);
>
>
--
Best regards,
Bingbu Cao
Hi Rafael,
Thanks for the comments.
On Fri, Jan 29, 2021 at 03:07:57PM +0100, Rafael J. Wysocki wrote:
> On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus
> <[email protected]> wrote:
> >
> > Store a device's desired enumeration power state in struct
> > acpi_device_power_flags during acpi_device object's initialisation.
> >
> > Signed-off-by: Sakari Ailus <[email protected]>
> > ---
> > drivers/acpi/scan.c | 6 ++++++
> > include/acpi/acpi_bus.h | 3 ++-
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 1d7a02ee45e05..b077c645c9845 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
> >
> > static void acpi_bus_get_power_flags(struct acpi_device *device)
> > {
> > + unsigned long long pre;
> > + acpi_status status;
> > u32 i;
> >
> > /* Presence of _PS0|_PR0 indicates 'power manageable' */
> > @@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
> > if (acpi_has_method(device->handle, "_DSW"))
> > device->power.flags.dsw_present = 1;
> >
> > + status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre);
> > + if (ACPI_SUCCESS(status) && !pre)
> > + device->power.flags.allow_low_power_probe = 1;
>
> While this is what has been discussed and thanks for taking it into
> account, I'm now thinking that it may be cleaner to introduce a new
> object to return the deepest power state of the device in which it can
> be enumerated, say _DSE (Device State for Enumeration) such that 4
> means D3cold, 3 - D3hot and so on, so the above check can be replaced
> with something like
>
> status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse);
s/_PRE/_DSE/
?
> if (ACPI_FAILURE(status))
ACPI_SUCCESS?
> device->power.state_for_enumeratin = dse;
>
> And then, it is a matter of comparing ->power.state_for_enumeratin
> with ->power.state and putting the device into D0 if the former is
> shallower than the latter.
>
> What do you think?
Sounds good. How about calling the function e.g.
acpi_device_resume_for_probe(), so runtime PM could be used to resume the
device if the function returns true?
--
Kind regards,
Sakari Ailus
On Fri, Jan 29, 2021 at 5:45 PM Sakari Ailus
<[email protected]> wrote:
>
> Hi Rafael,
>
> Thanks for the comments.
>
> On Fri, Jan 29, 2021 at 03:07:57PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus
> > <[email protected]> wrote:
> > >
> > > Store a device's desired enumeration power state in struct
> > > acpi_device_power_flags during acpi_device object's initialisation.
> > >
> > > Signed-off-by: Sakari Ailus <[email protected]>
> > > ---
> > > drivers/acpi/scan.c | 6 ++++++
> > > include/acpi/acpi_bus.h | 3 ++-
> > > 2 files changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 1d7a02ee45e05..b077c645c9845 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
> > >
> > > static void acpi_bus_get_power_flags(struct acpi_device *device)
> > > {
> > > + unsigned long long pre;
> > > + acpi_status status;
> > > u32 i;
> > >
> > > /* Presence of _PS0|_PR0 indicates 'power manageable' */
> > > @@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
> > > if (acpi_has_method(device->handle, "_DSW"))
> > > device->power.flags.dsw_present = 1;
> > >
> > > + status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre);
> > > + if (ACPI_SUCCESS(status) && !pre)
> > > + device->power.flags.allow_low_power_probe = 1;
> >
> > While this is what has been discussed and thanks for taking it into
> > account, I'm now thinking that it may be cleaner to introduce a new
> > object to return the deepest power state of the device in which it can
> > be enumerated, say _DSE (Device State for Enumeration) such that 4
> > means D3cold, 3 - D3hot and so on, so the above check can be replaced
> > with something like
> >
> > status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse);
>
> s/_PRE/_DSE/
>
> ?
Yes, sorry.
>
> > if (ACPI_FAILURE(status))
>
> ACPI_SUCCESS?
Yup.
> > device->power.state_for_enumeratin = dse;
> >
> > And then, it is a matter of comparing ->power.state_for_enumeratin
> > with ->power.state and putting the device into D0 if the former is
> > shallower than the latter.
> >
> > What do you think?
>
> Sounds good. How about calling the function e.g.
> acpi_device_resume_for_probe(), so runtime PM could be used to resume the
> device if the function returns true?
I'd rather try to power it up before enabling runtime PM, because in
order to do the latter properly, you need to know if the device is
active or suspended to start with.
So you need something like (pseudo-code)
if (this_device_needs_to_be_on(ACPI_COMPANION(dev))) {
acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0);
pm_runtime_set_active(dev);
} else {
pm_runtime_set_suspended(dev);
}
and then you can enable PM-runtime.
On Fri, Jan 29, 2021 at 05:57:17PM +0100, Rafael J. Wysocki wrote:
> On Fri, Jan 29, 2021 at 5:45 PM Sakari Ailus
> <[email protected]> wrote:
> >
> > Hi Rafael,
> >
> > Thanks for the comments.
> >
> > On Fri, Jan 29, 2021 at 03:07:57PM +0100, Rafael J. Wysocki wrote:
> > > On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus
> > > <[email protected]> wrote:
> > > >
> > > > Store a device's desired enumeration power state in struct
> > > > acpi_device_power_flags during acpi_device object's initialisation.
> > > >
> > > > Signed-off-by: Sakari Ailus <[email protected]>
> > > > ---
> > > > drivers/acpi/scan.c | 6 ++++++
> > > > include/acpi/acpi_bus.h | 3 ++-
> > > > 2 files changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > > index 1d7a02ee45e05..b077c645c9845 100644
> > > > --- a/drivers/acpi/scan.c
> > > > +++ b/drivers/acpi/scan.c
> > > > @@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
> > > >
> > > > static void acpi_bus_get_power_flags(struct acpi_device *device)
> > > > {
> > > > + unsigned long long pre;
> > > > + acpi_status status;
> > > > u32 i;
> > > >
> > > > /* Presence of _PS0|_PR0 indicates 'power manageable' */
> > > > @@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
> > > > if (acpi_has_method(device->handle, "_DSW"))
> > > > device->power.flags.dsw_present = 1;
> > > >
> > > > + status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre);
> > > > + if (ACPI_SUCCESS(status) && !pre)
> > > > + device->power.flags.allow_low_power_probe = 1;
> > >
> > > While this is what has been discussed and thanks for taking it into
> > > account, I'm now thinking that it may be cleaner to introduce a new
> > > object to return the deepest power state of the device in which it can
> > > be enumerated, say _DSE (Device State for Enumeration) such that 4
> > > means D3cold, 3 - D3hot and so on, so the above check can be replaced
> > > with something like
> > >
> > > status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse);
> >
> > s/_PRE/_DSE/
> >
> > ?
>
> Yes, sorry.
>
> >
> > > if (ACPI_FAILURE(status))
> >
> > ACPI_SUCCESS?
>
> Yup.
>
> > > device->power.state_for_enumeratin = dse;
> > >
> > > And then, it is a matter of comparing ->power.state_for_enumeratin
> > > with ->power.state and putting the device into D0 if the former is
> > > shallower than the latter.
> > >
> > > What do you think?
> >
> > Sounds good. How about calling the function e.g.
> > acpi_device_resume_for_probe(), so runtime PM could be used to resume the
> > device if the function returns true?
>
> I'd rather try to power it up before enabling runtime PM, because in
> order to do the latter properly, you need to know if the device is
> active or suspended to start with.
>
> So you need something like (pseudo-code)
>
> if (this_device_needs_to_be_on(ACPI_COMPANION(dev))) {
> acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0);
> pm_runtime_set_active(dev);
> } else {
> pm_runtime_set_suspended(dev);
I guess the else branch isn't needed? The device remains suspended if its
state hasn't been changed.
> }
>
> and then you can enable PM-runtime.
Yes, agreed, this is what drivers should do. The I?C framework would use
the function and conditionally power the device on before enabling runtime
PM.
This is how it's implemented by the set already but I think the change in
semantics requires a little more still.
--
Sakari Ailus
On Fri, Jan 29, 2021 at 1:20 PM Sakari Ailus
<[email protected]> wrote:
>
> Hi Bartosz,
>
> Thanks for the review.
>
> On Fri, Jan 29, 2021 at 11:56:00AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus
> > <[email protected]> wrote:
> > >
> > > 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 <[email protected]>
> > > ---
> > > drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
> > > 1 file changed, 26 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > > index 926408b41270c..dd0b3f24e3808 100644
> > > --- a/drivers/misc/eeprom/at24.c
> > > +++ b/drivers/misc/eeprom/at24.c
> > > @@ -595,6 +595,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 low_power;
> > > struct regmap *regmap;
> > > bool writable;
> > > u8 test_byte;
> > > @@ -750,14 +751,16 @@ static int at24_probe(struct i2c_client *client)
> > >
> > > i2c_set_clientdata(client, at24);
> > >
> > > - err = regulator_enable(at24->vcc_reg);
> > > - if (err) {
> > > - dev_err(dev, "Failed to enable vcc regulator\n");
> > > - return err;
> > > - }
> > > + low_power = acpi_dev_state_low_power(&client->dev);
> >
> > I've raised my concern about the naming of this before but no
> > discussion followed. Do we really want to name it: "low power"? This
> > is misleading as the device can actually be powered off at probe().
> > "Low power" suggests some low-power state or even low battery IMO.
>
> This was suggested by Rafael in place of "powered off" as it's not know the
> device is powered off. The same terms should be used in all contexts (ACPI
> and I²C frameworks and drivers). Others haven't expressed concerns.
>
So we're describing a situation where "device may be powered off" by
calling it "low_power". This doesn't make sense. Why not something
like: acpi_dev_may_be_off(), acpi_dev_powerdown_possible(),
acpi_dev_possibly_off(). If I'm reading a driver's code an see
"acpi_dev_state_low_power()", I would have never guessed it refers to
a situation where the device may be potentially powered-down.
> ACPI spec appears to be using terms "on" and "off".
>
> The use of the function is not limited to driver probe time.
>
> >
> > If anything: I'd prefer the 'low_power' local variable be changed to
> > "no_test_read".
>
> That misses the power management related suggestion now present in the name
> --- the device needs to be suspended using runtime PM if probe fails and
> it's not in "low power state".
>
> How about "off_during_probe"?
>
Yes, this is much better than low_power.
Bartosz
> --
> Kind regards,
>
> Sakari Ailus
On Fri, Jan 29, 2021 at 10:22 PM Sakari Ailus
<[email protected]> wrote:
>
> On Fri, Jan 29, 2021 at 05:57:17PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Jan 29, 2021 at 5:45 PM Sakari Ailus
> > <[email protected]> wrote:
> > >
> > > Hi Rafael,
> > >
> > > Thanks for the comments.
> > >
> > > On Fri, Jan 29, 2021 at 03:07:57PM +0100, Rafael J. Wysocki wrote:
> > > > On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus
> > > > <[email protected]> wrote:
> > > > >
> > > > > Store a device's desired enumeration power state in struct
> > > > > acpi_device_power_flags during acpi_device object's initialisation.
> > > > >
> > > > > Signed-off-by: Sakari Ailus <[email protected]>
> > > > > ---
> > > > > drivers/acpi/scan.c | 6 ++++++
> > > > > include/acpi/acpi_bus.h | 3 ++-
> > > > > 2 files changed, 8 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > > > index 1d7a02ee45e05..b077c645c9845 100644
> > > > > --- a/drivers/acpi/scan.c
> > > > > +++ b/drivers/acpi/scan.c
> > > > > @@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
> > > > >
> > > > > static void acpi_bus_get_power_flags(struct acpi_device *device)
> > > > > {
> > > > > + unsigned long long pre;
> > > > > + acpi_status status;
> > > > > u32 i;
> > > > >
> > > > > /* Presence of _PS0|_PR0 indicates 'power manageable' */
> > > > > @@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
> > > > > if (acpi_has_method(device->handle, "_DSW"))
> > > > > device->power.flags.dsw_present = 1;
> > > > >
> > > > > + status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre);
> > > > > + if (ACPI_SUCCESS(status) && !pre)
> > > > > + device->power.flags.allow_low_power_probe = 1;
> > > >
> > > > While this is what has been discussed and thanks for taking it into
> > > > account, I'm now thinking that it may be cleaner to introduce a new
> > > > object to return the deepest power state of the device in which it can
> > > > be enumerated, say _DSE (Device State for Enumeration) such that 4
> > > > means D3cold, 3 - D3hot and so on, so the above check can be replaced
> > > > with something like
> > > >
> > > > status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse);
> > >
> > > s/_PRE/_DSE/
> > >
> > > ?
> >
> > Yes, sorry.
> >
> > >
> > > > if (ACPI_FAILURE(status))
> > >
> > > ACPI_SUCCESS?
> >
> > Yup.
> >
> > > > device->power.state_for_enumeratin = dse;
> > > >
> > > > And then, it is a matter of comparing ->power.state_for_enumeratin
> > > > with ->power.state and putting the device into D0 if the former is
> > > > shallower than the latter.
> > > >
> > > > What do you think?
> > >
> > > Sounds good. How about calling the function e.g.
> > > acpi_device_resume_for_probe(), so runtime PM could be used to resume the
> > > device if the function returns true?
> >
> > I'd rather try to power it up before enabling runtime PM, because in
> > order to do the latter properly, you need to know if the device is
> > active or suspended to start with.
> >
> > So you need something like (pseudo-code)
> >
> > if (this_device_needs_to_be_on(ACPI_COMPANION(dev))) {
> > acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0);
> > pm_runtime_set_active(dev);
> > } else {
> > pm_runtime_set_suspended(dev);
>
> I guess the else branch isn't needed? The device remains suspended if its
> state hasn't been changed.
Assuming that the initial status is always "suspended", the else
branch is not needed.
> > }
> >
> > and then you can enable PM-runtime.
>
> Yes, agreed, this is what drivers should do. The I涎 framework would use
> the function and conditionally power the device on before enabling runtime
> PM.
>
> This is how it's implemented by the set already but I think the change in
> semantics requires a little more still.
Agreed.