Re: [Linuxwacom-devel] [PATCH ver.2 1/2] HID: wacom: Refactor battery/ac reporting for Graphire
On Sun, Mar 18, 2012 at 2:27 PM, Przemo Firszt wrote: > This patch doesn't change the way battery/ac is reported, but the changes are > required to facilitate battery reporting for Intuos4 WL. > wdata->battery_capacity now stores actual battery capacity as opposed to raw > value reported by wacom graphire previously. Power supply state is now stored > in a separate variable - it used to be calculated on-the-fly in > wacom_ac_get_property function. The raw value has to be stored as well to be > able to determine if it has changed. > > Signed-off-by: Przemo Firszt Reviewed-by: Chris Bagwell > --- > drivers/hid/hid-wacom.c | 34 +- > 1 files changed, 17 insertions(+), 17 deletions(-) > > diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c > index 067e296..1d19ccb 100644 > --- a/drivers/hid/hid-wacom.c > +++ b/drivers/hid/hid-wacom.c > @@ -42,15 +42,18 @@ struct wacom_data { > __u32 serial; > unsigned char high_speed; > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY > - int battery_capacity; > + __u8 battery_capacity; > + __u8 battery_raw; > + __u8 ps_connected; > struct power_supply battery; > struct power_supply ac; > #endif > }; > > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY > -/*percent of battery capacity, 0 means AC online*/ > -static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; > +/*percent of battery capacity for Graphire > + 7th value means AC online and show 100% capacity */ > +static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 100 }; > > static enum power_supply_property wacom_battery_props[] = { > POWER_SUPPLY_PROP_PRESENT, > @@ -70,7 +73,6 @@ static int wacom_battery_get_property(struct power_supply > *psy, > { > struct wacom_data *wdata = container_of(psy, > struct wacom_data, battery); > - int power_state = batcap[wdata->battery_capacity]; > int ret = 0; > > switch (psp) { > @@ -81,11 +83,7 @@ static int wacom_battery_get_property(struct power_supply > *psy, > val->intval = POWER_SUPPLY_SCOPE_DEVICE; > break; > case POWER_SUPPLY_PROP_CAPACITY: > - /* show 100% battery capacity when charging */ > - if (power_state == 0) > - val->intval = 100; > - else > - val->intval = power_state; > + val->intval = wdata->battery_capacity; > break; > default: > ret = -EINVAL; > @@ -99,17 +97,13 @@ static int wacom_ac_get_property(struct power_supply *psy, > union power_supply_propval *val) > { > struct wacom_data *wdata = container_of(psy, struct wacom_data, ac); > - int power_state = batcap[wdata->battery_capacity]; > int ret = 0; > > switch (psp) { > case POWER_SUPPLY_PROP_PRESENT: > /* fall through */ > case POWER_SUPPLY_PROP_ONLINE: > - if (power_state == 0) > - val->intval = 1; > - else > - val->intval = 0; > + val->intval = wdata->ps_connected; > break; > case POWER_SUPPLY_PROP_SCOPE: > val->intval = POWER_SUPPLY_SCOPE_DEVICE; > @@ -311,10 +305,16 @@ static int wacom_gr_parse_report(struct hid_device > *hdev, > } > > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY > - /* Store current battery capacity */ > + /* Store current battery capacity and power supply state*/ > rw = (data[7] >> 2 & 0x07); > - if (rw != wdata->battery_capacity) > - wdata->battery_capacity = rw; > + if (rw != wdata->battery_raw) { > + wdata->battery_raw = rw; > + wdata->battery_capacity = batcap_gr[rw]; > + if (rw == 7) > + wdata->ps_connected = 1; > + else > + wdata->ps_connected = 0; > + } > #endif > return 1; > } > -- > 1.7.6.4 > -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
Re: [Linuxwacom-devel] [PATCH ver.2 2/2] HID: wacom: Add battery/ac reporting for Intuos4 WL
On Sun, Mar 18, 2012 at 2:27 PM, Przemo Firszt wrote: > This patch adds battery/ac reporting for Intuos4 WL. It uses existing > sysfs code, but the device reports battery capacity in more fine-grained way, > so there has to be a separate lookup table (called batcap_i4). > > Signed-off-by: Przemo Firszt > --- > drivers/hid/hid-wacom.c | 12 > 1 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c > index 1d19ccb..2c89c8a 100644 > --- a/drivers/hid/hid-wacom.c > +++ b/drivers/hid/hid-wacom.c > @@ -54,6 +54,8 @@ struct wacom_data { > /*percent of battery capacity for Graphire > 7th value means AC online and show 100% capacity */ > static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 100 }; > +/*percent of battery capacity for Intuos4 WL, AC has a separate bit*/ > +static unsigned short batcap_i4[8] = { 1, 15, 30, 45, 60, 70, 85, 100 }; > > static enum power_supply_property wacom_battery_props[] = { > POWER_SUPPLY_PROP_PRESENT, > @@ -455,6 +457,8 @@ static int wacom_raw_event(struct hid_device *hdev, > struct hid_report *report, > struct input_dev *input; > unsigned char *data = (unsigned char *) raw_data; > int i; > + __u8 battery; > + __u8 ps_connected; > > if (!(hdev->claimed & HID_CLAIMED_INPUT)) > return 0; > @@ -482,6 +486,14 @@ static int wacom_raw_event(struct hid_device *hdev, > struct hid_report *report, > wacom_i4_parse_report(hdev, wdata, input, data + i); > i += 10; > wacom_i4_parse_report(hdev, wdata, input, data + i); > + battery = data[i+10] & 0x07; > + if (batcap_i4[battery] != wdata->battery_capacity) > + wdata->battery_capacity = batcap_i4[battery]; > + > + ps_connected = data[i+10] & 0x08; > + if (ps_connected != wdata->ps_connected) > + wdata->ps_connected = ps_connected; > + I think in long term it would be better if you kept same code flow for both Graphire and Intuos4. Your 2nd patch for Graphire looked closer to this: if (data[i+10] != wdata->battery_raw) { wdat->battery_raw = data[i+10]; battery = data[i+10] & 0x07; wdata->battery_capacity = batcap_i4[battery]; ps_connected = data[i+10] & 0x08; wdata->ps_connected = ps_connected; } > break; > default: > hid_err(hdev, "Unknown report: %d,%d size:%d\n", > -- > 1.7.6.4 > -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
Re: [Linuxwacom-devel] [PATCH 2/2] HID: wacom: Add battery/ac reporting for Intuos4 WL
On Sun, Mar 18, 2012 at 2:08 PM, Przemo Firszt wrote: > Dnia 2012-03-18, nie o godzinie 13:38 -0500, Chris Bagwell pisze: >> On Sun, Mar 18, 2012 at 11:24 AM, Przemo Firszt wrote: >> > This patch adds battery/ac reporting for Intuos4 WL. It uses existing >> > sysfs code, but the device reports battery capacity in more fine-grained >> > way, >> > so there has to be a separate lookup table (called batcap_i4). >> > >> > Signed-off-by: Przemo Firszt >> > --- >> > drivers/hid/hid-wacom.c | 12 >> > 1 files changed, 12 insertions(+), 0 deletions(-) >> > >> > diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c >> > index 516b468..708b909 100644 >> > --- a/drivers/hid/hid-wacom.c >> > +++ b/drivers/hid/hid-wacom.c >> > @@ -53,6 +53,8 @@ struct wacom_data { >> > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY >> > /*percent of battery capacity for Graphire, 0 means AC online*/ >> > static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; >> > +/*percent of battery capacity for Intuos4 WL, AC has a separate bit*/ >> > +static unsigned short batcap_i4[8] = { 1, 15, 30, 45, 60, 70, 85, 100 }; >> > >> > static enum power_supply_property wacom_battery_props[] = { >> > POWER_SUPPLY_PROP_PRESENT, >> > @@ -457,6 +459,8 @@ static int wacom_raw_event(struct hid_device *hdev, >> > struct hid_report *report, >> > struct input_dev *input; >> > unsigned char *data = (unsigned char *) raw_data; >> > int i; >> > + __u8 battery; >> > + __u8 ps_connected; >> > >> > if (!(hdev->claimed & HID_CLAIMED_INPUT)) >> > return 0; >> > @@ -484,6 +488,14 @@ static int wacom_raw_event(struct hid_device *hdev, >> > struct hid_report *report, >> > wacom_i4_parse_report(hdev, wdata, input, data + i); >> > i += 10; >> > wacom_i4_parse_report(hdev, wdata, input, data + i); >> > + battery = data[i+10] & 0x07; >> > + if (batcap_i4[battery] != wdata->battery_capacity) >> > + wdata->battery_capacity = >> > batcap_i4[battery]; >> >> Its less work to set update each time; even to same value; then it is >> to conditionally do it. > Hi Chris, > It is conditional. One line after "if" doesnt' require {brackets} > http://www.cprogramming.com/tutorial/c/lesson2.html What I meant was that writing only: wdata->battery_capacity = batcap_i4[battery]; will have same end result, generate smaller size code, and run as fast as or faster than this: if (batcap_i4[battery] != wdata->battery_capacity) wdata->battery_capacity = batcap_i4[battery]; Looking at the code block again and comparing it to Graphire, they flow differently. You may want to make the whole block only executed when it see's capacity change. Let me reply to other email though. Chris > >> Hopefully, you consider moving to formula outside IRQ... > I don't want to do that - see my first email. It would add checking the > device type in "sysfs section" instead of plain "report whatever is in > wdata->battery_capacity/ps_connected". > >> but do you >> want !ps_connected to show 100% like Graphire is doing? > No, Intuos4 WL seems to report proper battery value even when charging. > I'll confirm that as soon as I find a standalone charger (USB connection > switches the device to usb driver :-) ) > >> I assume you >> not doing it is a feature since "status" is something you could one >> day support and the difference between charging/full/discharging is if >> battery level is 100% or not + ps_connected. Forcing to 100% would >> hide that info. > Thanks for that - I'll check how to report it and I'll add it. > -- > Regards, > Przemo Firszt > -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
[Linuxwacom-devel] [PATCH ver.2 1/2] HID: wacom: Refactor battery/ac reporting for Graphire
This patch doesn't change the way battery/ac is reported, but the changes are required to facilitate battery reporting for Intuos4 WL. wdata->battery_capacity now stores actual battery capacity as opposed to raw value reported by wacom graphire previously. Power supply state is now stored in a separate variable - it used to be calculated on-the-fly in wacom_ac_get_property function. The raw value has to be stored as well to be able to determine if it has changed. Signed-off-by: Przemo Firszt --- drivers/hid/hid-wacom.c | 34 +- 1 files changed, 17 insertions(+), 17 deletions(-) diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c index 067e296..1d19ccb 100644 --- a/drivers/hid/hid-wacom.c +++ b/drivers/hid/hid-wacom.c @@ -42,15 +42,18 @@ struct wacom_data { __u32 serial; unsigned char high_speed; #ifdef CONFIG_HID_WACOM_POWER_SUPPLY - int battery_capacity; + __u8 battery_capacity; + __u8 battery_raw; + __u8 ps_connected; struct power_supply battery; struct power_supply ac; #endif }; #ifdef CONFIG_HID_WACOM_POWER_SUPPLY -/*percent of battery capacity, 0 means AC online*/ -static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; +/*percent of battery capacity for Graphire + 7th value means AC online and show 100% capacity */ +static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 100 }; static enum power_supply_property wacom_battery_props[] = { POWER_SUPPLY_PROP_PRESENT, @@ -70,7 +73,6 @@ static int wacom_battery_get_property(struct power_supply *psy, { struct wacom_data *wdata = container_of(psy, struct wacom_data, battery); - int power_state = batcap[wdata->battery_capacity]; int ret = 0; switch (psp) { @@ -81,11 +83,7 @@ static int wacom_battery_get_property(struct power_supply *psy, val->intval = POWER_SUPPLY_SCOPE_DEVICE; break; case POWER_SUPPLY_PROP_CAPACITY: - /* show 100% battery capacity when charging */ - if (power_state == 0) - val->intval = 100; - else - val->intval = power_state; + val->intval = wdata->battery_capacity; break; default: ret = -EINVAL; @@ -99,17 +97,13 @@ static int wacom_ac_get_property(struct power_supply *psy, union power_supply_propval *val) { struct wacom_data *wdata = container_of(psy, struct wacom_data, ac); - int power_state = batcap[wdata->battery_capacity]; int ret = 0; switch (psp) { case POWER_SUPPLY_PROP_PRESENT: /* fall through */ case POWER_SUPPLY_PROP_ONLINE: - if (power_state == 0) - val->intval = 1; - else - val->intval = 0; + val->intval = wdata->ps_connected; break; case POWER_SUPPLY_PROP_SCOPE: val->intval = POWER_SUPPLY_SCOPE_DEVICE; @@ -311,10 +305,16 @@ static int wacom_gr_parse_report(struct hid_device *hdev, } #ifdef CONFIG_HID_WACOM_POWER_SUPPLY - /* Store current battery capacity */ + /* Store current battery capacity and power supply state*/ rw = (data[7] >> 2 & 0x07); - if (rw != wdata->battery_capacity) - wdata->battery_capacity = rw; + if (rw != wdata->battery_raw) { + wdata->battery_raw = rw; + wdata->battery_capacity = batcap_gr[rw]; + if (rw == 7) + wdata->ps_connected = 1; + else + wdata->ps_connected = 0; + } #endif return 1; } -- 1.7.6.4 -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
[Linuxwacom-devel] [PATCH ver.2 2/2] HID: wacom: Add battery/ac reporting for Intuos4 WL
This patch adds battery/ac reporting for Intuos4 WL. It uses existing sysfs code, but the device reports battery capacity in more fine-grained way, so there has to be a separate lookup table (called batcap_i4). Signed-off-by: Przemo Firszt --- drivers/hid/hid-wacom.c | 12 1 files changed, 12 insertions(+), 0 deletions(-) diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c index 1d19ccb..2c89c8a 100644 --- a/drivers/hid/hid-wacom.c +++ b/drivers/hid/hid-wacom.c @@ -54,6 +54,8 @@ struct wacom_data { /*percent of battery capacity for Graphire 7th value means AC online and show 100% capacity */ static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 100 }; +/*percent of battery capacity for Intuos4 WL, AC has a separate bit*/ +static unsigned short batcap_i4[8] = { 1, 15, 30, 45, 60, 70, 85, 100 }; static enum power_supply_property wacom_battery_props[] = { POWER_SUPPLY_PROP_PRESENT, @@ -455,6 +457,8 @@ static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report, struct input_dev *input; unsigned char *data = (unsigned char *) raw_data; int i; + __u8 battery; + __u8 ps_connected; if (!(hdev->claimed & HID_CLAIMED_INPUT)) return 0; @@ -482,6 +486,14 @@ static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report, wacom_i4_parse_report(hdev, wdata, input, data + i); i += 10; wacom_i4_parse_report(hdev, wdata, input, data + i); + battery = data[i+10] & 0x07; + if (batcap_i4[battery] != wdata->battery_capacity) + wdata->battery_capacity = batcap_i4[battery]; + + ps_connected = data[i+10] & 0x08; + if (ps_connected != wdata->ps_connected) + wdata->ps_connected = ps_connected; + break; default: hid_err(hdev, "Unknown report: %d,%d size:%d\n", -- 1.7.6.4 -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
Re: [Linuxwacom-devel] [PATCH 2/2] HID: wacom: Add battery/ac reporting for Intuos4 WL
Dnia 2012-03-18, nie o godzinie 13:38 -0500, Chris Bagwell pisze: > On Sun, Mar 18, 2012 at 11:24 AM, Przemo Firszt wrote: > > This patch adds battery/ac reporting for Intuos4 WL. It uses existing > > sysfs code, but the device reports battery capacity in more fine-grained > > way, > > so there has to be a separate lookup table (called batcap_i4). > > > > Signed-off-by: Przemo Firszt > > --- > > drivers/hid/hid-wacom.c | 12 > > 1 files changed, 12 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c > > index 516b468..708b909 100644 > > --- a/drivers/hid/hid-wacom.c > > +++ b/drivers/hid/hid-wacom.c > > @@ -53,6 +53,8 @@ struct wacom_data { > > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY > > /*percent of battery capacity for Graphire, 0 means AC online*/ > > static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; > > +/*percent of battery capacity for Intuos4 WL, AC has a separate bit*/ > > +static unsigned short batcap_i4[8] = { 1, 15, 30, 45, 60, 70, 85, 100 }; > > > > static enum power_supply_property wacom_battery_props[] = { > >POWER_SUPPLY_PROP_PRESENT, > > @@ -457,6 +459,8 @@ static int wacom_raw_event(struct hid_device *hdev, > > struct hid_report *report, > >struct input_dev *input; > >unsigned char *data = (unsigned char *) raw_data; > >int i; > > + __u8 battery; > > + __u8 ps_connected; > > > >if (!(hdev->claimed & HID_CLAIMED_INPUT)) > >return 0; > > @@ -484,6 +488,14 @@ static int wacom_raw_event(struct hid_device *hdev, > > struct hid_report *report, > >wacom_i4_parse_report(hdev, wdata, input, data + i); > >i += 10; > >wacom_i4_parse_report(hdev, wdata, input, data + i); > > + battery = data[i+10] & 0x07; > > + if (batcap_i4[battery] != wdata->battery_capacity) > > + wdata->battery_capacity = > > batcap_i4[battery]; > > Its less work to set update each time; even to same value; then it is > to conditionally do it. Hi Chris, It is conditional. One line after "if" doesnt' require {brackets} http://www.cprogramming.com/tutorial/c/lesson2.html > Hopefully, you consider moving to formula outside IRQ... I don't want to do that - see my first email. It would add checking the device type in "sysfs section" instead of plain "report whatever is in wdata->battery_capacity/ps_connected". > but do you > want !ps_connected to show 100% like Graphire is doing? No, Intuos4 WL seems to report proper battery value even when charging. I'll confirm that as soon as I find a standalone charger (USB connection switches the device to usb driver :-) ) > I assume you > not doing it is a feature since "status" is something you could one > day support and the difference between charging/full/discharging is if > battery level is 100% or not + ps_connected. Forcing to 100% would > hide that info. Thanks for that - I'll check how to report it and I'll add it. -- Regards, Przemo Firszt -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
Re: [Linuxwacom-devel] [PATCH 1/2] HID: wacom: Refactor battery/ac reporting for Graphire
Dnia 2012-03-18, nie o godzinie 12:58 -0500, Chris Bagwell pisze: On Sun, Mar 18, 2012 at 11:24 AM, Przemo Firszt wrote: > > This patch doesn't change the way battery/ac is reported, but the changes are > > required to facilitate battery reporting for Intuos4 WL. > > wdata->battery_capacity now stores actual battery capacity as opposed to raw > > value reported by wacom graphire previously. Power supply state is now stored > > in a separate variable - it used to be calculated on-the-fly in > > wacom_ac_get_property function. The raw value has to be stored as well to be > > able to determine if it has changed. > > FYI: Here is link for my version of battery support for Wacom > Wireless Module for Bamboo and Intuos5. > > http://www.spinics.net/lists/linux-input/msg19720.html Hi Chris, Battery capacity can be 0 to 63 (0x3F)? What do you get when it's 100% charged? If it's 63 it would mean that this: val->intval = wacom->wacom_wac.battery_capacity * 100 / 31; returns 203% Not sure how does it work.. > One thing I liked about the original code in this patch is it did good > job at pushing as much battery work to on-demand processing (when user > queries battery status) instead of inside IRQ which will be throw away > math for majority cases. > > Let me see if I can summarize the math issue you have. Graphire's > seem to be reporting battery levels of 1-7 with a value of zero as > special case indicating battery is charging/connected to power. > Intuos4 reports value 0-7 for battery levels and bit 0x08 to indicate > battery charging/connected to power. BTW: My patch doesn't have > charging support because I couldn't find a bit that was working to > indicate this... Maybe I'll figure that out some time. > > I'll put rest of my comments inline of code. > > > > Signed-off-by: Przemo Firszt > > --- > > drivers/hid/hid-wacom.c | 36 +++- > > 1 files changed, 19 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c > > index 067e296..516b468 100644 > > --- a/drivers/hid/hid-wacom.c > > +++ b/drivers/hid/hid-wacom.c > > @@ -42,15 +42,17 @@ struct wacom_data { > >__u32 serial; > >unsigned char high_speed; > > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY > > - int battery_capacity; > > + __u8 battery_capacity; > > + __u8 battery_raw; > > + __u8 ps_connected; > >struct power_supply battery; > >struct power_supply ac; > > #endif > > }; > > > > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY > > -/*percent of battery capacity, 0 means AC online*/ > > -static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; > > +/*percent of battery capacity for Graphire, 0 means AC online*/ > > +static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; > > Something always confused me with original code. The index into this > array is (data[7] >> 2 & 0x07). So the only thing I know for sure is > it could be value of 0-7. Based on this array, a value of > batcap_gr[7] is not valid, right? So the battery range is 0-6. But > then zero is treated special (battery charging/connected to power) so > that means location batcap_gr[0] = 1 will never be shown to user > right? So real range is 1-6. > I don't have the tablet, but I think it reports "7" (3 bits on) when charging, 6 when 100% charged. So batcap_gr[7] is really meaningless - it's a "magic" value to show that the device is charging. > > > static enum power_supply_property wacom_battery_props[] = { > >POWER_SUPPLY_PROP_PRESENT, > > @@ -70,7 +72,6 @@ static int wacom_battery_get_property(struct power_supply *psy, > > { > >struct wacom_data *wdata = container_of(psy, > >struct wacom_data, battery); > > - int power_state = batcap[wdata->battery_capacity]; > >int ret = 0; > > > >switch (psp) { > > @@ -81,11 +82,7 @@ static int wacom_battery_get_property(struct power_supply *psy, > >val->intval = POWER_SUPPLY_SCOPE_DEVICE; > >break; > >case POWER_SUPPLY_PROP_CAPACITY: > > - /* show 100% battery capacity when charging */ > > - if (power_state == 0) > > - val->intval = 100; > > - else > > - val->intval = power_state; > > + val->intval = wdata->battery_capacity; > > Using an array always seemed a little odd to me. The math is pretty trivial. > > int min_range = 1; > int max_range = 6; > val->intval = ((wdata->battery_capacity - min_range) * 100) / max_range; > > I show it this way as alternative for you. You can set min/max range > in the probe function based on HW type and now you do not have to do > throw away conversion math each time you process battery packet... > only when user requests the conversion. Your math would be good as well, but arrays are quick and I'm sure the values are correct. Beside tha
Re: [Linuxwacom-devel] [PATCH 2/2] HID: wacom: Add battery/ac reporting for Intuos4 WL
On Sun, Mar 18, 2012 at 11:24 AM, Przemo Firszt wrote: > This patch adds battery/ac reporting for Intuos4 WL. It uses existing > sysfs code, but the device reports battery capacity in more fine-grained way, > so there has to be a separate lookup table (called batcap_i4). > > Signed-off-by: Przemo Firszt > --- > drivers/hid/hid-wacom.c | 12 > 1 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c > index 516b468..708b909 100644 > --- a/drivers/hid/hid-wacom.c > +++ b/drivers/hid/hid-wacom.c > @@ -53,6 +53,8 @@ struct wacom_data { > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY > /*percent of battery capacity for Graphire, 0 means AC online*/ > static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; > +/*percent of battery capacity for Intuos4 WL, AC has a separate bit*/ > +static unsigned short batcap_i4[8] = { 1, 15, 30, 45, 60, 70, 85, 100 }; > > static enum power_supply_property wacom_battery_props[] = { > POWER_SUPPLY_PROP_PRESENT, > @@ -457,6 +459,8 @@ static int wacom_raw_event(struct hid_device *hdev, > struct hid_report *report, > struct input_dev *input; > unsigned char *data = (unsigned char *) raw_data; > int i; > + __u8 battery; > + __u8 ps_connected; > > if (!(hdev->claimed & HID_CLAIMED_INPUT)) > return 0; > @@ -484,6 +488,14 @@ static int wacom_raw_event(struct hid_device *hdev, > struct hid_report *report, > wacom_i4_parse_report(hdev, wdata, input, data + i); > i += 10; > wacom_i4_parse_report(hdev, wdata, input, data + i); > + battery = data[i+10] & 0x07; > + if (batcap_i4[battery] != wdata->battery_capacity) > + wdata->battery_capacity = batcap_i4[battery]; Its less work to set update each time; even to same value; then it is to conditionally do it. Hopefully, you consider moving to formula outside IRQ... but do you want !ps_connected to show 100% like Graphire is doing? I assume you not doing it is a feature since "status" is something you could one day support and the difference between charging/full/discharging is if battery level is 100% or not + ps_connected. Forcing to 100% would hide that info. Chris > + > + ps_connected = data[i+10] & 0x08; > + if (ps_connected != wdata->ps_connected) > + wdata->ps_connected = ps_connected; Same here. > + > break; > default: > hid_err(hdev, "Unknown report: %d,%d size:%d\n", > -- > 1.7.6.4 > > > -- > This SF email is sponsosred by: > Try Windows Azure free for 90 days Click Here > http://p.sf.net/sfu/sfd2d-msazure > ___ > Linuxwacom-devel mailing list > Linuxwacom-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
Re: [Linuxwacom-devel] [PATCH 1/2] HID: wacom: Refactor battery/ac reporting for Graphire
On Sun, Mar 18, 2012 at 11:24 AM, Przemo Firszt wrote: > This patch doesn't change the way battery/ac is reported, but the changes are > required to facilitate battery reporting for Intuos4 WL. > wdata->battery_capacity now stores actual battery capacity as opposed to raw > value reported by wacom graphire previously. Power supply state is now stored > in a separate variable - it used to be calculated on-the-fly in > wacom_ac_get_property function. The raw value has to be stored as well to be > able to determine if it has changed. FYI: Here is link for my version of battery support for Wacom Wireless Module for Bamboo and Intuos5. http://www.spinics.net/lists/linux-input/msg19720.html One thing I liked about the original code in this patch is it did good job at pushing as much battery work to on-demand processing (when user queries battery status) instead of inside IRQ which will be throw away math for majority cases. Let me see if I can summarize the math issue you have. Graphire's seem to be reporting battery levels of 1-7 with a value of zero as special case indicating battery is charging/connected to power. Intuos4 reports value 0-7 for battery levels and bit 0x08 to indicate battery charging/connected to power. BTW: My patch doesn't have charging support because I couldn't find a bit that was working to indicate this... Maybe I'll figure that out some time. I'll put rest of my comments inline of code. > > Signed-off-by: Przemo Firszt > --- > drivers/hid/hid-wacom.c | 36 +++- > 1 files changed, 19 insertions(+), 17 deletions(-) > > diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c > index 067e296..516b468 100644 > --- a/drivers/hid/hid-wacom.c > +++ b/drivers/hid/hid-wacom.c > @@ -42,15 +42,17 @@ struct wacom_data { > __u32 serial; > unsigned char high_speed; > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY > - int battery_capacity; > + __u8 battery_capacity; > + __u8 battery_raw; > + __u8 ps_connected; > struct power_supply battery; > struct power_supply ac; > #endif > }; > > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY > -/*percent of battery capacity, 0 means AC online*/ > -static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; > +/*percent of battery capacity for Graphire, 0 means AC online*/ > +static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; Something always confused me with original code. The index into this array is (data[7] >> 2 & 0x07). So the only thing I know for sure is it could be value of 0-7. Based on this array, a value of batcap_gr[7] is not valid, right? So the battery range is 0-6. But then zero is treated special (battery charging/connected to power) so that means location batcap_gr[0] = 1 will never be shown to user right? So real range is 1-6. > > static enum power_supply_property wacom_battery_props[] = { > POWER_SUPPLY_PROP_PRESENT, > @@ -70,7 +72,6 @@ static int wacom_battery_get_property(struct power_supply > *psy, > { > struct wacom_data *wdata = container_of(psy, > struct wacom_data, battery); > - int power_state = batcap[wdata->battery_capacity]; > int ret = 0; > > switch (psp) { > @@ -81,11 +82,7 @@ static int wacom_battery_get_property(struct power_supply > *psy, > val->intval = POWER_SUPPLY_SCOPE_DEVICE; > break; > case POWER_SUPPLY_PROP_CAPACITY: > - /* show 100% battery capacity when charging */ > - if (power_state == 0) > - val->intval = 100; > - else > - val->intval = power_state; > + val->intval = wdata->battery_capacity; Using an array always seemed a little odd to me. The math is pretty trivial. int min_range = 1; int max_range = 6; val->intval = ((wdata->battery_capacity - min_range) * 100) / max_range; I show it this way as alternative for you. You can set min/max range in the probe function based on HW type and now you do not have to do throw away conversion math each time you process battery packet... only when user requests the conversion. > break; > default: > ret = -EINVAL; > @@ -99,17 +96,13 @@ static int wacom_ac_get_property(struct power_supply *psy, > union power_supply_propval *val) > { > struct wacom_data *wdata = container_of(psy, struct wacom_data, ac); > - int power_state = batcap[wdata->battery_capacity]; > int ret = 0; > > switch (psp) { > case POWER_SUPPLY_PROP_PRESENT: > /* fall through */ > case POWER_SUPPLY_PROP_ONLINE: > - if (power_state == 0) > - val->intval = 1; > - else > - val->intval = 0; > + val->intval = wdata->ps_connected; > break; > case POWER_SU
[Linuxwacom-devel] [PATCH 1/2] HID: wacom: Refactor battery/ac reporting for Graphire
This patch doesn't change the way battery/ac is reported, but the changes are required to facilitate battery reporting for Intuos4 WL. wdata->battery_capacity now stores actual battery capacity as opposed to raw value reported by wacom graphire previously. Power supply state is now stored in a separate variable - it used to be calculated on-the-fly in wacom_ac_get_property function. The raw value has to be stored as well to be able to determine if it has changed. Signed-off-by: Przemo Firszt --- drivers/hid/hid-wacom.c | 36 +++- 1 files changed, 19 insertions(+), 17 deletions(-) diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c index 067e296..516b468 100644 --- a/drivers/hid/hid-wacom.c +++ b/drivers/hid/hid-wacom.c @@ -42,15 +42,17 @@ struct wacom_data { __u32 serial; unsigned char high_speed; #ifdef CONFIG_HID_WACOM_POWER_SUPPLY - int battery_capacity; + __u8 battery_capacity; + __u8 battery_raw; + __u8 ps_connected; struct power_supply battery; struct power_supply ac; #endif }; #ifdef CONFIG_HID_WACOM_POWER_SUPPLY -/*percent of battery capacity, 0 means AC online*/ -static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; +/*percent of battery capacity for Graphire, 0 means AC online*/ +static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; static enum power_supply_property wacom_battery_props[] = { POWER_SUPPLY_PROP_PRESENT, @@ -70,7 +72,6 @@ static int wacom_battery_get_property(struct power_supply *psy, { struct wacom_data *wdata = container_of(psy, struct wacom_data, battery); - int power_state = batcap[wdata->battery_capacity]; int ret = 0; switch (psp) { @@ -81,11 +82,7 @@ static int wacom_battery_get_property(struct power_supply *psy, val->intval = POWER_SUPPLY_SCOPE_DEVICE; break; case POWER_SUPPLY_PROP_CAPACITY: - /* show 100% battery capacity when charging */ - if (power_state == 0) - val->intval = 100; - else - val->intval = power_state; + val->intval = wdata->battery_capacity; break; default: ret = -EINVAL; @@ -99,17 +96,13 @@ static int wacom_ac_get_property(struct power_supply *psy, union power_supply_propval *val) { struct wacom_data *wdata = container_of(psy, struct wacom_data, ac); - int power_state = batcap[wdata->battery_capacity]; int ret = 0; switch (psp) { case POWER_SUPPLY_PROP_PRESENT: /* fall through */ case POWER_SUPPLY_PROP_ONLINE: - if (power_state == 0) - val->intval = 1; - else - val->intval = 0; + val->intval = wdata->ps_connected; break; case POWER_SUPPLY_PROP_SCOPE: val->intval = POWER_SUPPLY_SCOPE_DEVICE; @@ -311,10 +304,19 @@ static int wacom_gr_parse_report(struct hid_device *hdev, } #ifdef CONFIG_HID_WACOM_POWER_SUPPLY - /* Store current battery capacity */ + /* Store current battery capacity and power supply state*/ rw = (data[7] >> 2 & 0x07); - if (rw != wdata->battery_capacity) - wdata->battery_capacity = rw; + if (rw != wdata->battery_raw) { + wdata->battery_raw = rw; + if (rw == 0) { + /* show 100% battery capacity when charging */ + wdata->battery_capacity = 100; + wdata->ps_connected = 1; + } else { + wdata->battery_capacity = batcap_gr[rw]; + wdata->ps_connected = 0; + } + } #endif return 1; } -- 1.7.6.4 -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
[Linuxwacom-devel] [PATCH 2/2] HID: wacom: Add battery/ac reporting for Intuos4 WL
This patch adds battery/ac reporting for Intuos4 WL. It uses existing sysfs code, but the device reports battery capacity in more fine-grained way, so there has to be a separate lookup table (called batcap_i4). Signed-off-by: Przemo Firszt --- drivers/hid/hid-wacom.c | 12 1 files changed, 12 insertions(+), 0 deletions(-) diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c index 516b468..708b909 100644 --- a/drivers/hid/hid-wacom.c +++ b/drivers/hid/hid-wacom.c @@ -53,6 +53,8 @@ struct wacom_data { #ifdef CONFIG_HID_WACOM_POWER_SUPPLY /*percent of battery capacity for Graphire, 0 means AC online*/ static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 0 }; +/*percent of battery capacity for Intuos4 WL, AC has a separate bit*/ +static unsigned short batcap_i4[8] = { 1, 15, 30, 45, 60, 70, 85, 100 }; static enum power_supply_property wacom_battery_props[] = { POWER_SUPPLY_PROP_PRESENT, @@ -457,6 +459,8 @@ static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report, struct input_dev *input; unsigned char *data = (unsigned char *) raw_data; int i; + __u8 battery; + __u8 ps_connected; if (!(hdev->claimed & HID_CLAIMED_INPUT)) return 0; @@ -484,6 +488,14 @@ static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report, wacom_i4_parse_report(hdev, wdata, input, data + i); i += 10; wacom_i4_parse_report(hdev, wdata, input, data + i); + battery = data[i+10] & 0x07; + if (batcap_i4[battery] != wdata->battery_capacity) + wdata->battery_capacity = batcap_i4[battery]; + + ps_connected = data[i+10] & 0x08; + if (ps_connected != wdata->ps_connected) + wdata->ps_connected = ps_connected; + break; default: hid_err(hdev, "Unknown report: %d,%d size:%d\n", -- 1.7.6.4 -- This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure ___ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel