Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
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 wrote: > > > > On Sun, Sep 27, 2020 at 9:44 PM Tomasz Figa wrote: > > > > > > On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang 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
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
On Mon, Sep 28, 2020 at 4:18 PM Rafael J. Wysocki wrote: > > On Sun, Sep 27, 2020 at 9:44 PM Tomasz Figa wrote: > > > > On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang 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
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
On Sun, Sep 27, 2020 at 9:44 PM Tomasz Figa wrote: > > On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang 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.
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang 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.
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
> 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. signature.asc Description: PGP signature
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
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 Best regards, Tomasz
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
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(>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
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
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(>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
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
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(>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
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
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(>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.
>
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
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(>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
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
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
Re: [PATCH v8 0/6] Support running driver's probe for a device powered off
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
