On Sat, 31 Dec 2011 12:27:43 +0100 Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> On 12/30/2011 12:13 PM, Grazvydas Ignotas wrote:
> > CCing Lars who added this. I vaguely recall something about generating
> > events to make some battery monitors update but I forget the details
> > now, maybe it was about something else. Also CCing Anton (the
> > maintainer).
> >
> > On Fri, Dec 30, 2011 at 2:58 AM, NeilBrown <neilb@xxxxxxx> wrote:
> >> A power_supply_changed should only be reported on significant changes
> >> such as transition between charging and not. Incremental changes
> >> such as charge increasing should not be reported - that can easily
> >> be polled for.
>
> Well, you can also poll for those "significant" changes, too. The point of
> adding this was to have a centralized point where polling takes place instead
> of letting each battery monitor do this on its own. Though if, as you wrote in
> the cover letter, the some properties change every time the values are read it
> might makes sense to exclude these from the comparison. On the other hand one
> event every 6 minutes doesn't really sound harmful and I would suspect that
> battery monitors will use a similar interval when manually polling the device.
Hi,
That is a very good point. user-space could poll for all of the changes and
polling the kernel provides no real benefit.
I don't really see the problem with each battery monitor doing their own
polling. At least that way they would have control over when polling
happens.
Polling every 6 minutes does not really seem like a lot - except that
polling at all in the kernel should be avoided. If the system is idle and
has managed to get into a lower power state, waking up just to check on the
battery would be the wrong thing to do.
A battery monitor could notice that no-one cared (e.g. A X11 client not
getting any 'expose' events) and could stop polling. The kernel cannot do
that.
However the part of the current code that really bothered me was that a
'change' event is generated every time anyone polls the battery status - but
at most every 5 seconds. These extra change events really aren't wanted.
I think we should always be cautious of adding change events. They are
not there just to report when any detail changed, else a 'keyboard' device
would report a 'change' event on every keystroke.
The main purpose of uevents are to report 'add' and 'remove' of devices.
'change' is for situations where a device changes in a way that is very
similar to an 'add' or a 'remove' but isn't implemented as a new device.
A simple example is inserting a CD into a CD drive. Before the device
couldn't do anything useful, now it can. It is a new medium, which is
almost like a new device but just not implemented that way. So it deserves a
change event.
Or when a power supply gets plugged in. We really have a new thing here -
we have added power. But as the power isn't a device we cannot have an
'add' uevent, so we have a 'change' uevent on the power supply.
So I would be in favour of removing the in-kernel polling altogether.
That puts complete control where it belongs: is the app that is monitoring
the battery.
Thoughts??
Thanks,
NeilBrown
>
> - Lars
>
> >>
> >> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> >> ---
> >>
> >> drivers/power/bq27x00_battery.c | 15 ++++++++++++---
> >> 1 files changed, 12 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
> >> index bb16f5b..7993a17 100644
> >> --- a/drivers/power/bq27x00_battery.c
> >> +++ b/drivers/power/bq27x00_battery.c
> >> @@ -57,11 +57,15 @@
> >> #define BQ27000_FLAG_CHGS BIT(7)
> >> #define BQ27000_FLAG_FC BIT(5)
> >>
> >> +#define BQ27000_FLAGS_IMPORTANT (BQ27000_FLAG_FC|BQ27000_FLAG_CHGS|BIT(31))
> >> +
> >> #define BQ27500_REG_SOC 0x2C
> >> #define BQ27500_REG_DCAP 0x3C /* Design capacity */
> >> #define BQ27500_FLAG_DSC BIT(0)
> >> #define BQ27500_FLAG_FC BIT(9)
> >>
> >> +#define BQ27500_FLAGS_IMPORTANT (BQ27500_FLAG_FC|BQ27500_FLAG_DSC|BIT(31))
> >> +
> >> #define BQ27000_RS 20 /* Resistor sense */
> >>
> >> struct bq27x00_device_info;
> >> @@ -259,6 +263,7 @@ static void bq27x00_update(struct bq27x00_device_info *di)
> >> {
> >> struct bq27x00_reg_cache cache = {0, };
> >> bool is_bq27500 = di->chip == BQ27500;
> >> + int flags_changed;
> >>
> >> cache.flags = bq27x00_read(di, BQ27x00_REG_FLAGS, is_bq27500);
> >> if (cache.flags >= 0) {
> >> @@ -280,10 +285,14 @@ static void bq27x00_update(struct bq27x00_device_info *di)
> >>
> >> /* Ignore current_now which is a snapshot of the current battery state
> >> * and is likely to be different even between two consecutive reads */
> >> - if (memcmp(&di->cache, &cache, sizeof(cache) - sizeof(int)) != 0) {
> >> - di->cache = cache;
> >> + flags_changed = di->cache.flags ^ cache.flags;
> >> + di->cache = cache;
> >> + if (is_bq27500)
> >> + flags_changed &= BQ27500_FLAGS_IMPORTANT;
> >> + else
> >> + flags_changed &= BQ27000_FLAGS_IMPORTANT;
> >> + if (flags_changed)
> >> power_supply_changed(&di->bat);
> >> - }
> >>
> >> di->last_update = jiffies;
> >> }
Attachment:
signature.asc
Description: PGP signature
