diff mbox series

[v2,3/7] ACPI: Documentation: Document acpi_dev_state_d0()

Message ID 20231117111433.1561669-4-sakari.ailus@linux.intel.com
State New
Headers show
Series Small Runtime PM API changes | expand

Commit Message

Sakari Ailus Nov. 17, 2023, 11:14 a.m. UTC
Document that acpi_dev_state_d0() can be used to tell if the device was
powered on for probe.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Laurent Pinchart Nov. 18, 2023, 6:50 p.m. UTC | #1
Hi Sakari,

Thank you for the patch.

On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> Document that acpi_dev_state_d0() can be used to tell if the device was
> powered on for probe.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> index 7afd16701a02..815bcc8db69f 100644
> --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> @@ -24,6 +24,14 @@ 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.
>  
> +ACPI framework
> +--------------
> +
> +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> +returns true if the device is powered on, false otherwise. For non-ACPI backed
> +devices it returns true always.
> +

While this is true, I don't want to see drivers having to call
ACPI-specific functions, the same way you dislike OF-specific functions
in drivers. Please find a better way to handle this.

>  I²C
>  ---
>
Rafael J. Wysocki Nov. 20, 2023, 12:52 p.m. UTC | #2
On Mon, Nov 20, 2023 at 10:31 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Laurent,
>
> On Sat, Nov 18, 2023 at 08:50:49PM +0200, Laurent Pinchart wrote:
> > Hi Sakari,
> >
> > Thank you for the patch.
> >
> > On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> > > Document that acpi_dev_state_d0() can be used to tell if the device was
> > > powered on for probe.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > index 7afd16701a02..815bcc8db69f 100644
> > > --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > @@ -24,6 +24,14 @@ 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.
> > >
> > > +ACPI framework
> > > +--------------
> > > +
> > > +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> > > +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> > > +returns true if the device is powered on, false otherwise. For non-ACPI backed
> > > +devices it returns true always.
> > > +
> >
> > While this is true, I don't want to see drivers having to call
> > ACPI-specific functions, the same way you dislike OF-specific functions
> > in drivers. Please find a better way to handle this.
>
> The functionality is only available on ACPI and the function does the right
> thing on non-ACPI platforms. I don't see an issue here.

The issue would be calling an ACPI-specific function from code that's
otherwise firmware-agnostic, AFAICS.

It would be good to have a more generic way of checking whether or not
a device is operational.

> Feel free to post DT binding patches on suggested device power state during
> probe. :-) I think DT would benefit from this as well: the at24 driver is
> widely used and suddenly making probe() not talk to the chip (or even power
> it up) at all would probably be seen as a regression.

In the DT case it is more complicated, though, at least in general,
because there may be multiple clocks and regulators the device depends
on and you may need to toggle a GPIO line too.
Sakari Ailus Nov. 20, 2023, 8:03 p.m. UTC | #3
Hi Rafael,

On Mon, Nov 20, 2023 at 01:52:39PM +0100, Rafael J. Wysocki wrote:
> On Mon, Nov 20, 2023 at 10:31 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Laurent,
> >
> > On Sat, Nov 18, 2023 at 08:50:49PM +0200, Laurent Pinchart wrote:
> > > Hi Sakari,
> > >
> > > Thank you for the patch.
> > >
> > > On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> > > > Document that acpi_dev_state_d0() can be used to tell if the device was
> > > > powered on for probe.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > index 7afd16701a02..815bcc8db69f 100644
> > > > --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > @@ -24,6 +24,14 @@ 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.
> > > >
> > > > +ACPI framework
> > > > +--------------
> > > > +
> > > > +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> > > > +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> > > > +returns true if the device is powered on, false otherwise. For non-ACPI backed
> > > > +devices it returns true always.
> > > > +
> > >
> > > While this is true, I don't want to see drivers having to call
> > > ACPI-specific functions, the same way you dislike OF-specific functions
> > > in drivers. Please find a better way to handle this.
> >
> > The functionality is only available on ACPI and the function does the right
> > thing on non-ACPI platforms. I don't see an issue here.
> 
> The issue would be calling an ACPI-specific function from code that's
> otherwise firmware-agnostic, AFAICS.
> 
> It would be good to have a more generic way of checking whether or not
> a device is operational.

In DT case it's up to the driver to do that, so the device is powered off.

> 
> > Feel free to post DT binding patches on suggested device power state during
> > probe. :-) I think DT would benefit from this as well: the at24 driver is
> > widely used and suddenly making probe() not talk to the chip (or even power
> > it up) at all would probably be seen as a regression.
> 
> In the DT case it is more complicated, though, at least in general,
> because there may be multiple clocks and regulators the device depends
> on and you may need to toggle a GPIO line too.

I don't think it is as what's missing is the desired power state during
probe, i.e. whether or not the device will be powered on. It wasn't there
in ACPI either before it was added.

The problem is slightly lesser on DT though as it's up to the driver
whether or not to power on the device. In the example I gave above,
however, e.g. the at24 driver can't be modified to keep the device powered
off and at the same time expected people would remain content with it. So
this information should come from firmware.
Rafael J. Wysocki Nov. 20, 2023, 8:22 p.m. UTC | #4
Hi Sakari,

On Mon, Nov 20, 2023 at 9:03 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Mon, Nov 20, 2023 at 01:52:39PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Nov 20, 2023 at 10:31 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Laurent,
> > >
> > > On Sat, Nov 18, 2023 at 08:50:49PM +0200, Laurent Pinchart wrote:
> > > > Hi Sakari,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> > > > > Document that acpi_dev_state_d0() can be used to tell if the device was
> > > > > powered on for probe.
> > > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > index 7afd16701a02..815bcc8db69f 100644
> > > > > --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > @@ -24,6 +24,14 @@ 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.
> > > > >
> > > > > +ACPI framework
> > > > > +--------------
> > > > > +
> > > > > +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> > > > > +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> > > > > +returns true if the device is powered on, false otherwise. For non-ACPI backed
> > > > > +devices it returns true always.
> > > > > +
> > > >
> > > > While this is true, I don't want to see drivers having to call
> > > > ACPI-specific functions, the same way you dislike OF-specific functions
> > > > in drivers. Please find a better way to handle this.
> > >
> > > The functionality is only available on ACPI and the function does the right
> > > thing on non-ACPI platforms. I don't see an issue here.
> >
> > The issue would be calling an ACPI-specific function from code that's
> > otherwise firmware-agnostic, AFAICS.
> >
> > It would be good to have a more generic way of checking whether or not
> > a device is operational.
>
> In DT case it's up to the driver to do that, so the device is powered off.

Unless the boot loader (or whatever happens to run before the kernel)
turns it on for some reason (whatever that may be).

I guess the original point has been that in the ACPI case the generic
enumeration code may power up the device if not instructed otherwise
by the platform firmware, whereas in the DT case this is entirely up
to the driver, but I'm not sure if this really matters.

I would suggest adding a generic wrapper around acpi_dev_state_d0()
that will just always return true in the DT case, something like
device_is_accessible() or device_is_operational(), that can be invoked
from generic code without any visible ACPI connotations.
Sakari Ailus Nov. 20, 2023, 8:53 p.m. UTC | #5
Hi Rafael,

On Mon, Nov 20, 2023 at 09:22:53PM +0100, Rafael J. Wysocki wrote:
> Hi Sakari,
> 
> On Mon, Nov 20, 2023 at 9:03 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Mon, Nov 20, 2023 at 01:52:39PM +0100, Rafael J. Wysocki wrote:
> > > On Mon, Nov 20, 2023 at 10:31 AM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Laurent,
> > > >
> > > > On Sat, Nov 18, 2023 at 08:50:49PM +0200, Laurent Pinchart wrote:
> > > > > Hi Sakari,
> > > > >
> > > > > Thank you for the patch.
> > > > >
> > > > > On Fri, Nov 17, 2023 at 01:14:29PM +0200, Sakari Ailus wrote:
> > > > > > Document that acpi_dev_state_d0() can be used to tell if the device was
> > > > > > powered on for probe.
> > > > > >
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > ---
> > > > > >  Documentation/firmware-guide/acpi/non-d0-probe.rst | 8 ++++++++
> > > > > >  1 file changed, 8 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > > index 7afd16701a02..815bcc8db69f 100644
> > > > > > --- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > > +++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
> > > > > > @@ -24,6 +24,14 @@ 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.
> > > > > >
> > > > > > +ACPI framework
> > > > > > +--------------
> > > > > > +
> > > > > > +Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
> > > > > > +whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
> > > > > > +returns true if the device is powered on, false otherwise. For non-ACPI backed
> > > > > > +devices it returns true always.
> > > > > > +
> > > > >
> > > > > While this is true, I don't want to see drivers having to call
> > > > > ACPI-specific functions, the same way you dislike OF-specific functions
> > > > > in drivers. Please find a better way to handle this.
> > > >
> > > > The functionality is only available on ACPI and the function does the right
> > > > thing on non-ACPI platforms. I don't see an issue here.
> > >
> > > The issue would be calling an ACPI-specific function from code that's
> > > otherwise firmware-agnostic, AFAICS.
> > >
> > > It would be good to have a more generic way of checking whether or not
> > > a device is operational.
> >
> > In DT case it's up to the driver to do that, so the device is powered off.
> 
> Unless the boot loader (or whatever happens to run before the kernel)
> turns it on for some reason (whatever that may be).

There are probably some exceptions but they should be quite rare.

If the boot loader already powered on the device, then it'd be no use
avoiding accessing it. That doesn't mean the rest of the device shouldn't
be accessed though.

> 
> I guess the original point has been that in the ACPI case the generic
> enumeration code may power up the device if not instructed otherwise
> by the platform firmware, whereas in the DT case this is entirely up
> to the driver, but I'm not sure if this really matters.
> 
> I would suggest adding a generic wrapper around acpi_dev_state_d0()
> that will just always return true in the DT case, something like
> device_is_accessible() or device_is_operational(), that can be invoked
> from generic code without any visible ACPI connotations.

The DT case may need a different API for that: telling whether the device
should be powered on for probe (by the driver) rather what
acpi_dev_state_d0() does.

And on ACPI we've only needed this for I²C but likely I3C will follow. It
appears to be lacking ACPI support at the moment.
diff mbox series

Patch

diff --git a/Documentation/firmware-guide/acpi/non-d0-probe.rst b/Documentation/firmware-guide/acpi/non-d0-probe.rst
index 7afd16701a02..815bcc8db69f 100644
--- a/Documentation/firmware-guide/acpi/non-d0-probe.rst
+++ b/Documentation/firmware-guide/acpi/non-d0-probe.rst
@@ -24,6 +24,14 @@  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.
 
+ACPI framework
+--------------
+
+Use the Linux ACPI framework function :c:func:`acpi_dev_state_d0()` to tell
+whether the device was powered on for probe. :c:func:`acpi_dev_state_d0()`
+returns true if the device is powered on, false otherwise. For non-ACPI backed
+devices it returns true always.
+
 I²C
 ---