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

Commit Message

Aniroop Mathur Dec. 28, 2014, 4:07 p.m. UTC

From: Aniroop Mathur <[e-mail]>

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 <[e-mail]>
---
 drivers/hid/wacom_sys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Jiri Kosina Dec. 29, 2014, 12:03 p.m. UTC | #1

On Sun, 28 Dec 2014, Aniroop Mathur wrote:

> From: Aniroop Mathur <[e-mail]>
> 
> 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.

> and correct format string of unsigned long from %ld to %lu.
> 
> Signed-off-by: Aniroop Mathur <[e-mail]>
> ---
>  drivers/hid/wacom_sys.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 129fd33..4b5ff84 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -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;
> -- 
> 1.9.1
>

Aniroop Mathur Jan. 6, 2015, 2:32 p.m. UTC | #2

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

On Mon, Dec 29, 2014 at 5:33 PM, Jiri Kosina <[e-mail]> wrote:
> On Sun, 28 Dec 2014, Aniroop Mathur wrote:
>
>> From: Aniroop Mathur <[e-mail]>
>>
>> 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.
>
>> and correct format string of unsigned long from %ld to %lu.
>>
>> Signed-off-by: Aniroop Mathur <[e-mail]>
>> ---
>>  drivers/hid/wacom_sys.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>> index 129fd33..4b5ff84 100644
>> --- a/drivers/hid/wacom_sys.c
>> +++ b/drivers/hid/wacom_sys.c
>> @@ -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;
>> --
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ping Cheng Jan. 7, 2015, 12:50 a.m. UTC | #3

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

On Tue, Jan 6, 2015 at 6:32 AM, Aniroop Mathur <[e-mail]> wrote:
> 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
>
> On Mon, Dec 29, 2014 at 5:33 PM, Jiri Kosina <[e-mail]> wrote:
>> On Sun, 28 Dec 2014, Aniroop Mathur wrote:
>>
>>> From: Aniroop Mathur <[e-mail]>
>>>
>>> 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.
>>
>>> and correct format string of unsigned long from %ld to %lu.
>>>
>>> Signed-off-by: Aniroop Mathur <[e-mail]>
>>> ---
>>>  drivers/hid/wacom_sys.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>>> index 129fd33..4b5ff84 100644
>>> --- a/drivers/hid/wacom_sys.c
>>> +++ b/drivers/hid/wacom_sys.c
>>> @@ -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;
>>> --
>>> 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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Benjamin Tissoires Jan. 7, 2015, 1:50 a.m. UTC | #4

On Tue, Jan 6, 2015 at 7:50 PM, Ping Cheng <[e-mail]> wrote:
> 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

>
> Thanks,
>
> Ping
>
> On Tue, Jan 6, 2015 at 6:32 AM, Aniroop Mathur <[e-mail]> wrote:
>> 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
>>
>> On Mon, Dec 29, 2014 at 5:33 PM, Jiri Kosina <[e-mail]> wrote:
>>> On Sun, 28 Dec 2014, Aniroop Mathur wrote:
>>>
>>>> From: Aniroop Mathur <[e-mail]>
>>>>
>>>> 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.
>>>
>>>> and correct format string of unsigned long from %ld to %lu.
>>>>
>>>> Signed-off-by: Aniroop Mathur <[e-mail]>
>>>> ---
>>>>  drivers/hid/wacom_sys.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>>>> index 129fd33..4b5ff84 100644
>>>> --- a/drivers/hid/wacom_sys.c
>>>> +++ b/drivers/hid/wacom_sys.c
>>>> @@ -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;
>>>> --
>>>> 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 [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Aniroop Mathur Jan. 9, 2015, 12:33 p.m. UTC | #5

Hello Mr. Benjamin,

On Wed, Jan 7, 2015 at 7:20 AM, Benjamin Tissoires
<[e-mail]> wrote:
> On Tue, Jan 6, 2015 at 7:50 PM, Ping Cheng <[e-mail]> wrote:
>> 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
>

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

>>
>> Thanks,
>>
>> Ping
>>
>> On Tue, Jan 6, 2015 at 6:32 AM, Aniroop Mathur <[e-mail]> wrote:
>>> 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
>>>
>>> On Mon, Dec 29, 2014 at 5:33 PM, Jiri Kosina <[e-mail]> wrote:
>>>> On Sun, 28 Dec 2014, Aniroop Mathur wrote:
>>>>
>>>>> From: Aniroop Mathur <[e-mail]>
>>>>>
>>>>> 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.
>>>>
>>>>> and correct format string of unsigned long from %ld to %lu.
>>>>>
>>>>> Signed-off-by: Aniroop Mathur <[e-mail]>
>>>>> ---
>>>>>  drivers/hid/wacom_sys.c | 8 ++++----
>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>>>>> index 129fd33..4b5ff84 100644
>>>>> --- a/drivers/hid/wacom_sys.c
>>>>> +++ b/drivers/hid/wacom_sys.c
>>>>> @@ -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;
>>>>> --
>>>>> 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 [e-mail]
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 129fd33..4b5ff84 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -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;