[patch] leds: renesas-tpu: cleanup a small type issue

[patch] leds: renesas-tpu: cleanup a small type issue

[Dan Carpenter]

Static checkers complain that this is declared as an unsigned long
but we only ever use the low 32 bits (ignoring sign expansion).
But from the context, it should just be an unsigned short.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/leds/leds-renesas-tpu.c b/drivers/leds/leds-renesas-tpu.c
index 9483f1c..fe1fbd0 100644
--- a/drivers/leds/leds-renesas-tpu.c
+++ b/drivers/leds/leds-renesas-tpu.c
@@ -93,7 +93,8 @@ static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr,
 static void r_tpu_start_stop_ch(struct r_tpu_priv *p, int start)
 {
 	struct led_renesas_tpu_config *cfg = p->pdev->dev.platform_data;
-	unsigned long flags, value;
+	unsigned long flags;
+	unsigned short value;
 
 	/* start stop register shared by multiple timer channels */
 	spin_lock_irqsave(&r_tpu_lock, flags);

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[walter harms]

Am 29.05.2013 09:02, schrieb Dan Carpenter:
> Static checkers complain that this is declared as an unsigned long
> but we only ever use the low 32 bits (ignoring sign expansion).
> But from the context, it should just be an unsigned short.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/leds/leds-renesas-tpu.c b/drivers/leds/leds-renesas-tpu.c
> index 9483f1c..fe1fbd0 100644
> --- a/drivers/leds/leds-renesas-tpu.c
> +++ b/drivers/leds/leds-renesas-tpu.c
> @@ -93,7 +93,8 @@ static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr,
>  static void r_tpu_start_stop_ch(struct r_tpu_priv *p, int start)
>  {
>  	struct led_renesas_tpu_config *cfg = p->pdev->dev.platform_data;
> -	unsigned long flags, value;
> +	unsigned long flags;
> +	unsigned short value;
>  


When it is using the lower 32bit may "int" is better ?

re,
 wh


>  	/* start stop register shared by multiple timer channels */
>  	spin_lock_irqsave(&r_tpu_lock, flags);
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[Dan Carpenter]

On Wed, May 29, 2013 at 09:21:00AM +0200, walter harms wrote:
> 
> 
> Am 29.05.2013 09:02, schrieb Dan Carpenter:
> > Static checkers complain that this is declared as an unsigned long
> > but we only ever use the low 32 bits (ignoring sign expansion).
> > But from the context, it should just be an unsigned short.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/leds/leds-renesas-tpu.c b/drivers/leds/leds-renesas-tpu.c
> > index 9483f1c..fe1fbd0 100644
> > --- a/drivers/leds/leds-renesas-tpu.c
> > +++ b/drivers/leds/leds-renesas-tpu.c
> > @@ -93,7 +93,8 @@ static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr,
> >  static void r_tpu_start_stop_ch(struct r_tpu_priv *p, int start)
> >  {
> >  	struct led_renesas_tpu_config *cfg = p->pdev->dev.platform_data;
> > -	unsigned long flags, value;
> > +	unsigned long flags;
> > +	unsigned short value;
> >  
> 
> 
> When it is using the lower 32bit may "int" is better ?

The static checkers think it's using the lower 32 bits, but it's
actually using the lower 16 bits.

regards,
dan carpenter

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[Bryan Wu]

On Wed, May 29, 2013 at 12:45 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Wed, May 29, 2013 at 09:21:00AM +0200, walter harms wrote:
>>
>>
>> Am 29.05.2013 09:02, schrieb Dan Carpenter:
>> > Static checkers complain that this is declared as an unsigned long
>> > but we only ever use the low 32 bits (ignoring sign expansion).
>> > But from the context, it should just be an unsigned short.
>> >
>> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> >
>> > diff --git a/drivers/leds/leds-renesas-tpu.c b/drivers/leds/leds-renesas-tpu.c
>> > index 9483f1c..fe1fbd0 100644
>> > --- a/drivers/leds/leds-renesas-tpu.c
>> > +++ b/drivers/leds/leds-renesas-tpu.c
>> > @@ -93,7 +93,8 @@ static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr,
>> >  static void r_tpu_start_stop_ch(struct r_tpu_priv *p, int start)
>> >  {
>> >     struct led_renesas_tpu_config *cfg = p->pdev->dev.platform_data;
>> > -   unsigned long flags, value;
>> > +   unsigned long flags;
>> > +   unsigned short value;
>> >
>>
>>
>> When it is using the lower 32bit may "int" is better ?
>
> The static checkers think it's using the lower 32 bits, but it's
> actually using the lower 16 bits.
>

Is "u16" better? I think we can replace all the unsigned short value
to u16 value.

Thanks,
-Bryan

[patch v2] leds: renesas-tpu: cleanup a small type issue

[Dan Carpenter]

Static checkers complain that, although this is declared as an
unsigned long, we can only use the lower 32 bits.  For anything
higher, we hit bugs widening then bitwise negate or wrapping bugs
doing the left shift.

From looking at the context, this is not a problem because we only
use 16 bits.  I've changed some types to make it more clear.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Changed unsigned short to u16.  Re-wrote the commit message.

diff --git a/drivers/leds/leds-renesas-tpu.c b/drivers/leds/leds-renesas-tpu.c
index 9483f1c..adebf49 100644
--- a/drivers/leds/leds-renesas-tpu.c
+++ b/drivers/leds/leds-renesas-tpu.c
@@ -63,7 +63,7 @@ static DEFINE_SPINLOCK(r_tpu_lock);
 #define TGRC 8 /* Timer general register C (+0x20) */
 #define TGRD 9 /* Timer general register D (+0x24) */
 
-static inline unsigned short r_tpu_read(struct r_tpu_priv *p, int reg_nr)
+static inline u16 r_tpu_read(struct r_tpu_priv *p, int reg_nr)
 {
 	struct led_renesas_tpu_config *cfg = p->pdev->dev.platform_data;
 	void __iomem *base = p->mapbase;
@@ -75,8 +75,7 @@ static inline unsigned short r_tpu_read(struct r_tpu_priv *p, int reg_nr)
 	return ioread16(base + offs);
 }
 
-static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr,
-			       unsigned short value)
+static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr, u16 value)
 {
 	struct led_renesas_tpu_config *cfg = p->pdev->dev.platform_data;
 	void __iomem *base = p->mapbase;
@@ -93,7 +92,8 @@ static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr,
 static void r_tpu_start_stop_ch(struct r_tpu_priv *p, int start)
 {
 	struct led_renesas_tpu_config *cfg = p->pdev->dev.platform_data;
-	unsigned long flags, value;
+	unsigned long flags;
+	u16 value;
 
 	/* start stop register shared by multiple timer channels */
 	spin_lock_irqsave(&r_tpu_lock, flags);

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[walter harms]

Am 29.05.2013 18:59, schrieb Bryan Wu:
> On Wed, May 29, 2013 at 12:45 AM, Dan Carpenter
> <dan.carpenter@oracle.com> wrote:
>> On Wed, May 29, 2013 at 09:21:00AM +0200, walter harms wrote:
>>>
>>>
>>> Am 29.05.2013 09:02, schrieb Dan Carpenter:
>>>> Static checkers complain that this is declared as an unsigned long
>>>> but we only ever use the low 32 bits (ignoring sign expansion).
>>>> But from the context, it should just be an unsigned short.
>>>>
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>
>>>> diff --git a/drivers/leds/leds-renesas-tpu.c b/drivers/leds/leds-renesas-tpu.c
>>>> index 9483f1c..fe1fbd0 100644
>>>> --- a/drivers/leds/leds-renesas-tpu.c
>>>> +++ b/drivers/leds/leds-renesas-tpu.c
>>>> @@ -93,7 +93,8 @@ static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr,
>>>>  static void r_tpu_start_stop_ch(struct r_tpu_priv *p, int start)
>>>>  {
>>>>     struct led_renesas_tpu_config *cfg = p->pdev->dev.platform_data;
>>>> -   unsigned long flags, value;
>>>> +   unsigned long flags;
>>>> +   unsigned short value;
>>>>
>>>
>>>
>>> When it is using the lower 32bit may "int" is better ?
>>
>> The static checkers think it's using the lower 32 bits, but it's
>> actually using the lower 16 bits.
>>
> 
> Is "u16" better? I think we can replace all the unsigned short value
> to u16 value.
> 

int would be a more "natural" choice.

re,
 wh

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[Dan Carpenter]

On Wed, May 29, 2013 at 07:34:39PM +0200, walter harms wrote:

> int would be a more "natural" choice.
> 

You should never use signed types for a bit field.  That's just
asking for a sign expansion bug.

regards,
dan carpenter

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[walter harms]

Am 29.05.2013 23:15, schrieb Dan Carpenter:
> On Wed, May 29, 2013 at 07:34:39PM +0200, walter harms wrote:
> 
>> int would be a more "natural" choice.
>>
> 
> You should never use signed types for a bit field.  That's just
> asking for a sign expansion bug.

my idea was more to use unsigned int instead of u16.
Personally i try to avoid this (artificial) types as much as possible,

re,
 wh

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[Dan Carpenter]

On Thu, May 30, 2013 at 10:59:35AM +0200, walter harms wrote:
> 
> 
> Am 29.05.2013 23:15, schrieb Dan Carpenter:
> > On Wed, May 29, 2013 at 07:34:39PM +0200, walter harms wrote:
> > 
> >> int would be a more "natural" choice.
> >>
> > 
> > You should never use signed types for a bit field.  That's just
> > asking for a sign expansion bug.
> 
> my idea was more to use unsigned int instead of u16.
> Personally i try to avoid this (artificial) types as much as possible,

Obviously no one wants to go nuts with the type specifiers like the
e1000e people who never use "int" and only "s32".  But in this case
u16 is more readable and more accurate.

regards,
dan carpenter

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[Bryan Wu]

On Thu, May 30, 2013 at 1:59 AM, walter harms <wharms@bfs.de> wrote:
>
>
> Am 29.05.2013 23:15, schrieb Dan Carpenter:
>> On Wed, May 29, 2013 at 07:34:39PM +0200, walter harms wrote:
>>
>>> int would be a more "natural" choice.
>>>
>>
>> You should never use signed types for a bit field.  That's just
>> asking for a sign expansion bug.
>
> my idea was more to use unsigned int instead of u16.
> Personally i try to avoid this (artificial) types as much as possible,
>

In this case, "val" is an unsigned 16 bits value for register
operation nothing else. 'u16' is correct for this usage and it's more
portable. It should be always unsigned 16 bits type crossing all the
architecture. But unsigned int is not always 32 bits and unsigned
short is not always 16 bits on every platform.

Thanks,
-Bryan

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[Bryan Wu]

On Thu, May 30, 2013 at 5:02 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, May 30, 2013 at 10:59:35AM +0200, walter harms wrote:
>>
>>
>> Am 29.05.2013 23:15, schrieb Dan Carpenter:
>> > On Wed, May 29, 2013 at 07:34:39PM +0200, walter harms wrote:
>> >
>> >> int would be a more "natural" choice.
>> >>
>> >
>> > You should never use signed types for a bit field.  That's just
>> > asking for a sign expansion bug.
>>
>> my idea was more to use unsigned int instead of u16.
>> Personally i try to avoid this (artificial) types as much as possible,
>
> Obviously no one wants to go nuts with the type specifiers like the
> e1000e people who never use "int" and only "s32".  But in this case
> u16 is more readable and more accurate.
>

Do you mind to clean up this driver with u16? or I will do it.

Thanks,
-Bryan

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[Dan Carpenter]

On Thu, May 30, 2013 at 10:59:49AM -0700, Bryan Wu wrote:
> But unsigned int is not always 32 bits and unsigned short is not
> always 16 bits on every platform.
> 

On all linux platforms they are though.

regards,
dan carpenter

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[Dan Carpenter]

On Thu, May 30, 2013 at 11:01:34AM -0700, Bryan Wu wrote:
> On Thu, May 30, 2013 at 5:02 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Thu, May 30, 2013 at 10:59:35AM +0200, walter harms wrote:
> >>
> >>
> >> Am 29.05.2013 23:15, schrieb Dan Carpenter:
> >> > On Wed, May 29, 2013 at 07:34:39PM +0200, walter harms wrote:
> >> >
> >> >> int would be a more "natural" choice.
> >> >>
> >> >
> >> > You should never use signed types for a bit field.  That's just
> >> > asking for a sign expansion bug.
> >>
> >> my idea was more to use unsigned int instead of u16.
> >> Personally i try to avoid this (artificial) types as much as possible,
> >
> > Obviously no one wants to go nuts with the type specifiers like the
> > e1000e people who never use "int" and only "s32".  But in this case
> > u16 is more readable and more accurate.
> >
> 
> Do you mind to clean up this driver with u16? or I will do it.

I resent the patch already.  Hopefully, that's ok?

regards,
dan carpenter

Re: [patch] leds: renesas-tpu: cleanup a small type issue

[Bryan Wu]

On Thu, May 30, 2013 at 10:45 PM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Thu, May 30, 2013 at 11:01:34AM -0700, Bryan Wu wrote:
>> On Thu, May 30, 2013 at 5:02 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Thu, May 30, 2013 at 10:59:35AM +0200, walter harms wrote:
>> >>
>> >>
>> >> Am 29.05.2013 23:15, schrieb Dan Carpenter:
>> >> > On Wed, May 29, 2013 at 07:34:39PM +0200, walter harms wrote:
>> >> >
>> >> >> int would be a more "natural" choice.
>> >> >>
>> >> >
>> >> > You should never use signed types for a bit field.  That's just
>> >> > asking for a sign expansion bug.
>> >>
>> >> my idea was more to use unsigned int instead of u16.
>> >> Personally i try to avoid this (artificial) types as much as possible,
>> >
>> > Obviously no one wants to go nuts with the type specifiers like the
>> > e1000e people who never use "int" and only "s32".  But in this case
>> > u16 is more readable and more accurate.
>> >
>>
>> Do you mind to clean up this driver with u16? or I will do it.
>
> I resent the patch already.  Hopefully, that's ok?
>

I didn't get it, did you resent it to linux-leds mail list or just LKML?

-Bryan