hid: Initialize battery_no to -1 & correct its format string

PATCH In review Aniroop Mathur 4 participants 6 messages Dec 28, 2014 – Jan 9, 2015

Subsystems (2)

HID HID wacom driver

lore

Share & tools

lore

Aniroop Mathur

author

2014-12-28 16:07 UTC #0 lore raw

PATCH 1/1 hid: Initialize battery_no to -1 & correct its format string

From: Aniroop Mathur <[email protected]>

This patch initializes battery_no to -1 to avoid extra subtraction operation performed everytime wacom battery is initialized and correct format string of unsigned long from %ld to %lu.

Signed-off-by Aniroop Mathur <[email protected]>

---
 drivers/hid/wacom_sys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/hid/wacom_sys.c +4 -4

index 129fd33..4b5ff84 100644
@@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply *psy,
 
 static int wacom_initialize_battery(struct wacom *wacom)
 {
-	static atomic_t battery_no = ATOMIC_INIT(0);
+	static atomic_t battery_no = ATOMIC_INIT(-1);
 	int error;
 	unsigned long n;
 
 	if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
-		n = atomic_inc_return(&battery_no) - 1;
+		n = atomic_inc_return(&battery_no);
 
 		wacom->battery.properties = wacom_battery_props;
 		wacom->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
 		wacom->battery.get_property = wacom_battery_get_property;
-		sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
+		sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
 		wacom->battery.name = wacom->wacom_wac.bat_name;
 		wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
 		wacom->battery.use_for_apm = 0;

@@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
 		wacom->ac.properties = wacom_ac_props;
 		wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
 		wacom->ac.get_property = wacom_ac_get_property;
-		sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
+		sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
 		wacom->ac.name = wacom->wacom_wac.ac_name;
 		wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;
 		wacom->ac.use_for_apm = 0;

Signature · 1 line

1.9.1

Jiri Kosina

2014-12-29 12:04 UTC #1 lore raw

Quoted 4 lines · Aniroop Mathur

From: Aniroop Mathur <[email protected]>
This patch initializes battery_no to -1 to avoid extra subtraction operation performed everytime wacom battery is initialized

This is so femto-optimization, that I don't really care deeply. Adding Jason and Ping to CC. If they want this, I'll apply the patch.

Quoted 44 lines

and correct format string of unsigned long from %ld to %lu.

Signed-off-by Aniroop Mathur <[email protected]>

---
 drivers/hid/wacom_sys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/hid/wacom_sys.c +4 -4

index 129fd33..4b5ff84 100644
@@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply *psy,
 
 static int wacom_initialize_battery(struct wacom *wacom)
 {
-	static atomic_t battery_no = ATOMIC_INIT(0);
+	static atomic_t battery_no = ATOMIC_INIT(-1);
 	int error;
 	unsigned long n;
 
 	if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
-		n = atomic_inc_return(&battery_no) - 1;
+		n = atomic_inc_return(&battery_no);
 
 		wacom->battery.properties = wacom_battery_props;
 		wacom->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
 		wacom->battery.get_property = wacom_battery_get_property;
-		sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
+		sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
 		wacom->battery.name = wacom->wacom_wac.bat_name;
 		wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
 		wacom->battery.use_for_apm = 0;

@@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
 		wacom->ac.properties = wacom_ac_props;
 		wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
 		wacom->ac.get_property = wacom_ac_get_property;
-		sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
+		sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
 		wacom->ac.name = wacom->wacom_wac.ac_name;
 		wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;
 		wacom->ac.use_for_apm = 0;

Signature · 1 line

1.9.1

Signature · 2 lines

Jiri Kosina
SUSE Labs

Aniroop Mathur

author

2015-01-06 14:32 UTC #2 lore raw

Dear Mr. Jason and Mr. Ping,

Please update about below patch.
Except avoiding subtraction, there is also a need to avoid negative battery name which may come due to %ld, so need to change to %lu.

Thanks,
Aniroop

Quoted 58 lines · Jiri Kosina

Quoted 4 lines · Aniroop Mathur

From: Aniroop Mathur <[email protected]>
This patch initializes battery_no to -1 to avoid extra subtraction operation performed everytime wacom battery is initialized

This is so femto-optimization, that I don't really care deeply. Adding Jason and Ping to CC. If they want this, I'll apply the patch.

Quoted 42 lines

and correct format string of unsigned long from %ld to %lu.

Signed-off-by Aniroop Mathur <[email protected]>

---
 drivers/hid/wacom_sys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/hid/wacom_sys.c +4 -5

index 129fd33..4b5ff84 100644
@@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply *psy,
 
 static int wacom_initialize_battery(struct wacom *wacom)
 {
-     static atomic_t battery_no = ATOMIC_INIT(0);
+     static atomic_t battery_no = ATOMIC_INIT(-1);
      int error;
      unsigned long n;
 
      if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
-             n = atomic_inc_return(&battery_no) - 1;
+             n = atomic_inc_return(&battery_no);
 
              wacom->battery.properties = wacom_battery_props;
              wacom->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
              wacom->battery.get_property = wacom_battery_get_property;
-             sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
+             sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
              wacom->battery.name = wacom->wacom_wac.bat_name;
              wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
              wacom->battery.use_for_apm = 0;

@@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
              wacom->ac.properties = wacom_ac_props;
              wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
              wacom->ac.get_property = wacom_ac_get_property;
-             sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
+             sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
              wacom->ac.name = wacom->wacom_wac.ac_name;
              wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;
              wacom->ac.use_for_apm = 0;
--

Quoted 2 lines

1.9.1

--
Jiri Kosina
SUSE Labs

Ping Cheng

2015-01-07 00:50 UTC #3 lore raw

Hi Benjamin,

The relevant code was introduced by d70420b914c98a3758674c6e9858810e0ab4ea30. Can you take a look and let us know if Aniroop's patch fits your original thought or not?

Thanks,

Ping

Quoted 72 lines · Aniroop Mathur

Dear Mr. Jason and Mr. Ping,

Please update about below patch.
Except avoiding subtraction, there is also a need to avoid negative battery name which may come due to %ld, so need to change to %lu.

Thanks,
Aniroop

Quoted 50 lines · Jiri Kosina

Quoted 4 lines · Aniroop Mathur

From: Aniroop Mathur <[email protected]>
This patch initializes battery_no to -1 to avoid extra subtraction operation performed everytime wacom battery is initialized

This is so femto-optimization, that I don't really care deeply. Adding Jason and Ping to CC. If they want this, I'll apply the patch.

Quoted 40 lines

and correct format string of unsigned long from %ld to %lu.

Signed-off-by Aniroop Mathur <[email protected]>

---
 drivers/hid/wacom_sys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/hid/wacom_sys.c +4 -4

index 129fd33..4b5ff84 100644
@@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply *psy,
 
 static int wacom_initialize_battery(struct wacom *wacom)
 {
-     static atomic_t battery_no = ATOMIC_INIT(0);
+     static atomic_t battery_no = ATOMIC_INIT(-1);
      int error;
      unsigned long n;
 
      if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
-             n = atomic_inc_return(&battery_no) - 1;
+             n = atomic_inc_return(&battery_no);
 
              wacom->battery.properties = wacom_battery_props;
              wacom->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
              wacom->battery.get_property = wacom_battery_get_property;
-             sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
+             sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
              wacom->battery.name = wacom->wacom_wac.bat_name;
              wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
              wacom->battery.use_for_apm = 0;

@@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
              wacom->ac.properties = wacom_ac_props;
              wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
              wacom->ac.get_property = wacom_ac_get_property;
-             sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
+             sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
              wacom->ac.name = wacom->wacom_wac.ac_name;
              wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;

Quoted 8 lines

Quoted 4 lines
drivers/hid/wacom_sys.c +0 -1
              wacom->ac.use_for_apm = 0;
--
1.9.1
--
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Benjamin Tissoires

2015-01-07 01:50 UTC #4 lore raw

Quoted 5 lines · Ping Cheng

Hi Benjamin,
The relevant code was introduced by d70420b914c98a3758674c6e9858810e0ab4ea30. Can you take a look and let us know if Aniroop's patch fits your original thought or not?

Hehe, indeed, that's my code.

I would agree with Jiri, I really do not care. This code is called only once at initialization and I am pretty sure it will not change the time to initialize the device (I am confident the compiler will do something better than blindly following the code).

So either way it is good. But I have a slight preference to initialize the variable to 0 and keep the unsigned in the name.

Cheers,
Benjamin

Quoted 78 lines

Thanks,

Ping

Quoted 72 lines · Aniroop Mathur

Dear Mr. Jason and Mr. Ping,

Please update about below patch.
Except avoiding subtraction, there is also a need to avoid negative battery name which may come due to %ld, so need to change to %lu.

Thanks,
Aniroop

Quoted 50 lines · Jiri Kosina

Quoted 4 lines · Aniroop Mathur

From: Aniroop Mathur <[email protected]>
This patch initializes battery_no to -1 to avoid extra subtraction operation performed everytime wacom battery is initialized

This is so femto-optimization, that I don't really care deeply. Adding Jason and Ping to CC. If they want this, I'll apply the patch.

Quoted 40 lines

and correct format string of unsigned long from %ld to %lu.

Signed-off-by Aniroop Mathur <[email protected]>

---
 drivers/hid/wacom_sys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/hid/wacom_sys.c +4 -4

index 129fd33..4b5ff84 100644
@@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply *psy,
 
 static int wacom_initialize_battery(struct wacom *wacom)
 {
-     static atomic_t battery_no = ATOMIC_INIT(0);
+     static atomic_t battery_no = ATOMIC_INIT(-1);
      int error;
      unsigned long n;
 
      if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
-             n = atomic_inc_return(&battery_no) - 1;
+             n = atomic_inc_return(&battery_no);
 
              wacom->battery.properties = wacom_battery_props;
              wacom->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
              wacom->battery.get_property = wacom_battery_get_property;
-             sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
+             sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
              wacom->battery.name = wacom->wacom_wac.bat_name;
              wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
              wacom->battery.use_for_apm = 0;

@@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
              wacom->ac.properties = wacom_ac_props;
              wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
              wacom->ac.get_property = wacom_ac_get_property;
-             sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
+             sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
              wacom->ac.name = wacom->wacom_wac.ac_name;
              wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;

Quoted 8 lines

Quoted 4 lines
drivers/hid/wacom_sys.c +0 -1
              wacom->ac.use_for_apm = 0;
--
1.9.1
--
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Aniroop Mathur

author

2015-01-09 12:33 UTC #5 lore raw

Hello Mr. Benjamin,

Quoted 14 lines · Benjamin Tissoires

Quoted 5 lines · Ping Cheng
Hi Benjamin,
The relevant code was introduced by d70420b914c98a3758674c6e9858810e0ab4ea30. Can you take a look and let us know if Aniroop's patch fits your original thought or not?
Hehe, indeed, that's my code.
I would agree with Jiri, I really do not care. This code is called only once at initialization and I am pretty sure it will not change the time to initialize the device (I am confident the compiler will do something better than blindly following the code).

Quoted 6 lines

So either way it is good. But I have a slight preference to initialize the variable to 0 and keep the unsigned in the name.
Cheers,
Benjamin

Atleast, there is a need to fix format specifier of "n" variable. "n" is declared as unsigned long so %ld should be changed to %lu.

Thanks !!

Quoted 78 lines

Quoted 78 lines
Thanks,
Ping
Quoted 72 lines · Aniroop Mathur
Dear Mr. Jason and Mr. Ping,
Please update about below patch.
Except avoiding subtraction, there is also a need to avoid negative battery name which may come due to %ld, so need to change to %lu.
Thanks,
Aniroop
Quoted 50 lines · Jiri Kosina
Quoted 4 lines · Aniroop Mathur
From: Aniroop Mathur <[email protected]>
This patch initializes battery_no to -1 to avoid extra subtraction operation performed everytime wacom battery is initialized
This is so femto-optimization, that I don't really care deeply. Adding Jason and Ping to CC. If they want this, I'll apply the patch.
Quoted 40 lines
and correct format string of unsigned long from %ld to %lu.
Signed-off-by Aniroop Mathur <[email protected]>
---
 drivers/hid/wacom_sys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/hid/wacom_sys.c +4 -4
index 129fd33..4b5ff84 100644
@@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply *psy,
 
 static int wacom_initialize_battery(struct wacom *wacom)
 {
-     static atomic_t battery_no = ATOMIC_INIT(0);
+     static atomic_t battery_no = ATOMIC_INIT(-1);
      int error;
      unsigned long n;
 
      if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
-             n = atomic_inc_return(&battery_no) - 1;
+             n = atomic_inc_return(&battery_no);
 
              wacom->battery.properties = wacom_battery_props;
              wacom->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
              wacom->battery.get_property = wacom_battery_get_property;
-             sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
+             sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
              wacom->battery.name = wacom->wacom_wac.bat_name;
              wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
              wacom->battery.use_for_apm = 0;
@@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
              wacom->ac.properties = wacom_ac_props;
              wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
              wacom->ac.get_property = wacom_ac_get_property;
-             sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
+             sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
              wacom->ac.name = wacom->wacom_wac.ac_name;
              wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;
Quoted 8 lines
Quoted 4 lines
drivers/hid/wacom_sys.c +0 -1
              wacom->ac.use_for_apm = 0;
--
1.9.1
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html