On 01/22/2015 02:08 PM, Maciej W. Rozycki wrote:

On Wed, 21 Jan 2015, Markos Chandras wrote:

quoted

quoted

quoted

diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
index 097fc8d14e42..a8fdf9685cad 100644
--- a/arch/mips/kernel/proc.c
+++ b/arch/mips/kernel/proc.c

@@ -82,7 +82,9 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 		seq_printf(m, "]\n");
 	}
 
-	seq_printf(m, "isa\t\t\t: mips1");
+	seq_printf(m, "isa\t\t\t:"); 
+	if (!cpu_has_mips_r6)
+		seq_printf(m, " mips1");

 I think define `cpu_has_mips_r1' instead and use it here.  It may turn 
out needed elsewhere too.  We probably don't need a new `MIPS_CPU_ISA_I' 
bit at this stage so this could be:

 Typo here, I meant `cpu_has_mips_1' actually, sorry about that.

quoted

the change is simple enough and I see no reason to define the
cpu_has_mips_r1 at the moment. If we ever need to explicitly handle r1,
we can reconsider that.

 It's a matter of code clarity, good code is self-explanatory.  Here the 
intent is to print `mips1' if it is supported.  By avoiding the extra 
definition you're detaching the intent from what code says.  Someone 
reading this code (who may not necessarily know the architecture documents 
by heart) has to scratch their head thinking: "why isn't `mips1' printed 
for R6, what the former has to do with the latter, and why is this case 
different to `mips2' and other ones that follow?"

 Whereas the intent is clear with this:

#define cpu_has_mips_1 (!cpu_has_mips_r6) // Aha, `mips1' is there if no R6!

	if (cpu_has_mips_1)
		seq_printf(m, " mips1");  // Well, this is obvious...

however, someone may wonder then why not have

if (cpu_has_mips_1)
print mips1
if (cpu_has_mips_2)
print mips2
if (cpu_has_mips_3)
print mips3

and only care about mips1.

Do you see what I mean?  Do you agree now?

the

if (!cpu_has_mips_r6)
    seq_printf(m, " mips1");

means exactly the same thing with

#define cpu_has_mips_1 (!cpu_has_mips_r6) // Aha, `mips1' is there if no R6!

especially since this is the only place that is being used.

I don't see how the differ.

In any case, i don't want such details to block the patchset, so I will
change it.

-- 
markos