[PATCH 1/3] CPU frequency scaling addon

[Richard Hughes]

On Fri, 2006-08-11 at 19:45 +0200, Holger Macht wrote:
> Patch adding the cpufreq addon itself.

First and foremost, this is *great* functionality. I'll add this patch
set to my utopia rpms and give them a good test myself.

More minor stuff I spotted after a quick review:

> +/** shortcut for g_array_index */
> +#define g_a_i(a,i)	g_array_index(a, unsigned, i)

Is this a good idea to redefine standard stuff?

> +/** frees data needed for CPU load calculation */

Are we doing doxygen in HAL now, or gtk-doc style comments? David?

> +void free_cpu_load_data()
> +{

Maybe I'm wrong, but I thought general hal CodingStyle was:

void
free_cpu_load_data (void)

David, what do you say, (as I also have a patch from Kay that I'm
waiting to send to the list and want to be correct.)

> +	if (cpuload.num_cpus != -1) {
> +		free(cpuload.last_working_time);
> +		free(cpuload.last_total_time);
> +		free(cpuload.load);

Really minor, but isn't it HAL code style to do:

free (foo);

rather than,

free(foo);

i.e. a space before the bracket?

> +		cpuload.num_cpus = -1;
> +		cpuload.load = NULL;
> +		cpuload.last_total_time = NULL;
> +		cpuload.last_working_time = NULL;
> +	}
> +}

> +/********************* userspace interface *********************/
> +static gboolean write_speed(unsigned kHz, int cpu_id)

Uppercase parameters? Surely you want something like frequency_khz?

> +	GString  *speed_file = g_string_new("");

Do you need to use GString here, or could you just use something like
g_sprintf ()?

> +	gboolean ret = TRUE;
> +
> +	if (!cpu_online(cpu_id)) {
> +		ret = FALSE;
> +		goto Out;
> +	}
> +
> +	g_string_printf(speed_file, SYSFS_SCALING_SETSPEED_FILE, cpu_id); 
> +
> +        if(!write_line(speed_file->str, "%u", kHz)){
> +                ERROR("Could not set speed to: %u kHz; %s", kHz, strerror(errno));
> +                ret = FALSE;
> +		goto Out;
> +        }
> +
> +	dbg("Speed set to: %uKHz  for CPU %d", kHz, cpu_id);
> +Out:

Uppercase labels?

> +	g_string_free(speed_file, TRUE);
> +	return ret;
> +}
> +

...

> #define USERSPACE_POLL_INTERVAL	333
> +
> +struct userspace_interface {
> +	int			base_cpu;
> +	int			last_step;
> +	int			current_speed;
> +	int			g_source_id;
> +	int			prev_cpu_load;
> +	GSList			*cpus;
> +	GArray			*speeds_kHz;

speeds_kHz -> speeds_khz ?

> +	GArray			*demotion;
> +};
> +

> +/** check if given CPU starting from 0 is online */
> +gboolean cpu_online(int cpu_id)
> +{
> +	gboolean	online;
> +	char		online_str[2];
> +
> +	GString *online_file = g_string_new("");
> +	g_string_printf(online_file, SYSFS_CPU_ONLINE_FILE, cpu_id); 
> +
> +	if (access(online_file->str, F_OK) < 0) {
> +		online = TRUE;
> +		goto Out;
> +	}
> +
> +	if (!read_line(online_file->str, online_str, 2)) {
> +		ERROR("Unable to open file: %s", online_file->str);
> +		online = FALSE;
> +		goto Out;
> +	}
> +	
> +	online = atoi(online_str);
> +
> +	if (!online)
> +		online = FALSE;
> +Out:

Uppercase?

> +	g_string_free(online_file, TRUE);
> +	return online;
> +}
> +
> +/** writes the new_governor string into the sysfs interface */ 
> +gboolean write_governor(char *new_governor, int cpu_id)
> +{
> +	gboolean	ret		= TRUE;
> +	GString		*governor_file	= g_string_new("");
> +	char		governor[MAX_LINE_SIZE + 1];
> +
> +	if (!cpu_online(cpu_id))
> +		goto Out;
> +
> +	g_string_printf(governor_file, SYSFS_GOVERNOR_FILE, cpu_id); 
> +	dbg("Trying ot write governor %s", new_governor);
> +
> +	if (!write_line(governor_file->str, "%s", new_governor)) {
> +		ret = FALSE;
> +		goto Out;
> +	}
> +	
> +	/* check if governor has been set */
> +	usleep(1000);
> +	read_line(governor_file->str, governor, MAX_LINE_SIZE);
> +	if (strstr(governor, new_governor))
> +		ret = TRUE;
> +	else
> +		ret = FALSE;
> +Out:

Again..

> +	g_string_free(governor_file, TRUE);
> +	return ret;
> +}
> +/******************** helper functions end ********************/
> +
> +/********************* ondemand interface *********************/
> +#define ONDEMAND_STRING "ondemand"
> +
> +struct ondemand_interface {
> +	int base_cpu;
> +};
> +
> +static gboolean ondemand_set_performance(void *data, int performance)
> +{
> +	struct ondemand_interface	*iface		   = data;
> +	GString				*up_threshold_file = g_string_new("");
> +
> +	g_string_printf(up_threshold_file, ONDEMAND_UP_THRESHOLD_FILE, iface->base_cpu); 

GString?

> +        if(!write_line(up_threshold_file->str, "%u", performance)){
> +                ERROR("Could not set up_threshold to %u kHz; %s", performance,
> +		      strerror(errno));
> +                return FALSE;
> +        }
> +	dbg("Up threshold set to %d for ondemand", performance);
> +	g_string_free(up_threshold_file, TRUE);
> +
> +	return TRUE;
> +}
> +

Cheers,

Richard.