cpufreq: intel_pstate: Trivial code cleanup

Remove unnecessary braces.

Quoted 1 line · Stratos Karafotis

Remove unnecessary braces.

[]

drivers/cpufreq/intel_pstate.c static inline void intel_pstate_busy_pid_reset(struct cpudata *cpu) · line 211

Quoted 11 lines

drivers/cpufreq/intel_pstate.c +2 -2

@@ -204,20 +203,16 @@ static inline void intel_pstate_busy_pid_reset(struct cpudata *cpu)
 
 static inline void intel_pstate_reset_all_pid(void)
 {
 	unsigned int cpu;
-	for_each_online_cpu(cpu) {
+
+	for_each_online_cpu(cpu)
 		if (all_cpu_data[cpu])
 			intel_pstate_busy_pid_reset(all_cpu_data[cpu]);
-	}

It's pretty traditional to keep the braces here
as it generally makes it clearer for the reader.

	for (...) {
		if (foo)
			bar();
	}

is generally used over

	for (...)
		if (foo)
			bar();

Just like using

	if (foo) {
		/* commment */
		bar();
	}

Review static int intel_pstate_init_cpu(unsigned int cpunum)

Quoted 1 line

(quoted diff) +0 -0

@@ -748,15 +744,14 @@ static int intel_pstate_init_cpu(unsigned int cpunum)

[]

drivers/cpufreq/intel_pstate.c static int intel_pstate_init_cpu(unsigned int cpunum) · line 754

Quoted 2 lines

drivers/cpufreq/intel_pstate.c +1 -1

-	pr_info("Intel pstate controlling: cpu %d\n", cpunum);
+	pr_info("Intel pstate controlling: CPU %d\n", cpunum);

cpu is very slightly preferred lower case.

$ git grep -E -i '^[^"]*"[^"]*\bcpu\b'|grep -w -i -o cpu | sort |uniq -c | sort -rn
   2705 cpu
   2084 CPU
     17 Cpu

Remove unnecessary blank lines.
Remove unnecessary parentheses.
Remove unnecessary braces.
Put the code in one line where possible.
Add blank lines after variable declarations.
Alignment to open parenthesis.

Signed-off-by Stratos Karafotis <[email protected]>

---
  drivers/cpufreq/intel_pstate.c | 96 ++++++++++++++++++++----------------------
  1 file changed, 45 insertions(+), 51 deletions(-)

drivers/cpufreq/intel_pstate.c +45 -51

index d4f0518..fa44f0f 100644
@@ -142,7 +142,7 @@ static struct perf_limits limits = {
  };
 
  static inline void pid_reset(struct _pid *pid, int setpoint, int busy,
-			int deadband, int integral) {
+			     int deadband, int integral) {
  	pid->setpoint = setpoint;
  	pid->deadband  = deadband;
  	pid->integral  = int_tofp(integral);

@@ -161,7 +161,6 @@ static inline void pid_i_gain_set(struct _pid *pid, int percent)
 
  static inline void pid_d_gain_set(struct _pid *pid, int percent)
  {
-
  	pid->d_gain = div_fp(int_tofp(percent), int_tofp(100));
  }
 

@@ -192,9 +191,9 @@ static signed int pid_calc(struct _pid *pid, int32_t busy)
 
  	result = pterm + mul_fp(pid->integral, pid->i_gain) + dterm;
  	if (result >= 0)
-		result = result + (1 << (FRAC_BITS-1));
+		result += 1 << (FRAC_BITS-1);
  	else
-		result = result - (1 << (FRAC_BITS-1));
+		result -= 1 << (FRAC_BITS-1);
  	return (signed int)fp_toint(result);
  }
 

@@ -204,20 +203,16 @@ static inline void intel_pstate_busy_pid_reset(struct cpudata *cpu)
  	pid_d_gain_set(&cpu->pid, pid_params.d_gain_pct);
  	pid_i_gain_set(&cpu->pid, pid_params.i_gain_pct);
 
-	pid_reset(&cpu->pid,
-		pid_params.setpoint,
-		100,
-		pid_params.deadband,
-		0);
+	pid_reset(&cpu->pid, pid_params.setpoint, 100, pid_params.deadband, 0);
  }
 
  static inline void intel_pstate_reset_all_pid(void)
  {
  	unsigned int cpu;
-	for_each_online_cpu(cpu) {
+
+	for_each_online_cpu(cpu)
  		if (all_cpu_data[cpu])
  			intel_pstate_busy_pid_reset(all_cpu_data[cpu]);
-	}
  }
 
  /************************** debugfs begin ************************/

