hal: 2 commits: fix up introspection bits

[David Zeuthen]

Hi,

Sorry for the lag - been traveling,

On Thu, 2007-05-10 at 23:37 +0200, Holger Macht wrote:
> Hi David,
> 
> Danny pointed me to some inconsistencies within the spec and the CPU
> frequency scaling addon. The introspection bits state that all three
> SetCPUFreq* methods return an integer. That's not true. That remained from
> the initial design is not true anymore. The three methods do not return
> anything and only throw an error if something goes wrong. I'm not sure how
> exactly to express this in the xml section. Suggestions welcome. Appended
> patch just removes the return value information from the c file.
> 
> When you fixed up the spec with the below quoted patch, you took this
> wrong information along to the spec. Patch fixes this, too.
> 
> Some further comments below. Sorry for not answering earlier, but I
> initially thought you just did some copy&paste magic without rephrasing
> sentences.
> 
> On Sat 24. Mar - 22:53:41, David Zeuthen wrote:
> [...]
> > +  <sect1 id="interface-cpufreq">
> > +    <title>org.freedesktop.Hal.Device.CPUFreq interface</title>
> > +    <para>
> > +      This interface provides a mechanism to configure CPU frequency
> > +      scaling. The following methods are available:
> > +    </para>
> > +
> > +    <informaltable>
> > +      <tgroup cols="2">
> > +        <thead>
> > +          <row>
> > +            <entry>Method</entry>
> > +            <entry>Returns</entry>
> > +            <entry>Parameters</entry>
> > +            <entry>Throws</entry>
> > +            <entry>Description</entry>
> > +          </row>
> > +        </thead>
> > +        <tbody>
> 
> > +          <row>
> > +            <entry>SetCPUFreqGovernor</entry>
> > +            <entry>Int</entry>
> > +            <entry>String governor</entry>
> > +            <entry>CPUFreq.UnknownGovernor</entry>
> > +            <entry>
> > +              Selects a CPU Frequency governor to use.
> > +            </entry>
> > +          </row>
> 
> > -            <row>
> > -              <entry>
> > -                <literal>SetCPUFreqGovernor</literal> (string)
> > -              </entry>
> > -              <entry>
> > -	        The name of the governor to set. Get a list of available governors
> > -		with the GetCPUFreqAvailableGovernors method.
> > -              </entry>
> > -              <entry>No</entry>
> > -              <entry>
> > -	        Sets a CPU frequency scaling governor for all CPUFreq
> > -		interfaces the kernel provides. If the userspace governor
> > -		is set, this interface also contains a proper scaling
> > -		mechanism. The default performance is set to 50.
> > -              </entry>
> > -            </row>
> 
> This description contains three important statements:
> 
>   1. It's always and has always been controversial discussed if it makes
>      sense to set different governors for different CPUs or not. The CPU
>      frequency HAL addon uses only one governor for all available
>      CPUs. This is a design decision. And this has to be documented
>      somewhere as part of the API. The question is - where to document if
>      not in the spec?
> 
>   2. Setting the userspace governor means that the kernel let's it up to
>      userspace how to control CPU frequencies. It doesn't implicate
>      that userspace in fact does. So your short description doesn't make
>      clear that HAL rally cares about frequency adjustments as soon as the
>      userspace governor is set into the kernel. I think this is an
>      important information for application developers.
> 
>   3. The default performance/tuning value is set to 50. Maybe this better
>      fits into the SetCPUFreqPerformance description, but again, I think
>      it has to be documented somewhere. Calling the SetCPUFreqGovernor
>      method doesn't necessarily mean that the calling application has to
>      touch the performance/tuning value at all.
> 
> Maybe you think that all this is a little bit too detailed for this
> specification, but there's no other place where we could document this or
> where it would fit.


Oh, I'm fine with having something like this in the spec; ideally this
would be in the preamble for the interface, e.g. here

http://people.freedesktop.org/~david/hal-spec/hal-spec.html#interface-cpufreq

Cheers,
David