Dnia 2012-03-18, nie o godzinie 12:58 -0500, Chris Bagwell pisze:
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
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 <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.
> 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 that I think it's better to see 50% instead
of 53% - the accurancy is not very important here.
> > 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);
I made a mistake here... "rw == 7" means we're charging, but you're
right - I could use double 100% for batcap_gr[6] and [7] and pull it out
of the "if". It was not possible previously as there had to be a "magic"
value to indicate charging.
I'm not sure if adding another "if" in IRQ is a serious performance
issue as battery value doesn't change too often. Moving the code into
"sysfs section" would force checking if it's Graphire or Intuos4 WL (see
the second patch). That's why I had to push it out of "sysfs section"
A word of explanation: I no longer have the device, so I want to keep
the modifications to minimum as I can't test them.
Thanks for your comments!
--
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
