Re: New ACPI power resource code for testing

[Paul Goyette <paul%whooppee.com@localhost>]

Comments on code:

May I suggest that you change the comment for acpi_power_init() from

        * Must be called before attachment of ACPI device nodes.

to

        * Called from acpi framework before attachment of ACPI
        * device nodes

In acpi_power_register() the DPRINTF(()) hardwires the text "acpi0" in 
the message;  might be better to grab this from the current device's 
parent?  Same for several other DPRINTF(())s...

In acpi_power_switch(), since variable "on" is a boolean, why compare it 
to another boolean?

        if (on != false) ...
   vs
        if (on) ...

(Similar comment for the call to acpi_power_set() in the patches to 
acpi_tz.c ...)


Comments on design:

As you've currently set things up, the user MUST include

        acpipower* at acpi?

in their config file if they have acpi at all - even if their system 
does not have any acpipower resources.  (Otherwise, the call to 
acpi_power_init() will refer to an undefined symbol.)  Either this 
requirement should be removed, or it should be explicitly documented.

(Would it be valid to use RUN_ONCE(9) here?  And then call this from 
the attach routine?)

Unfortunately, even if you resolve the above issue, there's another 
dependency with acpitz.  Is it conceivable that an acpitz can be fully 
functional without needing the acpipower device?  (I'm thinking of 
passive-only situations.)  If so, then the dependency should be removed.




On Mon, 1 Mar 2010, Jukka Ruohonen wrote:

> Hi.
>
> I have (re)written the ACPI power resource code.
>
> This tries to overcome some bugs and limitations in the old code. To name
> few issues, the old code lacks locking, never removes references to power
> resources, does not support retrieving the power state, does not support
> some newer control methods, contains some small bugs, among other thing.
> (This is not to say that the old code is bad, and Michael Smith's name still
> appears in the copyright.) The new code also tries to act more as a
> small-scale "framework".
>
> Currently ACPI power resources are used only by acpitz(4). But as power
> resources hopefully some day play a part in the overall x86 power management
> scheme(s), now would be a good time to consider the issue.
>
> Please test if you have time. How?
>
>         1. Drop the code ("acpi_power.c") to "src/sys/dev/acpi".[1]
>         2. Apply a patch ("acpi_power.diff").[2]
>         3. Add "acpipower* at acpinodebus?" to your kernel configuration file.
>
> On some laptops acpitz(4) uses these resources to make a switch to "active
> cooling", so possible regressions should be visible there (fan does not turn
> on/off, etc.). Otherwise the code should just, well, sit and idle.
>
> Comments are welcome too, of course.
>
> - Jukka.
>
> PS. Thanks to Paul Goyette for early review.
>
> [1] ftp://ftp.NetBSD.org/pub/NetBSD/misc/jruoho/acpi_power.c
> [2] ftp://ftp.NetBSD.org/pub/NetBSD/misc/jruoho/acpi_power.diff

-------------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:      |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul at whooppee.com   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer |                          | pgoyette at netbsd.org  |
-------------------------------------------------------------------------