@@ -227,13 +222,13 @@ static int pid_param_set(void *data, u64 val)
  	intel_pstate_reset_all_pid();
  	return 0;
  }
+
  static int pid_param_get(void *data, u64 *val)
  {
  	*val = *(u32 *)data;
  	return 0;
  }
-DEFINE_SIMPLE_ATTRIBUTE(fops_pid_param, pid_param_get,
-			pid_param_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_pid_param, pid_param_get, pid_param_set, "%llu\n");
 
  struct pid_param {
  	char *name;

@@ -310,8 +305,8 @@ static void intel_pstate_debug_expose_params(void)
  		return;
  	while (pid_files[i].name) {
  		debugfs_create_file(pid_files[i].name, 0660,
-				debugfs_parent, pid_files[i].value,
-				&fops_pid_param);
+				    debugfs_parent, pid_files[i].value,
+				    &fops_pid_param);
  		i++;
  	}
  	debugfs_create_file("stats", S_IRUSR | S_IRGRP, debugfs_parent, NULL,

@@ -329,10 +324,11 @@ static void intel_pstate_debug_expose_params(void)
  	}
 
  static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
-				const char *buf, size_t count)
+			      const char *buf, size_t count)
  {
  	unsigned int input;
  	int ret;
+
  	ret = sscanf(buf, "%u", &input);
  	if (ret != 1)
  		return -EINVAL;

@@ -342,10 +338,11 @@ static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
  }
 
  static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
-				const char *buf, size_t count)
+				  const char *buf, size_t count)
  {
  	unsigned int input;
  	int ret;
+
  	ret = sscanf(buf, "%u", &input);
  	if (ret != 1)
  		return -EINVAL;

@@ -353,14 +350,16 @@ static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
  	limits.max_sysfs_pct = clamp_t(int, input, 0 , 100);
  	limits.max_perf_pct = min(limits.max_policy_pct, limits.max_sysfs_pct);
  	limits.max_perf = div_fp(int_tofp(limits.max_perf_pct), int_tofp(100));
+
  	return count;
  }
 
  static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
-				const char *buf, size_t count)
+				  const char *buf, size_t count)
  {
  	unsigned int input;
  	int ret;
+
  	ret = sscanf(buf, "%u", &input);
  	if (ret != 1)
  		return -EINVAL;

@@ -397,8 +396,7 @@ static void intel_pstate_sysfs_expose_params(void)
  	intel_pstate_kobject = kobject_create_and_add("intel_pstate",
  						&cpu_subsys.dev_root->kobj);
  	BUG_ON(!intel_pstate_kobject);
-	rc = sysfs_create_group(intel_pstate_kobject,
-				&intel_pstate_attr_group);
+	rc = sysfs_create_group(intel_pstate_kobject, &intel_pstate_attr_group);
  	BUG_ON(rc);
  }
 

@@ -453,7 +451,6 @@ static void byt_get_vid(struct cpudata *cpudata)
  {
  	u64 value;
 
-
  	rdmsrl(BYT_VIDS, value);
  	cpudata->vid.min = int_tofp((value >> 8) & 0x3f);
  	cpudata->vid.max = int_tofp((value >> 16) & 0x3f);

@@ -466,7 +463,6 @@ static void byt_get_vid(struct cpudata *cpudata)
  	cpudata->vid.turbo = value & 0x7f;
  }
 
