linux-mips
[Top] [All Lists]

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

To: David Daney <ddaney.cavm@gmail.com>
Subject: RE: [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/
From: Qais Yousef <Qais.Yousef@imgtec.com>
Date: Mon, 9 Dec 2013 09:35:56 +0000
Accept-language: en-GB, en-US
Cc: "linux-mips@linux-mips.org" <linux-mips@linux-mips.org>
In-reply-to: <52A1FFA3.3000909@gmail.com>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <1386321659-30073-1-git-send-email-qais.yousef@imgtec.com> <52A1FC07.8030902@gmail.com> <392C4BDEFF12D14FA57A3F30B283D7D13C5935@LEMAIL01.le.imgtec.org> <52A1FFA3.3000909@gmail.com>
Sender: linux-mips-bounce@linux-mips.org
Thread-index: AQHO8mR+/DsSvBx3hkan8k2STnHNF5pHXJ+AgAAAdkCAAAPXgIAEPKqA
Thread-topic: [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/
> -----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

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;
> >>>    }
> >>>
> >

<Prev in Thread] Current Thread [Next in Thread>