Re: [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/

[David Daney <ddaney.cavm@gmail.com>]

On 12/09/2013 01:35 AM, Qais Yousef wrote:
>> -----Original Message-----
>> From: David Daney [mailto:ddaney.cavm@gmail.com]
>> Sent: 06 December 2013 16:48
>> To: Qais Yousef
>> Cc: linux-mips@linux-mips.org
>> Subject: Re: [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/
>>
>> On 12/06/2013 08:35 AM, Qais Yousef wrote:
>>>> -----Original Message-----
>>>> From: David Daney [mailto:ddaney.cavm@gmail.com]
>>>> Sent: 06 December 2013 16:32
>>>> To: Qais Yousef
>>>> Cc: linux-mips@linux-mips.org
>>>> Subject: Re: [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned
>>>> short/
>>>>
>>>> On 12/06/2013 01:20 AM, Qais Yousef wrote:
>>>>> I was getting this error when including this header in my driver:
>>>>>
>>>>>      arch/mips/include/asm/mipsregs.h:644:33: error: unknown type name
>> ‘u16’
>>>>>
>>>>> since the use of u16 is not really necessary, convert it to unsigned short.
>>>>>
>>>>> Signed-off-by: Qais Yousef <qais.yousef@imgtec.com>
>>>>> Reviewed-by: Steven J. Hill <Steven.Hill@imgtec.com>
>>>>
>>>> NAK.
>>>>
>>>> Just #include <linux/types.h> at the top of asm/mipsregs.h instead.
>>>
>>> Funnily that was my first solution before I changed it to this :)
>>>
>>> I'll resend but can you please give some explanation why changing u16 to
>> unsigned short is bad?
>>
>> This is the linux kernel.  People expect to see fixed width integer type definitions
>> using the conventional u8, u16, u32, etc.
>>
>> If you are doing something tricky enough that you need to explicitly use a type of
>> a given width, don't hide the fact, bring it to our attention by using the kernel
>> standard type.
>>
>> If you don't need exactly a u16, just make it an unsigned int and be done with it.
>>
>> It would appear that micro-MIPS instructions are 16 bit, so use u16 everywhere
>> for them.
>
> OK thanks for the explanation. u16 is more safe and future proof for sure.
>
>>
>> Also it looks like this function really should be declared as returning type bool, not
>> int.  For the same reason:  It cannot return any integer, only truth values.  Don't
>> hide this fact.  Bring it to our attention by using the proper types.
>
> I share this view about Booleans to be honest
> http://article.gmane.org/gmane.linux.kernel/1554183/match=bool
>

If you are storing Boolean values in an array, Linus has a point.  For 
function return values, he doesn't convince me.

David Daney

> v2 is on the way.
>
> Thanks,
> Qais
>
>>
>>
>> David Daney
>>
>>
>>>
>>> Thanks,
>>> Qais
>>>
>>>>
>>>> David Daney
>>>>
>>>>
>>>>> ---
>>>>>     arch/mips/include/asm/mipsregs.h |    4 ++--
>>>>>     1 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/mips/include/asm/mipsregs.h
>>>> b/arch/mips/include/asm/mipsregs.h
>>>>> index e033141..0a2d6ef 100644
>>>>> --- a/arch/mips/include/asm/mipsregs.h
>>>>> +++ b/arch/mips/include/asm/mipsregs.h
>>>>> @@ -641,9 +641,9 @@
>>>>>      * microMIPS instructions can be 16-bit or 32-bit in length. This
>>>>>      * returns a 1 if the instruction is 16-bit and a 0 if 32-bit.
>>>>>      */
>>>>> -static inline int mm_insn_16bit(u16 insn)
>>>>> +static inline int mm_insn_16bit(unsigned short insn)
>>>>>     {
>>>>> -	u16 opcode = (insn >> 10) & 0x7;
>>>>> +	unsigned short opcode = (insn >> 10) & 0x7;
>>>>>
>>>>>     	return (opcode >= 1 && opcode <= 3) ? 1 : 0;
>>>>>     }
>>>>>
>>>
>