-
  static int core_get_min_pstate(void)
  {
  	u64 value;

@@ -485,9 +481,10 @@ static int core_get_turbo_pstate(void)
  {
  	u64 value;
  	int nont, ret;
+
  	rdmsrl(MSR_NHM_TURBO_RATIO_LIMIT, value);
  	nont = core_get_max_pstate();
-	ret = ((value) & 255);
+	ret = value & 255;
  	if (ret <= nont)
  		ret = nont;
  	return ret;

@@ -539,12 +536,12 @@ static struct cpu_defaults byt_params = {
  	},
  };
 
-
  static void intel_pstate_get_min_max(struct cpudata *cpu, int *min, int *max)
  {
  	int max_perf = cpu->pstate.turbo_pstate;
  	int max_perf_adj;
  	int min_perf;
+
  	if (limits.no_turbo)
  		max_perf = cpu->pstate.max_pstate;
 

@@ -553,8 +550,7 @@ static void intel_pstate_get_min_max(struct cpudata *cpu, int *min, int *max)
  			cpu->pstate.min_pstate, cpu->pstate.turbo_pstate);
 
  	min_perf = fp_toint(mul_fp(int_tofp(max_perf), limits.min_perf));
-	*min = clamp_t(int, min_perf,
-			cpu->pstate.min_pstate, max_perf);
+	*min = clamp_t(int, min_perf, cpu->pstate.min_pstate, max_perf);
  }
 
  static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate)

@@ -648,12 +644,12 @@ static inline void intel_pstate_calc_scaled_busy(struct cpudata *cpu)
  	current_pstate = int_tofp(cpu->pstate.current_pstate);
  	core_busy = mul_fp(core_busy, div_fp(max_pstate, current_pstate));
 
-	sample_time = (pid_params.sample_rate_ms  * USEC_PER_MSEC);
+	sample_time = pid_params.sample_rate_ms * USEC_PER_MSEC;
  	duration_us = (u32) ktime_us_delta(cpu->sample.time,
-					cpu->last_sample_time);
+					   cpu->last_sample_time);
  	if (duration_us > sample_time * 3) {
  		sample_ratio = div_fp(int_tofp(sample_time),
-				int_tofp(duration_us));
+				      int_tofp(duration_us));
  		core_busy = mul_fp(core_busy, sample_ratio);
  	}
 

@@ -680,7 +676,7 @@ static inline void intel_pstate_adjust_busy_pstate(struct cpudata *cpu)
 
  static void intel_pstate_timer_func(unsigned long __data)
  {
-	struct cpudata *cpu = (struct cpudata *) __data;
+	struct cpudata *cpu = (struct cpudata *)__data;
  	struct sample *sample;
 
  	intel_pstate_sample(cpu);

@@ -690,11 +686,11 @@ static void intel_pstate_timer_func(unsigned long __data)
  	intel_pstate_adjust_busy_pstate(cpu);
 
  	trace_pstate_sample(fp_toint(sample->core_pct_busy),
-			fp_toint(sample->busy_scaled),
-			cpu->pstate.current_pstate,
-			sample->mperf,
-			sample->aperf,
-			sample->freq);
+			    fp_toint(sample->busy_scaled),
+			    cpu->pstate.current_pstate,
+			    sample->mperf,
+			    sample->aperf,
+			    sample->freq);
 
  	intel_pstate_set_sample_time(cpu);
  }

@@ -748,15 +744,14 @@ static int intel_pstate_init_cpu(unsigned int cpunum)
 
  	init_timer_deferrable(&cpu->timer);
  	cpu->timer.function = intel_pstate_timer_func;
-	cpu->timer.data =
-		(unsigned long)cpu;
+	cpu->timer.data = (unsigned long)cpu;
  	cpu->timer.expires = jiffies + HZ/100;
  	intel_pstate_busy_pid_reset(cpu);
  	intel_pstate_sample(cpu);
 
  	add_timer_on(&cpu->timer, cpunum);
 
-	pr_info("Intel pstate controlling: cpu %d\n", cpunum);
+	pr_info("Intel pstate controlling: CPU %d\n", cpunum);
 
  	return 0;
  }

@@ -790,7 +785,7 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
  		limits.no_turbo = 0;
  		return 0;
  	}
