On Sun, Mar 18, 2012 at 11:24 AM, Przemo Firszt <prz...@firszt.eu> 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 <prz...@firszt.eu> > --- > 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_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; > + } Since how to detect charging is so HW specific, I like this new variable set during IRQ. For battery_capacity, I'm hoping you can push the conversion to outside IRQ but at minimum, why not make batcap_gr[0] = 100 and then you can pull this part outside the if() statement and blindly set it. But it seems like it should be a specific check outside of IRQ for Intuos4 case as well. With batcap_gr[0] = 100 update, You can also make ps_connected a single line as well: wdata->battery_capacity = batcap_gr[rw]; wdata->ps_connected = (rw == 0); Chris > + } > #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 ------------------------------------------------------------------------------ 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
