| Message ID | 20231117111433.1561669-4-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Small Runtime PM API changes | expand |
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 > --- >
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. 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. > > > I²C > > --- > > >
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.
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.
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.
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 --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 ---
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(+)