-	limits.min_perf_pct = (policy->min * 100) / policy->cpuinfo.max_freq;
+	limits.min_perf_pct = policy->min * 100 / policy->cpuinfo.max_freq;
  	limits.min_perf_pct = clamp_t(int, limits.min_perf_pct, 0 , 100);
  	limits.min_perf = div_fp(int_tofp(limits.min_perf_pct), int_tofp(100));
 

@@ -806,8 +801,8 @@ static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
  {
  	cpufreq_verify_within_cpu_limits(policy);
 
-	if ((policy->policy != CPUFREQ_POLICY_POWERSAVE) &&
-		(policy->policy != CPUFREQ_POLICY_PERFORMANCE))
+	if (policy->policy != CPUFREQ_POLICY_POWERSAVE &&
+	    policy->policy != CPUFREQ_POLICY_PERFORMANCE)
  		return -EINVAL;
 
  	return 0;

@@ -839,7 +834,7 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
  	cpu = all_cpu_data[policy->cpu];
 
  	if (!limits.no_turbo &&
-		limits.min_perf_pct == 100 && limits.max_perf_pct == 100)
+	    limits.min_perf_pct == 100 && limits.max_perf_pct == 100)
  		policy->policy = CPUFREQ_POLICY_PERFORMANCE;
  	else
  		policy->policy = CPUFREQ_POLICY_POWERSAVE;

@@ -877,8 +872,8 @@ static int intel_pstate_msrs_not_valid(void)
  	rdmsrl(MSR_IA32_MPERF, mperf);
 
  	if (!pstate_funcs.get_max() ||
-		!pstate_funcs.get_min() ||
-		!pstate_funcs.get_turbo())
+	    !pstate_funcs.get_min() ||
+	    !pstate_funcs.get_turbo())
  		return -ENODEV;
 
  	rdmsrl(MSR_IA32_APERF, tmp);

@@ -960,14 +955,14 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
  	struct acpi_table_header hdr;
  	struct hw_vendor_info *v_info;
 
-	if (acpi_disabled
-	    || ACPI_FAILURE(acpi_get_table_header(ACPI_SIG_FADT, 0, &hdr)))
+	if (acpi_disabled ||
+	    ACPI_FAILURE(acpi_get_table_header(ACPI_SIG_FADT, 0, &hdr)))
  		return false;
 
  	for (v_info = vendor_info; v_info->valid; v_info++) {
-		if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE)
-		    && !strncmp(hdr.oem_table_id, v_info->oem_table_id, ACPI_OEM_TABLE_ID_SIZE)
-		    && intel_pstate_no_acpi_pss())
+		if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
+		    !strncmp(hdr.oem_table_id, v_info->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+		    intel_pstate_no_acpi_pss())
  			return true;
  	}
 

@@ -1021,13 +1016,12 @@ static int __init intel_pstate_init(void)
  	return rc;
  out:
  	get_online_cpus();
-	for_each_online_cpu(cpu) {
+	for_each_online_cpu(cpu)
  		if (all_cpu_data[cpu]) {
  			del_timer_sync(&all_cpu_data[cpu]->timer);
  			kfree(all_cpu_data[cpu]->stats);
  			kfree(all_cpu_data[cpu]);
  		}
-	}
 
  	put_online_cpus();
  	vfree(all_cpu_data);

Quoted 7 lines · Stratos Karafotis

Remove unnecessary blank lines.
Remove unnecessary parentheses.
Remove unnecessary braces.
Put the code in one line where possible.
Add blank lines after variable declarations.
Alignment to open parenthesis.

I don't have an issue with this patch in general but I would rather the cleanup be done when there is a functional change in the given hunk of code otherwise you are setting up a fence for stable/backporters of functional changes in the future.

Quoted 9 lines · Dirk Brandewie

Quoted 7 lines · Stratos Karafotis

Remove unnecessary blank lines.
Remove unnecessary parentheses.
Remove unnecessary braces.
Put the code in one line where possible.
Add blank lines after variable declarations.
Alignment to open parenthesis.

Quoted 4 lines

