leds: renesas-tpu: cleanup a small type issue

PATCH Superseded Dan Carpenter 3 participants 14 messages May 29, 2013 – May 31, 2013

Subsystems (2)

Kernel janitors LED

lore

Share & tools

lore

Dan Carpenter

author

2013-05-29 07:02 UTC #0 lore raw

PATCH 1/1 leds: renesas-tpu: cleanup a small type issue

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 <[email protected]>

drivers/leds/leds-renesas-tpu.c +2 -1

index 9483f1c..fe1fbd0 100644
@@ -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);

walter harms

2013-05-29 07:21 UTC #1 lore raw

Am 29.05.2013 09:02, schrieb Dan Carpenter:

Quoted 11 lines

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 <[email protected]>
drivers/leds/leds-renesas-tpu.c +0 -0
index 9483f1c..fe1fbd0 100644

drivers/leds/leds-renesas-tpu.c static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr, · line 98

Quoted 7 lines

drivers/leds/leds-renesas-tpu.c +2 -1

@@ -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

Quoted 7 lines

drivers/leds/leds-renesas-tpu.c +0 -1
 	/* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2013-05-29 07:45 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 9 lines · walter harms

Am 29.05.2013 09:02, schrieb Dan Carpenter:
Quoted 6 lines
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 <[email protected]>

Quoted 15 lines

drivers/leds/leds-renesas-tpu.c static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr, · line 98
Quoted 12 lines
drivers/leds/leds-renesas-tpu.c +2 -1
index 9483f1c..fe1fbd0 100644
@@ -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

Bryan Wu

2013-05-29 16:59 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 26 lines · Dan Carpenter

Quoted 9 lines · walter harms
Am 29.05.2013 09:02, schrieb Dan Carpenter:
Quoted 6 lines
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 <[email protected]>
Quoted 15 lines
drivers/leds/leds-renesas-tpu.c static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr, · line 98
Quoted 12 lines
drivers/leds/leds-renesas-tpu.c +2 -1
index 9483f1c..fe1fbd0 100644
@@ -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 ?

Quoted 3 lines

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

Dan Carpenter

author

2013-05-29 21:09 UTC #4 lore raw

PATCH v2 1/1 leds: renesas-tpu: cleanup a small type issue

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 <[email protected]>

---
v2: Changed unsigned short to u16. Re-wrote the commit message.

drivers/leds/leds-renesas-tpu.c +4 -4

index 9483f1c..adebf49 100644
@@ -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);

walter harms

2013-05-29 17:34 UTC #5 lore raw

In reply to Dan Carpenter

Am 29.05.2013 18:59, schrieb Bryan Wu:

Quoted 28 lines

Quoted 26 lines · Dan Carpenter
Quoted 24 lines · walter harms
Am 29.05.2013 09:02, schrieb Dan Carpenter:
Quoted 11 lines
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 <[email protected]>
drivers/leds/leds-renesas-tpu.c +0 -0
index 9483f1c..fe1fbd0 100644
drivers/leds/leds-renesas-tpu.c static inline void r_tpu_write(struct r_tpu_priv *p, int reg_nr, · line 98
Quoted 7 lines
drivers/leds/leds-renesas-tpu.c +2 -1
@@ -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 ?

Quoted 7 lines

Quoted 3 lines
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

Dan Carpenter

author

2013-05-29 21:15 UTC #6 lore raw

In reply to Dan Carpenter

Quoted 2 lines · walter harms

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

walter harms

2013-05-30 08:59 UTC #7 lore raw

In reply to Dan Carpenter

Am 29.05.2013 23:15, schrieb Dan Carpenter:

Quoted 7 lines · walter harms

Quoted 3 lines
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

Dan Carpenter

author

2013-05-30 12:02 UTC #8 lore raw

In reply to Dan Carpenter

Quoted 5 lines · walter harms

Am 29.05.2013 23:15, schrieb Dan Carpenter:
Quoted 2 lines · walter harms

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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

Bryan Wu

2013-05-30 17:59 UTC #9 lore raw

In reply to Dan Carpenter

Quoted 5 lines · walter harms

Am 29.05.2013 23:15, schrieb Dan Carpenter:
Quoted 2 lines · walter harms

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
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

Bryan Wu

2013-05-30 18:01 UTC #10 lore raw

In reply to Dan Carpenter

Quoted 12 lines · Dan Carpenter

Quoted 11 lines · walter harms
Am 29.05.2013 23:15, schrieb Dan Carpenter:
Quoted 7 lines · walter harms
Quoted 3 lines
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.

Quoted 7 lines

Quoted 2 lines
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

Dan Carpenter

author

2013-05-31 05:44 UTC #11 lore raw

In reply to Dan Carpenter

Quoted 3 lines · Bryan Wu

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

Dan Carpenter

author

2013-05-31 05:45 UTC #12 lore raw

In reply to Dan Carpenter

Quoted 16 lines · Bryan Wu

Quoted 15 lines · Dan Carpenter
Quoted 13 lines · walter harms
Am 29.05.2013 23:15, schrieb Dan Carpenter:
Quoted 7 lines · walter harms
Quoted 3 lines
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,

Quoted 6 lines

Quoted 4 lines
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

Bryan Wu

2013-05-31 06:38 UTC #13 lore raw

In reply to Dan Carpenter

Quoted 17 lines · Dan Carpenter

Quoted 16 lines · Bryan Wu
Quoted 15 lines · Dan Carpenter
Quoted 13 lines · walter harms
Am 29.05.2013 23:15, schrieb Dan Carpenter:
Quoted 7 lines · walter harms
Quoted 3 lines
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,

Quoted 9 lines

Quoted 6 lines
Quoted 4 lines
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