Re: New ACPI power resource code for testing

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

On Mon, 1 Mar 2010, Jukka Ruohonen wrote:

> > 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...
>
> That could be done, yes. Though these are only DPRINTFs...
>
> I thought about requiring a device_t as a parameter to all exposed
> functions, but then again, it is not entirely clear if all possible users of
> power resources are actual devices with a device_t. So, perhaps no.

It would be misleading for the acpi_power device to report messages 
(even if they are only DPRINTFs) that purport to come from another 
device.  I think it would be better to simply use %s/__func__

> > In acpi_power_switch(), since variable "on" is a boolean, why compare it
> > to another boolean?
> >
> >         if (on != false) ...
> >    vs
> >         if (on) ...
>
> This is just my personal coding style. I always like to explicitly compare
> things, and despise a style that, say, does things like (!p), especially if
> pointers are involved. But each to their own.

I agree, except for boolean variables!

> > 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.
>
> Yes, these attach as normal ACPI devices (the version you reviewed earlier
> did not do this). I thought this would have some benefits (e.g. easier
> modularization and possible integration with e.g. pmf(9), etc.), but
> changing this is only a matter of small refactoring.
>
> Yet, conceptually these are sort-of "[device] nodes", even though the
> current power resource code is builtin to the core ACPI.
>
> > (Would it be valid to use RUN_ONCE(9) here?  And then call this from
> > the attach routine?)
>
> I thought about this, but it won't fly as the globals need to be initialized
> even if no power resources are available.

Yup - good point.

> > 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.
>
> Yes, it is possible, even likely. Perhaps some autoconfiguration glue would
> help here?

Well, since there isn't any way for our configuration tree to indicate 
"sibling dependency", I would suggest moving the power stuff back into 
the generic acpi framework rather than making it a separate device?

-------------------------------------------------------------------------
|   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  |
-------------------------------------------------------------------------