I don't have an issue with this patch in general but I would rather the cleanup be done when there is a functional change in the given hunk of code otherwise you are setting up a fence for stable/backporters of functional changes in the future.

I actually prefer separate cleanups so as to avoid doing multiple things in one patch.

Rafael

Quoted 15 lines · Rafael J. Wysocki

Quoted 13 lines · Dirk Brandewie

Quoted 7 lines · Stratos Karafotis

Remove unnecessary blank lines.
Remove unnecessary parentheses.
Remove unnecessary braces.
Put the code in one line where possible.
Add blank lines after variable declarations.
Alignment to open parenthesis.

I don't have an issue with this patch in general but I would rather the cleanup be done when there is a functional change in the given hunk of code otherwise you are setting up a fence for stable/backporters of functional changes in the future.

Quoted 5 lines

I actually prefer separate cleanups so as to avoid doing multiple things in one patch.

Rafael

I don't have strong feelings either way I was just trying to be kind to the maintainers of distro kernels.

Quoted 19 lines · Dirk Brandewie

Quoted 18 lines · Rafael J. Wysocki

Quoted 9 lines · Dirk Brandewie

Quoted 7 lines · Stratos Karafotis

Remove unnecessary blank lines.
Remove unnecessary parentheses.
Remove unnecessary braces.
Put the code in one line where possible.
Add blank lines after variable declarations.
Alignment to open parenthesis.

Quoted 4 lines

I don't have an issue with this patch in general but I would rather the cleanup be done when there is a functional change in the given hunk of code otherwise you are setting up a fence for stable/backporters of functional changes in the future.

I actually prefer separate cleanups so as to avoid doing multiple things in one patch.

Quoted 4 lines

Quoted 2 lines

Rafael

I don't have strong feelings either way I was just trying to be kind to the maintainers of distro kernels.

And mixing fixes with cleanups in one patch doesn't do any good to them.

Trust me, I used to work for a distro. :-)

Quoted 25 lines · Rafael J. Wysocki

Quoted 23 lines · Dirk Brandewie

Quoted 15 lines · Rafael J. Wysocki

Quoted 9 lines · Dirk Brandewie

Quoted 7 lines · Stratos Karafotis

Remove unnecessary blank lines.
Remove unnecessary parentheses.
Remove unnecessary braces.
Put the code in one line where possible.
Add blank lines after variable declarations.
Alignment to open parenthesis.

Quoted 4 lines

I don't have an issue with this patch in general but I would rather the cleanup be done when there is a functional change in the given hunk of code otherwise you are setting up a fence for stable/backporters of functional changes in the future.

Quoted 5 lines

I actually prefer separate cleanups so as to avoid doing multiple things in one patch.

Rafael

I don't have strong feelings either way I was just trying to be kind to the maintainers of distro kernels.

Quoted 4 lines

And mixing fixes with cleanups in one patch doesn't do any good to them.

Trust me, I used to work for a distro. :-)

So, should I proceed and split the patch or drop it? :)

Quoted 26 lines · Stratos Karafotis

Quoted 25 lines · Rafael J. Wysocki

Quoted 23 lines · Dirk Brandewie

Quoted 15 lines · Rafael J. Wysocki

Quoted 9 lines · Dirk Brandewie

Quoted 7 lines · Stratos Karafotis

Remove unnecessary blank lines.
Remove unnecessary parentheses.
Remove unnecessary braces.
Put the code in one line where possible.
Add blank lines after variable declarations.
Alignment to open parenthesis.

Quoted 4 lines

I don't have an issue with this patch in general but I would rather the cleanup be done when there is a functional change in the given hunk of code otherwise you are setting up a fence for stable/backporters of functional changes in the future.

Quoted 5 lines

I actually prefer separate cleanups so as to avoid doing multiple things in one patch.

Rafael

I don't have strong feelings either way I was just trying to be kind to the maintainers of distro kernels.

Quoted 6 lines

Quoted 4 lines

And mixing fixes with cleanups in one patch doesn't do any good to them.

Trust me, I used to work for a distro. :-)

So, should I proceed and split the patch or drop it? :)

I'm not sure why you'd want to split it?

