hal: 2 commits: fix up introspection bits

[Holger Macht]

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.

[...]
> +            <entry>SetCPUFreqPerformance</entry>
> +            <entry>Int</entry>
> +            <entry>Int percent</entry>

It's not really percent. 0 percent is a valid percent value but not valid
to pass to this method. Attached patch adds a range into parenthesis.

[...]
> +          <row>
> +            <entry>GetCPUFreqPerformance</entry>
> +            <entry>Int</entry>
> +            <entry></entry>
> +            <entry>CPUFreq.NoSuitableGovernor</entry>
> +            <entry>
> +              Get the tuning value for the governor.
> +            </entry>
> +          </row>

I think this sentence misses the detail that this method always fails if
no dynamic scaling governor is currently set.

> -            <row>
> -              <entry>
> -                <literal>GetCPUFreqPerformance</literal> (void)
> -              </entry>
> -              <entry>
> -              </entry>
> -              <entry>No</entry>
> -              <entry>
> -	        Get the current active performance setting if a dynamic scaling
> -		mechanism is in use (integer between 1 and 100).
> -              </entry>
> -            </row>
[...]
> +          <row>
> +            <entry>SetCPUFreqConsiderNice</entry>

Second SetCPUFreqConsiderNice has to be GetCPUFreqConsiderNice. Patch
fixes this, too.

What do you think?

Regards,
	Holger