* [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/ @ 2013-12-06 9:20 ` Qais Yousef 0 siblings, 0 replies; 7+ messages in thread From: Qais Yousef @ 2013-12-06 9:20 UTC (permalink / raw) To: linux-mips; +Cc: Qais Yousef 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> --- 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; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/ @ 2013-12-06 9:20 ` Qais Yousef 0 siblings, 0 replies; 7+ messages in thread From: Qais Yousef @ 2013-12-06 9:20 UTC (permalink / raw) To: linux-mips; +Cc: Qais Yousef 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> --- 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; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/ 2013-12-06 9:20 ` Qais Yousef (?) @ 2013-12-06 16:32 ` David Daney 2013-12-06 16:35 ` Qais Yousef -1 siblings, 1 reply; 7+ messages in thread From: David Daney @ 2013-12-06 16:32 UTC (permalink / raw) To: Qais Yousef; +Cc: linux-mips 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. 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; > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/ 2013-12-06 16:32 ` David Daney @ 2013-12-06 16:35 ` Qais Yousef 2013-12-06 16:47 ` David Daney 0 siblings, 1 reply; 7+ messages in thread From: Qais Yousef @ 2013-12-06 16:35 UTC (permalink / raw) To: David Daney; +Cc: linux-mips@linux-mips.org > -----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? 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; > > } > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/ 2013-12-06 16:35 ` Qais Yousef @ 2013-12-06 16:47 ` David Daney 2013-12-09 9:35 ` Qais Yousef 0 siblings, 1 reply; 7+ messages in thread From: David Daney @ 2013-12-06 16:47 UTC (permalink / raw) To: Qais Yousef; +Cc: linux-mips@linux-mips.org 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. 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. 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; >>> } >>> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/ 2013-12-06 16:47 ` David Daney @ 2013-12-09 9:35 ` Qais Yousef 2013-12-09 18:47 ` David Daney 0 siblings, 1 reply; 7+ messages in thread From: Qais Yousef @ 2013-12-09 9:35 UTC (permalink / raw) To: David Daney; +Cc: linux-mips@linux-mips.org > -----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; > >>> } > >>> > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/ 2013-12-09 9:35 ` Qais Yousef @ 2013-12-09 18:47 ` David Daney 0 siblings, 0 replies; 7+ messages in thread From: David Daney @ 2013-12-09 18:47 UTC (permalink / raw) To: Qais Yousef; +Cc: linux-mips@linux-mips.org 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; >>>>> } >>>>> >>> > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-12-09 18:48 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-06 9:20 [PATCH] mips/include/asm/mipsregs.h: s/u16/unsigned short/ Qais Yousef 2013-12-06 9:20 ` Qais Yousef 2013-12-06 16:32 ` David Daney 2013-12-06 16:35 ` Qais Yousef 2013-12-06 16:47 ` David Daney 2013-12-09 9:35 ` Qais Yousef 2013-12-09 18:47 ` David Daney
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.