That said you're changing things that are intentional. For example, the

	if (acpi_disabled
	    || ...)

is. And the result of (a * 100) / b may generally be different from a * 100 / b for integers (if the division is carried out first).

Quoted 32 lines · Rafael J. Wysocki

Quoted 31 lines · Stratos Karafotis

Quoted 29 lines · Rafael J. Wysocki

Quoted 16 lines · Dirk Brandewie

Quoted 15 lines · Rafael J. Wysocki

Quoted 13 lines · Dirk Brandewie

Quoted 7 lines · Stratos Karafotis

Remove unnecessary blank lines.
Remove unnecessary parentheses.
Remove unnecessary braces.
Put the code in one line where possible.
Add blank lines after variable declarations.
Alignment to open parenthesis.

I don't have an issue with this patch in general but I would rather the cleanup be done when there is a functional change in the given hunk of code otherwise you are setting up a fence for stable/backporters of functional changes in the future.

Quoted 7 lines

Quoted 5 lines

I actually prefer separate cleanups so as to avoid doing multiple things in one patch.

Rafael

I don't have strong feelings either way I was just trying to be kind to the maintainers of distro kernels.

And mixing fixes with cleanups in one patch doesn't do any good to them.

Trust me, I used to work for a distro. :-)

Quoted 3 lines

Quoted 1 line

So, should I proceed and split the patch or drop it? :)

I'm not sure why you'd want to split it?

Forgive me, but I'm totally confused. I asked because you mentioned that you prefer separate cleanups.

So, my question was if you want me to separate this patch into more (one per change) or entirely drop it (because it would cause problems to backporters or maintainers).

Quoted 9 lines

That said you're changing things that are intentional. For example, the

	if (acpi_disabled
	    || ...)

is. And the result of (a * 100) / b may generally be different from a * 100 / b for integers (if the division is carried out first).

I thought that (a * 100) / b is always equivalent to a * 100 / b.

Quoted 4 lines

Quoted 2 lines

is. And the result of (a * 100) / b may generally be different from a * 100 / b for integers (if the division is carried out first).

I thought that (a * 100) / b is always equivalent to a * 100 / b.

I'm not actually sure if that's guaranteed by C standards.

It surely
wasn't some time ago (when there was no formal C standard).

Either way, in my opinion it's better to put the parens into the expression in this particular case to clearly state the intention.

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines

Quoted 2 lines

is. And the result of (a * 100) / b may generally be different from a * 100 / b for integers (if the division is carried out first).

I thought that (a * 100) / b is always equivalent to a * 100 / b.

Quoted 1 line

I'm not actually sure if that's guaranteed by C standards.

It is. left to right, same precedence.

Quoted 2 lines

It surely
wasn't some time ago (when there was no formal C standard).

c89 is 25 years ago now.

Quoted 2 lines

Either way, in my opinion it's better to put the parens into the expression in this particular case to clearly state the intention.

I don't think so.

Quoted 1 line · Joe Perches

c89 is 25 years ago now.

Apparently, I'm old.

Quoted 4 lines

Quoted 2 lines

Either way, in my opinion it's better to put the parens into the expression in this particular case to clearly state the intention.

I don't think so.

Of course, you're free to disagree, but I guess you'll admit that a * b / c is generally different from b / c * a and if you see something like this it is hard to say at first sight whether or not this is intentional or an expression ordering bug.

Quoted 33 lines · Stratos Karafotis

Quoted 32 lines · Rafael J. Wysocki

Quoted 31 lines · Stratos Karafotis

Quoted 29 lines · Rafael J. Wysocki

Quoted 16 lines · Dirk Brandewie

Quoted 15 lines · Rafael J. Wysocki

Quoted 13 lines · Dirk Brandewie

Quoted 7 lines · Stratos Karafotis

Remove unnecessary blank lines.
Remove unnecessary parentheses.
Remove unnecessary braces.
Put the code in one line where possible.
Add blank lines after variable declarations.
Alignment to open parenthesis.

I don't have an issue with this patch in general but I would rather the cleanup be done when there is a functional change in the given hunk of code otherwise you are setting up a fence for stable/backporters of functional changes in the future.

