Re: [PATCH RFC v2 30/70] MIPS: kernel: proc: Add MIPS R6 support to /proc/cpuinfo

[Markos Chandras <Markos.Chandras@imgtec.com>]

On 01/22/2015 02:43 PM, Markos Chandras wrote:
> On 01/22/2015 02:08 PM, Maciej W. Rozycki wrote:
>> On Wed, 21 Jan 2015, Markos Chandras wrote:
>>
>>>>> 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.
>>
>>> 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.
oops that's already there. Then I guess your proposal makes the whole
thing consistent indeed.

-- 
markos