Quoted 7 lines

Quoted 5 lines

I actually prefer separate cleanups so as to avoid doing multiple things in one patch.

Rafael

I don't have strong feelings either way I was just trying to be kind to the maintainers of distro kernels.

And mixing fixes with cleanups in one patch doesn't do any good to them.

Trust me, I used to work for a distro. :-)

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

So, should I proceed and split the patch or drop it? :)

I'm not sure why you'd want to split it?

Forgive me, but I'm totally confused. I asked because you mentioned that you prefer separate cleanups.

That was in a reply to Dirk who suggested doing cleanups along with fixes (or at least I understood what he said this way).

I tried to explain why I didn't think that this was a good idea.

Quoted 3 lines

So, my question was if you want me to separate this patch into more (one per change) or entirely drop it (because it would cause problems to backporters or maintainers).

Cleanups are generally OK, but it's better to do one kind of a cleanup per patch. Like whitespace fixes in one patch, cleanup of expressions in another.

Quoted 7 lines

Quoted 7 lines

That said you're changing things that are intentional. For example, the

	if (acpi_disabled
	    || ...)

Quoted 4 lines

Quoted 2 lines

is. And the result of (a * 100) / b may generally be different from a * 100 / b for integers (if the division is carried out first).

I thought that (a * 100) / b is always equivalent to a * 100 / b.

I'm not actually sure if that's guaranteed by C standards. It surely wasn't some time ago (when there was no formal C standard).

Quoted 46 lines · Rafael J. Wysocki

Quoted 33 lines · Stratos Karafotis

Quoted 32 lines · Rafael J. Wysocki

Quoted 31 lines · Stratos Karafotis

Quoted 29 lines · Rafael J. Wysocki

Quoted 16 lines · Dirk Brandewie

Quoted 15 lines · Rafael J. Wysocki

Quoted 13 lines · Dirk Brandewie

Quoted 7 lines · Stratos Karafotis

Remove unnecessary blank lines.
Remove unnecessary parentheses.
Remove unnecessary braces.
Put the code in one line where possible.
Add blank lines after variable declarations.
Alignment to open parenthesis.

I don't have an issue with this patch in general but I would rather the cleanup be done when there is a functional change in the given hunk of code otherwise you are setting up a fence for stable/backporters of functional changes in the future.

Quoted 7 lines

Quoted 5 lines

I actually prefer separate cleanups so as to avoid doing multiple things in one patch.

Rafael

I don't have strong feelings either way I was just trying to be kind to the maintainers of distro kernels.

And mixing fixes with cleanups in one patch doesn't do any good to them.

Trust me, I used to work for a distro. :-)

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

So, should I proceed and split the patch or drop it? :)

I'm not sure why you'd want to split it?

Forgive me, but I'm totally confused. I asked because you mentioned that you prefer separate cleanups.

That was in a reply to Dirk who suggested doing cleanups along with fixes (or at least I understood what he said this way).

I tried to explain why I didn't think that this was a good idea.

Quoted 8 lines

Quoted 3 lines

So, my question was if you want me to separate this patch into more (one per change) or entirely drop it (because it would cause problems to backporters or maintainers).

Cleanups are generally OK, but it's better to do one kind of a cleanup per patch. Like whitespace fixes in one patch, cleanup of expressions in another.

OK, thanks for the clarification! I will do it in separate patches.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

That said you're changing things that are intentional. For example, the

	if (acpi_disabled
	    || ...)

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

is. And the result of (a * 100) / b may generally be different from a * 100 / b for integers (if the division is carried out first).

I thought that (a * 100) / b is always equivalent to a * 100 / b.

I'm not actually sure if that's guaranteed by C standards. It surely wasn't some time ago (when there was no formal C standard).

I think it is, according to C precedence table.
But, anyway my motivation to the specific cleanup was the different style in the same block code:

        limits.min_perf_pct = (policy->min * 100) / policy->cpuinfo.max_freq;
	...
        limits.max_policy_pct = policy->max * 100 / policy->cpuinfo.max_freq;